Skip to content

Commit 9ce98e0

Browse files
dhechtConstellation
dhecht
authored and
Constellation
committed
[JSC] Ensure StructureStubInfoClearingWatchpoint does not outlive its CodeBlock
https://bugs.webkit.org/show_bug.cgi?id=279384 rdar://122375587 Reviewed by Yusuke Suzuki. StructureStubInfoClearingWatchpoint is kept alive by references from both the CodeBlock and the Executable. If the CodeBlock is destructed first and the Executable destruction is delayed, the watchpoint could fire after the CodeBlock destruction and the JSCell:isLive() call for the CodeBlock could try to walk a MarkedBlock that has already been freed, leading to crashes. Instead, proactively free the watchpoint when the CodeBlock is being destructed. Also add some debug-only code to help catch these sort of lifetime issues involving CodeBlock and Watchpoint. * Source/JavaScriptCore/bytecode/CodeBlock.cpp: (JSC::CodeBlock::CodeBlock): (JSC::CodeBlock::destroy): (JSC::CodeBlock::findStubInfo): (JSC::CodeBlock::resetBaselineJITData): Deleted. * Source/JavaScriptCore/bytecode/CodeBlock.h: * Source/JavaScriptCore/bytecode/CodeBlockInlines.h: (JSC::CodeBlock::wasDestructed): * Source/JavaScriptCore/bytecode/CodeBlockJettisoningWatchpoint.cpp: (JSC::CodeBlockJettisoningWatchpoint::fireInternal): * Source/JavaScriptCore/bytecode/InlineCacheCompiler.cpp: (JSC::InlineCacheHandler::aboutToDie): * Source/JavaScriptCore/bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.cpp: (JSC::LLIntPrototypeLoadAdaptiveStructureWatchpoint::~LLIntPrototypeLoadAdaptiveStructureWatchpoint): (JSC::LLIntPrototypeLoadAdaptiveStructureWatchpoint::fireInternal): * Source/JavaScriptCore/bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.h: * Source/JavaScriptCore/bytecode/StructureStubClearingWatchpoint.cpp: (JSC::StructureStubInfoClearingWatchpoint::~StructureStubInfoClearingWatchpoint): (JSC::StructureStubInfoClearingWatchpoint::fireInternal): * Source/JavaScriptCore/bytecode/StructureStubClearingWatchpoint.h: * Source/JavaScriptCore/dfg/DFGAdaptiveStructureWatchpoint.cpp: (JSC::DFG::AdaptiveStructureWatchpoint::fireInternal): Canonical link: https://commits.webkit.org/283454@main
1 parent 3fdd068 commit 9ce98e0

10 files changed

Lines changed: 50 additions & 43 deletions

‎Source/JavaScriptCore/bytecode/CodeBlock.cpp‎

