Attachment 343457 Details for Bug 186878 – patch for landing if EWS passes

[patch] patch for landing if EWS passes

b-backup.diff (text/plain), 9.28 KB, created by Saam Barati on 2018-06-23 16:07:36 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Saam Barati

Created: 2018-06-23 16:07:36 PDT

Size: 9.28 KB

patch

obsolete

>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 233132)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,28 @@
>+2018-06-23  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-23  Mark Lam  <mark.lam@apple.com>
> 
>         Add more debugging features to $vm.
>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,26 @@
>+2018-06-23  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.
>+
>+        All these tests modified in this patch are susceptible to conservative scan leaks breaking them.
>+
>+        * TestExpectations:
>+        Mark an editing test as flaky since a more conservative scan breaks the test.
>+
>+        * fast/dom/reference-cycle-leaks.html:
>+        Bump the repetition count to make it slightly less susceptible to
>+        conservative scan leaks.
>+
>+        * fast/misc/resources/test-observegc.js:
>+        * fast/misc/test-observegc-expected.txt:
>+        Make this test probabilistically not susceptible to conservative scan leaks by ensuring
>+        at least one object gets collected when we allocate many of them. Before it was just
>+        testing that a single object was collected.
>+
> 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/TestExpectations
>===================================================================
>--- LayoutTests/TestExpectations	(revision 233069)
>+++ LayoutTests/TestExpectations	(working copy)
>@@ -1755,6 +1755,9 @@ editing/selection/inconsistent-in-remove
> # Test disabled in r29882 after it mysteriously started passing
> editing/style/5091898.html [ Skip ]
> 
>+# This test is susceptible to passing/failing dependent on what the conservative GC sees
>+editing/selection/navigation-clears-editor-state.html [ Pass Failure ]
>+
> # Test disabled in r28820 since it has different results per hardware
> fast/css/css2-system-color.html [ Skip ]
> 
>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');

Flags:

ews-watchlist: commit-queue-

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