Bug 259007 - REGRESSION(265197@main) VM::performOpportunisticallyScheduledTasks() should hold the JSLock when calling the sweeper.
Summary: REGRESSION(265197@main) VM::performOpportunisticallyScheduledTasks() should h...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2023-07-07 16:28 PDT by Mark Lam
Modified: 2023-07-07 17:51 PDT (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2023-07-07 16:28:13 PDT
This makes it consistent with the incremental sweeper that currently fires off a timer, which also hold the JSLock while sweeping.

Not holding the JSLock while sweeping has resulted in the RELEASE_ASSERT in the JSLock::DropAllLocks constructor failing.  Here's how it happens:

1. For a normal timer triggered sweep, we go through JSRunLoopTimer::timerDidFire() which acquires the JSLock, and holds it while sweeping.
2. For an idle triggered VM::performOpportunisticallyScheduledTasks(), it does NOT hold the JSLock while sweeping.
3. While sweeping, we may call back into some ObjC code that wraps a JSValue.
4. Releasing that JSValue for the sweep, requires acquiring the JSLock.
5. On releasing that JSLock, if it’s the outermost lock (i.e. not a re-entrant lock), then the JSLock unlocking code will call Heap::releaseDelayedReleasedObjects().
6. Heap::releaseDelayedReleasedObjects() uses DropAllLocks, which fails the RELEASE_ASSERT because we’re currently holding the JSLock and doing sweeping.
7. In contrast, for the normal timer triggered sweep, the outer most lock of the JSLock is JSRunLoopTimer::timerDidFire().  Hence, Heap::releaseDelayedReleasedObjects() won’t be called, and we won’t encounter this issue.

The fix is simply to acquire and hold the JSLock while sweeping in VM::performOpportunisticallyScheduledTasks().

rdar://111505837
Comment 1 Mark Lam 2023-07-07 16:33:49 PDT
Pull request: https://github.com/WebKit/WebKit/pull/15658
Comment 2 EWS 2023-07-07 17:51:40 PDT
Committed 265871@main (779576407245): <https://commits.webkit.org/265871@main>

Reviewed commits have been landed. Closing PR #15658 and removing active labels.