Changeset 180992 in webkit


Ignore:
Timestamp:
Mar 3, 2015 9:33:37 PM (9 years ago)
Author:
msaboff@apple.com
Message:

DelayedReleaseScope drops locks during GC which can cause a thread switch and code reentry
https://bugs.webkit.org/show_bug.cgi?id=141275

Reviewed by Geoffrey Garen.

The original issue is that the CodeCache uses an unsafe method to add new UnlinkedCodeBlocks.
It basically adds a null UnlinkedCodeBlock if there isn't a cached entry and then later
updates the null entry to the result of the compilation. If during that compilation and
related processing we need to garbage collect, the DelayedReleaseScope would drop locks
possibly allowing another thread to try to get the same source out of the CodeCache.
This second thread would find the null entry and crash. The fix is to move the processing of
DelayedReleaseScope to when we drop locks and not drop locks during GC. That was done in
the original patch with the new function releaseDelayedReleasedObjects().

Updated releaseDelayedReleasedObjects() so that objects are released with all locks
dropped. Now its processing follows these steps

Increment recursion counter and do recursion check and exit if recursing
While there are objects to release

ASSERT that lock is held by current thread
Take all items from delayed release Vector and put into temporary Vector
Release API lock
Release and clear items from temporary vector
Reaquire API lock

This meets the requirement that we release while the API lock is released and it is
safer processing of the delayed release Vector.

Added new regression test to testapi.

Also added comment describing how recursion into releaseDelayedReleasedObjects() is
prevented.

  • API/tests/Regress141275.h: Added.
  • API/tests/Regress141275.mm: Added.

(+[JSTEvaluatorTask evaluatorTaskWithEvaluateBlock:completionHandler:]):
(-[JSTEvaluator init]):
(-[JSTEvaluator initWithScript:]):
(-[JSTEvaluator _accessPendingTasksWithBlock:]):
(-[JSTEvaluator insertSignPostWithCompletion:]):
(-[JSTEvaluator evaluateScript:completion:]):
(-[JSTEvaluator evaluateBlock:completion:]):
(-[JSTEvaluator waitForTasksDoneAndReportResults]):
(JSTRunLoopSourceScheduleCallBack):
(
JSTRunLoopSourcePerformCallBack):
(JSTRunLoopSourceCancelCallBack):
(-[JSTEvaluator _jsThreadMain]):
(-[JSTEvaluator _sourceScheduledOnRunLoop:]):
(-[JSTEvaluator _setupEvaluatorThreadContextIfNeeded]):
(-[JSTEvaluator _callCompletionHandler:ifNeededWithError:]):
(-[JSTEvaluator _sourcePerform]):
(-[JSTEvaluator _sourceCanceledOnRunLoop:]):
(runRegress141275):

  • API/tests/testapi.mm:

(testObjectiveCAPI):

(JSC::Heap::releaseDelayedReleasedObjects):

  • runtime/JSLock.cpp:

(JSC::JSLock::unlock):

