Attachment 361005 Details for Bug 194031 – Patch

[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:

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());

Flags:

saam: review+
ews-watchlist: commit-queue-

Actions: View | Formatted Diff | Diff

Attachments on bug 194031: 361003 | 361004 | 361005 | 361009