Title: [287421] trunk/Source/_javascript_Core
Revision
287421
Author
mark....@apple.com
Date
2021-12-23 21:58:47 -0800 (Thu, 23 Dec 2021)

Log Message

Make DeferredWorkTimer::addPendingWork() return a Ticket.
https://bugs.webkit.org/show_bug.cgi?id=234628
rdar://84260429

Reviewed by Yusuke Suzuki.

1. Make Ticket a unique token instead of the JSObject* target object.
   The Ticket is now a pointer to the TicketData in the pending work list.

2. Instead of taking a Ticket argument, DeferredWorkTimer::addPendingWork() now
   takes a JSObject* `target` argument explicitly, and returns the Ticket for the
   added TicketData instead.

   All the relevant DeferredWorkTimer APIS already take a Ticket as an argument.
   This ensures that addPendingWork() is called before we start doing work with
   these APIs (especially scheduleWorkSoon()).

3. Previously, addPendingWork() will only save one instance of TicketData for
   a given JSObject* key.  With this patch, we'll register a new TicketData
   instance for every call to addPendingWork(), and return a unique Ticket for it.

   This is needed because it may be possible for 2 different clients to call
   addPendingWork() and scheduleWorkSoon() with the same target JSObject* but with
   different sets of dependencies.

   Secondly, even is the both sets of dependencies are identical, a client may
   call addPendingWork() and scheduleWorkSoon() with the same JSObject* target
   more than once because it intended to schedule more than 1 task to run.

   Note that DeferredWorkTimer::doWork() consumes the corresponding TicketData
   (i.e. removes it from the m_pendingTickets list) for each task as it is run.
   To ensure that the dependencies for each task is protected, we'll either need
   to ref count the TicketData for the same target object (and hold off on removing
   it from the list), or we'll need to register a different TicketData instance
   for each task.  Ref counting can solve the second issue above, but does not
   solve the first.  So, this patch goes with the more generic solution to allow
   each task to have its own TicketData instance (and, its own unique Ticket).

4. Previously, if the client cancels pending work, we would remove the TicketData
   immediately from the m_pendingTickets list.  This opens up an opportunity for
   the same TicketData memory to be re-allocated by another client.  This, in turn,
   would make the Ticket token not unique and potentially allow a cancelled ticket
   to be reused before DeferredWorkTimer::doWork() is called.

   This patch changes DeferredWorkTimer::cancelPendingWork() to only clear the
   contents of the TicketData instead.  TicketData::scriptExecutionOwner being
   null is used as an indication that the ticket has been cancelled.  Since the
   TicketData itself is not "freed" yet, all TicketData will remain unique until
   DeferredWorkTimer::doWork().

   Consequently, DeferredWorkTimer::doWork() will now check for cancelled tickets
   and remove them from the m_pendingTickets list.

5. JSFinalizationRegistry was previously calling DeferredWorkTimer::hasPendingWork()
   to check if it has already scheduled a task, so as not to reschedule again until
   after the previously scheduled task has been run.  This does not play nice
   with the new Ticket API, because this hasPendingWork() check needs to be done
   before calling addPendingWork(), and hence, the Ticket is not available yet.

   Fortunately, JSFinalizationRegistry should know if it has already scheduled
   a task itself.  This patch adds a m_hasAlreadyScheduledWork flag to
   JSFinalizationRegistry that can be used for this check instead.

* jsc.cpp:
(JSC_DEFINE_HOST_FUNCTION):
* runtime/DeferredWorkTimer.cpp:
(JSC::DeferredWorkTimer::TicketData::TicketData):
(JSC::DeferredWorkTimer::TicketData::vm):
(JSC::DeferredWorkTimer::TicketData::cancel):
(JSC::DeferredWorkTimer::doWork):
(JSC::DeferredWorkTimer::addPendingWork):
(JSC::DeferredWorkTimer::hasPendingWork):
(JSC::DeferredWorkTimer::hasDependancyInPendingWork):
(JSC::DeferredWorkTimer::cancelPendingWork):
* runtime/DeferredWorkTimer.h:
(JSC::DeferredWorkTimer::TicketData::target):
* runtime/JSFinalizationRegistry.cpp:
(JSC::JSFinalizationRegistry::finalizeUnconditionally):
* runtime/JSFinalizationRegistry.h:
* wasm/WasmStreamingCompiler.cpp:
(JSC::Wasm::StreamingCompiler::StreamingCompiler):
(JSC::Wasm::StreamingCompiler::~StreamingCompiler):
(JSC::Wasm::StreamingCompiler::didComplete):
(JSC::Wasm::StreamingCompiler::fail):
(JSC::Wasm::StreamingCompiler::cancel):
* wasm/WasmStreamingCompiler.h:
* wasm/js/JSWebAssembly.cpp:
(JSC::JSWebAssembly::webAssemblyModuleValidateAsync):
(JSC::instantiate):
(JSC::compileAndInstantiate):
(JSC::JSWebAssembly::webAssemblyModuleInstantinateAsync):

Modified Paths

Diff