Location:
trunk/Source/JavaScriptCore
Files:
2 added
5 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/JavaScriptCore/API/tests/testapi.mm

    r180591 r180992  
    2929#import "DateTests.h"
    3030#import "JSExportTests.h"
     31#import "Regress141275.h"
    3132#import "Regress141809.h"
    3233
     
    487488{
    488489    NSLog(@"Testing Objective-C API");
    489 
    490490    @autoreleasepool {
    491491        JSVirtualMachine* vm = [[JSVirtualMachine alloc] init];
     
    13871387    runDateTests();
    13881388    runJSExportTests();
     1389    runRegress141275();
    13891390    runRegress141809();
    13901391}
  • trunk/Source/JavaScriptCore/ChangeLog

    r180989 r180992  
     12015-03-03  Michael Saboff  <msaboff@apple.com>
     2
     3        DelayedReleaseScope drops locks during GC which can cause a thread switch and code reentry
     4        https://bugs.webkit.org/show_bug.cgi?id=141275
     5
     6        Reviewed by Geoffrey Garen.
     7
     8        The original issue is that the CodeCache uses an unsafe method to add new UnlinkedCodeBlocks.
     9        It basically adds a null UnlinkedCodeBlock if there isn't a cached entry and then later
     10        updates the null entry to the result of the compilation.  If during that compilation and
     11        related processing we need to garbage collect, the DelayedReleaseScope would drop locks
     12        possibly allowing another thread to try to get the same source out of the CodeCache.
     13        This second thread would find the null entry and crash.  The fix is to move the processing of
     14        DelayedReleaseScope to when we drop locks and not drop locks during GC.  That was done in
     15        the original patch with the new function releaseDelayedReleasedObjects().
     16
     17        Updated releaseDelayedReleasedObjects() so that objects are released with all locks
     18        dropped.  Now its processing follows these steps
     19            Increment recursion counter and do recursion check and exit if recursing
     20            While there are objects to release
     21                ASSERT that lock is held by current thread
     22                Take all items from delayed release Vector and put into temporary Vector
     23                Release API lock
     24                Release and clear items from temporary vector
     25                Reaquire API lock
     26        This meets the requirement that we release while the API lock is released and it is
     27        safer processing of the delayed release Vector.
     28
     29        Added new regression test to testapi.
     30
     31        Also added comment describing how recursion into releaseDelayedReleasedObjects() is
     32        prevented.
     33
     34        * API/tests/Regress141275.h: Added.
     35        * API/tests/Regress141275.mm: Added.
     36        (+[JSTEvaluatorTask evaluatorTaskWithEvaluateBlock:completionHandler:]):
     37        (-[JSTEvaluator init]):
     38        (-[JSTEvaluator initWithScript:]):
     39        (-[JSTEvaluator _accessPendingTasksWithBlock:]):
     40        (-[JSTEvaluator insertSignPostWithCompletion:]):
     41        (-[JSTEvaluator evaluateScript:completion:]):
     42        (-[JSTEvaluator evaluateBlock:completion:]):
     43        (-[JSTEvaluator waitForTasksDoneAndReportResults]):
     44        (__JSTRunLoopSourceScheduleCallBack):
     45        (__JSTRunLoopSourcePerformCallBack):
     46        (__JSTRunLoopSourceCancelCallBack):
     47        (-[JSTEvaluator _jsThreadMain]):
     48        (-[JSTEvaluator _sourceScheduledOnRunLoop:]):
     49        (-[JSTEvaluator _setupEvaluatorThreadContextIfNeeded]):
     50        (-[JSTEvaluator _callCompletionHandler:ifNeededWithError:]):
     51        (-[JSTEvaluator _sourcePerform]):
     52        (-[JSTEvaluator _sourceCanceledOnRunLoop:]):
     53        (runRegress141275):
     54        * API/tests/testapi.mm:
     55        (testObjectiveCAPI):
     56        * JavaScriptCore.xcodeproj/project.pbxproj:
     57        * heap/Heap.cpp:
     58        (JSC::Heap::releaseDelayedReleasedObjects):
     59        * runtime/JSLock.cpp:
     60        (JSC::JSLock::unlock):
     61
    1622015-03-03  Filip Pizlo  <fpizlo@apple.com>
    263
  • trunk/Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj

    r180691 r180992  
    935935                6553A33117A1F1EE008CF6F3 /* CommonSlowPathsExceptions.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 6553A32F17A1F1EE008CF6F3 /* CommonSlowPathsExceptions.cpp */; };
    936936                6553A33217A1F1EE008CF6F3 /* CommonSlowPathsExceptions.h in Headers */ = {isa = PBXBuildFile; fileRef = 6553A33017A1F1EE008CF6F3 /* CommonSlowPathsExceptions.h */; };
     937                65570F5A1AA4C3EA009B3C23 /* Regress141275.mm in Sources */ = {isa = PBXBuildFile; fileRef = 65570F591AA4C00A009B3C23 /* Regress141275.mm */; };
    937938                655EB29B10CE2581001A990E /* NodesCodegen.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 655EB29A10CE2581001A990E /* NodesCodegen.cpp */; };
    938939                657CF45819BF6662004ACBF2 /* JSCallee.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 657CF45619BF6662004ACBF2 /* JSCallee.cpp */; };
     
    25982599                6553A32F17A1F1EE008CF6F3 /* CommonSlowPathsExceptions.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = CommonSlowPathsExceptions.cpp; sourceTree = "<group>"; };
    25992600                6553A33017A1F1EE008CF6F3 /* CommonSlowPathsExceptions.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = CommonSlowPathsExceptions.h; sourceTree = "<group>"; };
     2601                65570F581AA4C00A009B3C23 /* Regress141275.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = Regress141275.h; path = API/tests/Regress141275.h; sourceTree = "<group>"; };
     2602                65570F591AA4C00A009B3C23 /* Regress141275.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; name = Regress141275.mm; path = API/tests/Regress141275.mm; sourceTree = "<group>"; };
    26002603                655EB29A10CE2581001A990E /* NodesCodegen.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = NodesCodegen.cpp; sourceTree = "<group>"; };
    26012604                6560A4CF04B3B3E7008AE952 /* CoreFoundation.framework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.framework; name = CoreFoundation.framework; path = /System/Library/Frameworks/CoreFoundation.framework; sourceTree = "<absolute>"; };
     
    37293732                                C2181FC018A948FB0025A235 /* JSExportTests.h */,
    37303733                                C2181FC118A948FB0025A235 /* JSExportTests.mm */,
     3734                                65570F581AA4C00A009B3C23 /* Regress141275.h */,
     3735                                65570F591AA4C00A009B3C23 /* Regress141275.mm */,
    37313736                                FEB51F6A1A97B688001F921C /* Regress141809.h */,
    37323737                                FEB51F6B1A97B688001F921C /* Regress141809.mm */,
     
    67586763                        buildActionMask = 2147483647;
    67596764                        files = (
     6765                                65570F5A1AA4C3EA009B3C23 /* Regress141275.mm in Sources */,
    67606766                                C29ECB031804D0ED00D2CBB4 /* CurrentThisInsideBlockGetterTest.mm in Sources */,
    67616767                                FEB51F6C1A97B688001F921C /* Regress141809.mm in Sources */,
  • trunk/Source/JavaScriptCore/heap/Heap.cpp

    r180690 r180992  
    369369{
    370370#if USE(CF)
     371    // We need to guard against the case that releasing an object can create more objects due to the
     372    // release calling into JS. When those JS call(s) exit and all locks are being dropped we end up
     373    // back here and could try to recursively release objects. We guard that with a recursive entry
     374    // count. Only the initial call will release objects, recursive calls simple return and let the
     375    // the initial call to the function take care of any objects created during release time.
     376    // This also means that we need to loop until there are no objects in m_delayedReleaseObjects
     377    // and use a temp Vector for the actual releasing.
    371378    if (!m_delayedReleaseRecursionCount++) {
    372379        while (!m_delayedReleaseObjects.isEmpty()) {
    373             RetainPtr<CFTypeRef> objectToRelease = m_delayedReleaseObjects.takeLast();
    374             objectToRelease.clear();
     380            ASSERT(m_vm->currentThreadIsHoldingAPILock());
     381
     382            Vector<RetainPtr<CFTypeRef>> objectsToRelease = WTF::move(m_delayedReleaseObjects);
     383
     384            {
     385                // We need to drop locks before calling out to arbitrary code.
     386                JSLock::DropAllLocks dropAllLocks(m_vm);
     387
     388                objectsToRelease.clear();
     389            }
    375390        }
    376391    }
  • trunk/Source/JavaScriptCore/runtime/JSLock.cpp

    r180690 r180992  
    158158    ASSERT(m_lockCount >= unlockCount);
    159159
     160    // Maintain m_lockCount while calling willReleaseLock() so that its callees know that
     161    // they still have the lock.
     162    if (unlockCount == m_lockCount)
     163        willReleaseLock();
     164
    160165    m_lockCount -= unlockCount;
    161166
    162167    if (!m_lockCount) {
    163         willReleaseLock();
    164168
    165169        if (!m_hasExclusiveThread) {
Note: See TracChangeset for help on using the changeset viewer.