Attachment 343368 Details for Bug 186878 – patch

[patch] patch

b-backup.diff (text/plain), 7.42 KB, created by Saam Barati on 2018-06-22 14:30:08 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Saam Barati

Created: 2018-06-22 14:30:08 PDT

Size: 7.42 KB

patch

obsolete

>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 233099)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,28 @@
>+2018-06-22  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 NOBODY (OOPS!).
>+
>+        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 were 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-22  Saam Barati  <sbarati@apple.com>
> 
>         ensureWritableX should only convert away from CoW when it will succeed
>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,21 +102,15 @@ public:
>         if (!set.contains(candidate))
>             return;
> 
>-        HeapCell::Kind cellKind = candidate->handle().cellKind();
>-        
>         auto tryPointer = [&] (void* pointer) {
>             if (candidate->handle().isLiveCell(markingVersion, newlyAllocatedVersion, isMarking, pointer))
>-                func(pointer, cellKind);
>+                func(pointer, candidate->handle().cellKind());
>         };
>     
>-        if (candidate->handle().cellKind() == HeapCell::JSCell) {
>-            if (!MarkedBlock::isAtomAligned(pointer))
>-                return;
>-        
>-            tryPointer(pointer);
>-            return;
>-        }
>-    
>+        // We don't distinguish interior pointers based on HeapCell::Auxiliary because:
>+        // - JSImmutableButterfly had kind HeapCell::JSCell
>+        // - We think the compiler may emit code where it points into the middle of a cell
>+
>         // A butterfly could point into the middle of an object.
>         char* alignedPointer = static_cast<char*>(candidate->handle().cellAlign(pointer));
>         tryPointer(alignedPointer);
>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,14 @@
>+2018-06-22  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 NOBODY (OOPS!).
>+
>+        * fast/misc/resources/test-observegc.js:
>+        * fast/misc/test-observegc-expected.txt:
>+
> 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/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:

mark.lam: review+
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