Modified: trunk/Source/_javascript_Core/ChangeLog (287420 => 287421)


--- trunk/Source/_javascript_Core/ChangeLog	2021-12-24 05:19:32 UTC (rev 287420)
+++ trunk/Source/_javascript_Core/ChangeLog	2021-12-24 05:58:47 UTC (rev 287421)
@@ -1,3 +1,97 @@
+2021-12-23  Mark Lam  <mark....@apple.com>
+
+        Make DeferredWorkTimer::addPendingWork() return a Ticket.
+        https://bugs.webkit.org/show_bug.cgi?id=234628
+        rdar://84260429
+
+        Reviewed by Yusuke Suzuki.
+
+        1. Make Ticket a unique token instead of the JSObject* target object.
+           The Ticket is now a pointer to the TicketData in the pending work list.
+
+        2. Instead of taking a Ticket argument, DeferredWorkTimer::addPendingWork() now
+           takes a JSObject* `target` argument explicitly, and returns the Ticket for the
+           added TicketData instead.
+
+           All the relevant DeferredWorkTimer APIS already take a Ticket as an argument.
+           This ensures that addPendingWork() is called before we start doing work with
+           these APIs (especially scheduleWorkSoon()).
+
+        3. Previously, addPendingWork() will only save one instance of TicketData for
+           a given JSObject* key.  With this patch, we'll register a new TicketData
+           instance for every call to addPendingWork(), and return a unique Ticket for it.
+
+           This is needed because it may be possible for 2 different clients to call
+           addPendingWork() and scheduleWorkSoon() with the same target JSObject* but with
+           different sets of dependencies.
+
+           Secondly, even is the both sets of dependencies are identical, a client may
+           call addPendingWork() and scheduleWorkSoon() with the same JSObject* target
+           more than once because it intended to schedule more than 1 task to run.
+
+           Note that DeferredWorkTimer::doWork() consumes the corresponding TicketData
+           (i.e. removes it from the m_pendingTickets list) for each task as it is run.
+           To ensure that the dependencies for each task is protected, we'll either need
+           to ref count the TicketData for the same target object (and hold off on removing
+           it from the list), or we'll need to register a different TicketData instance
+           for each task.  Ref counting can solve the second issue above, but does not
+           solve the first.  So, this patch goes with the more generic solution to allow
+           each task to have its own TicketData instance (and, its own unique Ticket).
+
+        4. Previously, if the client cancels pending work, we would remove the TicketData
+           immediately from the m_pendingTickets list.  This opens up an opportunity for
+           the same TicketData memory to be re-allocated by another client.  This, in turn,
+           would make the Ticket token not unique and potentially allow a cancelled ticket
+           to be reused before DeferredWorkTimer::doWork() is called.
+
+           This patch changes DeferredWorkTimer::cancelPendingWork() to only clear the
+           contents of the TicketData instead.  TicketData::scriptExecutionOwner being
+           null is used as an indication that the ticket has been cancelled.  Since the
+           TicketData itself is not "freed" yet, all TicketData will remain unique until
+           DeferredWorkTimer::doWork().
+
+           Consequently, DeferredWorkTimer::doWork() will now check for cancelled tickets
+           and remove them from the m_pendingTickets list.
+
+        5. JSFinalizationRegistry was previously calling DeferredWorkTimer::hasPendingWork()
+           to check if it has already scheduled a task, so as not to reschedule again until
+           after the previously scheduled task has been run.  This does not play nice
+           with the new Ticket API, because this hasPendingWork() check needs to be done
+           before calling addPendingWork(), and hence, the Ticket is not available yet.
+
+           Fortunately, JSFinalizationRegistry should know if it has already scheduled
+           a task itself.  This patch adds a m_hasAlreadyScheduledWork flag to 
+           JSFinalizationRegistry that can be used for this check instead.
+
+        * jsc.cpp:
+        (JSC_DEFINE_HOST_FUNCTION):
+        * runtime/DeferredWorkTimer.cpp:
+        (JSC::DeferredWorkTimer::TicketData::TicketData):
+        (JSC::DeferredWorkTimer::TicketData::vm):
+        (JSC::DeferredWorkTimer::TicketData::cancel):
+        (JSC::DeferredWorkTimer::doWork):
+        (JSC::DeferredWorkTimer::addPendingWork):
+        (JSC::DeferredWorkTimer::hasPendingWork):
+        (JSC::DeferredWorkTimer::hasDependancyInPendingWork):
+        (JSC::DeferredWorkTimer::cancelPendingWork):
+        * runtime/DeferredWorkTimer.h:
+        (JSC::DeferredWorkTimer::TicketData::target):
+        * runtime/JSFinalizationRegistry.cpp:
+        (JSC::JSFinalizationRegistry::finalizeUnconditionally):
+        * runtime/JSFinalizationRegistry.h:
+        * wasm/WasmStreamingCompiler.cpp:
+        (JSC::Wasm::StreamingCompiler::StreamingCompiler):
+        (JSC::Wasm::StreamingCompiler::~StreamingCompiler):
+        (JSC::Wasm::StreamingCompiler::didComplete):
+        (JSC::Wasm::StreamingCompiler::fail):
+        (JSC::Wasm::StreamingCompiler::cancel):
+        * wasm/WasmStreamingCompiler.h:
+        * wasm/js/JSWebAssembly.cpp:
+        (JSC::JSWebAssembly::webAssemblyModuleValidateAsync):
+        (JSC::instantiate):
+        (JSC::compileAndInstantiate):
+        (JSC::JSWebAssembly::webAssemblyModuleInstantinateAsync):
+
 2021-12-22  Saam Barati  <sbar...@apple.com>
 
         LLInt should loop OSR into BBQ and BBQ should loop OSR into OMG

