Fix trivial debug-only race-that-crashes in CallLinkStatus and explain why the remain...
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 1 May 2014 15:48:05 +0000 (15:48 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 1 May 2014 15:48:05 +0000 (15:48 +0000)
https://bugs.webkit.org/show_bug.cgi?id=132427

Reviewed by Mark Hahnenberg.

* bytecode/CallLinkStatus.cpp:
(JSC::CallLinkStatus::computeFor):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@168101 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/CallLinkStatus.cpp

index 1cc8274..7c34b88 100644 (file)
@@ -1,3 +1,13 @@
+2014-05-01  Filip Pizlo  <fpizlo@apple.com>
+
+        Fix trivial debug-only race-that-crashes in CallLinkStatus and explain why the remaining races are totally awesome
+        https://bugs.webkit.org/show_bug.cgi?id=132427
+
+        Reviewed by Mark Hahnenberg.
+
+        * bytecode/CallLinkStatus.cpp:
+        (JSC::CallLinkStatus::computeFor):
+
 2014-04-30  Simon Fraser  <simon.fraser@apple.com>
 
         Remove ENABLE_PLUGIN_PROXY_FOR_VIDEO
index 91c5ed8..0bab076 100644 (file)
@@ -149,11 +149,24 @@ CallLinkStatus CallLinkStatus::computeFor(
 #if ENABLE(JIT)
 CallLinkStatus CallLinkStatus::computeFor(const ConcurrentJITLocker&, CallLinkInfo& callLinkInfo)
 {
+    // Note that despite requiring that the locker is held, this code is racy with respect
+    // to the CallLinkInfo: it may get cleared while this code runs! This is because
+    // CallLinkInfo::unlink() may be called from a different CodeBlock than the one that owns
+    // the CallLinkInfo and currently we save space by not having CallLinkInfos know who owns
+    // them. So, there is no way for either the caller of CallLinkInfo::unlock() or unlock()
+    // itself to figure out which lock to lock.
+    //
+    // Fortunately, that doesn't matter. The only things we ask of CallLinkInfo - the slow
+    // path count, the stub, and the target - can all be asked racily. Stubs and targets can
+    // only be deleted at next GC, so if we load a non-null one, then it must contain data
+    // that is still marginally valid (i.e. the pointers ain't stale). This kind of raciness
+    // is probably OK for now.
+    
     if (callLinkInfo.slowPathCount >= Options::couldTakeSlowCaseMinimumCount())
         return takesSlowPath();
     
-    if (callLinkInfo.stub)
-        return CallLinkStatus(callLinkInfo.stub->executable(), callLinkInfo.stub->structure());
+    if (ClosureCallStubRoutine* stub = callLinkInfo.stub.get())
+        return CallLinkStatus(stub->executable(), stub->structure());
     
     JSFunction* target = callLinkInfo.lastSeenCallee.get();
     if (!target)