Finalizers shouldn't run if events can't fire

https://bugs.webkit.org/show_bug.cgi?id=214508

Reviewed by Ryosuke Niwa.

Source/JavaScriptCore:

This patch makes it so the DeferredWorkTimer won't run scheduled
tasks if those would not have run if they were scheduled in
WebCore. To do this there is now a concept of a
ScriptExecutionOwner. The ScriptExecutionOwner is almost always
the same as the global object of the pending task (referred to as
the ticket). The only exception to this is if the global object
is a JSDOMWindowBase, then the ScriptExecutionOwner is the
Document's JS wrapper. To tell the status of a
ScriptExecutionOwner, the DeferredWorkTimer calls a virtual
function on the global object of the ticket, for JSC-only this
just always returns Running. For WebCore, we ask the
ScriptExecutionContext associated with the ScriptExecutionOwner.

* API/JSAPIGlobalObject.cpp:
* API/JSAPIGlobalObject.mm:
* jsc.cpp:
* runtime/DeferredWorkTimer.cpp:
(JSC::DeferredWorkTimer::doWork):
(JSC::DeferredWorkTimer::addPendingWork):
(JSC::DeferredWorkTimer::hasDependancyInPendingWork):
(JSC::DeferredWorkTimer::didResumeScriptExecutionOwner):
* runtime/DeferredWorkTimer.h:
* runtime/JSFinalizationRegistry.cpp:
(JSC::JSFinalizationRegistry::create):
(JSC::JSFinalizationRegistry::finishCreation):
* runtime/JSFinalizationRegistry.h:
* runtime/JSGlobalObject.cpp:
* runtime/JSGlobalObject.h:
(JSC::JSGlobalObject::currentScriptExecutionOwner):
(JSC::JSGlobalObject::scriptExecutionStatus):

Source/WebCore:

This patch makes it so the DeferredWorkTimer won't run scheduled
tasks if those would not have run if they were scheduled in
WebCore. To do this there is now a concept of a
ScriptExecutionOwner. The ScriptExecutionOwner is almost always
the same as the global object of the pending task (referred to as
the ticket). The only exception to this is if the global object
is a JSDOMWindowBase, then the ScriptExecutionOwner is the
Document's JS wrapper. To tell the status of a
ScriptExecutionOwner, the DeferredWorkTimer calls a virtual
function on the global object of the ticket, for JSC-only this
just always returns Running. For WebCore, we ask the
ScriptExecutionContext associated with the ScriptExecutionOwner.

* bindings/js/JSDOMWindowBase.cpp:
(WebCore::JSDOMWindowBase::currentScriptExecutionOwner):
(WebCore::JSDOMWindowBase::scriptExecutionStatus):
* bindings/js/JSDOMWindowBase.h:
* bindings/js/JSDOMWrapperCache.h:
* bindings/js/JSRemoteDOMWindowBase.cpp:
* bindings/js/JSWorkerGlobalScopeBase.cpp:
(WebCore::JSWorkerGlobalScopeBase::scriptExecutionStatus):
* bindings/js/JSWorkerGlobalScopeBase.h:
* bindings/js/JSWorkletGlobalScopeBase.cpp:
(WebCore::JSWorkletGlobalScopeBase::scriptExecutionStatus):
* bindings/js/JSWorkletGlobalScopeBase.h:
* dom/ScriptExecutionContext.cpp:
(WebCore::ScriptExecutionContext::contextIdentifier const):
(WebCore::ScriptExecutionContext::removeFromContextsMap):
(WebCore::ScriptExecutionContext::~ScriptExecutionContext):
(WebCore::ScriptExecutionContext::jscScriptExecutionStatus const):
(WebCore::ScriptExecutionContext::resumeActiveDOMObjects):
(WebCore::ScriptExecutionContext::postTaskTo):
* dom/ScriptExecutionContext.h:

Source/WTF:

Add a DropLockScope to make it easier to drop a lock for a short
piece of code.  Also, instead of deleting int Locker constructor
we should just delete the underlying type of the
NoLockingNecessary enum.

