Changeset 233184 in webkit


Ignore:
Timestamp:
Jun 25, 2018 4:56:35 PM (6 years ago)
Author:
sbarati@apple.com
Message:

JSImmutableButterfly can't be allocated from a subspace with HeapCell::Kind::Auxiliary
​https://bugs.webkit.org/show_bug.cgi?id=186878
<rdar://problem/40568659>

Reviewed by Mark Lam.

Source/JavaScriptCore:

This patch fixes a bug in our JSImmutableButterfly implementation uncovered by
our stress GC bots. Before this patch, JSImmutableButterfly was allocated
with HeapCell::Kind::Auxiliary. This is wrong. Things that are JSCells must be
allocated from HeapCell::Kind::JSCell. The way this broke on the stress GC
bots is that our conservative marking won't do cell marking for things that
are Auxiliary. This means that if the stack is the only thing pointing to a
JSImmutableButterfly when a GC took place, that JSImmutableButterfly would
not be visited. This patch fixes this bug. This patch also extends our conservative
marking to understand that there may be interior pointers to things that are HeapCell::Kind::JSCell.

  • bytecompiler/NodesCodegen.cpp:

(JSC::ArrayNode::emitBytecode):

  • heap/HeapUtil.h:

(JSC::HeapUtil::findGCObjectPointersForMarking):

  • runtime/JSImmutableButterfly.h:

(JSC::JSImmutableButterfly::subspaceFor):

LayoutTests:

Make these test not susceptible to conservative scan leaks by ensuring at least
one object gets collected when we allocate many of them. Before, these were just
testing that a fixed number of objects were collected.

  • editing/selection/navigation-clears-editor-state-expected.txt:
  • editing/selection/navigation-clears-editor-state.html:
  • fast/dom/reference-cycle-leaks.html:
  • fast/misc/resources/test-observegc.js:
  • fast/misc/test-observegc-expected.txt:
  • platform/mac-wk2/plugins/refcount-leaks-expected.txt:
  • plugins/refcount-leaks-expected.txt:
  • plugins/refcount-leaks.html:
Location:
trunk
Files:
13 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r233180 r233184  
     12018-06-25  Saam Barati  <sbarati@apple.com>
     2
     3        JSImmutableButterfly can't be allocated from a subspace with HeapCell::Kind::Auxiliary
     4        https://bugs.webkit.org/show_bug.cgi?id=186878
     5        <rdar://problem/40568659>
     6
     7        Reviewed by Mark Lam.
     8
     9        Make these test not susceptible to conservative scan leaks by ensuring at least
     10        one object gets collected when we allocate many of them. Before, these were just
     11        testing that a fixed number of objects were collected.
     12
     13        * editing/selection/navigation-clears-editor-state-expected.txt:
     14        * editing/selection/navigation-clears-editor-state.html:
     15        * fast/dom/reference-cycle-leaks.html:
     16        * fast/misc/resources/test-observegc.js:
     17        * fast/misc/test-observegc-expected.txt:
     18        * platform/mac-wk2/plugins/refcount-leaks-expected.txt:
     19        * plugins/refcount-leaks-expected.txt:
     20        * plugins/refcount-leaks.html:
     21
    1222018-06-25  John Wilander  <wilander@apple.com>
    223

  • trunk/LayoutTests/editing/selection/navigation-clears-editor-state-expected.txt

    r232434 r233184  
    44
    55
    6 iframe = appendIframe()
    7 PASS internals.numberOfLiveDocuments() is initialDocumentCount + 1
    8 iframe.src = "resources/select-iframe-focusin-document-crash-frame.html"
    9 PASS internals.numberOfLiveDocuments() is initialDocumentCount + 1
     6PASS freed more than 60 documents
    107PASS successfullyParsed is true
    118

  • trunk/LayoutTests/editing/selection/navigation-clears-editor-state.html

    r232434 r233184  
    3636    initialDocumentCount = internals.numberOfLiveDocuments();
    3737
    38     evalAndLog('iframe = appendIframe()');
     38    let frames = [];
     39    const count = 200;
     40    for (let i = 0; i < count; ++i)
     41        frames.push(appendIframe());
    3942
    4043    await wait(0); // Make sure the transient document created by inserting an iframe is removed.
    4144    GCController.collect();
    4245
    43     shouldBe('internals.numberOfLiveDocuments()', 'initialDocumentCount + 1');
    44     setEditorStates(iframe);
     46    for (let frame of frames)
     47        setEditorStates(frame);
    4548
    4649    await wait(0); // Wait for UI update timer to fire.
    4750
    48     evalAndLog('iframe.src = "resources/select-iframe-focusin-document-crash-frame.html"');
    49     iframe.onload = () => {
    50         GCController.collect();
    51         shouldBe('internals.numberOfLiveDocuments()', 'initialDocumentCount + 1');
    52         finishJSTest();               
     51    let resolved = 0;
     52    for (let frame of frames) {
     53        frame.src = "resources/select-iframe-focusin-document-crash-frame.html";
     54        frame.onload = () => {
     55            ++resolved;
     56            if (resolved !== count)
     57                return;
     58            let before = internals.numberOfLiveDocuments();
     59            GCController.collect();
     60            let after = internals.numberOfLiveDocuments();
     61            let delta = before - after;
     62            if (delta > 0.3 * count)
     63                debug("PASS freed more than 60 documents");
     64            else
     65                debug("FAIL freed fewer than 60 documents");
     66            finishJSTest();               
     67        };
    5368    }
    5469}
    … …  
    5873} else {
    5974    if (window.testRunner)
    60         setTimeout(() => testRunner.notifyDone(), 3000);
     75        setTimeout(() => testRunner.notifyDone(), 5000);
    6176    // Clear out any lingering documents from the previous tests.
    6277    GCController.collect();

  • trunk/LayoutTests/fast/dom/reference-cycle-leaks.html

    r225729 r233184  
    88{
    99    // Bump this number as high as we need to, to get reproducible results.
    10     const repetitions = 20;
     10    const repetitions = 40;
    1111
    1212    gc();

  • trunk/LayoutTests/fast/misc/resources/test-observegc.js

    r201425 r233184  
    11description("Ensures that window.internals.observegc works as expected");
    22
    3 var testObject = { testProperty : "testValue" };
     3var observers = [];
     4for (let i = 0; i < 1000; ++i) {
     5    let testObject = { testProperty : "testValue" };
     6    let observer = internals.observeGC(testObject);
     7    observers.push(observer);
     8    testObject = null;
     9}
    410
    5 var observer = internals.observeGC(testObject);
    6 testObject = null;
    711gc();
    812
    9 shouldBe('observer.wasCollected', 'true');
     13var anyCollected = false;
     14for (let observer of observers) {
     15    if (observer.wasCollected)
     16        anyCollected = true;
     17}
     18
     19shouldBe('anyCollected', 'true');

  • trunk/LayoutTests/fast/misc/test-observegc-expected.txt

    r201425 r233184  
    44
    55
    6 PASS observer.wasCollected is true
     6PASS anyCollected is true
    77PASS successfullyParsed is true
    88

  • trunk/LayoutTests/platform/mac-wk2/plugins/refcount-leaks-expected.txt

    r111070 r233184  
     1PASS countAfterCreate === countOrig + 50 is true
     2FAIL countAfterGC < countAfterCreate should be true. Was false.
     3PASS refAfterGet === refOrig + 50 is true
     4FAIL refAfterGetGC < refAfterGet should be true. Was false.
     5PASS refAfterPass === refBeforePass + 50 is true
     6FAIL refAfterPassAndGC < refAfterPass should be true. Was false.
    17Test that we can get an NPObject returned through a method on an NPAPI Object.
    2 Prints "SUCCESS" on success, "FAILURE" on failure. 
    38
    4 --- num test objects:
    5 countAfterCreate == countOrig + 3? PASS
    6 countOrig == countAfterGC? FAIL
    7 
    8 --- refcount on plug.testObject:
    9 originally: 2
    10 after GC: 5
    11 after passing: 8
    12 FAILURE

  • trunk/LayoutTests/plugins/refcount-leaks-expected.txt

    r72302 r233184  
     1PASS countAfterCreate === countOrig + 50 is true
     2PASS countAfterGC < countAfterCreate is true
     3PASS refAfterGet === refOrig + 50 is true
     4PASS refAfterGetGC < refAfterGet is true
     5PASS refAfterPass === refBeforePass + 50 is true
     6PASS refAfterPassAndGC < refAfterPass is true
    17Test that we can get an NPObject returned through a method on an NPAPI Object.
    2 Prints "SUCCESS" on success, "FAILURE" on failure. 
    38
    4 --- num test objects:
    5 countAfterCreate == countOrig + 3? PASS
    6 countOrig == countAfterGC? PASS
    7 
    8 --- refcount on plug.testObject:
    9 originally: 2
    10 after GC: 2
    11 after passing: 2
    12 SUCCESS

  • trunk/LayoutTests/plugins/refcount-leaks.html

    r120417 r233184  
     1<head>
     2<script src="../resources/js-test-pre.js"></script>
     3</head>
     4
    15<script>
    2   function noop(x) {
    3   }
     6function noop(x) {
     7}
    48
    5   function doGC() {
    6     if (window.GCController) {
    7       // GC twice to make sure everything is cleaned up.
    8       for (var i = 0; i < 2; i++) {
    9         window.GCController.collect();
    10       }
     9function doGC() {
     10    // GC twice to make sure everything is cleaned up.
     11    for (var i = 0; i < 2; i++) {
     12        gc();
    1113    }
    12   }
     14}
    1315
    14   function runtest() {
     16var countOrig;
     17var countAfterCreate;
     18var countAfterGC;
     19var testObj;
     20var refOrig;
     21var refAfterGet;
     22var refAfterGetGC;
     23var refBeforePass;
     24var refAfterPass;
     25var refAfterPassAndGC;
     26
     27function runtest() {
    1528    if (window.testRunner)
    16       testRunner.dumpAsText();
    17 
    18 
    19     var output = document.getElementById("output");
    20     output.innerHTML = "";
     29        testRunner.dumpAsText();
    2130
    2231    // Test that objects are deleted after their JS references are released.
    23     var countOrig = plug.testObjectCount;
    24     o1 = plug.testCreateTestObject();
    25     o2 = plug.testCreateTestObject();
    26     o3 = plug.testCreateTestObject();
    27     var countAfterCreate = plug.testObjectCount;
    28     o1 = o2 = o3 = null;
     32    countOrig = plug.testObjectCount;
     33    let x;
     34    for (let i = 0; i < 50; ++i)
     35        x = plug.testCreateTestObject();
     36    countAfterCreate = plug.testObjectCount;
     37    x = null;
    2938    doGC();
    30     var countAfterGC = plug.testObjectCount;
    31 
    32     output.innerHTML += "--- num test objects:<br>";
    33     output.innerHTML += "countAfterCreate == countOrig + 3? "
    34         + ((countAfterCreate == countOrig + 3) ? "PASS" : "FAIL")
    35         + "<br>";
    36     output.innerHTML += "countOrig == countAfterGC? "
    37         + ((countOrig == countAfterGC) ? "PASS" : "FAIL")
    38         + "<br>";
    39     output.innerHTML += "<br>";
     39    countAfterGC = plug.testObjectCount;
     40    shouldBe('countAfterCreate === countOrig + 50', 'true');
     41    shouldBe('countAfterGC < countAfterCreate', 'true');
    4042
    4143    // Test that the object refcount returns to normal after JS references
    4244    // are released.
    43     var testObj = plug.testObject;
    44     var refOrig = testObj.refCount;
    45     var o1 = plug.testObject;
    46     var o2 = plug.testObject;
    47     var o3 = plug.testObject;
    48     var refAfterGet = testObj.refCount;
    49     o1 = o2 = o3 = null;
     45    testObj = plug.testObject;
     46    refOrig = testObj.refCount;
     47    for (let i = 0; i < 50; ++i)
     48        plug.testObject;
     49    refAfterGet = testObj.refCount;
    5050    doGC();
    51     var refAfterGetGC = testObj.refCount;
     51    refAfterGetGC = testObj.refCount;
     52    shouldBe('refAfterGet === refOrig + 50', 'true');
     53    shouldBe('refAfterGetGC < refAfterGet', 'true');
    5254
    5355    // Test that calling NPN_Invoke with our object as a parameter returns
    5456    // our refcount to normal (may require a GC).
    55     plug.testPassTestObject("noop", testObj);
    56     plug.testPassTestObject("noop", testObj);
    57     plug.testPassTestObject("noop", testObj);
     57    refBeforePass = testObj.refCount;
     58    for (let i = 0; i < 50; ++i)
     59        plug.testPassTestObject("noop", testObj);
     60    refAfterPass = testObj.refCount;
    5861    doGC();
    59     var refAfterPass = testObj.refCount;
    60 
    61     output.innerHTML += "--- refcount on plug.testObject:<br>";
    62     output.innerHTML += "originally: " + refOrig + "<br>";
    63     output.innerHTML += "after GC: " + refAfterGetGC + "<br>";
    64     output.innerHTML += "after passing: " + refAfterPass + "<br>";
    65 
    66     var success = (countAfterGC == countOrig) && (refAfterPass == refOrig);
    67     output.innerHTML += (success ? "SUCCESS" : "FAILURE");
    68   }
     62    refAfterPassAndGC = testObj.refCount;
     63    shouldBe('refAfterPass === refBeforePass + 50', 'true');
     64    shouldBe('refAfterPassAndGC < refAfterPass', 'true');
     65}
    6966</script>
    7067
    7168<body onload="runtest()">
    7269
    73 Test that we can get an NPObject returned through a method on
    74 an NPAPI Object.<P>
     70    Test that we can get an NPObject returned through a method on
     71    an NPAPI Object.<P>
    7572
    76 Prints "SUCCESS" on success, "FAILURE" on failure.
    77 
    78 <embed name="plug" type="application/x-webkit-test-netscape">
    79 
    80 <div id=output>FAILURE</div>
    81 
     73    <embed name="plug" type="application/x-webkit-test-netscape">
    8274</body>
    83 

  • trunk/Source/JavaScriptCore/ChangeLog

    r233167 r233184  
     12018-06-25  Saam Barati  <sbarati@apple.com>
     2
     3        JSImmutableButterfly can't be allocated from a subspace with HeapCell::Kind::Auxiliary
     4        https://bugs.webkit.org/show_bug.cgi?id=186878
     5        <rdar://problem/40568659>
     6
     7        Reviewed by Mark Lam.
     8
     9        This patch fixes a bug in our JSImmutableButterfly implementation uncovered by
     10        our stress GC bots. Before this patch, JSImmutableButterfly was allocated
     11        with HeapCell::Kind::Auxiliary. This is wrong. Things that are JSCells must be
     12        allocated from HeapCell::Kind::JSCell. The way this broke on the stress GC
     13        bots is that our conservative marking won't do cell marking for things that
     14        are Auxiliary. This means that if the stack is the only thing pointing to a
     15        JSImmutableButterfly when a GC took place, that JSImmutableButterfly would
     16        not be visited. This patch fixes this bug. This patch also extends our conservative
     17        marking to understand that there may be interior pointers to things that are HeapCell::Kind::JSCell.
     18
     19        * bytecompiler/NodesCodegen.cpp:
     20        (JSC::ArrayNode::emitBytecode):
     21        * heap/HeapUtil.h:
     22        (JSC::HeapUtil::findGCObjectPointersForMarking):
     23        * runtime/JSImmutableButterfly.h:
     24        (JSC::JSImmutableButterfly::subspaceFor):
     25
    1262018-06-25  Mark Lam  <mark.lam@apple.com>
    227

  • trunk/Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp

    r233122 r233184  
    407407        if (length && !hadVariableExpression) {
    408408            recommendedIndexingType |= CopyOnWrite;
     409            ASSERT(generator.vm()->heap.isDeferred()); // We run bytecode generator under a DeferGC. If we stopped doing that, we'd need to put a DeferGC here as we filled in these slots.
    409410            auto* array = JSImmutableButterfly::create(*generator.vm(), recommendedIndexingType, length);
    410411            unsigned index = 0;

  • trunk/Source/JavaScriptCore/heap/HeapUtil.h

    r227717 r233184  
    8888            MarkedBlock* previousCandidate = MarkedBlock::blockFor(previousPointer);
    8989            if (!filter.ruleOut(bitwise_cast<Bits>(previousCandidate))
    90                 && set.contains(previousCandidate)
    91                 && previousCandidate->handle().cellKind() == HeapCell::Auxiliary) {
     90                && set.contains(previousCandidate)) {
    9291                previousPointer = static_cast<char*>(previousCandidate->handle().cellAlign(previousPointer));
    9392                if (previousCandidate->handle().isLiveCell(markingVersion, newlyAllocatedVersion, isMarking, previousPointer))
    … …  
    104103            return;
    105104
    106         HeapCell::Kind cellKind = candidate->handle().cellKind();
     105        MarkedBlock::Handle& handle = candidate->handle();
     106        HeapCell::Kind cellKind = handle.cellKind();
    107107       
    108108        auto tryPointer = [&] (void* pointer) {
    109             if (candidate->handle().isLiveCell(markingVersion, newlyAllocatedVersion, isMarking, pointer))
     109            if (handle.isLiveCell(markingVersion, newlyAllocatedVersion, isMarking, pointer))
    110110                func(pointer, cellKind);
    111111        };
    112112   
    113         if (candidate->handle().cellKind() == HeapCell::JSCell) {
    114             if (!MarkedBlock::isAtomAligned(pointer))
    115                 return;
    116        
    117             tryPointer(pointer);
    118             return;
    119         }
    120    
    121113        // A butterfly could point into the middle of an object.
    122         char* alignedPointer = static_cast<char*>(candidate->handle().cellAlign(pointer));
     114        char* alignedPointer = static_cast<char*>(handle.cellAlign(pointer));
    123115        tryPointer(alignedPointer);
    124116   

  • trunk/Source/JavaScriptCore/runtime/JSImmutableButterfly.h

    r232954 r233184  
    9090    {
    9191        // We allocate out of the JSValue gigacage as other code expects all butterflies to live there.
    92         return &vm.jsValueGigacageAuxiliarySpace;
     92        return &vm.jsValueGigacageCellSpace;
    9393    }
    9494
Note: See TracChangeset for help on using the changeset viewer.