REGRESSION(265596@main) VM::performOpportunisticallyScheduledTasks() …

…should hold the JSLock when calling the sweeper.

https://bugs.webkit.org/show_bug.cgi?id=259007
rdar://111505837

Reviewed by Justin Michaud.

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().

* Source/JavaScriptCore/runtime/JSLock.cpp:
(JSC::JSLock::DropAllLocks::DropAllLocks):
* Source/JavaScriptCore/runtime/VM.cpp:
(JSC::VM::performOpportunisticallyScheduledTasks):

Canonical link: https://commits.webkit.org/265871@main

Source/JavaScriptCore/runtime/JSLock.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2005-2022 Apple Inc. All rights reserved.
+ * Copyright (C) 2005-2023 Apple Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ -288,7 +288,12 @@ JSLock::DropAllLocks::DropAllLocks(VM* vm)
 {
     if (!m_vm)
         return;
-    RELEASE_ASSERT(!m_vm->apiLock().currentThreadIsHoldingLock() || !m_vm->isCollectorBusyOnCurrentThread());
+
+    // Contrary to intuition, DropAllLocks does not require that we are actually holding
+    // the JSLock before getting here. Its goal is to release the lock if it is held. So,
+    // if the lock isn't already held, there's nothing to do, and that's fine.
+    // See https://bugs.webkit.org/show_bug.cgi?id=139654#c11.
+    RELEASE_ASSERT(!m_vm->apiLock().currentThreadIsHoldingLock() || !m_vm->isCollectorBusyOnCurrentThread(), m_vm->apiLock().currentThreadIsHoldingLock(), m_vm->isCollectorBusyOnCurrentThread());
     m_droppedLockCount = m_vm->apiLock().dropAllLocks(this);
 }
 

Source/JavaScriptCore/runtime/VM.cpp

@@ -1629,13 +1629,8 @@ void VM::removeDebugger(Debugger& debugger)
 
 void VM::performOpportunisticallyScheduledTasks(MonotonicTime deadline)
 {
-    bool hasPendingWork;
-    {
-        JSLockHolder locker { *this };
-        hasPendingWork = deferredWorkTimer->hasAnyPendingWork();
-    }
-
-    if (!hasPendingWork)
+    JSLockHolder locker { *this };
+    if (!deferredWorkTimer->hasAnyPendingWork())
         heap.sweeper().doWorkUntil(*this, deadline);
 }