From c2b6fa71cc287586837d43c6b1474598c1468092 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 21 Aug 2026 23:12:13 +0000 Subject: [PATCH 1/2] DeferredWorkTimer: keep the embedder's tickets in m_pendingTickets With the embedder hooks installed, addPendingWork handed the ticket to the embedder instead of storing it. cancelPendingWork(VM&), which the end of every collection runs to cancel the tickets whose realm died, therefore walked an empty set, and a task the embedder had already queued for such a ticket still ran: on a destructed FinalizationRegistry, or it resolved a dead promise when an Atomics.waitAsync waiter was notified. The timer now owns the tickets in both modes. The hooks become notifications: onAddPendingWork(Ticket&) when a ticket becomes pending, onCancelPendingWork(Ticket&) exactly once when a pending ticket is cancelled. The embedder calls takePendingWork() before it runs a task, which removes the ticket from the set and says whether the task may run. Since doWork does not run in this mode, the cancellation paths remove the tickets from the set themselves. Every access to the set is under m_taskLock, including the four queries: a cancellation can come from a collector thread, or from another VM's thread through WaiterListManager (~SharedArrayBufferContents), while the embedder takes tickets on the VM's thread. A ticket gets one byte of embedder data, written from onAddPendingWork and read from onScheduleWorkSoon on whichever thread schedules the work. Bun records which of its event loops the work belongs to. WaiterList::takeFirst no longer asserts that an async waiter's ticket is alive. The end of a collection can now release the ticket before the realm's sweep unregisters the waiter; notifyWaiterImpl already handles that and schedules nothing. --- .../runtime/DeferredWorkTimer.cpp | 101 ++++++++++++------ .../runtime/DeferredWorkTimer.h | 34 +++++- .../runtime/WaiterListManager.h | 7 +- 3 files changed, 107 insertions(+), 35 deletions(-) diff --git a/Source/JavaScriptCore/runtime/DeferredWorkTimer.cpp b/Source/JavaScriptCore/runtime/DeferredWorkTimer.cpp index 1aded7422b2d..fef9256ee703 100644 --- a/Source/JavaScriptCore/runtime/DeferredWorkTimer.cpp +++ b/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::takePendingTicket(Ticket& ticket) +{ + Locker locker { m_taskLock }; + auto it = m_pendingTickets.find(&ticket); + if (it == m_pendingTickets.end()) + return nullptr; + RefPtr 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 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 : *globalObject->m_weakTickets) + cancelPendingWork(ticket.get()); + return; + } + + Locker locker { m_taskLock }; for (Ref ticket : *globalObject->m_weakTickets) { if (!ticket->isCancelled()) cancelPendingWork(ticket.get()); @@ -308,6 +336,23 @@ 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> 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)) { @@ -315,17 +360,9 @@ void DeferredWorkTimer::cancelPendingWork(VM& vm) // 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; diff --git a/Source/JavaScriptCore/runtime/DeferredWorkTimer.h b/Source/JavaScriptCore/runtime/DeferredWorkTimer.h index 03b2a45da15c..08108fb84d08 100644 --- a/Source/JavaScriptCore/runtime/DeferredWorkTimer.h +++ b/Source/JavaScriptCore/runtime/DeferredWorkTimer.h @@ -70,6 +70,11 @@ 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&& dependencies); @@ -77,6 +82,7 @@ class DeferredWorkTimer final : public JSRunLoopTimer { FixedVector m_dependencies; JSObject* m_scriptExecutionOwner { nullptr }; bool m_isCancelled { false }; + uint8_t m_embedderData { 0 }; }; using WeakTicket = ThreadSafeWeakPtr; @@ -113,17 +119,41 @@ class DeferredWorkTimer final : public JSRunLoopTimer { static Ref create(VM& vm) { return adoptRef(*new DeferredWorkTimer(vm)); } - WTF::Function&&, 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. + WTF::Function onAddPendingWork; WTF::Function&&, Task&&)> onScheduleWorkSoon; WTF::Function onCancelPendingWork; + JS_EXPORT_PRIVATE bool takePendingWork(Ticket&); + private: JS_EXPORT_PRIVATE DeferredWorkTimer(VM&); - Lock m_taskLock; + RefPtr takePendingTicket(Ticket&); + + mutable Lock m_taskLock; bool m_runTasks { true }; bool m_shouldStopRunLoopWhenAllTicketsFinish { false }; bool m_currentlyRunningTask { false }; Deque, 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> m_pendingTickets; }; diff --git a/Source/JavaScriptCore/runtime/WaiterListManager.h b/Source/JavaScriptCore/runtime/WaiterListManager.h index 4c239885df37..5665f4fd5772 100644 --- a/Source/JavaScriptCore/runtime/WaiterListManager.h +++ b/Source/JavaScriptCore/runtime/WaiterListManager.h @@ -153,9 +153,12 @@ class WaiterList : public ThreadSafeRefCounted { Ref 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 protectedWaiter = Ref { waiter }; removeWithUpdate(waiter); return protectedWaiter; From b5c71022f1d69ba6e592460168699230c09a99bb Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Mon, 24 Aug 2026 14:23:32 +0000 Subject: [PATCH 2/2] DeferredWorkTimer: document the collector-phase constraint on onCancelPendingWork --- Source/JavaScriptCore/runtime/DeferredWorkTimer.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Source/JavaScriptCore/runtime/DeferredWorkTimer.h b/Source/JavaScriptCore/runtime/DeferredWorkTimer.h index 08108fb84d08..ffa66d8d9126 100644 --- a/Source/JavaScriptCore/runtime/DeferredWorkTimer.h +++ b/Source/JavaScriptCore/runtime/DeferredWorkTimer.h @@ -135,7 +135,9 @@ class DeferredWorkTimer final : public JSRunLoopTimer { // 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. + // 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 onAddPendingWork; WTF::Function&&, Task&&)> onScheduleWorkSoon; WTF::Function onCancelPendingWork;