Changeset 163700 in webkit

Timestamp:

Feb 8, 2014 12:27:13 AM (10 years ago)

Author:

mark.lam@apple.com

Message:

JSLock should not "restore" VM stack values if it did not re-grab locks.
<​https://webkit.org/b/128447>

Reviewed by Geoffrey Garen.

In the existing code, if DropAllLocks is instantiate with DontAlwaysDropLocks
in a thread that does not own the JSLock, then a bug will manifest where:

1. The DropAllLocks constructor will save the VM's stackPointerAtEntry, lastStackTop, and reservedZoneSize even though it will not drop the JSLock.
2. The DropAllLocks destructor will restore those 3 values to the VM even though the JSLock will not grab its internal lock.

The former only causes busy work but does not impact correctness. The latter
however, will corrupt those 3 VM values which belong to the thread that
actually owns the JSLock.

The fix is to only save the values when the JSLock will actually drop its
internal lock, and only restore the values if it did re-grab the internal lock.

• runtime/JSLock.cpp:

(JSC::JSLock::dropAllLocks):
(JSC::JSLock::dropAllLocksUnconditionally):
(JSC::JSLock::grabAllLocks):
(JSC::JSLock::DropAllLocks::DropAllLocks):

• Moved the saving of VM stack values to dropAllLocks() and dropAllLocksUnconditionally().

(JSC::JSLock::DropAllLocks::~DropAllLocks):

• Moved the restoring of VM stack values to grabAllLocks().

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• runtime/JSLock.cpp (modified) (6 diffs)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2014-02-07  Mark Lam  <mark.lam@apple.com>
+
+        JSLock should not "restore" VM stack values if it did not re-grab locks.
+        <https://webkit.org/b/128447>
+
+        Reviewed by Geoffrey Garen.
+
+        In the existing code, if DropAllLocks is instantiate with DontAlwaysDropLocks
+        in a thread that does not own the JSLock, then a bug will manifest where:
+
+        1. The DropAllLocks constructor will save the VM's stackPointerAtEntry,
+           lastStackTop, and reservedZoneSize even though it will not drop the JSLock.
+        2. The DropAllLocks destructor will restore those 3 values to the VM even
+           though the JSLock will not grab its internal lock.
+
+        The former only causes busy work but does not impact correctness. The latter
+        however, will corrupt those 3 VM values which belong to the thread that
+        actually owns the JSLock.
+
+        The fix is to only save the values when the JSLock will actually drop its
+        internal lock, and only restore the values if it did re-grab the internal lock.
+
+        * runtime/JSLock.cpp:
+        (JSC::JSLock::dropAllLocks):
+        (JSC::JSLock::dropAllLocksUnconditionally):
+        (JSC::JSLock::grabAllLocks):
+        (JSC::JSLock::DropAllLocks::DropAllLocks):
+        - Moved the saving of VM stack values to dropAllLocks() and
+          dropAllLocksUnconditionally().
+        (JSC::JSLock::DropAllLocks::~DropAllLocks):
+        - Moved the restoring of VM stack values to grabAllLocks().
+
 2014-02-07  Filip Pizlo  <fpizlo@apple.com>
 

trunk/Source/JavaScriptCore/runtime/JSLock.cpp

@@ -222 +222 @@
         return 0;
 
+    WTFThreadData& threadData = wtfThreadData();
+    threadData.setSavedStackPointerAtVMEntry(m_vm->stackPointerAtVMEntry);
+    threadData.setSavedLastStackTop(m_vm->lastStackTop());
+    threadData.setSavedReservedZoneSize(m_vm->reservedZoneSize());
+
     // m_lockDropDepth is only incremented if any locks were dropped.
     ++m_lockDropDepth;
@@ -242 +247 @@
         return 0;
 
+    WTFThreadData& threadData = wtfThreadData();
+    threadData.setSavedStackPointerAtVMEntry(m_vm->stackPointerAtVMEntry);
+    threadData.setSavedLastStackTop(m_vm->lastStackTop());
+    threadData.setSavedReservedZoneSize(m_vm->reservedZoneSize());
+
     // m_lockDropDepth is only incremented if any locks were dropped.
     ++m_lockDropDepth;
@@ -277 +287 @@
     m_lockCount = lockCount;
     --m_lockDropDepth;
+
+    WTFThreadData& threadData = wtfThreadData();
+    m_vm->stackPointerAtVMEntry = threadData.savedStackPointerAtVMEntry();
+    m_vm->setLastStackTop(threadData.savedLastStackTop());
+    m_vm->updateStackLimitWithReservedZoneSize(threadData.savedReservedZoneSize());
 }
 
@@ -287 +302 @@
     SpinLock& spinLock = m_vm->apiLock().m_spinLock;
     SpinLockHolder holder(&spinLock);
-
-    WTFThreadData& threadData = wtfThreadData();
-    
-    threadData.setSavedStackPointerAtVMEntry(m_vm->stackPointerAtVMEntry);
-    threadData.setSavedLastStackTop(m_vm->lastStackTop());
-    threadData.setSavedReservedZoneSize(m_vm->reservedZoneSize());
 
     if (alwaysDropLocks)
@@ -309 +318 @@
     SpinLockHolder holder(&spinLock);
 
-    WTFThreadData& threadData = wtfThreadData();
-    
-    threadData.setSavedStackPointerAtVMEntry(m_vm->stackPointerAtVMEntry);
-    threadData.setSavedLastStackTop(m_vm->lastStackTop());
-    threadData.setSavedReservedZoneSize(m_vm->reservedZoneSize());
-
     if (alwaysDropLocks)
         m_lockCount = m_vm->apiLock().dropAllLocksUnconditionally(spinLock);
@@ -328 +331 @@
     SpinLockHolder holder(&spinLock);
     m_vm->apiLock().grabAllLocks(m_lockCount, spinLock);
-
-    WTFThreadData& threadData = wtfThreadData();
-
-    m_vm->stackPointerAtVMEntry = threadData.savedStackPointerAtVMEntry();
-    m_vm->setLastStackTop(threadData.savedLastStackTop());
-    m_vm->updateStackLimitWithReservedZoneSize(threadData.savedReservedZoneSize());
 }