Modified: trunk/Source/_javascript_Core/jsc.cpp (287420 => 287421)


--- trunk/Source/_javascript_Core/jsc.cpp	2021-12-24 05:19:32 UTC (rev 287420)
+++ trunk/Source/_javascript_Core/jsc.cpp	2021-12-24 05:58:47 UTC (rev 287421)
@@ -2504,8 +2504,8 @@
         return throwVMTypeError(globalObject, scope, "First argument is not a JS function"_s);
 
     // FIXME: We don't look at the timeout parameter because we don't have a schedule work later API.
-    vm.deferredWorkTimer->addPendingWork(vm, callback, { });
-    vm.deferredWorkTimer->scheduleWorkSoon(callback, [callback](DeferredWorkTimer::Ticket, DeferredWorkTimer::TicketData&&) {
+    auto ticket = vm.deferredWorkTimer->addPendingWork(vm, callback, { });
+    vm.deferredWorkTimer->scheduleWorkSoon(ticket, [callback](DeferredWorkTimer::Ticket) {
         JSGlobalObject* globalObject = callback->globalObject();
         MarkedArgumentBuffer args;
         call(globalObject, callback, jsUndefined(), args, "You shouldn't see this...");

Modified: trunk/Source/_javascript_Core/runtime/DeferredWorkTimer.cpp (287420 => 287421)


--- trunk/Source/_javascript_Core/runtime/DeferredWorkTimer.cpp	2021-12-24 05:19:32 UTC (rev 287420)
+++ trunk/Source/_javascript_Core/runtime/DeferredWorkTimer.cpp	2021-12-24 05:58:47 UTC (rev 287421)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2017-2020 Apple Inc. All rights reserved.
+ * Copyright (C) 2017-2021 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -37,6 +37,25 @@
 static const bool verbose = false;
 }
 
+inline DeferredWorkTimer::TicketData::TicketData(VM& vm, JSObject* scriptExecutionOwner, Vector<Strong<JSCell>>&& dependencies)
+    : dependencies(WTFMove(dependencies))
+    , scriptExecutionOwner(vm, scriptExecutionOwner)
+{
+}
+
+inline VM& DeferredWorkTimer::TicketData::vm()
+{
+    ASSERT(!isCancelled());
+    return target()->vm();
+}
+
+inline void DeferredWorkTimer::TicketData::cancel()
+{
+    scriptExecutionOwner.clear();
+    dependencies.clear();
+}
+
+
 DeferredWorkTimer::DeferredWorkTimer(VM& vm)
     : Base(vm)
 {
@@ -54,7 +73,6 @@
 
     while (!m_tasks.isEmpty()) {
         auto [ticket, task] = m_tasks.takeFirst();
-        auto globalObject = ticket->structure(vm)->globalObject();
         dataLogLnIf(DeferredWorkTimerInternal::verbose, "Doing work on: ", RawPointer(ticket));
 
         auto pendingTicket = m_pendingTickets.find(ticket);
@@ -61,8 +79,17 @@
         // We may have already canceled this task or its owner may have been canceled.
         if (pendingTicket == m_pendingTickets.end())
             continue;
+        ASSERT(ticket == pendingTicket->get());
 
-        switch (globalObject->globalObjectMethodTable()->scriptExecutionStatus(globalObject, pendingTicket->value.scriptExecutionOwner.get())) {
+        if (ticket->isCancelled()) {
+            m_pendingTickets.remove(pendingTicket);
+            continue;
+        }
+
+        // We shouldn't access the TicketData to get this globalObject until
+        // after we confirm that the ticket is still valid (which we did above).
+        auto globalObject = ticket->target()->structure(vm)->globalObject();
+        switch (globalObject->globalObjectMethodTable()->scriptExecutionStatus(globalObject, ticket->scriptExecutionOwner.get())) {
         case ScriptExecutionStatus::Suspended:
             suspendedTasks.append(std::make_tuple(ticket, WTFMove(task)));
             continue;
@@ -75,8 +102,7 @@
 
         // Remove ticket from m_pendingTickets since we are going to run it.
         // But we want to keep ticketData while running task since it ensures dependencies are strongly held.
-        auto ticketData = WTFMove(pendingTicket->value);
-        m_pendingTickets.remove(pendingTicket);
+        std::unique_ptr<TicketData> ticketData = m_pendingTickets.take(pendingTicket);
 
         // Allow tasks we are about to run to schedule work.
         m_currentlyRunningTask = true;
@@ -87,9 +113,9 @@
             vm.finalizeSynchronousJSExecution();
 
             auto scope = DECLARE_CATCH_SCOPE(vm);
-            task(ticket, WTFMove(ticketData));
+            task(ticket);
+            ticketData = nullptr;
             if (Exception* exception = scope.exception()) {
-                auto* globalObject = ticket->globalObject();
                 scope.clearException();
                 globalObject->globalObjectMethodTable()->reportUncaughtExceptionAtEventLoop(globalObject, exception);
             }
@@ -103,6 +129,13 @@
     while (!suspendedTasks.isEmpty())
         m_tasks.prepend(suspendedTasks.takeLast());
 
+    // It is theoretically possible that a client may cancel a pending ticket and
+    // never call scheduleWorkSoon() on it. As such, it would not be found when
+    // we iterated m_tasks above. We'll need to make sure to purge them here.
+    m_pendingTickets.removeIf([] (auto& ticket) {
+        return ticket->isCancelled();
+    });
+
     if (m_pendingTickets.isEmpty() && m_shouldStopRunLoopWhenAllTicketsFinish) {
         ASSERT(m_tasks.isEmpty());
         RunLoop::current().stop();
@@ -118,38 +151,42 @@
         RunLoop::run();
 }
 
-void DeferredWorkTimer::addPendingWork(VM& vm, Ticket ticket, Vector<Strong<JSCell>>&& dependencies)
+DeferredWorkTimer::Ticket DeferredWorkTimer::addPendingWork(VM& vm, JSObject* target, Vector<Strong<JSCell>>&& dependencies)
 {
     ASSERT(vm.currentThreadIsHoldingAPILock() || (Thread::mayBeGCThread() && vm.heap.worldIsStopped()));
     for (unsigned i = 0; i < dependencies.size(); ++i)
-        ASSERT(dependencies[i].get() != ticket);
+        ASSERT(dependencies[i].get() != target);
 
-    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));
-        return TicketData { WTFMove(dependencies), Strong<JSObject>(vm, scriptExecutionOwner) };
-    });
-    if (!result.isNewEntry) {
-        dataLogLnIf(DeferredWorkTimerInternal::verbose, "Adding new dependencies for ticket: ", RawPointer(ticket));
-        result.iterator->value.dependencies.appendVector(WTFMove(dependencies));
-    }
+    auto* globalObject = target->globalObject();
+    JSObject* scriptExecutionOwner = globalObject->globalObjectMethodTable()->currentScriptExecutionOwner(globalObject);
+    dependencies.append(Strong<JSCell>(vm, target));
+
+    auto ticketData = makeUnique<TicketData>(vm, scriptExecutionOwner, WTFMove(dependencies));
+    Ticket ticket = ticketData.get();
+
+    dataLogLnIf(DeferredWorkTimerInternal::verbose, "Adding new pending ticket: ", RawPointer(ticket));
+    auto result = m_pendingTickets.add(WTFMove(ticketData));
+    RELEASE_ASSERT(result.isNewEntry);
+
+    return ticket;
 }
 
 bool DeferredWorkTimer::hasPendingWork(Ticket ticket)
 {
+    auto result = m_pendingTickets.find(ticket);
+    if (result == m_pendingTickets.end() || ticket->isCancelled())
+        return false;
     ASSERT(ticket->vm().currentThreadIsHoldingAPILock() || (Thread::mayBeGCThread() && ticket->vm().heap.worldIsStopped()));
-    return m_pendingTickets.contains(ticket);
+    return true;
 }
 
 bool DeferredWorkTimer::hasDependancyInPendingWork(Ticket ticket, JSCell* dependency)
 {
+    auto result = m_pendingTickets.find(ticket);
+    if (result == m_pendingTickets.end() || ticket->isCancelled())
+        return false;
     ASSERT(ticket->vm().currentThreadIsHoldingAPILock() || (Thread::mayBeGCThread() && ticket->vm().heap.worldIsStopped()));
-    ASSERT(m_pendingTickets.contains(ticket));
-
-    auto result = m_pendingTickets.get(ticket);
-    return result.dependencies.contains(dependency);
+    return (*result)->dependencies.contains(dependency);
 }
 
 void DeferredWorkTimer::scheduleWorkSoon(Ticket ticket, Task&& task)
@@ -162,11 +199,15 @@
 
 bool DeferredWorkTimer::cancelPendingWork(Ticket ticket)
 {
-    ASSERT(ticket->vm().currentThreadIsHoldingAPILock() || (Thread::mayBeGCThread() && ticket->vm().heap.worldIsStopped()));
-    bool result = m_pendingTickets.remove(ticket);
+    ASSERT(m_pendingTickets.contains(ticket));
+    ASSERT(ticket->isCancelled() || ticket->vm().currentThreadIsHoldingAPILock() || (Thread::mayBeGCThread() && ticket->vm().heap.worldIsStopped()));
 
-    if (result)
+    bool result = false;
+    if (!ticket->isCancelled()) {
         dataLogLnIf(DeferredWorkTimerInternal::verbose, "Canceling ticket: ", RawPointer(ticket));
+        ticket->cancel();
+        result = true;
+    }
 
     return result;
 }

Modified: trunk/Source/_javascript_Core/runtime/DeferredWorkTimer.h (287420 => 287421)


--- trunk/Source/_javascript_Core/runtime/DeferredWorkTimer.h	2021-12-24 05:19:32 UTC (rev 287420)
+++ trunk/Source/_javascript_Core/runtime/DeferredWorkTimer.h	2021-12-24 05:58:47 UTC (rev 287421)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2017-2020 Apple Inc. All rights reserved.
+ * Copyright (C) 2017-2021 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -29,7 +29,7 @@
 #include "Strong.h"
 
 #include <wtf/Deque.h>
-#include <wtf/HashMap.h>
+#include <wtf/HashSet.h>
 #include <wtf/Lock.h>
 #include <wtf/Vector.h>
 
@@ -44,14 +44,26 @@
     using Base = JSRunLoopTimer;
 
     struct TicketData {
+    private:
+        WTF_MAKE_FAST_ALLOCATED;
+    public:
+        TicketData(VM&, JSObject* scriptExecutionOwner, Vector<Strong<JSCell>>&& dependencies);
+
+        VM& vm();
+        JSObject* target();
+
+        void cancel();
+        bool isCancelled() const { return !scriptExecutionOwner.get(); }
+
         Vector<Strong<JSCell>> dependencies;
         Strong<JSObject> scriptExecutionOwner;
     };
 
+    using Ticket = TicketData*;
+
     void doWork(VM&) final;
 
-    using Ticket = JSObject*;
-    void addPendingWork(VM&, Ticket, Vector<Strong<JSCell>>&& dependencies);
+    Ticket addPendingWork(VM&, JSObject* target, Vector<Strong<JSCell>>&& dependencies);
     bool hasPendingWork(Ticket);
     bool hasDependancyInPendingWork(Ticket, JSCell* dependency);
     bool cancelPendingWork(Ticket);
@@ -61,7 +73,7 @@
     // 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(Ticket, TicketData&&)>;
+    using Task = Function<void(Ticket)>;
     void scheduleWorkSoon(Ticket, Task&&);
     void didResumeScriptExecutionOwner();
 
@@ -77,7 +89,13 @@
     bool m_shouldStopRunLoopWhenAllTicketsFinish { false };
     bool m_currentlyRunningTask { false };
     Deque<std::tuple<Ticket, Task>> m_tasks WTF_GUARDED_BY_LOCK(m_taskLock);
-    HashMap<Ticket, TicketData> m_pendingTickets;
+    HashSet<std::unique_ptr<TicketData>> m_pendingTickets;
 };
 
+inline JSObject* DeferredWorkTimer::TicketData::target()
+{
+    ASSERT(!isCancelled());
+    return jsCast<JSObject*>(dependencies.last().get());
+}
+
 } // namespace JSC

Modified: trunk/Source/_javascript_Core/runtime/JSFinalizationRegistry.cpp (287420 => 287421)


--- trunk/Source/_javascript_Core/runtime/JSFinalizationRegistry.cpp	2021-12-24 05:19:32 UTC (rev 287420)
+++ trunk/Source/_javascript_Core/runtime/JSFinalizationRegistry.cpp	2021-12-24 05:58:47 UTC (rev 287421)
@@ -149,13 +149,15 @@
         return !bucket.value.size();
     });
 