* wtf/Locker.h:
(WTF::Locker::~Locker):
(WTF::Locker::unlockEarly):
(WTF::Locker::Locker):
(WTF::Locker::operator=):
(WTF::Locker::unlock):
(WTF::DropLockForScope::DropLockForScope):
(WTF::DropLockForScope::~DropLockForScope):

LayoutTests:

Add tests that check we don't run any tasks from JSC's event loop while contexts
are suspended/stopped. Also skip the WASM tests on Win because WASM doesn't work
there.

* fast/frames/detached-frame-wasm-resolve-expected.txt: Added.
* fast/frames/detached-frame-wasm-resolve.html: Added.
* fast/history/page-cache-active-finalization-registry-callback-expected.txt: Added.
* fast/history/page-cache-active-finalization-registry-callback.html: Added.
* fast/history/page-cache-wasm-promise-resolve-expected.txt: Added.
* fast/history/page-cache-wasm-promise-resolve.html: Added.
* platform/win/TestExpectations:


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@268271 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Author: [email protected]

LayoutTests/ChangeLog

@@ -1,3 +1,22 @@
+2020-10-09  Keith Miller  <[email protected]>
+
+        Finalizers shouldn't run if events can't fire
+        https://bugs.webkit.org/show_bug.cgi?id=214508
+
+        Reviewed by Ryosuke Niwa.
+
+        Add tests that check we don't run any tasks from JSC's event loop while contexts
+        are suspended/stopped. Also skip the WASM tests on Win because WASM doesn't work
+        there.
+
+        * fast/frames/detached-frame-wasm-resolve-expected.txt: Added.
+        * fast/frames/detached-frame-wasm-resolve.html: Added.
+        * fast/history/page-cache-active-finalization-registry-callback-expected.txt: Added.
+        * fast/history/page-cache-active-finalization-registry-callback.html: Added.
+        * fast/history/page-cache-wasm-promise-resolve-expected.txt: Added.
+        * fast/history/page-cache-wasm-promise-resolve.html: Added.
+        * platform/win/TestExpectations:
+
 2020-10-09  Andres Gonzalez  <[email protected]>
 
         Fix for accessibility tests keyevents-posted-for-increment-actions.html and keyevents-for-increment-actions-with-node-removal.html in isolated tree mode.

LayoutTests/platform/win/TestExpectations

@@ -700,6 +700,10 @@ fast/visual-viewport/zoomed-fixed-scroll-down-then-up.html [ Failure ]
 webkit.org/b/77568 fast/text/locale-shaping.html [ ImageOnlyFailure ]
 webkit.org/b/77568 fast/text/locale-shaping-complex.html [ ImageOnlyFailure ]
 
+# WebAssembly doesn't exist on Windows
+fast/frames/detached-frame-wasm-resolve.html [ Skip ]
+fast/history/page-cache-wasm-promise-resolve.html [ Skip ]
+
 ################################################################################
 ###########    End Missing Functionality Prevents Testing         ##############
 ################################################################################

Source/JavaScriptCore/API/JSAPIGlobalObject.cpp

