JSImmutableButterfly can't be allocated from a subspace with HeapCell::Kind::Auxiliary
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 25 Jun 2018 23:56:35 +0000 (23:56 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 25 Jun 2018 23:56:35 +0000 (23:56 +0000)
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:

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

13 files changed:
LayoutTests/ChangeLog
LayoutTests/editing/selection/navigation-clears-editor-state-expected.txt
LayoutTests/editing/selection/navigation-clears-editor-state.html
LayoutTests/fast/dom/reference-cycle-leaks.html
LayoutTests/fast/misc/resources/test-observegc.js
LayoutTests/fast/misc/test-observegc-expected.txt
LayoutTests/platform/mac-wk2/plugins/refcount-leaks-expected.txt
LayoutTests/plugins/refcount-leaks-expected.txt
LayoutTests/plugins/refcount-leaks.html
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp
Source/JavaScriptCore/heap/HeapUtil.h
Source/JavaScriptCore/runtime/JSImmutableButterfly.h

index 7b20ee5..4cb4035 100644 (file)
@@ -1,3 +1,24 @@
+2018-06-25  Saam Barati  <sbarati@apple.com>
+
+        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.
+
+        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:
+
 2018-06-25  John Wilander  <wilander@apple.com>
 
         Resource Load Statistics: Make WebResourceLoadStatisticsStore::updateCookiePartitioningForDomains() wait for the network process before calling its callback
index b40cca5..369e27c 100644 (file)
@@ -3,10 +3,7 @@ This tests navigating away from a document after setting a selection deletes the
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-iframe = appendIframe()
-PASS internals.numberOfLiveDocuments() is initialDocumentCount + 1
-iframe.src = "resources/select-iframe-focusin-document-crash-frame.html"
-PASS internals.numberOfLiveDocuments() is initialDocumentCount + 1
+PASS freed more than 60 documents
 PASS successfullyParsed is true
 
 TEST COMPLETE
index a0ee4fb..d3e0952 100644 (file)
@@ -35,21 +35,36 @@ async function runTest()
 {
     initialDocumentCount = internals.numberOfLiveDocuments();
 
-    evalAndLog('iframe = appendIframe()');
+    let frames = [];
+    const count = 200;
+    for (let i = 0; i < count; ++i)
+        frames.push(appendIframe());
 
     await wait(0); // Make sure the transient document created by inserting an iframe is removed.
     GCController.collect();
 
-    shouldBe('internals.numberOfLiveDocuments()', 'initialDocumentCount + 1');
-    setEditorStates(iframe);
+    for (let frame of frames)
+        setEditorStates(frame);
 
     await wait(0); // Wait for UI update timer to fire.
 
-    evalAndLog('iframe.src = "resources/select-iframe-focusin-document-crash-frame.html"');
-    iframe.onload = () => {
-        GCController.collect();
-        shouldBe('internals.numberOfLiveDocuments()', 'initialDocumentCount + 1');
-        finishJSTest();                
+    let resolved = 0;
+    for (let frame of frames) {
+        frame.src = "resources/select-iframe-focusin-document-crash-frame.html";
+        frame.onload = () => {
+            ++resolved;
+            if (resolved !== count)
+                return;
+            let before = internals.numberOfLiveDocuments();
+            GCController.collect();
+            let after = internals.numberOfLiveDocuments();
+            let delta = before - after;
+            if (delta > 0.3 * count)
+                debug("PASS freed more than 60 documents");
+            else
+                debug("FAIL freed fewer than 60 documents");
+            finishJSTest();                
+        };
     }
 }
 
@@ -57,7 +72,7 @@ if (!window.GCController || !window.internals) {
     debug('This test requires GCController and internals');
 } else {
     if (window.testRunner)
-        setTimeout(() => testRunner.notifyDone(), 3000);
+        setTimeout(() => testRunner.notifyDone(), 5000);
     // Clear out any lingering documents from the previous tests.
     GCController.collect();
     GCController.collect();
index 0953d57..e69b2a3 100644 (file)
@@ -7,7 +7,7 @@ description('Tests for leaks caused by reference cycles that pass through the DO
 function checkForNodeLeaks(testFunction, underlyingClass)
 {
     // Bump this number as high as we need to, to get reproducible results.
-    const repetitions = 20;
+    const repetitions = 40;
 
     gc();
     const beforeCount = internals.numberOfLiveNodes();
index 65e68f5..dc27d7b 100644 (file)
@@ -1,9 +1,19 @@
 description("Ensures that window.internals.observegc works as expected");
 
-var testObject = { testProperty : "testValue" };
+var observers = [];
+for (let i = 0; i < 1000; ++i) {
+    let testObject = { testProperty : "testValue" };
+    let observer = internals.observeGC(testObject);
+    observers.push(observer);
+    testObject = null;
+}
 
-var observer = internals.observeGC(testObject);
-testObject = null;
 gc();
 
-shouldBe('observer.wasCollected', 'true');
+var anyCollected = false;
+for (let observer of observers) {
+    if (observer.wasCollected)
+        anyCollected = true;
+}
+
+shouldBe('anyCollected', 'true');
index c9edc87..808d13c 100644 (file)
@@ -3,7 +3,7 @@ Ensures that window.internals.observegc works as expected
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-PASS observer.wasCollected is true
+PASS anyCollected is true
 PASS successfullyParsed is true
 
 TEST COMPLETE
index 8c6dc35..6f752dc 100644 (file)
@@ -1,12 +1,8 @@
+PASS countAfterCreate === countOrig + 50 is true
+FAIL countAfterGC < countAfterCreate should be true. Was false.
+PASS refAfterGet === refOrig + 50 is true
+FAIL refAfterGetGC < refAfterGet should be true. Was false.
+PASS refAfterPass === refBeforePass + 50 is true
+FAIL refAfterPassAndGC < refAfterPass should be true. Was false.
 Test that we can get an NPObject returned through a method on an NPAPI Object.
-Prints "SUCCESS" on success, "FAILURE" on failure.  
 
---- num test objects:
-countAfterCreate == countOrig + 3? PASS
-countOrig == countAfterGC? FAIL
-
---- refcount on plug.testObject:
-originally: 2
-after GC: 5
-after passing: 8
-FAILURE
index d51a342..9e35f69 100644 (file)
@@ -1,12 +1,8 @@
+PASS countAfterCreate === countOrig + 50 is true
+PASS countAfterGC < countAfterCreate is true
+PASS refAfterGet === refOrig + 50 is true
+PASS refAfterGetGC < refAfterGet is true
+PASS refAfterPass === refBeforePass + 50 is true
+PASS refAfterPassAndGC < refAfterPass is true
 Test that we can get an NPObject returned through a method on an NPAPI Object.
-Prints "SUCCESS" on success, "FAILURE" on failure.  
 
---- num test objects:
-countAfterCreate == countOrig + 3? PASS
-countOrig == countAfterGC? PASS
-
---- refcount on plug.testObject:
-originally: 2
-after GC: 2
-after passing: 2
-SUCCESS
index 69f6ee0..0eb4d79 100644 (file)
@@ -1,83 +1,74 @@
+<head>
+<script src="../resources/js-test-pre.js"></script>
+</head>
+
 <script>
-  function noop(x) {
-  }
+function noop(x) {
+}
 
-  function doGC() {
-    if (window.GCController) {
-      // GC twice to make sure everything is cleaned up.
-      for (var i = 0; i < 2; i++) {
-        window.GCController.collect();
-      }
+function doGC() {
+    // GC twice to make sure everything is cleaned up.
+    for (var i = 0; i < 2; i++) {
+        gc();
     }
-  }
-
-  function runtest() {
+}
+
+var countOrig;
+var countAfterCreate;
+var countAfterGC;
+var testObj;
+var refOrig;
+var refAfterGet;
+var refAfterGetGC;
+var refBeforePass;
+var refAfterPass;
+var refAfterPassAndGC;
+
+function runtest() {
     if (window.testRunner)
-      testRunner.dumpAsText();
-
-
-    var output = document.getElementById("output");
-    output.innerHTML = "";
+        testRunner.dumpAsText();
 
     // Test that objects are deleted after their JS references are released.
-    var countOrig = plug.testObjectCount;
-    o1 = plug.testCreateTestObject();
-    o2 = plug.testCreateTestObject();
-    o3 = plug.testCreateTestObject();
-    var countAfterCreate = plug.testObjectCount;
-    o1 = o2 = o3 = null;
+    countOrig = plug.testObjectCount;
+    let x;
+    for (let i = 0; i < 50; ++i)
+        x = plug.testCreateTestObject();
+    countAfterCreate = plug.testObjectCount;
+    x = null;
     doGC();
-    var countAfterGC = plug.testObjectCount;
-
-    output.innerHTML += "--- num test objects:<br>";
-    output.innerHTML += "countAfterCreate == countOrig + 3? "
-        + ((countAfterCreate == countOrig + 3) ? "PASS" : "FAIL")
-        + "<br>";
-    output.innerHTML += "countOrig == countAfterGC? "
-        + ((countOrig == countAfterGC) ? "PASS" : "FAIL")
-        + "<br>";
-    output.innerHTML += "<br>";
+    countAfterGC = plug.testObjectCount;
+    shouldBe('countAfterCreate === countOrig + 50', 'true');
+    shouldBe('countAfterGC < countAfterCreate', 'true');
 
     // Test that the object refcount returns to normal after JS references
     // are released.
-    var testObj = plug.testObject;
-    var refOrig = testObj.refCount;
-    var o1 = plug.testObject;
-    var o2 = plug.testObject;
-    var o3 = plug.testObject;
-    var refAfterGet = testObj.refCount;
-    o1 = o2 = o3 = null;
+    testObj = plug.testObject;
+    refOrig = testObj.refCount;
+    for (let i = 0; i < 50; ++i)
+        plug.testObject;
+    refAfterGet = testObj.refCount;
     doGC();
-    var refAfterGetGC = testObj.refCount;
+    refAfterGetGC = testObj.refCount;
+    shouldBe('refAfterGet === refOrig + 50', 'true');
+    shouldBe('refAfterGetGC < refAfterGet', 'true');
 
     // Test that calling NPN_Invoke with our object as a parameter returns
     // our refcount to normal (may require a GC).
-    plug.testPassTestObject("noop", testObj);
-    plug.testPassTestObject("noop", testObj);
-    plug.testPassTestObject("noop", testObj);
+    refBeforePass = testObj.refCount;
+    for (let i = 0; i < 50; ++i)
+        plug.testPassTestObject("noop", testObj);
+    refAfterPass = testObj.refCount;
     doGC();
-    var refAfterPass = testObj.refCount;
-
-    output.innerHTML += "--- refcount on plug.testObject:<br>";
-    output.innerHTML += "originally: " + refOrig + "<br>";
-    output.innerHTML += "after GC: " + refAfterGetGC + "<br>";
-    output.innerHTML += "after passing: " + refAfterPass + "<br>";
-
-    var success = (countAfterGC == countOrig) && (refAfterPass == refOrig);
-    output.innerHTML += (success ? "SUCCESS" : "FAILURE");
-  }
+    refAfterPassAndGC = testObj.refCount;
+    shouldBe('refAfterPass === refBeforePass + 50', 'true');
+    shouldBe('refAfterPassAndGC < refAfterPass', 'true');
+}
 </script>
 
 <body onload="runtest()">
 
-Test that we can get an NPObject returned through a method on
-an NPAPI Object.<P>
-
-Prints "SUCCESS" on success, "FAILURE" on failure.
-
-<embed name="plug" type="application/x-webkit-test-netscape">
-
-<div id=output>FAILURE</div>
+    Test that we can get an NPObject returned through a method on
+    an NPAPI Object.<P>
 
+    <embed name="plug" type="application/x-webkit-test-netscape">
 </body>
-
index 86515f7..6138c15 100644 (file)
@@ -1,3 +1,28 @@
+2018-06-25  Saam Barati  <sbarati@apple.com>
+
+        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.
+
+        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):
+
 2018-06-25  Mark Lam  <mark.lam@apple.com>
 
         constructArray() should set m_numValuesInVector to the specified length.
index 0c909d0..850b3d2 100644 (file)
@@ -406,6 +406,7 @@ RegisterID* ArrayNode::emitBytecode(BytecodeGenerator& generator, RegisterID* ds
     auto newArray = [&] (RegisterID* dst, ElementNode* elements, unsigned length, bool hadVariableExpression) {
         if (length && !hadVariableExpression) {
             recommendedIndexingType |= CopyOnWrite;
+            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.
             auto* array = JSImmutableButterfly::create(*generator.vm(), recommendedIndexingType, length);
             unsigned index = 0;
             for (ElementNode* element = elements; index < length; element = element->next()) {
index 01cfdbc..dd6d8de 100644 (file)
@@ -87,8 +87,7 @@ public:
             char* previousPointer = pointer - sizeof(IndexingHeader) - 1;
             MarkedBlock* previousCandidate = MarkedBlock::blockFor(previousPointer);
             if (!filter.ruleOut(bitwise_cast<Bits>(previousCandidate))
-                && set.contains(previousCandidate)
-                && previousCandidate->handle().cellKind() == HeapCell::Auxiliary) {
+                && set.contains(previousCandidate)) {
                 previousPointer = static_cast<char*>(previousCandidate->handle().cellAlign(previousPointer));
                 if (previousCandidate->handle().isLiveCell(markingVersion, newlyAllocatedVersion, isMarking, previousPointer))
                     func(previousPointer, previousCandidate->handle().cellKind());
@@ -103,23 +102,16 @@ public:
         if (!set.contains(candidate))
             return;
 
-        HeapCell::Kind cellKind = candidate->handle().cellKind();
+        MarkedBlock::Handle& handle = candidate->handle();
+        HeapCell::Kind cellKind = handle.cellKind();
         
         auto tryPointer = [&] (void* pointer) {
-            if (candidate->handle().isLiveCell(markingVersion, newlyAllocatedVersion, isMarking, pointer))
+            if (handle.isLiveCell(markingVersion, newlyAllocatedVersion, isMarking, pointer))
                 func(pointer, cellKind);
         };
     
-        if (candidate->handle().cellKind() == HeapCell::JSCell) {
-            if (!MarkedBlock::isAtomAligned(pointer))
-                return;
-        
-            tryPointer(pointer);
-            return;
-        }
-    
         // A butterfly could point into the middle of an object.
-        char* alignedPointer = static_cast<char*>(candidate->handle().cellAlign(pointer));
+        char* alignedPointer = static_cast<char*>(handle.cellAlign(pointer));
         tryPointer(alignedPointer);
     
         // Also, a butterfly could point at the end of an object plus sizeof(IndexingHeader). In that
index 3ace0ad..e5fcc18 100644 (file)
@@ -89,7 +89,7 @@ public:
     static CompleteSubspace* subspaceFor(VM& vm)
     {
         // We allocate out of the JSValue gigacage as other code expects all butterflies to live there.
-        return &vm.jsValueGigacageAuxiliarySpace;
+        return &vm.jsValueGigacageCellSpace;
     }
 
     // Only call this if you just allocated this butterfly.