Skip to content

Commit 03ec722

Browse files
saambaratisaambarati
committed
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 Filip Pizlo. 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 can't be allocated from HeapCell::Kind::Auxiliary. This patch adds a new HeapCell::Kind called JSCellWithInteriorPointers. It behaves like JSCell in all ways, except conservative scan knows to treat it like a butterfly in when we we may be pointing into the middle of it. The way we were crashing on the stress GC bots is that our conservative marking won't do cell visiting for things that are Auxiliary. This meant that if the stack were the only thing pointing to a JSImmutableButterfly when a GC took place, that JSImmutableButterfly would not be visited. This is now fixed. * bytecompiler/NodesCodegen.cpp: (JSC::ArrayNode::emitBytecode): * debugger/Debugger.cpp: * heap/ConservativeRoots.cpp: (JSC::ConservativeRoots::genericAddPointer): * heap/Heap.cpp: (JSC::GatherHeapSnapshotData::operator() const): (JSC::RemoveDeadHeapSnapshotNodes::operator() const): (JSC::Heap::globalObjectCount): (JSC::Heap::objectTypeCounts): (JSC::Heap::deleteAllCodeBlocks): * heap/HeapCell.cpp: (WTF::printInternal): * heap/HeapCell.h: (JSC::isJSCellKind): (JSC::hasInteriorPointers): * heap/HeapUtil.h: (JSC::HeapUtil::findGCObjectPointersForMarking): (JSC::HeapUtil::isPointerGCObjectJSCell): * heap/MarkedBlock.cpp: (JSC::MarkedBlock::Handle::didAddToDirectory): * heap/SlotVisitor.cpp: (JSC::SlotVisitor::appendJSCellOrAuxiliary): * runtime/JSGlobalObject.cpp: * runtime/JSImmutableButterfly.h: (JSC::JSImmutableButterfly::subspaceFor): * runtime/VM.cpp: (JSC::VM::VM): * runtime/VM.h: * tools/CellProfile.h: (JSC::CellProfile::CellProfile): (JSC::CellProfile::isJSCell const): * tools/HeapVerifier.cpp: (JSC::HeapVerifier::validateCell): 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: Canonical link: https://commits.webkit.org/202325@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@233236 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent 573b3d4 commit 03ec722

25 files changed

Lines changed: 226 additions & 125 deletions

‎LayoutTests/ChangeLog‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,24 @@
1+
2018-06-26 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 Filip Pizlo.
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+
122
2018-06-26 Truitt Savell <tsavell@apple.com>
223

324
[iOS] Rebaseline two webanimations tests after r233164

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

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,7 @@ This tests navigating away from a document after setting a selection deletes the
33
On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
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
6+
PASS freed more than 60 documents
107
PASS successfullyParsed is true
118

129
TEST COMPLETE

‎LayoutTests/editing/selection/navigation-clears-editor-state.html‎

Lines changed: 24 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -35,29 +35,44 @@
3535
{
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
}
5570

