Title: [264473] trunk
- Revision
- 264473
- Author
- [email protected]
- Date
- 2020-07-16 12:42:11 -0700 (Thu, 16 Jul 2020)
Log Message
[JSC] Use unvalidatedGet instead of get to access UnlinkedCodeBlock from CodeBlock destructor
https://bugs.webkit.org/show_bug.cgi?id=214403
<rdar://problem/65527229>
Reviewed by Mark Lam.
JSTests:
* stress/codeblock-destructor-access-unlinkedcodeblock.js: Added.
(foo):
Source/_javascript_Core:
WriteBarrier<>::get has a check whether this is not executed when sweeping cells. This is good assertion since
destruction order of cells are not defined in general, so member cell access from a destructor is almost always wrong.
But in CodeBlock case, this is OK because (1) CodeBlock destructor accesses UnlinkedCodeBlock and (2) GC ensures that
CodeBlock gets destroyed before UnlinkedCodeBlock gets destroyed. So this assertion is hit incorrectly.
In this patch, we use WriteBarrier<>::unvalidatedGet in CodeBlock destructor to bypass the above assertion explicitly.
* bytecode/CodeBlock.cpp:
(JSC::CodeBlock::~CodeBlock):
* heap/Heap.cpp:
(JSC::Heap::deleteUnmarkedCompiledCode):
Modified Paths
Added Paths
Diff
Modified: trunk/JSTests/ChangeLog (264472 => 264473)
--- trunk/JSTests/ChangeLog 2020-07-16 19:04:56 UTC (rev 264472)
+++ trunk/JSTests/ChangeLog 2020-07-16 19:42:11 UTC (rev 264473)
@@ -1,3 +1,14 @@
+2020-07-16 Yusuke Suzuki <[email protected]>
+
+ [JSC] Use unvalidatedGet instead of get to access UnlinkedCodeBlock from CodeBlock destructor
+ https://bugs.webkit.org/show_bug.cgi?id=214403
+ <rdar://problem/65527229>
+
+ Reviewed by Mark Lam.
+
+ * stress/codeblock-destructor-access-unlinkedcodeblock.js: Added.
+ (foo):
+
2020-07-15 Alexey Shvayka <[email protected]>
Emit HasOwnPropertyFunctionCallDotNode for "Reflect" identifiers
Added: trunk/JSTests/stress/codeblock-destructor-access-unlinkedcodeblock.js (0 => 264473)
--- trunk/JSTests/stress/codeblock-destructor-access-unlinkedcodeblock.js (rev 0)
+++ trunk/JSTests/stress/codeblock-destructor-access-unlinkedcodeblock.js 2020-07-16 19:42:11 UTC (rev 264473)
@@ -0,0 +1,15 @@
+//@ skip if $memoryLimited
+//@ runDefault("--returnEarlyFromInfiniteLoopsForFuzzing=1")
+//@ slow!
+
+// This test needs 3 seconds to reproduce the crash.
+
+function foo() {
+ let a=0;
+ while(1) a++;
+}
+
+for (let i=0; i<10; i++) {
+ runString(`${foo.toString()};foo();foo();`);
+}
+gc();
Modified: trunk/Source/_javascript_Core/ChangeLog (264472 => 264473)
--- trunk/Source/_javascript_Core/ChangeLog 2020-07-16 19:04:56 UTC (rev 264472)
+++ trunk/Source/_javascript_Core/ChangeLog 2020-07-16 19:42:11 UTC (rev 264473)
@@ -1,3 +1,22 @@
+2020-07-16 Yusuke Suzuki <[email protected]>
+
+ [JSC] Use unvalidatedGet instead of get to access UnlinkedCodeBlock from CodeBlock destructor
+ https://bugs.webkit.org/show_bug.cgi?id=214403
+ <rdar://problem/65527229>
+
+ Reviewed by Mark Lam.
+
+ WriteBarrier<>::get has a check whether this is not executed when sweeping cells. This is good assertion since
+ destruction order of cells are not defined in general, so member cell access from a destructor is almost always wrong.
+ But in CodeBlock case, this is OK because (1) CodeBlock destructor accesses UnlinkedCodeBlock and (2) GC ensures that
+ CodeBlock gets destroyed before UnlinkedCodeBlock gets destroyed. So this assertion is hit incorrectly.
+ In this patch, we use WriteBarrier<>::unvalidatedGet in CodeBlock destructor to bypass the above assertion explicitly.
+
+ * bytecode/CodeBlock.cpp:
+ (JSC::CodeBlock::~CodeBlock):
+ * heap/Heap.cpp:
+ (JSC::Heap::deleteUnmarkedCompiledCode):
+
2020-07-15 Fujii Hironori <[email protected]>
[CMake][WebDriver] Generating WebDriverAtoms.cpp is rarely failing as "ImportError: No module named jsmin"
Modified: trunk/Source/_javascript_Core/bytecode/CodeBlock.cpp (264472 => 264473)
--- trunk/Source/_javascript_Core/bytecode/CodeBlock.cpp 2020-07-16 19:04:56 UTC (rev 264472)
+++ trunk/Source/_javascript_Core/bytecode/CodeBlock.cpp 2020-07-16 19:42:11 UTC (rev 264473)
@@ -817,8 +817,14 @@
{
VM& vm = *m_vm;
+ // We use unvalidatedGet because get() has a validation assertion that rejects access.
+ // This assertion is correct since destruction order of cells is not guaranteed, and member cells could already be destroyed.
+ // But for CodeBlock, we are ensuring the order: CodeBlock gets destroyed before UnlinkedCodeBlock gets destroyed.
+ // So, we can access member UnlinkedCodeBlock safely here. We bypass the assertion by using unvalidatedGet.
+ UnlinkedCodeBlock* unlinkedCodeBlock = m_unlinkedCode.unvalidatedGet();
+
if (Options::returnEarlyFromInfiniteLoopsForFuzzing() && JITCode::isBaselineCode(jitType())) {
- for (const auto& instruction : instructions()) {
+ for (const auto& instruction : unlinkedCodeBlock->instructions()) {
if (instruction->is<OpLoopHint>())
vm.removeLoopHintExecutionCounter(instruction.ptr());
}
@@ -849,8 +855,8 @@
if (UNLIKELY(vm.m_perBytecodeProfiler))
vm.m_perBytecodeProfiler->notifyDestruction(this);
- if (!vm.heap.isShuttingDown() && unlinkedCodeBlock()->didOptimize() == TriState::Indeterminate)
- unlinkedCodeBlock()->setDidOptimize(TriState::False);
+ if (!vm.heap.isShuttingDown() && unlinkedCodeBlock->didOptimize() == TriState::Indeterminate)
+ unlinkedCodeBlock->setDidOptimize(TriState::False);
#if ENABLE(VERBOSE_VALUE_PROFILE)
dumpValueProfiles();
Modified: trunk/Source/_javascript_Core/heap/Heap.cpp (264472 => 264473)
--- trunk/Source/_javascript_Core/heap/Heap.cpp 2020-07-16 19:04:56 UTC (rev 264472)
+++ trunk/Source/_javascript_Core/heap/Heap.cpp 2020-07-16 19:42:11 UTC (rev 264473)
@@ -972,7 +972,9 @@
void Heap::deleteUnmarkedCompiledCode()
{
vm().forEachScriptExecutableSpace([] (auto& space) { space.space.sweep(); });
- vm().forEachCodeBlockSpace([] (auto& space) { space.space.sweep(); }); // Sweeping must occur before deleting stubs, otherwise the stubs might still think they're alive as they get deleted.
+ // Sweeping must occur before deleting stubs, otherwise the stubs might still think they're alive as they get deleted.
+ // And CodeBlock destructor is assuming that CodeBlock gets destroyed before UnlinkedCodeBlock gets destroyed.
+ vm().forEachCodeBlockSpace([] (auto& space) { space.space.sweep(); });
m_jitStubRoutines->deleteUnmarkedJettisonedStubRoutines();
}
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
