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..ffa66d8d9126 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,43 @@ 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, 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; + 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;