Diff
Modified: trunk/JSTests/ChangeLog (264387 => 264388)
--- trunk/JSTests/ChangeLog 2020-07-15 05:24:29 UTC (rev 264387)
+++ trunk/JSTests/ChangeLog 2020-07-15 05:54:46 UTC (rev 264388)
@@ -1,3 +1,16 @@
+2020-07-14 Saam Barati <[email protected]>
+
+ We must hold the CodeBlock lock when calling StructureStubInfo::reset
+ https://bugs.webkit.org/show_bug.cgi?id=214332
+ <rdar://problem/64940787>
+
+ Reviewed by Yusuke Suzuki.
+
+ * stress/hold-lock-when-resetting-structure-stub-info.js: Added.
+ (foo.bar.C):
+ (foo.bar):
+ (foo):
+
2020-07-14 Mark Lam <[email protected]>
Handle out of memory error while creating an error message in the literal parser.
Added: trunk/JSTests/stress/hold-lock-when-resetting-structure-stub-info.js (0 => 264388)
--- trunk/JSTests/stress/hold-lock-when-resetting-structure-stub-info.js (rev 0)
+++ trunk/JSTests/stress/hold-lock-when-resetting-structure-stub-info.js 2020-07-15 05:54:46 UTC (rev 264388)
@@ -0,0 +1,22 @@
+//@ runDefault("--jitPolicyScale=0")
+
+function foo() {
+ function bar() {
+ class C {
+ constructor() {
+ this.x = 42;
+ }
+ }
+ let c = new C();
+ for (let i=0; i<100; i++) {
+ c.x;
+ }
+ };
+ for (let i=0; i<1000; i++) {
+ bar();
+ }
+}
+
+for (let i=0; i<25; i++) {
+ runString(`${foo};foo();`);
+}
Modified: trunk/Source/_javascript_Core/ChangeLog (264387 => 264388)
--- trunk/Source/_javascript_Core/ChangeLog 2020-07-15 05:24:29 UTC (rev 264387)
+++ trunk/Source/_javascript_Core/ChangeLog 2020-07-15 05:54:46 UTC (rev 264388)
@@ -1,3 +1,41 @@
+2020-07-14 Saam Barati <[email protected]>
+
+ We must hold the CodeBlock lock when calling StructureStubInfo::reset
+ https://bugs.webkit.org/show_bug.cgi?id=214332
+ <rdar://problem/64940787>
+
+ Reviewed by Yusuke Suzuki.
+
+ There was a race between resetting the StructureStubInfo, and reading from
+ it from the compiler thread. There was one place inside Repatch where we
+ didn't hold the CodeBlock's lock when calling StructureStubInfo::reset.
+
+ To make it clear which functions require the CodeBlock's lock to be
+ held when called, I've changed all such functions to take the
+ LockHolder as a parameter.
+
+ * bytecode/CodeBlock.cpp:
+ (JSC::CodeBlock::finalizeBaselineJITInlineCaches):
+ * bytecode/StructureStubClearingWatchpoint.cpp:
+ (JSC::StructureTransitionStructureStubClearingWatchpoint::fireInternal):
+ (JSC::AdaptiveValueStructureStubClearingWatchpoint::handleFire):
+ * bytecode/StructureStubInfo.cpp:
+ (JSC::StructureStubInfo::initGetByIdSelf):
+ (JSC::StructureStubInfo::initArrayLength):
+ (JSC::StructureStubInfo::initStringLength):
+ (JSC::StructureStubInfo::initPutByIdReplace):
+ (JSC::StructureStubInfo::initInByIdSelf):
+ (JSC::StructureStubInfo::addAccessCase):
+ (JSC::StructureStubInfo::reset):
+ (JSC::StructureStubInfo::visitWeakReferences):
+ (JSC::StructureStubInfo::setCacheType):
+ * bytecode/StructureStubInfo.h:
+ * jit/Repatch.cpp:
+ (JSC::fireWatchpointsAndClearStubIfNeeded):
+ (JSC::tryCacheGetBy):
+ (JSC::tryCachePutByID):
+ (JSC::tryCacheInByID):
+
2020-07-14 Mark Lam <[email protected]>
Handle out of memory error while creating an error message in the literal parser.
Modified: trunk/Source/_javascript_Core/bytecode/CodeBlock.cpp (264387 => 264388)
--- trunk/Source/_javascript_Core/bytecode/CodeBlock.cpp 2020-07-15 05:24:29 UTC (rev 264387)
+++ trunk/Source/_javascript_Core/bytecode/CodeBlock.cpp 2020-07-15 05:54:46 UTC (rev 264388)
@@ -1427,8 +1427,10 @@
for (CallLinkInfo* callLinkInfo : jitData->m_callLinkInfos)
callLinkInfo->visitWeak(vm());
- for (StructureStubInfo* stubInfo : jitData->m_stubInfos)
- stubInfo->visitWeakReferences(this);
+ for (StructureStubInfo* stubInfo : jitData->m_stubInfos) {
+ ConcurrentJSLockerBase locker(NoLockingNecessary);
+ stubInfo->visitWeakReferences(locker, this);
+ }
}
}
#endif
Modified: trunk/Source/_javascript_Core/bytecode/StructureStubClearingWatchpoint.cpp (264387 => 264388)
--- trunk/Source/_javascript_Core/bytecode/StructureStubClearingWatchpoint.cpp 2020-07-15 05:24:29 UTC (rev 264387)
+++ trunk/Source/_javascript_Core/bytecode/StructureStubClearingWatchpoint.cpp 2020-07-15 05:54:46 UTC (rev 264388)
@@ -44,7 +44,7 @@
// That works, because deleting a watchpoint removes it from the set's list, and
// the set's list traversal for firing is robust against the set changing.
ConcurrentJSLocker locker(m_holder->codeBlock()->m_lock);
- m_holder->stubInfo()->reset(m_holder->codeBlock());
+ m_holder->stubInfo()->reset(locker, m_holder->codeBlock());
return;
}
@@ -115,7 +115,7 @@
// That works, because deleting a watchpoint removes it from the set's list, and
// the set's list traversal for firing is robust against the set changing.
ConcurrentJSLocker locker(m_holder->codeBlock()->m_lock);
- m_holder->stubInfo()->reset(m_holder->codeBlock());
+ m_holder->stubInfo()->reset(locker, m_holder->codeBlock());
}
} // namespace JSC
Modified: trunk/Source/_javascript_Core/bytecode/StructureStubInfo.cpp (264387 => 264388)
--- trunk/Source/_javascript_Core/bytecode/StructureStubInfo.cpp 2020-07-15 05:24:29 UTC (rev 264387)
+++ trunk/Source/_javascript_Core/bytecode/StructureStubInfo.cpp 2020-07-15 05:54:46 UTC (rev 264388)
@@ -57,10 +57,10 @@
{
}
-void StructureStubInfo::initGetByIdSelf(CodeBlock* codeBlock, Structure* baseObjectStructure, PropertyOffset offset, CacheableIdentifier identifier)
+void StructureStubInfo::initGetByIdSelf(const ConcurrentJSLockerBase& locker, CodeBlock* codeBlock, Structure* baseObjectStructure, PropertyOffset offset, CacheableIdentifier identifier)
{
ASSERT(hasConstantIdentifier);
- setCacheType(CacheType::GetByIdSelf);
+ setCacheType(locker, CacheType::GetByIdSelf);
m_identifier = identifier;
codeBlock->vm().heap.writeBarrier(codeBlock);
@@ -69,19 +69,19 @@
u.byIdSelf.offset = offset;
}
-void StructureStubInfo::initArrayLength()
+void StructureStubInfo::initArrayLength(const ConcurrentJSLockerBase& locker)
{
- setCacheType(CacheType::ArrayLength);
+ setCacheType(locker, CacheType::ArrayLength);
}
-void StructureStubInfo::initStringLength()
+void StructureStubInfo::initStringLength(const ConcurrentJSLockerBase& locker)
{
- setCacheType(CacheType::StringLength);
+ setCacheType(locker, CacheType::StringLength);
}
-void StructureStubInfo::initPutByIdReplace(CodeBlock* codeBlock, Structure* baseObjectStructure, PropertyOffset offset, CacheableIdentifier identifier)
+void StructureStubInfo::initPutByIdReplace(const ConcurrentJSLockerBase& locker, CodeBlock* codeBlock, Structure* baseObjectStructure, PropertyOffset offset, CacheableIdentifier identifier)
{
- setCacheType(CacheType::PutByIdReplace);
+ setCacheType(locker, CacheType::PutByIdReplace);
m_identifier = identifier;
codeBlock->vm().heap.writeBarrier(codeBlock);
@@ -90,9 +90,9 @@
u.byIdSelf.offset = offset;
}
-void StructureStubInfo::initInByIdSelf(CodeBlock* codeBlock, Structure* baseObjectStructure, PropertyOffset offset, CacheableIdentifier identifier)
+void StructureStubInfo::initInByIdSelf(const ConcurrentJSLockerBase& locker, CodeBlock* codeBlock, Structure* baseObjectStructure, PropertyOffset offset, CacheableIdentifier identifier)
{
- setCacheType(CacheType::InByIdSelf);
+ setCacheType(locker, CacheType::InByIdSelf);
m_identifier = identifier;
codeBlock->vm().heap.writeBarrier(codeBlock);
@@ -190,7 +190,7 @@
return result;
}
- setCacheType(CacheType::Stub);
+ setCacheType(locker, CacheType::Stub);
u.stub = access.release();
}
@@ -236,7 +236,7 @@
return result;
}
-void StructureStubInfo::reset(CodeBlock* codeBlock)
+void StructureStubInfo::reset(const ConcurrentJSLockerBase& locker, CodeBlock* codeBlock)
{
clearBufferedStructures();
@@ -283,7 +283,7 @@
}
deref();
- setCacheType(CacheType::Unset);
+ setCacheType(locker, CacheType::Unset);
}
void StructureStubInfo::visitAggregate(SlotVisitor& visitor)
@@ -312,7 +312,7 @@
return;
}
-void StructureStubInfo::visitWeakReferences(CodeBlock* codeBlock)
+void StructureStubInfo::visitWeakReferences(const ConcurrentJSLockerBase& locker, CodeBlock* codeBlock)
{
VM& vm = codeBlock->vm();
{
@@ -338,7 +338,7 @@
return;
}
- reset(codeBlock);
+ reset(locker, codeBlock);
resetByGC = true;
}
@@ -401,7 +401,7 @@
return u.stub->containsPC(pc);
}
-void StructureStubInfo::setCacheType(CacheType newCacheType)
+void StructureStubInfo::setCacheType(const ConcurrentJSLockerBase&, CacheType newCacheType)
{
switch (m_cacheType) {
case CacheType::Unset:
Modified: trunk/Source/_javascript_Core/bytecode/StructureStubInfo.h (264387 => 264388)
--- trunk/Source/_javascript_Core/bytecode/StructureStubInfo.h 2020-07-15 05:24:29 UTC (rev 264387)
+++ trunk/Source/_javascript_Core/bytecode/StructureStubInfo.h 2020-07-15 05:54:46 UTC (rev 264388)
@@ -77,15 +77,15 @@
StructureStubInfo(AccessType);
~StructureStubInfo();
- void initGetByIdSelf(CodeBlock*, Structure* baseObjectStructure, PropertyOffset, CacheableIdentifier);
- void initArrayLength();
- void initStringLength();
- void initPutByIdReplace(CodeBlock*, Structure* baseObjectStructure, PropertyOffset, CacheableIdentifier);
- void initInByIdSelf(CodeBlock*, Structure* baseObjectStructure, PropertyOffset, CacheableIdentifier);
+ void initGetByIdSelf(const ConcurrentJSLockerBase&, CodeBlock*, Structure* baseObjectStructure, PropertyOffset, CacheableIdentifier);
+ void initArrayLength(const ConcurrentJSLockerBase&);
+ void initStringLength(const ConcurrentJSLockerBase&);
+ void initPutByIdReplace(const ConcurrentJSLockerBase&, CodeBlock*, Structure* baseObjectStructure, PropertyOffset, CacheableIdentifier);
+ void initInByIdSelf(const ConcurrentJSLockerBase&, CodeBlock*, Structure* baseObjectStructure, PropertyOffset, CacheableIdentifier);
AccessGenerationResult addAccessCase(const GCSafeConcurrentJSLocker&, JSGlobalObject*, CodeBlock*, ECMAMode, CacheableIdentifier, std::unique_ptr<AccessCase>);
- void reset(CodeBlock*);
+ void reset(const ConcurrentJSLockerBase&, CodeBlock*);
void deref();
void aboutToDie();
@@ -94,7 +94,7 @@
// Check if the stub has weak references that are dead. If it does, then it resets itself,
// either entirely or just enough to ensure that those dead pointers don't get used anymore.
- void visitWeakReferences(CodeBlock*);
+ void visitWeakReferences(const ConcurrentJSLockerBase&, CodeBlock*);
// This returns true if it has marked everything that it will ever mark.
bool propagateTransitions(SlotVisitor&);
@@ -257,7 +257,7 @@
return false;
}
- void setCacheType(CacheType);
+ void setCacheType(const ConcurrentJSLockerBase&, CacheType);
void clearBufferedStructures()
{
Modified: trunk/Source/_javascript_Core/jit/Repatch.cpp (264387 => 264388)
--- trunk/Source/_javascript_Core/jit/Repatch.cpp 2020-07-15 05:24:29 UTC (rev 264387)
+++ trunk/Source/_javascript_Core/jit/Repatch.cpp 2020-07-15 05:54:46 UTC (rev 264388)
@@ -141,7 +141,11 @@
{
if (result.shouldResetStubAndFireWatchpoints()) {
result.fireWatchpoints(vm);
- stubInfo.reset(codeBlock);
+
+ {
+ GCSafeConcurrentJSLocker locker(codeBlock->m_lock, vm.heap);
+ stubInfo.reset(locker, codeBlock);
+ }
}
}
@@ -206,7 +210,7 @@
bool generatedCodeInline = InlineAccess::generateArrayLength(stubInfo, jsCast<JSArray*>(baseCell));
if (generatedCodeInline) {
ftlThunkAwareRepatchCall(codeBlock, stubInfo.slowPathCallLocation, appropriateOptimizingGetByFunction(kind));
- stubInfo.initArrayLength();
+ stubInfo.initArrayLength(locker);
return RetryCacheLater;
}
}
@@ -217,7 +221,7 @@
bool generatedCodeInline = InlineAccess::generateStringLength(stubInfo);
if (generatedCodeInline) {
ftlThunkAwareRepatchCall(codeBlock, stubInfo.slowPathCallLocation, appropriateOptimizingGetByFunction(kind));
- stubInfo.initStringLength();
+ stubInfo.initStringLength(locker);
return RetryCacheLater;
}
}
@@ -272,7 +276,7 @@
LOG_IC((ICEvent::GetBySelfPatch, structure->classInfo(), Identifier::fromUid(vm, propertyName.uid()), slot.slotBase() == baseValue));
structure->startWatchingPropertyForReplacements(vm, slot.cachedOffset());
ftlThunkAwareRepatchCall(codeBlock, stubInfo.slowPathCallLocation, appropriateOptimizingGetByFunction(kind));
- stubInfo.initGetByIdSelf(codeBlock, structure, slot.cachedOffset(), propertyName);
+ stubInfo.initGetByIdSelf(locker, codeBlock, structure, slot.cachedOffset(), propertyName);
return RetryCacheLater;
}
}
@@ -596,7 +600,7 @@
if (generatedCodeInline) {
LOG_IC((ICEvent::PutByIdSelfPatch, oldStructure->classInfo(), ident, slot.base() == baseValue));
ftlThunkAwareRepatchCall(codeBlock, stubInfo.slowPathCallLocation, appropriateOptimizingPutByIdFunction(slot, putKind));
- stubInfo.initPutByIdReplace(codeBlock, oldStructure, slot.cachedOffset(), propertyName);
+ stubInfo.initPutByIdReplace(locker, codeBlock, oldStructure, slot.cachedOffset(), propertyName);
return RetryCacheLater;
}
}
@@ -874,7 +878,7 @@
LOG_IC((ICEvent::InByIdSelfPatch, structure->classInfo(), ident, slot.slotBase() == base));
structure->startWatchingPropertyForReplacements(vm, slot.cachedOffset());
ftlThunkAwareRepatchCall(codeBlock, stubInfo.slowPathCallLocation, operationInByIdOptimize);
- stubInfo.initInByIdSelf(codeBlock, structure, slot.cachedOffset(), propertyName);
+ stubInfo.initInByIdSelf(locker, codeBlock, structure, slot.cachedOffset(), propertyName);
return RetryCacheLater;
}
}