-    if (!vm.deferredWorkTimer->hasPendingWork(this) && (readiedCell || deadCount(locker))) {
-        vm.deferredWorkTimer->addPendingWork(vm, this, { });
-        ASSERT(vm.deferredWorkTimer->hasPendingWork(this));
-        vm.deferredWorkTimer->scheduleWorkSoon(this, [this](DeferredWorkTimer::Ticket, DeferredWorkTimer::TicketData&&) {
+    if (!m_hasAlreadyScheduledWork && (readiedCell || deadCount(locker))) {
+        auto ticket = vm.deferredWorkTimer->addPendingWork(vm, this, { });
+        ASSERT(vm.deferredWorkTimer->hasPendingWork(ticket));
+        vm.deferredWorkTimer->scheduleWorkSoon(ticket, [this](DeferredWorkTimer::Ticket) {
             JSGlobalObject* globalObject = this->globalObject();
+            this->m_hasAlreadyScheduledWork = false;
             this->runFinalizationCleanup(globalObject);
         });
+        m_hasAlreadyScheduledWork = true;
     }
 }
 

Modified: trunk/Source/_javascript_Core/runtime/JSFinalizationRegistry.h (287420 => 287421)


--- trunk/Source/_javascript_Core/runtime/JSFinalizationRegistry.h	2021-12-24 05:19:32 UTC (rev 287420)
+++ trunk/Source/_javascript_Core/runtime/JSFinalizationRegistry.h	2021-12-24 05:58:47 UTC (rev 287421)
@@ -106,6 +106,7 @@
     // We use a separate list for no unregister values instead of a special key in the tables above because the HashMap has a tendency to reallocate under us when iterating...
     LiveRegistrations m_noUnregistrationLive;
     DeadRegistrations m_noUnregistrationDead;
+    bool m_hasAlreadyScheduledWork { false };
 };
 
 } // namespace JSC

