Modified: trunk/Source/WebCore/ChangeLog (234225 => 234226)
--- trunk/Source/WebCore/ChangeLog 2018-07-26 01:08:18 UTC (rev 234225)
+++ trunk/Source/WebCore/ChangeLog 2018-07-26 01:29:11 UTC (rev 234226)
@@ -1,3 +1,29 @@
+2018-07-25 Chris Dumez <[email protected]>
+
+ Allow ActiveDOMObject's canSuspend / suspend / resume overrides to destroy ActiveDOMObjects
+ https://bugs.webkit.org/show_bug.cgi?id=188025
+
+ Reviewed by Alex Christensen.
+
+ 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 Justin Fan <[email protected]>
Systems with no NSScreens hitting assertion in rendererIDForDisplay when creating WebGL context
Modified: trunk/Source/WebCore/dom/ScriptExecutionContext.cpp (234225 => 234226)
--- trunk/Source/WebCore/dom/ScriptExecutionContext.cpp 2018-07-26 01:08:18 UTC (rev 234225)
+++ trunk/Source/WebCore/dom/ScriptExecutionContext.cpp 2018-07-26 01:29:11 UTC (rev 234226)
@@ -61,6 +61,7 @@
#include <_javascript_Core/StrongInlines.h>
#include <wtf/MainThread.h>
#include <wtf/Ref.h>
+#include <wtf/SetForScope.h>
namespace WebCore {
using namespace Inspector;
@@ -228,30 +229,51 @@
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;
+ });
+
+ if (unsuspendableObjects) {
+ // Remove activeDOMObjects that have been destroyed while we were iterating above.
+ unsuspendableObjects->removeAllMatching([&](auto* activeDOMObject) {
+ return !m_activeDOMObjects.contains(activeDOMObject);
+ });
}
- m_activeDOMObjectAdditionForbidden = false;
- m_activeDOMObjectRemovalForbidden = false;
-
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,20 +288,11 @@
m_activeDOMObjectsAreSuspended = true;
- m_activeDOMObjectAdditionForbidden = true;
- m_activeDOMObjectRemovalForbidden = true;
+ forEachActiveDOMObject([why](auto& activeDOMObject) {
+ activeDOMObject.suspend(why);
+ return ShouldContinue::Yes;
+ });
- // 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;
-
m_reasonForSuspendingActiveDOMObjects = why;
}
@@ -291,19 +304,10 @@
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 @@
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::willDestroyActiveDOMObject(ActiveDOMObject& activeDOMObject)
{
- RELEASE_ASSERT(!m_activeDOMObjectRemovalForbidden);
m_activeDOMObjects.remove(&activeDOMObject);
}
Modified: trunk/Source/WebCore/dom/ScriptExecutionContext.h (234225 => 234226)
--- trunk/Source/WebCore/dom/ScriptExecutionContext.h 2018-07-26 01:08:18 UTC (rev 234225)
+++ trunk/Source/WebCore/dom/ScriptExecutionContext.h 2018-07-26 01:29:11 UTC (rev 234226)
@@ -294,6 +294,9 @@
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 @@
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
