Attachment 343546 Details for Bug 186878 – patch for landing

[patch] patch for landing

b-backup.diff (text/plain), 17.50 KB, created by Saam Barati on 2018-06-25 15:06:18 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Saam Barati

Created: 2018-06-25 15:06:18 PDT

Size: 17.50 KB

patch

obsolete

>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 233176)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -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: Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp
>===================================================================
>--- Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp	(revision 233069)
>+++ Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp	(working copy)
>@@ -406,6 +406,7 @@ RegisterID* ArrayNode::emitBytecode(Byte
>     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: Source/JavaScriptCore/heap/HeapUtil.h
>===================================================================
>--- Source/JavaScriptCore/heap/HeapUtil.h	(revision 233069)
>+++ Source/JavaScriptCore/heap/HeapUtil.h	(working copy)
>@@ -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: Source/JavaScriptCore/runtime/JSImmutableButterfly.h
>===================================================================
>--- Source/JavaScriptCore/runtime/JSImmutableButterfly.h	(revision 233069)
>+++ Source/JavaScriptCore/runtime/JSImmutableButterfly.h	(working copy)
>@@ -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.
>Index: LayoutTests/ChangeLog
>===================================================================
>--- LayoutTests/ChangeLog	(revision 233069)
>+++ LayoutTests/ChangeLog	(working copy)
>@@ -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-21  David Fenton  <david_fenton@apple.com>
> 
>         Skip imported/w3c/web-platform-tests/css/css-display/display-contents-first-letter-002.html.
>Index: LayoutTests/editing/selection/navigation-clears-editor-state-expected.txt
>===================================================================
>--- LayoutTests/editing/selection/navigation-clears-editor-state-expected.txt	(revision 233069)
>+++ LayoutTests/editing/selection/navigation-clears-editor-state-expected.txt	(working copy)
>@@ -3,10 +3,7 @@ This tests navigating away from a docume
> 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: LayoutTests/editing/selection/navigation-clears-editor-state.html
>===================================================================
>--- LayoutTests/editing/selection/navigation-clears-editor-state.html	(revision 233069)
>+++ LayoutTests/editing/selection/navigation-clears-editor-state.html	(working copy)
>@@ -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 > 60)
>+                debug("PASS freed more than 60 documents");
>+            else
>+                debug("FAIL freed fewer than 60 documents");
>+            finishJSTest();                
>+        };
>     }
> }
> 
>Index: LayoutTests/fast/dom/reference-cycle-leaks.html
>===================================================================
>--- LayoutTests/fast/dom/reference-cycle-leaks.html	(revision 233069)
>+++ LayoutTests/fast/dom/reference-cycle-leaks.html	(working copy)
>@@ -7,7 +7,7 @@ description('Tests for leaks caused by r
> 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: LayoutTests/fast/misc/test-observegc-expected.txt
>===================================================================
>--- LayoutTests/fast/misc/test-observegc-expected.txt	(revision 233069)
>+++ LayoutTests/fast/misc/test-observegc-expected.txt	(working copy)
>@@ -3,7 +3,7 @@ Ensures that window.internals.observegc 
> 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: LayoutTests/fast/misc/resources/test-observegc.js
>===================================================================
>--- LayoutTests/fast/misc/resources/test-observegc.js	(revision 233069)
>+++ LayoutTests/fast/misc/resources/test-observegc.js	(working copy)
>@@ -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: LayoutTests/platform/mac-wk2/plugins/refcount-leaks-expected.txt
>===================================================================
>--- LayoutTests/platform/mac-wk2/plugins/refcount-leaks-expected.txt	(revision 233069)
>+++ LayoutTests/platform/mac-wk2/plugins/refcount-leaks-expected.txt	(working copy)
>@@ -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: LayoutTests/plugins/refcount-leaks-expected.txt
>===================================================================
>--- LayoutTests/plugins/refcount-leaks-expected.txt	(revision 233069)
>+++ LayoutTests/plugins/refcount-leaks-expected.txt	(working copy)
>@@ -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: LayoutTests/plugins/refcount-leaks.html
>===================================================================
>--- LayoutTests/plugins/refcount-leaks.html	(revision 233069)
>+++ LayoutTests/plugins/refcount-leaks.html	(working copy)
>@@ -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() {
>-    if (window.testRunner)
>-      testRunner.dumpAsText();
>+}
> 
>+var countOrig;
>+var countAfterCreate;
>+var countAfterGC;
>+var testObj;
>+var refOrig;
>+var refAfterGet;
>+var refAfterGetGC;
>+var refBeforePass;
>+var refAfterPass;
>+var refAfterPassAndGC;
> 
>-    var output = document.getElementById("output");
>-    output.innerHTML = "";
>+function runtest() {
>+    if (window.testRunner)
>+        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>
>-

Actions: View | Formatted Diff | Diff

Attachments on bug 186878: 343258 | 343266 | 343267 | 343274 | 343275 | 343277 | 343368 | 343385 | 343386 | 343388 | 343391 | 343412 | 343457 | 343461 | 343464 | 343477 | 343484 | 343489 | 343523 | 343546 | 343552 | 343645 | 343653