Modified: trunk/Source/_javascript_Core/wasm/WasmStreamingCompiler.cpp (287420 => 287421)


--- trunk/Source/_javascript_Core/wasm/WasmStreamingCompiler.cpp	2021-12-24 05:19:32 UTC (rev 287420)
+++ trunk/Source/_javascript_Core/wasm/WasmStreamingCompiler.cpp	2021-12-24 05:58:47 UTC (rev 287421)
@@ -26,7 +26,6 @@
 #include "config.h"
 #include "WasmStreamingCompiler.h"
 
-#include "DeferredWorkTimer.h"
 #include "JSBigInt.h"
 #include "JSWebAssembly.h"
 #include "JSWebAssemblyCompileError.h"
@@ -45,7 +44,6 @@
 StreamingCompiler::StreamingCompiler(VM& vm, CompilerMode compilerMode, JSGlobalObject* globalObject, JSPromise* promise, JSObject* importObject)
     : m_vm(vm)
     , m_compilerMode(compilerMode)
-    , m_promise(promise)
     , m_info(Wasm::ModuleInformation::create())
     , m_parser(m_info.get(), *this)
 {
@@ -53,17 +51,17 @@
     dependencies.append(Strong<JSCell>(vm, globalObject));
     if (importObject)
         dependencies.append(Strong<JSCell>(vm, importObject));
-    vm.deferredWorkTimer->addPendingWork(vm, promise, WTFMove(dependencies));
-    ASSERT(vm.deferredWorkTimer->hasPendingWork(promise));
-    ASSERT(vm.deferredWorkTimer->hasDependancyInPendingWork(promise, globalObject));
-    ASSERT(!importObject || vm.deferredWorkTimer->hasDependancyInPendingWork(promise, importObject));
+    m_ticket = vm.deferredWorkTimer->addPendingWork(vm, promise, WTFMove(dependencies));
+    ASSERT(vm.deferredWorkTimer->hasPendingWork(m_ticket));
+    ASSERT(vm.deferredWorkTimer->hasDependancyInPendingWork(m_ticket, globalObject));
+    ASSERT(!importObject || vm.deferredWorkTimer->hasDependancyInPendingWork(m_ticket, importObject));
 }
 
 StreamingCompiler::~StreamingCompiler()
 {
-    if (m_promise) {
-        auto* promise = std::exchange(m_promise, nullptr);
-        m_vm.deferredWorkTimer->scheduleWorkSoon(promise, [](DeferredWorkTimer::Ticket, DeferredWorkTimer::TicketData&&) mutable { });
+    if (m_ticket) {
+        auto ticket = std::exchange(m_ticket, nullptr);
+        m_vm.deferredWorkTimer->scheduleWorkSoon(ticket, [](DeferredWorkTimer::Ticket) mutable { });
     }
 }
 
@@ -138,12 +136,12 @@
     };
 
     auto result = makeValidationResult(*m_plan);