Lines changed: 13 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -297,6 +297,9 @@ CodeBlock::CodeBlock(VM& vm, Structure* structure, CopyParsedBlockTag, CodeBlock
297297
, m_functionDecls(other.m_functionDecls)
298298
, m_functionExprs(other.m_functionExprs)
299299
, m_creationTime(ApproximateTime::now())
300+
#if ASSERT_ENABLED
301+
, m_magic(CODEBLOCK_MAGIC)
302+
#endif
300303
{
301304
ASSERT(heap()->isDeferred());
302305
ASSERT(m_scopeRegister.isLocal());
@@ -344,6 +347,9 @@ CodeBlock::CodeBlock(VM& vm, Structure* structure, ScriptExecutable* ownerExecut
344347
, m_instructionsRawPointer(unlinkedCodeBlock->instructions().rawPointer())
345348
, m_metadata(unlinkedCodeBlock->metadata().link())
346349
, m_creationTime(ApproximateTime::now())
350+
#if ASSERT_ENABLED
351+
, m_magic(CODEBLOCK_MAGIC)
352+
#endif
347353
{
348354
ASSERT(heap()->isDeferred());
349355
ASSERT(m_scopeRegister.isLocal());
@@ -1699,7 +1705,13 @@ void CodeBlock::finalizeUnconditionally(VM& vm, CollectionScope)
16991705

17001706
void CodeBlock::destroy(JSCell* cell)
17011707
{
1702-
static_cast<CodeBlock*>(cell)->~CodeBlock();
1708+
auto cb = static_cast<CodeBlock*>(cell);
1709+
ASSERT(!cb->wasDestructed());
1710+
cb->~CodeBlock();
1711+
#if ASSERT_ENABLED
1712+
cb->m_magic = 0;
1713+
#endif
1714+
ASSERT(cb->wasDestructed());
17031715
}
17041716

17051717
void CodeBlock::getICStatusMap(const ConcurrentJSLocker&, ICStatusMap& result)
@@ -1764,37 +1776,6 @@ StructureStubInfo* CodeBlock::findStubInfo(CodeOrigin codeOrigin)
17641776
});
17651777
return result;
17661778
}
1767-
1768-
void CodeBlock::resetBaselineJITData()
1769-
{
1770-
RELEASE_ASSERT(!JITCode::isJIT(jitType()));
1771-
ConcurrentJSLocker locker(m_lock);
1772-
1773-
if (auto* jitData = baselineJITData()) {
1774-
// We can clear these because no other thread will have references to any stub infos, call
1775-
// link infos, or by val infos if we don't have JIT code. Attempts to query these data
1776-
// structures using the concurrent API (getICStatusMap and friends) will return nothing if we
1777-
// don't have JIT code. So it's safe to call this if we fail a baseline JIT compile.
1778-
//
1779-
// We also call this from finalizeUnconditionally when we degrade from baseline JIT to LLInt
1780-
// code. This is safe to do since all compiler threads are safepointed in finalizeUnconditionally,
1781-
// which means we've made it past bytecode parsing. Only the bytecode parser will hold onto
1782-
// references to these various *infos via its use of ICStatusMap. Also, OSR exit might point to
1783-
// these *infos, but when we have an OSR exit linked to this CodeBlock, we won't downgrade
1784-
// to LLInt.
1785-
1786-
for (auto& stubInfo : jitData->stubInfos()) {
1787-
stubInfo.aboutToDie();
1788-
stubInfo.deref();
1789-
}
1790-
1791-
// We can clear this because the DFG's queries to these data structures are guarded by whether
1792-
// there is JIT code.
1793-
1794-
m_jitData = nullptr;
1795-
delete jitData;
1796-
}
1797-
}
17981779
#endif
17991780

18001781
template<typename Visitor>

‎Source/JavaScriptCore/bytecode/CodeBlock.h‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -269,13 +269,6 @@ class CodeBlock : public JSCell {
269269
const JITCodeMap& jitCodeMap();
270270

271271
std::optional<CodeOrigin> findPC(void* pc);
272-
273-
// We call this when we want to reattempt compiling something with the baseline JIT. Ideally
274-
// the baseline JIT would not add data to CodeBlock, but instead it would put its data into
275-
// a newly created JITCode, which could be thrown away if we bail on JIT compilation. Then we
276-
// would be able to get rid of this silly function.
277-
// FIXME: https://bugs.webkit.org/show_bug.cgi?id=159061
278-
void resetBaselineJITData();
279272
#endif // ENABLE(JIT)
280273

281274
void unlinkOrUpgradeIncomingCalls(VM&, CodeBlock*);
@@ -473,6 +466,7 @@ class CodeBlock : public JSCell {
473466
#endif
474467
#if ASSERT_ENABLED
475468
bool hasIdentifier(UniquedStringImpl*);
469+
bool wasDestructed();
476470
#endif
477471

478472
Vector<WriteBarrier<Unknown>>& constants() { return m_constantRegisters; }
@@ -992,6 +986,7 @@ class CodeBlock : public JSCell {
992986
#if ASSERT_ENABLED
993987
Lock m_cachedIdentifierUidsLock;
994988
HashSet<UniquedStringImpl*> m_cachedIdentifierUids;
989+
uint32_t m_magic;
995990
#endif
996991
};
997992
/* This check is for normal Release builds; ASSERT_ENABLED changes the size. */

‎Source/JavaScriptCore/bytecode/CodeBlockInlines.h‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,8 @@
3333

3434
namespace JSC {
3535

36+
#define CODEBLOCK_MAGIC 0xc0deb10c
37+
3638
template<typename Functor>
3739
void CodeBlock::forEachValueProfile(const Functor& func)
3840
{
@@ -120,4 +122,11 @@ ALWAYS_INLINE StringJumpTable& CodeBlock::dfgStringSwitchJumpTable(int tableInde
120122
}
121123
#endif
122124

125+
#if ASSERT_ENABLED
126+
ALWAYS_INLINE bool CodeBlock::wasDestructed()
127+
{
128+
return m_magic != CODEBLOCK_MAGIC;
129+
}
130+
#endif
131+
123132
} // namespace JSC

‎Source/JavaScriptCore/bytecode/CodeBlockJettisoningWatchpoint.cpp‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,13 +26,14 @@
2626
#include "config.h"
2727
#include "CodeBlockJettisoningWatchpoint.h"
2828

29-
#include "CodeBlock.h"
29+
#include "CodeBlockInlines.h"
3030
#include "DFGCommon.h"
3131

3232
namespace JSC {
3333

3434
void CodeBlockJettisoningWatchpoint::fireInternal(VM&, const FireDetail& detail)
3535
{
36+
ASSERT(!m_owner->wasDestructed());
3637
// If CodeBlock is no longer live, we do not fire it.
3738
// This works since CodeBlock is the owner of this watchpoint. When it gets destroyed, then this watchpoint also gets destroyed.
3839
// Only problematic case is, (1) CodeBlock is dead, but (2) destructor is not called yet. In this case, isLive() check guards correctly.

‎Source/JavaScriptCore/bytecode/InlineCacheCompiler.cpp‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7828,6 +7828,10 @@ void InlineCacheHandler::aboutToDie()
78287828
{
78297829
if (m_stubRoutine)
78307830
m_stubRoutine->aboutToDie();
7831+
// A reference to InlineCacheHandler may keep it alive later than the CodeBlock that "owns" this
7832+
// watchpoint but the watchpoint must not fire after the CodeBlock has finished destruction,
7833+
// so clear the watchpoint eagerly.
7834+
m_watchpoint.reset();
78317835
}
78327836

78337837
CallLinkInfo* InlineCacheHandler::callLinkInfoAt(const ConcurrentJSLocker& locker, unsigned index)

‎Source/JavaScriptCore/bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.cpp‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@
2626
#include "config.h"
2727
#include "LLIntPrototypeLoadAdaptiveStructureWatchpoint.h"
2828

29-
#include "CodeBlock.h"
29+
#include "CodeBlockInlines.h"
3030
#include "Instruction.h"
3131
#include "JSCellInlines.h"
3232

@@ -48,6 +48,11 @@ LLIntPrototypeLoadAdaptiveStructureWatchpoint::LLIntPrototypeLoadAdaptiveStructu
4848
{
4949
}
5050

51+
LLIntPrototypeLoadAdaptiveStructureWatchpoint::~LLIntPrototypeLoadAdaptiveStructureWatchpoint()
52+
{
53+
ASSERT(!m_owner || !m_owner->wasDestructed());
54+
}
55+
5156
void LLIntPrototypeLoadAdaptiveStructureWatchpoint::initialize(CodeBlock* codeBlock, const ObjectPropertyCondition& key, BytecodeIndex bytecodeOffset)
5257
{
5358
m_owner = codeBlock;
@@ -64,6 +69,7 @@ void LLIntPrototypeLoadAdaptiveStructureWatchpoint::install(VM&)
6469

6570
void LLIntPrototypeLoadAdaptiveStructureWatchpoint::fireInternal(VM& vm, const FireDetail&)
6671
{
72+
ASSERT(!m_owner->wasDestructed());
6773
if (!m_owner->isLive())
6874
return;
6975

‎Source/JavaScriptCore/bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,8 @@ class LLIntPrototypeLoadAdaptiveStructureWatchpoint final : public Watchpoint {
3838
LLIntPrototypeLoadAdaptiveStructureWatchpoint(CodeBlock*, const ObjectPropertyCondition&, BytecodeIndex);
3939
LLIntPrototypeLoadAdaptiveStructureWatchpoint();
4040

41+
~LLIntPrototypeLoadAdaptiveStructureWatchpoint();
42+
4143
void initialize(CodeBlock*, const ObjectPropertyCondition&, BytecodeIndex);
4244

4345
void install(VM&);

‎Source/JavaScriptCore/bytecode/StructureStubClearingWatchpoint.cpp‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@
2828

2929
#if ENABLE(JIT)
3030

31-
#include "CodeBlock.h"
31+
#include "CodeBlockInlines.h"
3232
#include "JSCellInlines.h"
3333
#include "StructureStubInfo.h"
3434
#include <wtf/TZoneMallocInlines.h>
@@ -39,8 +39,14 @@ WTF_MAKE_TZONE_ALLOCATED_IMPL(StructureStubInfoClearingWatchpoint);
3939
WTF_MAKE_TZONE_ALLOCATED_IMPL(AdaptiveValueStructureStubClearingWatchpoint);
4040
WTF_MAKE_TZONE_ALLOCATED_IMPL(StructureTransitionStructureStubClearingWatchpoint);
4141

42+
StructureStubInfoClearingWatchpoint::~StructureStubInfoClearingWatchpoint()
43+
{
44+
ASSERT(!m_owner->wasDestructed());
45+
}
46+
4247
void StructureStubInfoClearingWatchpoint::fireInternal(VM&, const FireDetail&)
4348
{
49+
ASSERT(!m_owner->wasDestructed());
4450
if (!m_owner->isLive())
4551
return;
4652

‎Source/JavaScriptCore/bytecode/StructureStubClearingWatchpoint.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,8 @@ class StructureStubInfoClearingWatchpoint final : public Watchpoint {
5252
{
5353
}
5454

55+
~StructureStubInfoClearingWatchpoint();
56+
5557
void fireInternal(VM&, const FireDetail&);
5658

5759
private:

‎Source/JavaScriptCore/dfg/DFGAdaptiveStructureWatchpoint.cpp‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@
2828

2929
#if ENABLE(DFG_JIT)
3030

31-
#include "CodeBlock.h"
31+
#include "CodeBlockInlines.h"
3232
#include "JSCellInlines.h"
3333

3434
namespace JSC { namespace DFG {
@@ -65,6 +65,7 @@ void AdaptiveStructureWatchpoint::install(VM&)
6565

6666
void AdaptiveStructureWatchpoint::fireInternal(VM& vm, const FireDetail& detail)
6767
{
68+
ASSERT(!m_codeBlock->wasDestructed());
6869
if (!m_codeBlock->isLive())
6970
return;
7071

0 commit comments

Comments
 (0)