WebKit Bugzilla
Attachment 361005 Details for
Bug 194031
: [JSC] UnlinkedMetadataTable assumes that MetadataTable is destroyed before it is destructed, but order of destruction of JS heap cells are not guaranteed
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[patch]
Patch
bug-194031-20190203023448.patch (text/plain), 12.51 KB, created by
Yusuke Suzuki
on 2019-02-03 02:34:49 PST
(
hide
)
Description:
Patch
Filename:
MIME Type:
Creator:
Yusuke Suzuki
Created:
2019-02-03 02:34:49 PST
Size:
12.51 KB
patch
obsolete
>Subversion Revision: 240902 >diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog >index d5e4549ed1a770c4c16e2d8d49ae7dfa83c3dac6..86cf9ec147e48e4a6f222c999f32e52b767d0b49 100644 >--- a/Source/JavaScriptCore/ChangeLog >+++ b/Source/JavaScriptCore/ChangeLog >@@ -1,3 +1,40 @@ >+2019-02-03 Yusuke Suzuki <ysuzuki@apple.com> >+ >+ [JSC] UnlinkedMetadataTable assumes that MetadataTable is destroyed before it is destructed, but order of destruction of JS heap cells are not guaranteed >+ https://bugs.webkit.org/show_bug.cgi?id=194031 >+ >+ Reviewed by NOBODY (OOPS!). >+ >+ UnlinkedMetadataTable assumes that MetadataTable linked against this UnlinkedMetadataTable is already destroyed when UnlinkedMetadataTable is destroyed. >+ This means that UnlinkedCodeBlock is destroyed after all the linked CodeBlocks are destroyed. But this assumption is not valid since GC's finalizer >+ sweeps objects without considering the dependencies among swept objects. UnlinkedMetadataTable can be destroyed even before linked MetadataTable is >+ destroyed if UnlinkedCodeBlock is destroyed before linked CodeBlock is destroyed. >+ >+ To make the above assumption valid, we make UnlinkedMetadataTable RefCounted object, and make MetadataTable hold the strong ref to UnlinkedMetadataTable. >+ This ensures that UnlinkedMetadataTable is destroyed after all the linked MetadataTables are destroyed. >+ >+ * bytecode/MetadataTable.cpp: >+ (JSC::MetadataTable::MetadataTable): >+ (JSC::MetadataTable::~MetadataTable): >+ * bytecode/UnlinkedCodeBlock.cpp: >+ (JSC::UnlinkedCodeBlock::UnlinkedCodeBlock): >+ (JSC::UnlinkedCodeBlock::visitChildren): >+ (JSC::UnlinkedCodeBlock::estimatedSize): >+ (JSC::UnlinkedCodeBlock::setInstructions): >+ * bytecode/UnlinkedCodeBlock.h: >+ (JSC::UnlinkedCodeBlock::metadata): >+ (JSC::UnlinkedCodeBlock::metadataSizeInBytes): >+ * bytecode/UnlinkedMetadataTable.h: >+ (JSC::UnlinkedMetadataTable::create): >+ * bytecode/UnlinkedMetadataTableInlines.h: >+ (JSC::UnlinkedMetadataTable::UnlinkedMetadataTable): >+ * runtime/CachedTypes.cpp: >+ (JSC::CachedMetadataTable::decode const): >+ (JSC::CachedCodeBlock::metadata const): >+ (JSC::UnlinkedCodeBlock::UnlinkedCodeBlock): >+ (JSC::CachedCodeBlock<CodeBlockType>::decode const): >+ (JSC::CachedCodeBlock<CodeBlockType>::encode): >+ > 2019-02-01 Yusuke Suzuki <ysuzuki@apple.com> > > [JSC] Decouple JIT related data from CodeBlock >diff --git a/Source/JavaScriptCore/bytecode/MetadataTable.cpp b/Source/JavaScriptCore/bytecode/MetadataTable.cpp >index 9d89db4adfd0c4257d4a37da0867afb11c76a023..6868dc48e44ca6558cda34b1adcbb4f82e2d93bb 100644 >--- a/Source/JavaScriptCore/bytecode/MetadataTable.cpp >+++ b/Source/JavaScriptCore/bytecode/MetadataTable.cpp >@@ -35,8 +35,8 @@ namespace JSC { > > MetadataTable::MetadataTable(UnlinkedMetadataTable& unlinkedMetadata) > { >- linkingData() = UnlinkedMetadataTable::LinkingData { >- &unlinkedMetadata, >+ new (&linkingData()) UnlinkedMetadataTable::LinkingData { >+ unlinkedMetadata, > 1, > }; > } >@@ -55,7 +55,11 @@ MetadataTable::~MetadataTable() > { > for (unsigned i = 0; i < NUMBER_OF_BYTECODE_WITH_METADATA; i++) > getOpcodeType<DeallocTable>(static_cast<OpcodeID>(i), this); >- linkingData().unlinkedMetadata->unlink(*this); >+ Ref<UnlinkedMetadataTable> unlinkedMetadata = WTFMove(linkingData().unlinkedMetadata); >+ linkingData().~LinkingData(); >+ // Since UnlinkedMetadata::unlink frees the underlying memory of MetadataTable. >+ // We need to destroy LinkingData before calling it. >+ unlinkedMetadata->unlink(*this); > } > > size_t MetadataTable::sizeInBytes() >diff --git a/Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp b/Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp >index 6bb94964c1c5ae62410614968722720108640594..6095d589df8495de113bb329600fb3c7f528535f 100644 >--- a/Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp >+++ b/Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp >@@ -57,6 +57,7 @@ const ClassInfo UnlinkedCodeBlock::s_info = { "UnlinkedCodeBlock", nullptr, null > UnlinkedCodeBlock::UnlinkedCodeBlock(VM* vm, Structure* structure, CodeType codeType, const ExecutableInfo& info, DebuggerMode debuggerMode) > : Base(*vm, structure) > , m_globalObjectRegister(VirtualRegister()) >+ , m_metadata(UnlinkedMetadataTable::create()) > , m_usesEval(info.usesEval()) > , m_isStrictMode(info.isStrictMode()) > , m_isConstructor(info.isConstructor()) >@@ -92,7 +93,7 @@ void UnlinkedCodeBlock::visitChildren(JSCell* cell, SlotVisitor& visitor) > for (FunctionExpressionVector::iterator ptr = thisObject->m_functionExprs.begin(), end = thisObject->m_functionExprs.end(); ptr != end; ++ptr) > visitor.append(*ptr); > visitor.appendValues(thisObject->m_constantRegisters.data(), thisObject->m_constantRegisters.size()); >- size_t extraMemory = thisObject->m_metadata.sizeInBytes(); >+ size_t extraMemory = thisObject->m_metadata->sizeInBytes(); > if (thisObject->m_instructions) > extraMemory += thisObject->m_instructions->sizeInBytes(); > visitor.reportExtraMemoryVisited(extraMemory); >@@ -101,7 +102,7 @@ void UnlinkedCodeBlock::visitChildren(JSCell* cell, SlotVisitor& visitor) > size_t UnlinkedCodeBlock::estimatedSize(JSCell* cell, VM& vm) > { > UnlinkedCodeBlock* thisObject = jsCast<UnlinkedCodeBlock*>(cell); >- size_t extraSize = thisObject->m_metadata.sizeInBytes(); >+ size_t extraSize = thisObject->m_metadata->sizeInBytes(); > if (thisObject->m_instructions) > extraSize += thisObject->m_instructions->sizeInBytes(); > return Base::estimatedSize(cell, vm) + extraSize; >@@ -310,9 +311,9 @@ void UnlinkedCodeBlock::setInstructions(std::unique_ptr<InstructionStream> instr > { > auto locker = holdLock(cellLock()); > m_instructions = WTFMove(instructions); >- m_metadata.finalize(); >+ m_metadata->finalize(); > } >- Heap::heap(this)->reportExtraMemoryAllocated(m_instructions->sizeInBytes() + m_metadata.sizeInBytes()); >+ Heap::heap(this)->reportExtraMemoryAllocated(m_instructions->sizeInBytes() + m_metadata->sizeInBytes()); > } > > const InstructionStream& UnlinkedCodeBlock::instructions() const >diff --git a/Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h b/Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h >index 148a53521fa762c257a652bbe3d3c1284b6686ec..d0bb0c7ef9994abe635ebaa3d4075a4600fbbf8f 100644 >--- a/Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h >+++ b/Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h >@@ -360,11 +360,11 @@ class UnlinkedCodeBlock : public JSCell { > DFG::ExitProfile& exitProfile() { return m_exitProfile; } > #endif > >- UnlinkedMetadataTable& metadata() { return m_metadata; } >+ UnlinkedMetadataTable& metadata() { return m_metadata.get(); } > > size_t metadataSizeInBytes() > { >- return m_metadata.sizeInBytes(); >+ return m_metadata->sizeInBytes(); > } > > >@@ -410,7 +410,7 @@ class UnlinkedCodeBlock : public JSCell { > > String m_sourceURLDirective; > String m_sourceMappingURLDirective; >- UnlinkedMetadataTable m_metadata; >+ Ref<UnlinkedMetadataTable> m_metadata; > > #if ENABLE(DFG_JIT) > DFG::ExitProfile m_exitProfile; >diff --git a/Source/JavaScriptCore/bytecode/UnlinkedMetadataTable.h b/Source/JavaScriptCore/bytecode/UnlinkedMetadataTable.h >index f94707e3b185d7716b416a4933eca6c951a4d5ef..7d426b79fa6a532e709515677f9594daeed77e48 100644 >--- a/Source/JavaScriptCore/bytecode/UnlinkedMetadataTable.h >+++ b/Source/JavaScriptCore/bytecode/UnlinkedMetadataTable.h >@@ -32,18 +32,17 @@ namespace JSC { > > class MetadataTable; > >-class UnlinkedMetadataTable { >+class UnlinkedMetadataTable : public RefCounted<UnlinkedMetadataTable> { > friend class LLIntOffsetsExtractor; > friend class MetadataTable; > friend class CachedMetadataTable; > > public: > struct LinkingData { >- UnlinkedMetadataTable* unlinkedMetadata; >+ Ref<UnlinkedMetadataTable> unlinkedMetadata; > unsigned refCount; > }; > >- UnlinkedMetadataTable(); > ~UnlinkedMetadataTable(); > > unsigned addEntry(OpcodeID); >@@ -54,7 +53,14 @@ class UnlinkedMetadataTable { > > RefPtr<MetadataTable> link(); > >+ static Ref<UnlinkedMetadataTable> create() >+ { >+ return adoptRef(*new UnlinkedMetadataTable); >+ } >+ > private: >+ UnlinkedMetadataTable(); >+ > void unlink(MetadataTable&); > > size_t sizeInBytes(MetadataTable&); >diff --git a/Source/JavaScriptCore/bytecode/UnlinkedMetadataTableInlines.h b/Source/JavaScriptCore/bytecode/UnlinkedMetadataTableInlines.h >index 05fb04b790cfed2aac09412510a2f9037b637bf2..3353922ca83c31d6697ea5e6ba5238c16386938a 100644 >--- a/Source/JavaScriptCore/bytecode/UnlinkedMetadataTableInlines.h >+++ b/Source/JavaScriptCore/bytecode/UnlinkedMetadataTableInlines.h >@@ -32,11 +32,11 @@ > namespace JSC { > > ALWAYS_INLINE UnlinkedMetadataTable::UnlinkedMetadataTable() >+ : m_hasMetadata(false) >+ , m_isFinalized(false) >+ , m_isLinked(false) >+ , m_rawBuffer(fastZeroedMalloc(sizeof(LinkingData) + s_offsetTableSize)) > { >- m_hasMetadata = false; >- m_isFinalized = false; >- m_isLinked = false; >- m_rawBuffer = fastZeroedMalloc(sizeof(LinkingData) + s_offsetTableSize); > } > > ALWAYS_INLINE UnlinkedMetadataTable::~UnlinkedMetadataTable() >diff --git a/Source/JavaScriptCore/runtime/CachedTypes.cpp b/Source/JavaScriptCore/runtime/CachedTypes.cpp >index 05e8f0786320522d682e7de9569e30a0a2fbab72..c67e17df483550806ac6da26bf45346c9452f5b7 100644 >--- a/Source/JavaScriptCore/runtime/CachedTypes.cpp >+++ b/Source/JavaScriptCore/runtime/CachedTypes.cpp >@@ -1170,13 +1170,15 @@ class CachedMetadataTable : public CachedObject<UnlinkedMetadataTable> { > m_metadata[i] = metadataTable.buffer()[i]; > } > >- void decode(Decoder&, UnlinkedMetadataTable& metadataTable) const >+ Ref<UnlinkedMetadataTable> decode(Decoder&) const > { >- metadataTable.m_isFinalized = true; >- metadataTable.m_isLinked = false; >- metadataTable.m_hasMetadata = m_hasMetadata; >+ Ref<UnlinkedMetadataTable> metadataTable = UnlinkedMetadataTable::create(); >+ metadataTable->m_isFinalized = true; >+ metadataTable->m_isLinked = false; >+ metadataTable->m_hasMetadata = m_hasMetadata; > for (unsigned i = UnlinkedMetadataTable::s_offsetTableEntries; i--;) >- metadataTable.buffer()[i] = m_metadata[i]; >+ metadataTable->buffer()[i] = m_metadata[i]; >+ return metadataTable; > } > > private: >@@ -1475,6 +1477,8 @@ class CachedCodeBlock : public CachedObject<CodeBlockType> { > String sourceURLDirective(Decoder& decoder) const { return m_sourceURLDirective.decode(decoder); } > String sourceMappingURLDirective(Decoder& decoder) const { return m_sourceMappingURLDirective.decode(decoder); } > >+ Ref<UnlinkedMetadataTable> metadata(Decoder& decoder) const { return m_metadata.decode(decoder); } >+ > unsigned usesEval() const { return m_usesEval; } > unsigned isStrictMode() const { return m_isStrictMode; } > unsigned isConstructor() const { return m_isConstructor; } >@@ -1696,6 +1700,8 @@ ALWAYS_INLINE UnlinkedCodeBlock::UnlinkedCodeBlock(Decoder& decoder, Structure* > , m_sourceURLDirective(cachedCodeBlock.sourceURLDirective(decoder)) > , m_sourceMappingURLDirective(cachedCodeBlock.sourceMappingURLDirective(decoder)) > >+ , m_metadata(cachedCodeBlock.metadata(decoder)) >+ > , m_usesEval(cachedCodeBlock.usesEval()) > , m_isStrictMode(cachedCodeBlock.isStrictMode()) > , m_isConstructor(cachedCodeBlock.isConstructor()) >@@ -1728,7 +1734,6 @@ ALWAYS_INLINE void CachedCodeBlock<CodeBlockType>::decode(Decoder& decoder, Unli > for (unsigned i = LinkTimeConstantCount; i--;) > codeBlock.m_linkTimeConstants[i] = m_linkTimeConstants[i]; > >- m_metadata.decode(decoder, codeBlock.m_metadata); > m_propertyAccessInstructions.decode(decoder, codeBlock.m_propertyAccessInstructions); > m_constantRegisters.decode(decoder, codeBlock.m_constantRegisters, &codeBlock); > m_constantsSourceCodeRepresentation.decode(decoder, codeBlock.m_constantsSourceCodeRepresentation); >@@ -1878,7 +1883,7 @@ ALWAYS_INLINE void CachedCodeBlock<CodeBlockType>::encode(Encoder& encoder, cons > for (unsigned i = LinkTimeConstantCount; i--;) > m_linkTimeConstants[i] = codeBlock.m_linkTimeConstants[i]; > >- m_metadata.encode(encoder, codeBlock.m_metadata); >+ m_metadata.encode(encoder, codeBlock.m_metadata.get()); > m_rareData.encode(encoder, codeBlock.m_rareData); > > m_sourceURLDirective.encode(encoder, codeBlock.m_sourceURLDirective.impl());
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Flags:
saam
:
review+
ews-watchlist
:
commit-queue-
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 194031
:
361003
|
361004
| 361005 |
361009