-    auto* promise = std::exchange(m_promise, nullptr);
+    auto ticket = std::exchange(m_ticket, nullptr);
     switch (m_compilerMode) {
     case CompilerMode::Validation: {
-        m_vm.deferredWorkTimer->scheduleWorkSoon(promise, [result = WTFMove(result)](DeferredWorkTimer::Ticket ticket, DeferredWorkTimer::TicketData&& ticketData) mutable {
-            JSPromise* promise = jsCast<JSPromise*>(ticket);
-            JSGlobalObject* globalObject = jsCast<JSGlobalObject*>(ticketData.dependencies[0].get());
+        m_vm.deferredWorkTimer->scheduleWorkSoon(ticket, [result = WTFMove(result)](DeferredWorkTimer::Ticket ticket) mutable {
+            JSPromise* promise = jsCast<JSPromise*>(ticket->target());
+            JSGlobalObject* globalObject = jsCast<JSGlobalObject*>(ticket->dependencies[0].get());
             VM& vm = globalObject->vm();
             auto scope = DECLARE_THROW_SCOPE(vm);
 
@@ -160,10 +158,10 @@
     }
 
     case CompilerMode::FullCompile: {
-        m_vm.deferredWorkTimer->scheduleWorkSoon(promise, [result = WTFMove(result)](DeferredWorkTimer::Ticket ticket, DeferredWorkTimer::TicketData&& ticketData) mutable {
-            JSPromise* promise = jsCast<JSPromise*>(ticket);
-            JSGlobalObject* globalObject = jsCast<JSGlobalObject*>(ticketData.dependencies[0].get());
-            JSObject* importObject = jsCast<JSObject*>(ticketData.dependencies[1].get());
+        m_vm.deferredWorkTimer->scheduleWorkSoon(ticket, [result = WTFMove(result)](DeferredWorkTimer::Ticket ticket) mutable {
+            JSPromise* promise = jsCast<JSPromise*>(ticket->target());
+            JSGlobalObject* globalObject = jsCast<JSGlobalObject*>(ticket->dependencies[0].get());
+            JSObject* importObject = jsCast<JSObject*>(ticket->dependencies[1].get());
             VM& vm = globalObject->vm();
             auto scope = DECLARE_THROW_SCOPE(vm);
 
@@ -207,8 +205,14 @@
             return;
         m_eagerFailed = true;
     }
-    auto* promise = std::exchange(m_promise, nullptr);
-    m_vm.deferredWorkTimer->cancelPendingWork(promise);
+    auto ticket = std::exchange(m_ticket, nullptr);
+    JSPromise* promise = jsCast<JSPromise*>(ticket->target());
+    // The pending work TicketData was keeping the promise alive. We need to
+    // make sure it is reachable from the stack before we remove it from the
+    // pending work list. Note: m_ticket stores it as a PackedPtr, which is not
+    // scannable by the GC.
+    WTF::compilerFence();
+    m_vm.deferredWorkTimer->cancelPendingWork(ticket);
     promise->reject(globalObject, error);
 }
 
@@ -221,8 +225,8 @@
             return;
         m_eagerFailed = true;
     }
-    auto* promise = std::exchange(m_promise, nullptr);
-    m_vm.deferredWorkTimer->cancelPendingWork(promise);
+    auto ticket = std::exchange(m_ticket, nullptr);
+    m_vm.deferredWorkTimer->cancelPendingWork(ticket);
 }
 
 

Modified: trunk/Source/_javascript_Core/wasm/WasmStreamingCompiler.h (287420 => 287421)


--- trunk/Source/_javascript_Core/wasm/WasmStreamingCompiler.h	2021-12-24 05:19:32 UTC (rev 287420)
+++ trunk/Source/_javascript_Core/wasm/WasmStreamingCompiler.h	2021-12-24 05:58:47 UTC (rev 287421)
@@ -29,6 +29,7 @@
 
 #if ENABLE(WEBASSEMBLY)
 
+#include "DeferredWorkTimer.h"
 #include "JSCJSValue.h"
 
 namespace JSC {
@@ -72,7 +73,7 @@
     bool m_threadedCompilationStarted { false };
     Lock m_lock;
     unsigned m_remainingCompilationRequests { 0 };
-    JSPromise* m_promise; // Raw pointer, but held by DeferredWorkTimer.
+    DeferredWorkTimer::Ticket m_ticket;
     Ref<Wasm::ModuleInformation> m_info;
     StreamingParser m_parser;
     RefPtr<LLIntPlan> m_plan;

Modified: trunk/Source/_javascript_Core/wasm/js/JSWebAssembly.cpp (287420 => 287421)


--- trunk/Source/_javascript_Core/wasm/js/JSWebAssembly.cpp	2021-12-24 05:19:32 UTC (rev 287420)
+++ trunk/Source/_javascript_Core/wasm/js/JSWebAssembly.cpp	2021-12-24 05:58:47 UTC (rev 287421)
@@ -170,9 +170,9 @@
     Vector<Strong<JSCell>> dependencies;
     dependencies.append(Strong<JSCell>(vm, globalObject));
 
-    vm.deferredWorkTimer->addPendingWork(vm, promise, WTFMove(dependencies));
-    Wasm::Module::validateAsync(&vm.wasmContext, WTFMove(source), createSharedTask<Wasm::Module::CallbackType>([promise, globalObject, &vm] (Wasm::Module::ValidationResult&& result) mutable {
-        vm.deferredWorkTimer->scheduleWorkSoon(promise, [promise, globalObject, result = WTFMove(result), &vm](DeferredWorkTimer::Ticket, DeferredWorkTimer::TicketData&&) mutable {
+    auto ticket = vm.deferredWorkTimer->addPendingWork(vm, promise, WTFMove(dependencies));
+    Wasm::Module::validateAsync(&vm.wasmContext, WTFMove(source), createSharedTask<Wasm::Module::CallbackType>([ticket, promise, globalObject, &vm] (Wasm::Module::ValidationResult&& result) mutable {
+        vm.deferredWorkTimer->scheduleWorkSoon(ticket, [promise, globalObject, result = WTFMove(result), &vm](DeferredWorkTimer::Ticket) mutable {
             auto scope = DECLARE_THROW_SCOPE(vm);
             JSValue module = JSWebAssemblyModule::createStub(vm, globalObject, globalObject->webAssemblyModuleStructure(), WTFMove(result));
             if (UNLIKELY(scope.exception())) {
@@ -202,11 +202,11 @@
     dependencies.append(Strong<JSCell>(vm, promise));
     dependencies.append(Strong<JSCell>(vm, importObject));
 
-    vm.deferredWorkTimer->addPendingWork(vm, instance, WTFMove(dependencies));
+    auto ticket = vm.deferredWorkTimer->addPendingWork(vm, instance, WTFMove(dependencies));
     // Note: This completion task may or may not get called immediately.
-    module->module().compileAsync(&vm.wasmContext, instance->memoryMode(), createSharedTask<Wasm::CalleeGroup::CallbackType>([promise, instance, module, importObject, resolveKind, creationMode, &vm] (Ref<Wasm::CalleeGroup>&& refCalleeGroup) mutable {
+    module->module().compileAsync(&vm.wasmContext, instance->memoryMode(), createSharedTask<Wasm::CalleeGroup::CallbackType>([ticket, promise, instance, module, importObject, resolveKind, creationMode, &vm] (Ref<Wasm::CalleeGroup>&& refCalleeGroup) mutable {
         RefPtr<Wasm::CalleeGroup> calleeGroup = WTFMove(refCalleeGroup);
-        vm.deferredWorkTimer->scheduleWorkSoon(instance, [promise, instance, module, importObject, resolveKind, creationMode, &vm, calleeGroup = WTFMove(calleeGroup)](DeferredWorkTimer::Ticket, DeferredWorkTimer::TicketData&&) mutable {
+        vm.deferredWorkTimer->scheduleWorkSoon(ticket, [promise, instance, module, importObject, resolveKind, creationMode, &vm, calleeGroup = WTFMove(calleeGroup)](DeferredWorkTimer::Ticket) mutable {
             JSGlobalObject* globalObject = instance->globalObject();
             resolve(vm, globalObject, promise, instance, module, importObject, calleeGroup.releaseNonNull(), resolveKind, creationMode);
         });
@@ -227,9 +227,9 @@
     Vector<Strong<JSCell>> dependencies;
     dependencies.append(Strong<JSCell>(vm, importObject));
     dependencies.append(Strong<JSCell>(vm, moduleKeyCell));
-    vm.deferredWorkTimer->addPendingWork(vm, promise, WTFMove(dependencies));
-    Wasm::Module::validateAsync(&vm.wasmContext, WTFMove(source), createSharedTask<Wasm::Module::CallbackType>([promise, importObject, moduleKeyCell, globalObject, resolveKind, creationMode, &vm] (Wasm::Module::ValidationResult&& result) mutable {
-        vm.deferredWorkTimer->scheduleWorkSoon(promise, [promise, importObject, moduleKeyCell, globalObject, result = WTFMove(result), resolveKind, creationMode, &vm](DeferredWorkTimer::Ticket, DeferredWorkTimer::TicketData&&) mutable {
+    auto ticket = vm.deferredWorkTimer->addPendingWork(vm, promise, WTFMove(dependencies));
+    Wasm::Module::validateAsync(&vm.wasmContext, WTFMove(source), createSharedTask<Wasm::Module::CallbackType>([ticket, promise, importObject, moduleKeyCell, globalObject, resolveKind, creationMode, &vm] (Wasm::Module::ValidationResult&& result) mutable {
+        vm.deferredWorkTimer->scheduleWorkSoon(ticket, [promise, importObject, moduleKeyCell, globalObject, result = WTFMove(result), resolveKind, creationMode, &vm](DeferredWorkTimer::Ticket) mutable {
             auto scope = DECLARE_THROW_SCOPE(vm);
             JSWebAssemblyModule* module = JSWebAssemblyModule::createStub(vm, globalObject, globalObject->webAssemblyModuleStructure(), WTFMove(result));
             if (UNLIKELY(scope.exception())) {
@@ -271,9 +271,9 @@
     Vector<Strong<JSCell>> dependencies;
     dependencies.append(Strong<JSCell>(vm, importObject));
     dependencies.append(Strong<JSCell>(vm, globalObject));
-    vm.deferredWorkTimer->addPendingWork(vm, promise, WTFMove(dependencies));
-    Wasm::Module::validateAsync(&vm.wasmContext, WTFMove(source), createSharedTask<Wasm::Module::CallbackType>([promise, importObject, globalObject, &vm] (Wasm::Module::ValidationResult&& result) mutable {
-        vm.deferredWorkTimer->scheduleWorkSoon(promise, [promise, importObject, globalObject, result = WTFMove(result), &vm](DeferredWorkTimer::Ticket, DeferredWorkTimer::TicketData&&) mutable {
+    auto ticket = vm.deferredWorkTimer->addPendingWork(vm, promise, WTFMove(dependencies));
+    Wasm::Module::validateAsync(&vm.wasmContext, WTFMove(source), createSharedTask<Wasm::Module::CallbackType>([ticket, promise, importObject, globalObject, &vm] (Wasm::Module::ValidationResult&& result) mutable {
+        vm.deferredWorkTimer->scheduleWorkSoon(ticket, [promise, importObject, globalObject, result = WTFMove(result), &vm](DeferredWorkTimer::Ticket) mutable {
             auto scope = DECLARE_THROW_SCOPE(vm);
             JSWebAssemblyModule* module = JSWebAssemblyModule::createStub(vm, globalObject, globalObject->webAssemblyModuleStructure(), WTFMove(result));
             if (UNLIKELY(scope.exception())) {
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to