DeferredWorkTimer: keep the embedder's tickets in m_pendingTickets so the end of a collection cancels them

Source/JavaScriptCore/runtime/DeferredWorkTimer.cpp

@@ -205,18 +205,42 @@ DeferredWorkTimer::WeakTicket DeferredWorkTimer::addPendingWork(WorkType type, V
     WeakTicket weakTicket { ticket.get() };
 
     dataLogLnIf(DeferredWorkTimerInternal::verbose, "Adding new pending ticket: ", RawPointer(ticket.ptr()));
-    if (onAddPendingWork) {
-        onAddPendingWork(WTF::move(ticket), type);
-    } else {
-        auto result = m_pendingTickets.add(WTF::move(ticket));
+    {
+        Locker locker { m_taskLock };
+        auto result = m_pendingTickets.add(ticket.copyRef());
         RELEASE_ASSERT(result.isNewEntry);
     }
+    if (onAddPendingWork)
+        onAddPendingWork(ticket.get());
 
     return weakTicket;
 }
 
+RefPtr<DeferredWorkTimer::Ticket> DeferredWorkTimer::takePendingTicket(Ticket& ticket)
+{
+    Locker locker { m_taskLock };
+    auto it = m_pendingTickets.find(&ticket);
+    if (it == m_pendingTickets.end())
+        return nullptr;
+    RefPtr<Ticket> taken = it->ptr();
+    m_pendingTickets.remove(it);
+    return taken;
+}
+
+bool DeferredWorkTimer::takePendingWork(Ticket& ticket)
+{
+    ASSERT(onScheduleWorkSoon);
+    if (!takePendingTicket(ticket))
+        return false;
+    // Cancelling removes a ticket from the set before it flags it, so a ticket that was still in the set is live.
+    ASSERT(!ticket.isCancelled());
+    ASSERT(ticket.vm().currentThreadIsHoldingAPILock());
+    return true;
+}
+
 bool DeferredWorkTimer::hasPendingWork(Ticket& ticket)
 {
+    Locker locker { m_taskLock };
     auto result = m_pendingTickets.find(&ticket);
     if (result == m_pendingTickets.end() || ticket.isCancelled())
         return false;
@@ -226,6 +250,7 @@ bool DeferredWorkTimer::hasPendingWork(Ticket& ticket)
 
 bool DeferredWorkTimer::hasDependencyInPendingWork(Ticket& ticket, JSCell* dependency)
 {
+    Locker locker { m_taskLock };
     auto result = m_pendingTickets.find(&ticket);
     if (result == m_pendingTickets.end() || ticket.isCancelled())
         return false;
@@ -254,35 +279,38 @@ bool DeferredWorkTimer::scheduleWorkSoonIfActive(const WeakTicket& weakTicket, T
 // https://bugs.webkit.org/show_bug.cgi?id=276538
 bool DeferredWorkTimer::cancelPendingWork(Ticket& ticket)
 {
-#if ASSERT_ENABLED
-    if (!onCancelPendingWork) {
-        ASSERT(m_pendingTickets.contains(&ticket));
-    }
-#endif
-
+    ASSERT(onCancelPendingWork || m_pendingTickets.contains(&ticket));
     ASSERT(ticket.isCancelled() || ticket.vm().currentThreadIsHoldingAPILock() || (Thread::mayBeGCThread() && ticket.vm().heap.worldIsStopped()));
 
-    bool result = false;
-    if (!ticket.isCancelled()) {
-        ticket.cancel();
-        result = true;
+    if (ticket.isCancelled())
+        return false;
 
-        // Script execution context is cleared in ->cancel().
-        // But, onCancelPendingWork may dereference the ticket.
-        // So your WTF::Function has to be careful about the ticket.
-        if (onCancelPendingWork) {
-            onCancelPendingWork(ticket);
-        }
+    if (onCancelPendingWork) {
+        // doWork does not run, so nothing would purge the ticket from the set later: it leaves here. A ticket
+        // the embedder has already taken is no longer pending. It is only flagged, and the embedder is not told.
+        RefPtr<Ticket> pending = takePendingTicket(ticket);
+        ticket.cancel();
+        if (pending)
+            onCancelPendingWork(*pending);
+        return true;
     }
 
-    return result;
+    ticket.cancel();
+    return true;
 }
 
 void DeferredWorkTimer::cancelPendingWorkSafe(JSGlobalObject* globalObject)
 {
-    Locker locker { m_taskLock };
-
     dataLogLnIf(DeferredWorkTimerInternal::verbose, "Cancel pending work for globalObject ", RawPointer(globalObject));
+
+    if (onCancelPendingWork) {
+        // cancelPendingWork takes m_taskLock itself, and there is no timer to fire.
+        for (Ref<Ticket> ticket : *globalObject->m_weakTickets)
+            cancelPendingWork(ticket.get());
+        return;
+    }
+
+    Locker locker { m_taskLock };
     for (Ref<Ticket> ticket : *globalObject->m_weakTickets) {
         if (!ticket->isCancelled())
             cancelPendingWork(ticket.get());
@@ -308,24 +336,33 @@ void DeferredWorkTimer::cancelPendingWork(VM& vm)
         return isTargetGlobalObjectLive && vm.heap.isMarked(ticket->scriptExecutionOwner());
     };
 
+    if (onCancelPendingWork) {
+        // Same as below, except that the dead tickets leave the set here instead of in doWork. Whatever the
+        // embedder has queued for them is dropped when it calls takePendingWork.
+        Vector<Ref<Ticket>> deadTickets;
+        m_pendingTickets.removeIf([&](auto& ticket) {
+            if (!ticket->isCancelled() && isValid(ticket))
+                return false;
+            ticket->cancelAndClear();
+            deadTickets.append(ticket.copyRef());
+            return true;
+        });
+        locker.unlockEarly();
+        for (auto& ticket : deadTickets)
+            onCancelPendingWork(ticket.get());
+        return;
+    }
+
     bool needToFire = false;
     for (auto& ticket : m_pendingTickets) {
         if (ticket->isCancelled() || !isValid(ticket)) {
             // At this point, no one can visit or need the dependencies.
             // So, they are safe to clear here for better debugging and testing.
             ticket->cancelAndClear();
             needToFire = true;
-
-            if (onCancelPendingWork) {
-                onCancelPendingWork(ticket.get());
-            }
         }
     }
 
-    if (onCancelPendingWork) {
-        return;
-    }
-
     // GC can be triggered before an invalid and scheduled ticket is fired. In that case,
     // we also need to remove the corresponding pending task. Since doWork handles all cases
     // for removal, we should let it handle that for consistency.
@@ -344,12 +381,14 @@ void DeferredWorkTimer::didResumeScriptExecutionOwner()
 bool DeferredWorkTimer::hasAnyPendingWork() const
 {
     ASSERT(m_apiLock->vm()->currentThreadIsHoldingAPILock() || (Thread::mayBeGCThread() && m_apiLock->vm()->heap.worldIsStopped()));
+    Locker locker { m_taskLock };
     return !m_pendingTickets.isEmpty();
 }
 
 bool DeferredWorkTimer::hasImminentlyScheduledWork() const
 {
     ASSERT(m_apiLock->vm()->currentThreadIsHoldingAPILock() || (Thread::mayBeGCThread() && m_apiLock->vm()->heap.worldIsStopped()));
+    Locker locker { m_taskLock };
     for (auto& ticket : m_pendingTickets) {
         if (ticket->isCancelled())
             continue;

Source/JavaScriptCore/runtime/DeferredWorkTimer.h

@@ -70,13 +70,19 @@ class DeferredWorkTimer final : public JSRunLoopTimer {
         inline void cancelAndClear();
         bool isCancelled() const { return m_isCancelled; }
 
+        // A byte for the embedder, written from onAddPendingWork (before the ticket is handed
+        // out) and read back from onScheduleWorkSoon, on whichever thread schedules the work.
+        uint8_t embedderData() const { return m_embedderData; }
+        void setEmbedderData(uint8_t data) { m_embedderData = data; }
+
     private:
         inline Ticket(WorkType, JSObject* scriptExecutionOwner, Vector<JSCell*>&& dependencies);
 
         WorkType m_type;
         FixedVector<JSCell*> m_dependencies;
         JSObject* m_scriptExecutionOwner { nullptr };
         bool m_isCancelled { false };
+        uint8_t m_embedderData { 0 };
     };
 
     using WeakTicket = ThreadSafeWeakPtr<Ticket>;
@@ -113,17 +119,43 @@ class DeferredWorkTimer final : public JSRunLoopTimer {
 
     static Ref<DeferredWorkTimer> create(VM& vm) { return adoptRef(*new DeferredWorkTimer(vm)); }
 
-    WTF::Function<void(Ref<Ticket>&&, WorkType)> onAddPendingWork;
+    // An embedder that runs the tasks from its own event loop installs all three hooks.
+    // The timer still owns the tickets: m_pendingTickets holds every ticket from
+    // addPendingWork until it is cancelled or taken, so cancelPendingWork(VM&) cancels
+    // the tickets of dead realms at the end of every collection exactly as it does when
+    // doWork runs the tasks.
+    //
+    //   onAddPendingWork     a ticket became pending. Called on the VM's thread, or on a
+    //                        collector thread with the world stopped. The place to set
+    //                        the ticket's embedderData().
+    //   onScheduleWorkSoon   run this task for this ticket from the event loop. Before
+    //                        running it, call takePendingWork(); run the task only if it
+    //                        returns true. May be called from any thread.
+    //   onCancelPendingWork  a pending ticket was cancelled. Called once per ticket, and
+    //                        never for a ticket that takePendingWork() returned true for.
+    //                        The ticket's dependencies may already be cleared; its type()
+    //                        and embedderData() are still valid. May be called from any
+    //                        thread, including a collector thread at the end of a collection
+    //                        with the world stopped: it must not run script, allocate JS
+    //                        cells, or take the API lock.
+    WTF::Function<void(Ticket&)> onAddPendingWork;
     WTF::Function<void(Ref<Ticket>&&, Task&&)> onScheduleWorkSoon;
     WTF::Function<void(Ticket&)> onCancelPendingWork;
+    JS_EXPORT_PRIVATE bool takePendingWork(Ticket&);
+
 private:
     JS_EXPORT_PRIVATE DeferredWorkTimer(VM&);
 
-    Lock m_taskLock;
+    RefPtr<Ticket> takePendingTicket(Ticket&);
+
+    mutable Lock m_taskLock;
     bool m_runTasks { true };
     bool m_shouldStopRunLoopWhenAllTicketsFinish { false };
     bool m_currentlyRunningTask { false };
     Deque<std::tuple<Ref<Ticket>, Task>> m_tasks WTF_GUARDED_BY_LOCK(m_taskLock);
+    // With the hooks installed, every access to this set happens under m_taskLock:
+    // a cancellation can come from a collector thread, or from another VM's thread
+    // through WaiterListManager, while the embedder takes tickets on the VM's thread.
     UncheckedKeyHashSet<Ref<Ticket>> m_pendingTickets;
 };
 

Source/JavaScriptCore/runtime/WaiterListManager.h

@@ -153,9 +153,12 @@ class WaiterList : public ThreadSafeRefCounted<WaiterList> {
     Ref<Waiter> takeFirst(const AbstractLocker&)
     {
         // `takeFisrt` is used to consume a waiter (either notify, timeout, or remove).
-        // So, the waiter must not be removed and belong to this list.
+        // So, the waiter must not be removed and belong to this list. An async waiter's
+        // ticket may be gone already: the end of a collection cancels and releases the
+        // tickets of a dead realm before the realm's sweep unregisters its waiters.
+        // notifyWaiterImpl then finds nothing to schedule.
         Waiter& waiter = *m_waiters.begin();
-        ASSERT((!waiter.isAsync() || waiter.ticket(NoLockingNecessary)) && waiter.vm() && waiter.isOnList());
+        ASSERT(waiter.vm() && waiter.isOnList());
         Ref<Waiter> protectedWaiter = Ref { waiter };
         removeWithUpdate(waiter);
         return protectedWaiter;