Diff
Modified: trunk/LayoutTests/ChangeLog (233183 => 233184)
--- trunk/LayoutTests/ChangeLog 2018-06-25 23:56:01 UTC (rev 233183)
+++ trunk/LayoutTests/ChangeLog 2018-06-25 23:56:35 UTC (rev 233184)
@@ -1,3 +1,24 @@
+2018-06-25 Saam Barati <sbar...@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 <wilan...@apple.com>
Resource Load Statistics: Make WebResourceLoadStatisticsStore::updateCookiePartitioningForDomains() wait for the network process before calling its callback
Modified: trunk/LayoutTests/editing/selection/navigation-clears-editor-state-expected.txt (233183 => 233184)
--- trunk/LayoutTests/editing/selection/navigation-clears-editor-state-expected.txt 2018-06-25 23:56:01 UTC (rev 233183)
+++ trunk/LayoutTests/editing/selection/navigation-clears-editor-state-expected.txt 2018-06-25 23:56:35 UTC (rev 233184)
@@ -3,10 +3,7 @@
On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
-iframe = appendIframe()
-PASS internals.numberOfLiveDocuments() is initialDocumentCount + 1
-iframe.src = ""
-PASS internals.numberOfLiveDocuments() is initialDocumentCount + 1
+PASS freed more than 60 documents
PASS successfullyParsed is true
TEST COMPLETE
Modified: trunk/LayoutTests/editing/selection/navigation-clears-editor-state.html (233183 => 233184)
--- trunk/LayoutTests/editing/selection/navigation-clears-editor-state.html 2018-06-25 23:56:01 UTC (rev 233183)
+++ trunk/LayoutTests/editing/selection/navigation-clears-editor-state.html 2018-06-25 23:56:35 UTC (rev 233184)
@@ -35,21 +35,36 @@
{
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 = ""
- iframe._onload_ = () => {
- GCController.collect();
- shouldBe('internals.numberOfLiveDocuments()', 'initialDocumentCount + 1');
- finishJSTest();
+ let resolved = 0;
+ for (let frame of frames) {
+ frame.src = ""
+ 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 @@
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();
Modified: trunk/LayoutTests/fast/dom/reference-cycle-leaks.html (233183 => 233184)
--- trunk/LayoutTests/fast/dom/reference-cycle-leaks.html 2018-06-25 23:56:01 UTC (rev 233183)
+++ trunk/LayoutTests/fast/dom/reference-cycle-leaks.html 2018-06-25 23:56:35 UTC (rev 233184)
@@ -7,7 +7,7 @@
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();
Modified: trunk/LayoutTests/fast/misc/resources/test-observegc.js (233183 => 233184)
--- trunk/LayoutTests/fast/misc/resources/test-observegc.js 2018-06-25 23:56:01 UTC (rev 233183)
+++ trunk/LayoutTests/fast/misc/resources/test-observegc.js 2018-06-25 23:56:35 UTC (rev 233184)
@@ -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');
Modified: trunk/LayoutTests/fast/misc/test-observegc-expected.txt (233183 => 233184)
--- trunk/LayoutTests/fast/misc/test-observegc-expected.txt 2018-06-25 23:56:01 UTC (rev 233183)
+++ trunk/LayoutTests/fast/misc/test-observegc-expected.txt 2018-06-25 23:56:35 UTC (rev 233184)
@@ -3,7 +3,7 @@
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
Modified: trunk/LayoutTests/platform/mac-wk2/plugins/refcount-leaks-expected.txt (233183 => 233184)
--- trunk/LayoutTests/platform/mac-wk2/plugins/refcount-leaks-expected.txt 2018-06-25 23:56:01 UTC (rev 233183)
+++ trunk/LayoutTests/platform/mac-wk2/plugins/refcount-leaks-expected.txt 2018-06-25 23:56:35 UTC (rev 233184)
@@ -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
Modified: trunk/LayoutTests/plugins/refcount-leaks-expected.txt (233183 => 233184)
--- trunk/LayoutTests/plugins/refcount-leaks-expected.txt 2018-06-25 23:56:01 UTC (rev 233183)
+++ trunk/LayoutTests/plugins/refcount-leaks-expected.txt 2018-06-25 23:56:35 UTC (rev 233184)
@@ -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
Modified: trunk/LayoutTests/plugins/refcount-leaks.html (233183 => 233184)
--- trunk/LayoutTests/plugins/refcount-leaks.html 2018-06-25 23:56:01 UTC (rev 233183)
+++ trunk/LayoutTests/plugins/refcount-leaks.html 2018-06-25 23:56:35 UTC (rev 233184)
@@ -1,83 +1,74 @@
+<head>
+<script src=""
+</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();
+ testRunner.dumpAsText();
-
- var output = document.getElementById("output");
- output.innerHTML = "";
-
// 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;
+ countAfterGC = plug.testObjectCount;
+ shouldBe('countAfterCreate === countOrig + 50', 'true');
+ shouldBe('countAfterGC < countAfterCreate', 'true');
- 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>";
-
// 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>
+ 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>
-
+ <embed name="plug" type="application/x-webkit-test-netscape">
</body>
-
Modified: trunk/Source/_javascript_Core/ChangeLog (233183 => 233184)
--- trunk/Source/_javascript_Core/ChangeLog 2018-06-25 23:56:01 UTC (rev 233183)
+++ trunk/Source/_javascript_Core/ChangeLog 2018-06-25 23:56:35 UTC (rev 233184)
@@ -1,3 +1,28 @@
+2018-06-25 Saam Barati <sbar...@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....@apple.com>
constructArray() should set m_numValuesInVector to the specified length.
Modified: trunk/Source/_javascript_Core/bytecompiler/NodesCodegen.cpp (233183 => 233184)
--- trunk/Source/_javascript_Core/bytecompiler/NodesCodegen.cpp 2018-06-25 23:56:01 UTC (rev 233183)
+++ trunk/Source/_javascript_Core/bytecompiler/NodesCodegen.cpp 2018-06-25 23:56:35 UTC (rev 233184)
@@ -406,6 +406,7 @@
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()) {
Modified: trunk/Source/_javascript_Core/heap/HeapUtil.h (233183 => 233184)
--- trunk/Source/_javascript_Core/heap/HeapUtil.h 2018-06-25 23:56:01 UTC (rev 233183)
+++ trunk/Source/_javascript_Core/heap/HeapUtil.h 2018-06-25 23:56:35 UTC (rev 233184)
@@ -87,8 +87,7 @@
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 @@
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
Modified: trunk/Source/_javascript_Core/runtime/JSImmutableButterfly.h (233183 => 233184)
--- trunk/Source/_javascript_Core/runtime/JSImmutableButterfly.h 2018-06-25 23:56:01 UTC (rev 233183)
+++ trunk/Source/_javascript_Core/runtime/JSImmutableButterfly.h 2018-06-25 23:56:35 UTC (rev 233184)
@@ -89,7 +89,7 @@
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.
