Changeset 40208 in webkit for trunk/JavaScriptCore/runtime

Timestamp:

Jan 23, 2009, 7:03:25 PM (16 years ago)

Author:

[email protected]

Message:

2009-01-23 Gavin Barraclough <​[email protected]>

Reviewed by Geoff Garen.

Fix for <rdar://problem/6126212>
Ensure that callbacks out from the JSC interface are only allowed
to return in reverse-chronological order to that in which they were
made. If we allow earlier callbacks to return first, then this may
result in setions of the RegisterFile in use by another thread
being trampled.

See uber-comment in JSLock.h for details.

• runtime/JSLock.cpp: (JSC::JSLock::DropAllLocks::DropAllLocks): (JSC::JSLock::DropAllLocks::~DropAllLocks):

File:

• trunk/JavaScriptCore/runtime/JSLock.cpp (modified) (3 diffs)

trunk/JavaScriptCore/runtime/JSLock.cpp

@@ -120 +120 @@
 }
 
+// This is fairly nasty.  We allow multiple threads to run on the same
+// context, and we do not require any locking semantics in doing so -
+// clients of the API may simply use the context from multiple threads
+// concurently, and assume this will work.  In order to make this work,
+// We lock the context when a thread enters, and unlock it when it leaves.
+// However we do not only unlock when the thread returns from its
+// entry point (evaluate script or call function), we also unlock the
+// context if the thread leaves JSC by making a call out to an external
+// function through a callback.
+//
+// All threads using the context share the same JS stack (the RegisterFile).
+// Whenever a thread calls into JSC it starts using the RegisterFile from the
+// previous 'high water mark' - the maximum point the stack has ever grown to
+// (returned by RegisterFile::end()).  So if a first thread calls out to a
+// callback, and a second thread enters JSC, then also exits by calling out
+// to a callback, we can be left with stackframes from both threads in the
+// RegisterFile.  As such, a problem may occur should the first thread's
+// callback complete first, and attempt to return to JSC.  Were we to allow
+// this to happen, and were its stack to grow further, then it may potentially
+// write over the second thread's call frames.
+//
+// In avoid JS stack corruption we enforce a policy of only ever allowing two
+// threads to use a JS context concurrently, and only allowing the second of
+// these threads to execute until it has completed and fully returned from its
+// outermost call into JSC.  We enforce this policy using 'lockDropDepth'.  The
+// first time a thread exits it will call DropAllLocks - which will do as expected
+// and drop locks allowing another thread to enter.  Should another thread, or the
+// same thread again, enter JSC (through evaluate script or call function), and exit
+// again through a callback, then the locks will not be dropped when DropAllLocks
+// is called (since lockDropDepth is non-zero).  Since this thread is still holding
+// the locks, only it will re able to re-enter JSC (either be returning from the
+// callback, or by re-entering through another call to evaulate script or call
+// function).
+//
+// This policy is slightly more restricive than it needs to be for correctness -
+// we could validly allow futher entries into JSC from other threads, we only
+// need ensure that callbacks return in the reverse chronological order of the
+// order in which they were made - though implementing the less restrictive policy
+// would likely increase complexity and overhead.
+//
+static unsigned lockDropDepth = 0;
+
 JSLock::DropAllLocks::DropAllLocks(ExecState* exec)
     : m_lockingForReal(exec->globalData().isSharedInstance)
 {
     pthread_once(&createJSLockCountOnce, createJSLockCount);
+
+    if (lockDropDepth++) {
+        m_lockCount = 0;
+        return;
+    }
 
     m_lockCount = JSLock::lockCount();
@@ -135 +182 @@
     pthread_once(&createJSLockCountOnce, createJSLockCount);
 
+    if (lockDropDepth++) {
+        m_lockCount = 0;
+        return;
+    }
+
     // It is necessary to drop even "unreal" locks, because having a non-zero lock count
     // will prevent a real lock from being taken.
@@ -147 +199 @@
     for (intptr_t i = 0; i < m_lockCount; i++)
         JSLock::lock(m_lockingForReal);
+
+    --lockDropDepth;
 }