[JSC] DFG::CommonData modification by DFG reallyAdd should be guarded by CodeBlock's lock
https://bugs.webkit.org/show_bug.cgi?id=203177
Reviewed by Mark Lam.
JSTests:
* stress/dfg-really-add-locking.js: Added.
Source/JavaScriptCore:
When doing DFG reallyAdd, DFG::JITCode is already set in CodeBlock and DFG::CommonData can be
reachable from CodeBlock. So concurrent collector can trace entries of DFG::CommonData while DFG reallyAdd
is modifying it. It would be possible that we install DFG::JITCode after performing DFG reallyAdd, but for now,
we just protect DFG reallyAdd's DFG::CommonData modification by CodeBlock's lock so that concurrent collector
does not trace them in a racy manner.
* dfg/DFGDesiredGlobalProperties.cpp:
(JSC::DFG::DesiredGlobalProperties::reallyAdd):
* dfg/DFGDesiredIdentifiers.cpp:
(JSC::DFG::DesiredIdentifiers::reallyAdd):
* dfg/DFGDesiredTransitions.cpp:
(JSC::DFG::DesiredTransition::reallyAdd):
* dfg/DFGDesiredWatchpoints.cpp:
(JSC::DFG::ArrayBufferViewWatchpointAdaptor::add):
(JSC::DFG::SymbolTableAdaptor::add):
(JSC::DFG::FunctionExecutableAdaptor::add):
(JSC::DFG::AdaptiveStructureWatchpointAdaptor::add):
* dfg/DFGDesiredWatchpoints.h:
(JSC::DFG::SetPointerAdaptor::add):
* dfg/DFGDesiredWeakReferences.cpp:
(JSC::DFG::DesiredWeakReferences::reallyAdd):
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@251321 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
index 5273355..87272cc 100644
--- a/JSTests/ChangeLog
+++ b/JSTests/ChangeLog
@@ -1,3 +1,12 @@
+2019-10-18 Yusuke Suzuki <ysuzuki@apple.com>
+
+ [JSC] DFG::CommonData modification by DFG reallyAdd should be guarded by CodeBlock's lock
+ https://bugs.webkit.org/show_bug.cgi?id=203177
+
+ Reviewed by Mark Lam.
+
+ * stress/dfg-really-add-locking.js: Added.
+
2019-10-17 Mark Lam <mark.lam@apple.com>
Add missing checks after calls to the sameValue() JSValue comparator.
diff --git a/JSTests/stress/dfg-really-add-locking.js b/JSTests/stress/dfg-really-add-locking.js
new file mode 100644
index 0000000..901620b
--- /dev/null
+++ b/JSTests/stress/dfg-really-add-locking.js
@@ -0,0 +1,41 @@
+//@ slow!
+//@ runDefault("--collectContinuously=1", "--useGenerationalGC=0")
+
+for (var i = 0; i < 10; ++i) {
+ runString(`
+ var g;
+ (function () {
+ for (var i = 0; i < 100000; ++i) {
+ var o = {};
+ o.a = 0;
+ o.b = 1;
+ o.c = 2;
+ o.d = 3;
+ o.e = 4;
+ o.f = 5;
+ o.g = 6;
+ o.h = 7;
+ o.i = 8;
+ o.j = 9;
+ o.k = 10;
+ o.l = 11;
+ o.m = 12;
+ o.n = 13;
+ o.o = 14;
+ o.p = 15;
+ o.q = 0;
+ o.r = 1;
+ o.s = 2;
+ o.t = 3;
+ o.u = 4;
+ o.v = 5;
+ o.w = 6;
+ o.x = 7;
+ o.y = 8;
+ o.z = 9;
+ g = o;
+ }
+ return g;
+ }());
+ `);
+}
diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
index 7a899b6..753f3bc 100644
--- a/Source/JavaScriptCore/ChangeLog
+++ b/Source/JavaScriptCore/ChangeLog
@@ -1,5 +1,34 @@
2019-10-18 Yusuke Suzuki <ysuzuki@apple.com>
+ [JSC] DFG::CommonData modification by DFG reallyAdd should be guarded by CodeBlock's lock
+ https://bugs.webkit.org/show_bug.cgi?id=203177
+
+ Reviewed by Mark Lam.
+
+ When doing DFG reallyAdd, DFG::JITCode is already set in CodeBlock and DFG::CommonData can be
+ reachable from CodeBlock. So concurrent collector can trace entries of DFG::CommonData while DFG reallyAdd
+ is modifying it. It would be possible that we install DFG::JITCode after performing DFG reallyAdd, but for now,
+ we just protect DFG reallyAdd's DFG::CommonData modification by CodeBlock's lock so that concurrent collector
+ does not trace them in a racy manner.
+
+ * dfg/DFGDesiredGlobalProperties.cpp:
+ (JSC::DFG::DesiredGlobalProperties::reallyAdd):
+ * dfg/DFGDesiredIdentifiers.cpp:
+ (JSC::DFG::DesiredIdentifiers::reallyAdd):
+ * dfg/DFGDesiredTransitions.cpp:
+ (JSC::DFG::DesiredTransition::reallyAdd):
+ * dfg/DFGDesiredWatchpoints.cpp:
+ (JSC::DFG::ArrayBufferViewWatchpointAdaptor::add):
+ (JSC::DFG::SymbolTableAdaptor::add):
+ (JSC::DFG::FunctionExecutableAdaptor::add):
+ (JSC::DFG::AdaptiveStructureWatchpointAdaptor::add):
+ * dfg/DFGDesiredWatchpoints.h:
+ (JSC::DFG::SetPointerAdaptor::add):
+ * dfg/DFGDesiredWeakReferences.cpp:
+ (JSC::DFG::DesiredWeakReferences::reallyAdd):
+
+2019-10-18 Yusuke Suzuki <ysuzuki@apple.com>
+
[JSC] Make ConcurrentJSLock Lock even if ENABLE_CONCURRENT_JS=OFF
https://bugs.webkit.org/show_bug.cgi?id=202892
diff --git a/Source/JavaScriptCore/dfg/DFGDesiredGlobalProperties.cpp b/Source/JavaScriptCore/dfg/DFGDesiredGlobalProperties.cpp
index ba6841d..6416449 100644
--- a/Source/JavaScriptCore/dfg/DFGDesiredGlobalProperties.cpp
+++ b/Source/JavaScriptCore/dfg/DFGDesiredGlobalProperties.cpp
@@ -61,7 +61,12 @@
auto* uid = identifiers.at(property.identifierNumber());
auto& watchpointSet = property.globalObject()->ensureReferencedPropertyWatchpointSet(uid);
ASSERT(watchpointSet.isStillValid());
- watchpointSet.add(common.watchpoints.add(codeBlock));
+ CodeBlockJettisoningWatchpoint* watchpoint = nullptr;
+ {
+ ConcurrentJSLocker locker(codeBlock->m_lock);
+ watchpoint = common.watchpoints.add(codeBlock);
+ }
+ watchpointSet.add(WTFMove(watchpoint));
}
}
diff --git a/Source/JavaScriptCore/dfg/DFGDesiredIdentifiers.cpp b/Source/JavaScriptCore/dfg/DFGDesiredIdentifiers.cpp
index 77c0eb0..be35647 100644
--- a/Source/JavaScriptCore/dfg/DFGDesiredIdentifiers.cpp
+++ b/Source/JavaScriptCore/dfg/DFGDesiredIdentifiers.cpp
@@ -89,7 +89,11 @@
{
for (auto rep : m_addedIdentifiers) {
ASSERT(rep->hasAtLeastOneRef());
- commonData->dfgIdentifiers.append(Identifier::fromUid(vm, rep));
+ Identifier uid = Identifier::fromUid(vm, rep);
+ {
+ ConcurrentJSLocker locker(m_codeBlock->m_lock);
+ commonData->dfgIdentifiers.append(WTFMove(uid));
+ }
}
}
diff --git a/Source/JavaScriptCore/dfg/DFGDesiredTransitions.cpp b/Source/JavaScriptCore/dfg/DFGDesiredTransitions.cpp
index 4b82b03..0910ba8 100644
--- a/Source/JavaScriptCore/dfg/DFGDesiredTransitions.cpp
+++ b/Source/JavaScriptCore/dfg/DFGDesiredTransitions.cpp
@@ -44,6 +44,7 @@
void DesiredTransition::reallyAdd(VM& vm, CommonData* common)
{
+ ConcurrentJSLocker locker(m_codeBlock->m_lock);
common->transitions.append(
WeakReferenceTransition(
vm, m_codeBlock,
diff --git a/Source/JavaScriptCore/dfg/DFGDesiredWatchpoints.cpp b/Source/JavaScriptCore/dfg/DFGDesiredWatchpoints.cpp
index 64cd3d2..c2431f0 100644
--- a/Source/JavaScriptCore/dfg/DFGDesiredWatchpoints.cpp
+++ b/Source/JavaScriptCore/dfg/DFGDesiredWatchpoints.cpp
@@ -39,10 +39,14 @@
CodeBlock* codeBlock, JSArrayBufferView* view, CommonData& common)
{
VM& vm = codeBlock->vm();
- Watchpoint* watchpoint = common.watchpoints.add(codeBlock);
+ CodeBlockJettisoningWatchpoint* watchpoint = nullptr;
+ {
+ ConcurrentJSLocker locker(codeBlock->m_lock);
+ watchpoint = common.watchpoints.add(codeBlock);
+ }
ArrayBufferNeuteringWatchpointSet* neuteringWatchpoint =
ArrayBufferNeuteringWatchpointSet::create(vm);
- neuteringWatchpoint->set().add(watchpoint);
+ neuteringWatchpoint->set().add(WTFMove(watchpoint));
codeBlock->addConstant(ConcurrentJSLocker(codeBlock->m_lock), neuteringWatchpoint);
// FIXME: We don't need to set this watchpoint at all for shared buffers.
// https://bugs.webkit.org/show_bug.cgi?id=164108
@@ -53,14 +57,24 @@
CodeBlock* codeBlock, SymbolTable* symbolTable, CommonData& common)
{
codeBlock->addConstant(ConcurrentJSLocker(codeBlock->m_lock), symbolTable); // For common users, it doesn't really matter if it's weak or not. If references to it go away, we go away, too.
- symbolTable->singleton().add(common.watchpoints.add(codeBlock));
+ CodeBlockJettisoningWatchpoint* watchpoint = nullptr;
+ {
+ ConcurrentJSLocker locker(codeBlock->m_lock);
+ watchpoint = common.watchpoints.add(codeBlock);
+ }
+ symbolTable->singleton().add(WTFMove(watchpoint));
}
void FunctionExecutableAdaptor::add(
CodeBlock* codeBlock, FunctionExecutable* executable, CommonData& common)
{
codeBlock->addConstant(ConcurrentJSLocker(codeBlock->m_lock), executable); // For common users, it doesn't really matter if it's weak or not. If references to it go away, we go away, too.
- executable->singleton().add(common.watchpoints.add(codeBlock));
+ CodeBlockJettisoningWatchpoint* watchpoint = nullptr;
+ {
+ ConcurrentJSLocker locker(codeBlock->m_lock);
+ watchpoint = common.watchpoints.add(codeBlock);
+ }
+ executable->singleton().add(WTFMove(watchpoint));
}
void AdaptiveStructureWatchpointAdaptor::add(
@@ -68,13 +82,25 @@
{
VM& vm = codeBlock->vm();
switch (key.kind()) {
- case PropertyCondition::Equivalence:
- common.adaptiveInferredPropertyValueWatchpoints.add(key, codeBlock)->install(vm);
+ case PropertyCondition::Equivalence: {
+ AdaptiveInferredPropertyValueWatchpoint* watchpoint = nullptr;
+ {
+ ConcurrentJSLocker locker(codeBlock->m_lock);
+ watchpoint = common.adaptiveInferredPropertyValueWatchpoints.add(key, codeBlock);
+ }
+ watchpoint->install(vm);
break;
- default:
- common.adaptiveStructureWatchpoints.add(key, codeBlock)->install(vm);
+ }
+ default: {
+ AdaptiveStructureWatchpoint* watchpoint = nullptr;
+ {
+ ConcurrentJSLocker locker(codeBlock->m_lock);
+ watchpoint = common.adaptiveStructureWatchpoints.add(key, codeBlock);
+ }
+ watchpoint->install(vm);
break;
}
+ }
}
DesiredWatchpoints::DesiredWatchpoints() { }
diff --git a/Source/JavaScriptCore/dfg/DFGDesiredWatchpoints.h b/Source/JavaScriptCore/dfg/DFGDesiredWatchpoints.h
index f89e789..9e13b62 100644
--- a/Source/JavaScriptCore/dfg/DFGDesiredWatchpoints.h
+++ b/Source/JavaScriptCore/dfg/DFGDesiredWatchpoints.h
@@ -45,7 +45,12 @@
struct SetPointerAdaptor {
static void add(CodeBlock* codeBlock, T set, CommonData& common)
{
- return set->add(common.watchpoints.add(codeBlock));
+ CodeBlockJettisoningWatchpoint* watchpoint = nullptr;
+ {
+ ConcurrentJSLocker locker(codeBlock->m_lock);
+ watchpoint = common.watchpoints.add(codeBlock);
+ }
+ return set->add(WTFMove(watchpoint));
}
static bool hasBeenInvalidated(T set)
{
diff --git a/Source/JavaScriptCore/dfg/DFGDesiredWeakReferences.cpp b/Source/JavaScriptCore/dfg/DFGDesiredWeakReferences.cpp
index 68ff2a6..fc27745 100644
--- a/Source/JavaScriptCore/dfg/DFGDesiredWeakReferences.cpp
+++ b/Source/JavaScriptCore/dfg/DFGDesiredWeakReferences.cpp
@@ -69,6 +69,7 @@
{
for (JSCell* target : m_references) {
if (Structure* structure = jsDynamicCast<Structure*>(vm, target)) {
+ ConcurrentJSLocker locker(m_codeBlock->m_lock);
common->weakStructureReferences.append(
WriteBarrier<Structure>(vm, m_codeBlock, structure));
} else {
@@ -78,6 +79,7 @@
// having a weak pointer to itself will cause it to get collected.
RELEASE_ASSERT(!jsDynamicCast<CodeBlock*>(vm, target));
+ ConcurrentJSLocker locker(m_codeBlock->m_lock);
common->weakReferences.append(
WriteBarrier<JSCell>(vm, m_codeBlock, target));
}