5671
if (!window.GCController || !window.internals) {
5772
debug('This test requires GCController and internals');
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();
6378
GCController.collect();

‎LayoutTests/fast/dom/reference-cycle-leaks.html‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
function checkForNodeLeaks(testFunction, underlyingClass)
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();
1313
const beforeCount = internals.numberOfLiveNodes();

‎LayoutTests/fast/misc/resources/test-observegc.js‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,19 @@
11
description("Ensures that window.internals.observegc works as expected");
22

3-
var testObject = { testProperty : "testValue" };
3+
var observers = [];
4+
for (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;
711
gc();
812

9-
shouldBe('observer.wasCollected', 'true');
13+
var anyCollected = false;
14+
for (let observer of observers) {
15+
if (observer.wasCollected)
16+
anyCollected = true;
17+
}
18+
19+
shouldBe('anyCollected', 'true');

‎LayoutTests/fast/misc/test-observegc-expected.txt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ Ensures that window.internals.observegc works as expected
33
On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
44

55

6-
PASS observer.wasCollected is true
6+
PASS anyCollected is true
77
PASS successfullyParsed is true
88

99
TEST COMPLETE

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

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,8 @@
1+
PASS countAfterCreate === countOrig + 50 is true
2+
FAIL countAfterGC < countAfterCreate should be true. Was false.
3+
PASS refAfterGet === refOrig + 50 is true
4+
FAIL refAfterGetGC < refAfterGet should be true. Was false.
5+
PASS refAfterPass === refBeforePass + 50 is true
6+
FAIL refAfterPassAndGC < refAfterPass should be true. Was false.
17
Test 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

‎LayoutTests/plugins/refcount-leaks-expected.txt‎

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,8 @@
1+
PASS countAfterCreate === countOrig + 50 is true
2+
PASS countAfterGC < countAfterCreate is true
3+
PASS refAfterGet === refOrig + 50 is true
4+
PASS refAfterGetGC < refAfterGet is true
5+
PASS refAfterPass === refBeforePass + 50 is true
6+
PASS refAfterPassAndGC < refAfterPass is true
17
Test 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

‎LayoutTests/plugins/refcount-leaks.html‎

Lines changed: 53 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -1,83 +1,74 @@
1+
<head>
2+
<script src="../resources/js-test-pre.js"></script>
3+
</head>
4+
15
<script>
2-
function noop(x) {
3-
}
6+
function 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-
}
9+
function doGC() {
10+
// GC twice to make sure everything is cleaned up.
11+
for (var i = 0; i < 2; i++) {
12+
gc();
1113
}
12-
}
13-
14-
function runtest() {
14+
}
15+
16+
var countOrig;
17+
var countAfterCreate;
18+
var countAfterGC;
19+
var testObj;
20+
var refOrig;
21+
var refAfterGet;
22+
var refAfterGetGC;
23+
var refBeforePass;
24+
var refAfterPass;
25+
var refAfterPassAndGC;
26+
27+
function 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>
75-
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>
70+
Test that we can get an NPObject returned through a method on
71+
an NPAPI Object.<P>
8172

73+
<embed name="plug" type="application/x-webkit-test-netscape">
8274
</body>
83-

‎Source/JavaScriptCore/ChangeLog‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,59 @@
1+
2018-06-26 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 Filip Pizlo.
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 can't
12+
be allocated from HeapCell::Kind::Auxiliary. This patch adds a new HeapCell::Kind
13+
called JSCellWithInteriorPointers. It behaves like JSCell in all ways, except
14+
conservative scan knows to treat it like a butterfly in when we we may be
15+
pointing into the middle of it.
16+
17+
The way we were crashing on the stress GC bots is that our conservative marking
18+
won't do cell visiting for things that are Auxiliary. This meant that if the
19+
stack were the only thing pointing to a JSImmutableButterfly when a GC took place,
20+
that JSImmutableButterfly would not be visited. This is now fixed.
21+
22+
* bytecompiler/NodesCodegen.cpp:
23+
(JSC::ArrayNode::emitBytecode):
24+
* debugger/Debugger.cpp:
25+
* heap/ConservativeRoots.cpp:
26+
(JSC::ConservativeRoots::genericAddPointer):
27+
* heap/Heap.cpp:
28+
(JSC::GatherHeapSnapshotData::operator() const):
29+
(JSC::RemoveDeadHeapSnapshotNodes::operator() const):
30+
(JSC::Heap::globalObjectCount):
31+
(JSC::Heap::objectTypeCounts):
32+
(JSC::Heap::deleteAllCodeBlocks):
33+
* heap/HeapCell.cpp:
34+
(WTF::printInternal):
35+
* heap/HeapCell.h:
36+
(JSC::isJSCellKind):
37+
(JSC::hasInteriorPointers):
38+
* heap/HeapUtil.h:
39+
(JSC::HeapUtil::findGCObjectPointersForMarking):
40+
(JSC::HeapUtil::isPointerGCObjectJSCell):
41+
* heap/MarkedBlock.cpp:
42+
(JSC::MarkedBlock::Handle::didAddToDirectory):
43+
* heap/SlotVisitor.cpp:
44+
(JSC::SlotVisitor::appendJSCellOrAuxiliary):
45+
* runtime/JSGlobalObject.cpp:
46+
* runtime/JSImmutableButterfly.h:
47+
(JSC::JSImmutableButterfly::subspaceFor):
48+
* runtime/VM.cpp:
49+
(JSC::VM::VM):
50+
* runtime/VM.h:
51+
* tools/CellProfile.h:
52+
(JSC::CellProfile::CellProfile):
53+
(JSC::CellProfile::isJSCell const):
54+
* tools/HeapVerifier.cpp:
55+
(JSC::HeapVerifier::validateCell):
56+
157
2018-06-26 Mark Lam <mark.lam@apple.com>
258

359
Skip some unnecessary work in Interpreter::getStackTrace().

0 commit comments

Comments
 (0)