Attachment 345798 Details for Bug 188025 – Patch

[patch] Patch

bug-188025-20180725162124.patch (text/plain), 10.47 KB, created by Chris Dumez on 2018-07-25 16:21:25 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2018-07-25 16:21:25 PDT

Size: 10.47 KB

patch

obsolete

>Subversion Revision: 234218
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 7e46fcca5e69bd7b3bff824da6459ee6698fb77d..0596195678e74401bb48cab6dcc69018a0119010 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,29 @@
>+2018-07-25  Chris Dumez  <cdumez@apple.com>
>+
>+        Allow ActiveDOMObject's canSuspend / suspend / resume overrides to destroy ActiveDOMObjects
>+        https://bugs.webkit.org/show_bug.cgi?id=188025
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Apply the same logic as in ScriptExecutionContext::stopActiveDOMObjects() to support destruction of
>+        ActiveDOMObjects while we're calling ActiveDOMObject's canSuspend / suspend / resume overrides.
>+
>+        We copy m_activeDOMObjects into a Vector and iterate over the copy instead of m_activeDOMObjects.
>+        Since ActiveDOMObject is not RefCounted or CanMakeWeakPtr, we verify that the raw pointer is still
>+        valid by checking if m_activeDOMObjects still contains it, as we iterate. This is safe as the
>+        ActiveDOMObject destructor removes the object from ScriptExecutionContext::m_activeDOMObjects.
>+        New ActiveDOMObjects with the same pointer value cannot be created while we iterate as we already
>+        prevent the construction of new ActiveDOMObjects while we iterate via RELEASE_ASSERT().
>+
>+        * dom/ScriptExecutionContext.cpp:
>+        (WebCore::ScriptExecutionContext::canSuspendActiveDOMObjectsForDocumentSuspension):
>+        (WebCore::ScriptExecutionContext::forEachActiveDOMObject const):
>+        (WebCore::ScriptExecutionContext::suspendActiveDOMObjects):
>+        (WebCore::ScriptExecutionContext::resumeActiveDOMObjects):
>+        (WebCore::ScriptExecutionContext::stopActiveDOMObjects):
>+        (WebCore::ScriptExecutionContext::willDestroyActiveDOMObject):
>+        * dom/ScriptExecutionContext.h:
>+
> 2018-07-25  Zalan Bujtas  <zalan@apple.com>
> 
>         REGRESSION(r227577) Text on TV & Movies page doesn't wrap properly in iTunes
>diff --git a/Source/WebCore/dom/ScriptExecutionContext.cpp b/Source/WebCore/dom/ScriptExecutionContext.cpp
>index 43944941e86b54208220be6def6fae5f17235d8a..afe26f25f190c0463c9171b8d80642653172fd1e 100644
>--- a/Source/WebCore/dom/ScriptExecutionContext.cpp
>+++ b/Source/WebCore/dom/ScriptExecutionContext.cpp
>@@ -61,6 +61,7 @@
> #include <JavaScriptCore/StrongInlines.h>
> #include <wtf/MainThread.h>
> #include <wtf/Ref.h>
>+#include <wtf/SetForScope.h>
> 
> namespace WebCore {
> using namespace Inspector;
>@@ -228,30 +229,51 @@ bool ScriptExecutionContext::canSuspendActiveDOMObjectsForDocumentSuspension(Vec
> 
>     bool canSuspend = true;
> 
>-    m_activeDOMObjectAdditionForbidden = true;
>-    m_activeDOMObjectRemovalForbidden = true;
>-
>-    // We assume that m_activeDOMObjects will not change during iteration: canSuspend
>-    // functions should not add new active DOM objects, nor execute arbitrary JavaScript.
>-    // An ASSERT_WITH_SECURITY_IMPLICATION or RELEASE_ASSERT will fire if this happens, but it's important to code
>-    // canSuspend functions so it will not happen!
>-    ScriptDisallowedScope::InMainThread scriptDisallowedScope;
>-    for (auto* activeDOMObject : m_activeDOMObjects) {
>-        if (!activeDOMObject->canSuspendForDocumentSuspension()) {
>+    forEachActiveDOMObject([&](auto& activeDOMObject) {
>+        if (!activeDOMObject.canSuspendForDocumentSuspension()) {
>             canSuspend = false;
>             if (unsuspendableObjects)
>-                unsuspendableObjects->append(activeDOMObject);
>+                unsuspendableObjects->append(&activeDOMObject);
>             else
>-                break;
>+                return ShouldContinue::No;
>         }
>-    }
>+        return ShouldContinue::Yes;
>+    });
> 
>-    m_activeDOMObjectAdditionForbidden = false;
>-    m_activeDOMObjectRemovalForbidden = false;
>+    if (unsuspendableObjects) {
>+        // Remove activeDOMObjects that have been destroyed while we were iterating above.
>+        unsuspendableObjects->removeAllMatching([&](auto* activeDOMObject) {
>+            return !m_activeDOMObjects.contains(activeDOMObject);
>+        });
>+    }
> 
>     return canSuspend;
> }
> 
>+void ScriptExecutionContext::forEachActiveDOMObject(const Function<ShouldContinue(ActiveDOMObject&)>& apply) const
>+{
>+    // It is not allowed to run arbitrary script or construct new ActiveDOMObjects while we are iterating over ActiveDOMObjects.
>+    // An ASSERT_WITH_SECURITY_IMPLICATION or RELEASE_ASSERT will fire if this happens, but it's important to code
>+    // canSuspendActiveDOMObjectsForDocumentSuspension() / suspend() / resume() / stop() functions so it will not happen!
>+    ScriptDisallowedScope scriptDisallowedScope;
>+    SetForScope<bool> activeDOMObjectAdditionForbiddenScope(m_activeDOMObjectAdditionForbidden, true);
>+
>+    // Make a frozen copy of the objects so we can iterate while new ones might be destroyed.
>+    auto possibleActiveDOMObjects = copyToVector(m_activeDOMObjects);
>+
>+    for (auto* activeDOMObject : possibleActiveDOMObjects) {
>+        // Check if this object was deleted already. If so, just skip it.
>+        // Calling contains on a possibly-already-deleted object is OK because we guarantee
>+        // no new object can be added, so even if a new object ends up allocated with the
>+        // same address, that will be *after* this function exits.
>+        if (!m_activeDOMObjects.contains(activeDOMObject))
>+            continue;
>+
>+        if (apply(*activeDOMObject) == ShouldContinue::No)
>+            break;
>+    }
>+}
>+
> void ScriptExecutionContext::suspendActiveDOMObjects(ReasonForSuspension why)
> {
>     checkConsistency();
>@@ -266,19 +288,10 @@ void ScriptExecutionContext::suspendActiveDOMObjects(ReasonForSuspension why)
> 
>     m_activeDOMObjectsAreSuspended = true;
> 
>-    m_activeDOMObjectAdditionForbidden = true;
>-    m_activeDOMObjectRemovalForbidden = true;
>-
>-    // We assume that m_activeDOMObjects will not change during iteration: suspend
>-    // functions should not add new active DOM objects, nor execute arbitrary JavaScript.
>-    // An ASSERT_WITH_SECURITY_IMPLICATION or RELEASE_ASSERT will fire if this happens, but it's important to code
>-    // suspend functions so it will not happen!
>-    ScriptDisallowedScope::InMainThread scriptDisallowedScope;
>-    for (auto* activeDOMObject : m_activeDOMObjects)
>-        activeDOMObject->suspend(why);
>-
>-    m_activeDOMObjectAdditionForbidden = false;
>-    m_activeDOMObjectRemovalForbidden = false;
>+    forEachActiveDOMObject([why](auto& activeDOMObject) {
>+        activeDOMObject.suspend(why);
>+        return ShouldContinue::Yes;
>+    });
> 
>     m_reasonForSuspendingActiveDOMObjects = why;
> }
>@@ -291,19 +304,10 @@ void ScriptExecutionContext::resumeActiveDOMObjects(ReasonForSuspension why)
>         return;
>     m_activeDOMObjectsAreSuspended = false;
> 
>-    m_activeDOMObjectAdditionForbidden = true;
>-    m_activeDOMObjectRemovalForbidden = true;
>-
>-    // We assume that m_activeDOMObjects will not change during iteration: resume
>-    // functions should not add new active DOM objects, nor execute arbitrary JavaScript.
>-    // An ASSERT_WITH_SECURITY_IMPLICATION or RELEASE_ASSERT will fire if this happens, but it's important to code
>-    // resume functions so it will not happen!
>-    ScriptDisallowedScope::InMainThread scriptDisallowedScope;
>-    for (auto* activeDOMObject : m_activeDOMObjects)
>-        activeDOMObject->resume();
>-
>-    m_activeDOMObjectAdditionForbidden = false;
>-    m_activeDOMObjectRemovalForbidden = false;
>+    forEachActiveDOMObject([](auto& activeDOMObject) {
>+        activeDOMObject.resume();
>+        return ShouldContinue::Yes;
>+    });
> }
> 
> void ScriptExecutionContext::stopActiveDOMObjects()
>@@ -314,27 +318,10 @@ void ScriptExecutionContext::stopActiveDOMObjects()
>         return;
>     m_activeDOMObjectsAreStopped = true;
> 
>-    // Make a frozen copy of the objects so we can iterate while new ones might be destroyed.
>-    auto possibleActiveDOMObjects = copyToVector(m_activeDOMObjects);
>-
>-    m_activeDOMObjectAdditionForbidden = true;
>-
>-    // We assume that new objects will not be added to m_activeDOMObjects during iteration:
>-    // stop functions should not add new active DOM objects, nor execute arbitrary JavaScript.
>-    // An ASSERT_WITH_SECURITY_IMPLICATION or RELEASE_ASSERT will fire if this happens, but it's important to code stop functions
>-    // so it will not happen!
>-    ScriptDisallowedScope scriptDisallowedScope;
>-    for (auto* activeDOMObject : possibleActiveDOMObjects) {
>-        // Check if this object was deleted already. If so, just skip it.
>-        // Calling contains on a possibly-already-deleted object is OK because we guarantee
>-        // no new object can be added, so even if a new object ends up allocated with the
>-        // same address, that will be *after* this function exits.
>-        if (!m_activeDOMObjects.contains(activeDOMObject))
>-            continue;
>-        activeDOMObject->stop();
>-    }
>-
>-    m_activeDOMObjectAdditionForbidden = false;
>+    forEachActiveDOMObject([](auto& activeDOMObject) {
>+        activeDOMObject.stop();
>+        return ShouldContinue::Yes;
>+    });
> }
> 
> void ScriptExecutionContext::suspendActiveDOMObjectIfNeeded(ActiveDOMObject& activeDOMObject)
>@@ -359,7 +346,6 @@ void ScriptExecutionContext::didCreateActiveDOMObject(ActiveDOMObject& activeDOM
> 
> void ScriptExecutionContext::willDestroyActiveDOMObject(ActiveDOMObject& activeDOMObject)
> {
>-    RELEASE_ASSERT(!m_activeDOMObjectRemovalForbidden);
>     m_activeDOMObjects.remove(&activeDOMObject);
> }
> 
>diff --git a/Source/WebCore/dom/ScriptExecutionContext.h b/Source/WebCore/dom/ScriptExecutionContext.h
>index 224b3af90a245aa5ce24336b7bbdb79e1421114b..16b6c53cb13d8ea4765d7deb26847bc6ff0d192d 100644
>--- a/Source/WebCore/dom/ScriptExecutionContext.h
>+++ b/Source/WebCore/dom/ScriptExecutionContext.h
>@@ -294,6 +294,9 @@ private:
>     virtual void refScriptExecutionContext() = 0;
>     virtual void derefScriptExecutionContext() = 0;
> 
>+    enum class ShouldContinue { No, Yes };
>+    void forEachActiveDOMObject(const Function<ShouldContinue(ActiveDOMObject&)>&) const;
>+
>     RejectedPromiseTracker& ensureRejectedPromiseTrackerSlow();
> 
>     void checkConsistency() const;
>@@ -320,8 +323,7 @@ private:
>     bool m_activeDOMObjectsAreSuspended { false };
>     bool m_activeDOMObjectsAreStopped { false };
>     bool m_inDispatchErrorEvent { false };
>-    bool m_activeDOMObjectAdditionForbidden { false };
>-    bool m_activeDOMObjectRemovalForbidden { false };
>+    mutable bool m_activeDOMObjectAdditionForbidden { false };
>     bool m_willprocessMessageWithMessagePortsSoon { false };
> 
> #if !ASSERT_DISABLED

Actions: View | Formatted Diff | Diff

Attachments on bug 188025: 345794 | 345798