@@ -45,6 +45,8 @@ const GlobalObjectMethodTable JSAPIGlobalObject::s_globalObjectMethodTable = {
     nullptr, // moduleLoaderEvaluate
     nullptr, // promiseRejectionTracker
     &reportUncaughtExceptionAtEventLoop,
+    &currentScriptExecutionOwner,
+    &scriptExecutionStatus,
     nullptr, // defaultLanguage
     nullptr, // compileStreaming
     nullptr, // instantiateStreaming

Source/JavaScriptCore/API/JSAPIGlobalObject.mm

@@ -65,6 +65,8 @@
     &moduleLoaderEvaluate, // moduleLoaderEvaluate
     nullptr, // promiseRejectionTracker
     &reportUncaughtExceptionAtEventLoop,
+    &currentScriptExecutionOwner,
+    &scriptExecutionStatus,
     nullptr, // defaultLanguage
     nullptr, // compileStreaming
     nullptr, // instantiateStreaming

Source/JavaScriptCore/ChangeLog

@@ -1,3 +1,41 @@
+2020-10-09  Keith Miller  <[email protected]>
+
+        Finalizers shouldn't run if events can't fire
+        https://bugs.webkit.org/show_bug.cgi?id=214508
+
+        Reviewed by Ryosuke Niwa.
+
+        This patch makes it so the DeferredWorkTimer won't run scheduled
+        tasks if those would not have run if they were scheduled in
+        WebCore. To do this there is now a concept of a
+        ScriptExecutionOwner. The ScriptExecutionOwner is almost always
+        the same as the global object of the pending task (referred to as
+        the ticket). The only exception to this is if the global object
+        is a JSDOMWindowBase, then the ScriptExecutionOwner is the
+        Document's JS wrapper. To tell the status of a
+        ScriptExecutionOwner, the DeferredWorkTimer calls a virtual
+        function on the global object of the ticket, for JSC-only this
+        just always returns Running. For WebCore, we ask the
+        ScriptExecutionContext associated with the ScriptExecutionOwner.
+
+        * API/JSAPIGlobalObject.cpp:
+        * API/JSAPIGlobalObject.mm:
+        * jsc.cpp:
+        * runtime/DeferredWorkTimer.cpp:
+        (JSC::DeferredWorkTimer::doWork):
+        (JSC::DeferredWorkTimer::addPendingWork):
+        (JSC::DeferredWorkTimer::hasDependancyInPendingWork):
+        (JSC::DeferredWorkTimer::didResumeScriptExecutionOwner):
+        * runtime/DeferredWorkTimer.h:
+        * runtime/JSFinalizationRegistry.cpp:
+        (JSC::JSFinalizationRegistry::create):
+        (JSC::JSFinalizationRegistry::finishCreation):
+        * runtime/JSFinalizationRegistry.h:
+        * runtime/JSGlobalObject.cpp:
+        * runtime/JSGlobalObject.h:
+        (JSC::JSGlobalObject::currentScriptExecutionOwner):
+        (JSC::JSGlobalObject::scriptExecutionStatus):
+
 2020-10-08  Yusuke Suzuki  <[email protected]>
 
         Unreviewed, reland r268170

Source/JavaScriptCore/jsc.cpp

@@ -720,6 +720,8 @@ const GlobalObjectMethodTable GlobalObject::s_globalObjectMethodTable = {
     nullptr, // moduleLoaderEvaluate
     nullptr, // promiseRejectionTracker
     &reportUncaughtExceptionAtEventLoop,
+    &currentScriptExecutionOwner,
+    &scriptExecutionStatus,
     nullptr, // defaultLanguage
     nullptr, // compileStreaming
     nullptr, // instantinateStreaming

Source/JavaScriptCore/runtime/DeferredWorkTimer.cpp

@@ -45,22 +45,37 @@ DeferredWorkTimer::DeferredWorkTimer(VM& vm)
 void DeferredWorkTimer::doWork(VM& vm)
 {
     ASSERT(vm.currentThreadIsHoldingAPILock());
-    m_taskLock.lock();
+    auto locker = holdLock(m_taskLock);
     cancelTimer();
-    if (!m_runTasks) {
-        m_taskLock.unlock();
+    if (!m_runTasks)
         return;
-    }
+
+    Vector<std::tuple<Ticket, Task>> suspendedTasks;
 
     while (!m_tasks.isEmpty()) {
         auto [ticket, task] = m_tasks.takeFirst();
+        auto globalObject = ticket->structure(vm)->globalObject();
         dataLogLnIf(DeferredWorkTimerInternal::verbose, "Doing work on: ", RawPointer(ticket));
 
-        // We may have already canceled these promises.
-        if (m_pendingTickets.contains(ticket)) {
-            // Allow tasks we run now to schedule work.
-            m_currentlyRunningTask = true;
-            m_taskLock.unlock(); 
+        auto pendingTicket = m_pendingTickets.find(ticket);
+        // We may have already canceled this task or its owner may have been canceled.
+        if (pendingTicket == m_pendingTickets.end())
+            continue;
+
+        switch (globalObject->globalObjectMethodTable()->scriptExecutionStatus(globalObject, pendingTicket->value.scriptExecutionOwner.get())) {
+        case ScriptExecutionStatus::Suspended:
+            suspendedTasks.append(std::make_tuple(ticket, WTFMove(task)));
+            continue;
+        case ScriptExecutionStatus::Stopped:
+            continue;
+        case ScriptExecutionStatus::Running:
+            break;
+        }
+
+        // Allow tasks we are about to run to schedule work.
+        m_currentlyRunningTask = true;
+        {
+            auto dropper = DropLockForScope(locker);
 
             // This is the start of a runloop turn, we can release any weakrefs here.
             vm.finalizeSynchronousJSExecution();
@@ -75,16 +90,17 @@ void DeferredWorkTimer::doWork(VM& vm)
 
             vm.drainMicrotasks();
             ASSERT(!vm.exceptionForInspection());
-
-            m_taskLock.lock();
-            m_currentlyRunningTask = false;
         }
+        m_currentlyRunningTask = false;
     }
 
-    if (m_pendingTickets.isEmpty() && m_shouldStopRunLoopWhenAllTicketsFinish)
-        RunLoop::current().stop();
+    while (!suspendedTasks.isEmpty())
+        m_tasks.prepend(suspendedTasks.takeLast());
 
-    m_taskLock.unlock();
+    if (m_pendingTickets.isEmpty() && m_shouldStopRunLoopWhenAllTicketsFinish) {
+        ASSERT(m_tasks.isEmpty());
+        RunLoop::current().stop();
+    }
 }
 
 void DeferredWorkTimer::runRunLoop()
@@ -102,14 +118,16 @@ void DeferredWorkTimer::addPendingWork(VM& vm, Ticket ticket, Vector<Strong<JSCe
     for (unsigned i = 0; i < dependencies.size(); ++i)
         ASSERT(dependencies[i].get() != ticket);
 
-    auto result = m_pendingTickets.add(ticket, Vector<Strong<JSCell>>());
-    if (result.isNewEntry) {
+    auto globalObject = ticket->globalObject();
+    auto result = m_pendingTickets.ensure(ticket, [&] {
         dataLogLnIf(DeferredWorkTimerInternal::verbose, "Adding new pending ticket: ", RawPointer(ticket));
+        JSObject* scriptExecutionOwner = globalObject->globalObjectMethodTable()->currentScriptExecutionOwner(globalObject);
         dependencies.append(Strong<JSCell>(vm, ticket));
-        result.iterator->value = WTFMove(dependencies);
-    } else {
+        return TicketData { WTFMove(dependencies), Strong<JSObject>(vm, scriptExecutionOwner) };
+    });
+    if (!result.isNewEntry) {
         dataLogLnIf(DeferredWorkTimerInternal::verbose, "Adding new dependencies for ticket: ", RawPointer(ticket));
-        result.iterator->value.appendVector(dependencies);
+        result.iterator->value.dependencies.appendVector(WTFMove(dependencies));
     }
 }
 
@@ -125,7 +143,7 @@ bool DeferredWorkTimer::hasDependancyInPendingWork(Ticket ticket, JSCell* depend
     ASSERT(m_pendingTickets.contains(ticket));
 
     auto result = m_pendingTickets.get(ticket);
-    return result.contains(dependency);
+    return result.dependencies.contains(dependency);
 }
 
 bool DeferredWorkTimer::cancelPendingWork(Ticket ticket)
@@ -147,4 +165,12 @@ void DeferredWorkTimer::scheduleWorkSoon(Ticket ticket, Task&& task)
         setTimeUntilFire(0_s);
 }
 
+void DeferredWorkTimer::didResumeScriptExecutionOwner()
+{
+    ASSERT(!m_currentlyRunningTask);
+    LockHolder locker(m_taskLock);
+    if (!isScheduled() && m_tasks.size())
+        setTimeUntilFire(0_s);
+}
+
 } // namespace JSC

Source/JavaScriptCore/runtime/DeferredWorkTimer.h

@@ -51,18 +51,19 @@ class JS_EXPORT_PRIVATE DeferredWorkTimer final : public JSRunLoopTimer {
     bool hasDependancyInPendingWork(Ticket, JSCell* dependency);
     bool cancelPendingWork(Ticket);
 
+    // If the script execution owner your ticket is associated with gets canceled
+    // the Task will not be called and will be deallocated. So it's important
+    // to make sure your memory ownership model won't leak memory when
+    // this occurs. The easiest way is to make sure everything is either owned
+    // by a GC'd value in dependencies or by the Task lambda.
     using Task = Function<void()>;
     void scheduleWorkSoon(Ticket, Task&&);
+    void didResumeScriptExecutionOwner();
 
     void stopRunningTasks() { m_runTasks = false; }
-
     void runRunLoop();
 
-    static Ref<DeferredWorkTimer> create(VM& vm)
-    {
-        return adoptRef(*new DeferredWorkTimer(vm));
-    }
-
+    static Ref<DeferredWorkTimer> create(VM& vm) { return adoptRef(*new DeferredWorkTimer(vm)); }
 private:
     DeferredWorkTimer(VM&);
 
@@ -71,7 +72,11 @@ class JS_EXPORT_PRIVATE DeferredWorkTimer final : public JSRunLoopTimer {
     bool m_shouldStopRunLoopWhenAllTicketsFinish { false };
     bool m_currentlyRunningTask { false };
     Deque<std::tuple<Ticket, Task>> m_tasks;
-    HashMap<Ticket, Vector<Strong<JSCell>>> m_pendingTickets;
+    struct TicketData {
+        Vector<Strong<JSCell>> dependencies;
+        Strong<JSObject> scriptExecutionOwner;
+    };
+    HashMap<Ticket, TicketData> m_pendingTickets;
 };
 
 } // namespace JSC

Source/JavaScriptCore/runtime/JSFinalizationRegistry.cpp

@@ -42,18 +42,22 @@ Structure* JSFinalizationRegistry::createStructure(VM& vm, JSGlobalObject* globa
 JSFinalizationRegistry* JSFinalizationRegistry::create(VM& vm, Structure* structure, JSObject* callback)
 {
     JSFinalizationRegistry* instance = new (NotNull, allocateCell<JSFinalizationRegistry>(vm.heap)) JSFinalizationRegistry(vm, structure);
-    instance->finishCreation(vm, callback);
+    instance->finishCreation(vm, structure->globalObject(), callback);
     return instance;
 }
 
-void JSFinalizationRegistry::finishCreation(VM& vm, JSObject* callback)
+void JSFinalizationRegistry::finishCreation(VM& vm, JSGlobalObject* globalObject, JSObject* callback)
 {
     Base::finishCreation(vm);
     ASSERT(callback->isCallable(vm));
     auto values = initialValues();
     for (unsigned index = 0; index < values.size(); ++index)
         Base::internalField(index).setWithoutWriteBarrier(values[index]);
     internalField(Field::Callback).setWithoutWriteBarrier(callback);
+
+    // Make sure we init the DOM wrapper for our document since it must be allocated before finalizeUnconditionally is called. finalizeUnconditionally,
+    // is called during the GC flip so no JS objects can be allocated there. This only works because we no longer weakly hold on to DOM wrappers.
+    globalObject->globalObjectMethodTable()->currentScriptExecutionOwner(globalObject);
 }
 
 void JSFinalizationRegistry::visitChildren(JSCell* cell, SlotVisitor& visitor)

Source/JavaScriptCore/runtime/JSFinalizationRegistry.h

@@ -89,7 +89,7 @@ class JSFinalizationRegistry final : public JSInternalFieldObjectImpl<1> {
     {
     }
 
-    JS_EXPORT_PRIVATE void finishCreation(VM&, JSObject* callback);
+    JS_EXPORT_PRIVATE void finishCreation(VM&, JSGlobalObject*, JSObject* callback);
 
     static String toStringName(const JSObject*, JSGlobalObject*);