[webkit-changes] [240915] trunk/Source/JavaScriptCore

Sun, 03 Feb 2019 23:17:11 -0800

Title: [240915] trunk/Source/_javascript_Core
Revision
240915
Author
[email protected]
Date
2019-02-03 23:13:00 -0800 (Sun, 03 Feb 2019)

Log Message

[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 Saam Barati.

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

Modified Paths

Diff

Modified: trunk/Source/_javascript_Core/ChangeLog (240914 => 240915)


--- trunk/Source/_javascript_Core/ChangeLog	2019-02-04 02:15:55 UTC (rev 240914)
+++ trunk/Source/_javascript_Core/ChangeLog	2019-02-04 07:13:00 UTC (rev 240915)
@@ -1,3 +1,40 @@
+2019-02-03  Yusuke Suzuki  <[email protected]>
+
+        [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 Saam Barati.
+
+        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  <[email protected]>
 
         [JSC] Decouple JIT related data from CodeBlock

Modified: trunk/Source/_javascript_Core/bytecode/MetadataTable.cpp (240914 => 240915)


--- trunk/Source/_javascript_Core/bytecode/MetadataTable.cpp	2019-02-04 02:15:55 UTC (rev 240914)
+++ trunk/Source/_javascript_Core/bytecode/MetadataTable.cpp	2019-02-04 07:13:00 UTC (rev 240915)
@@ -35,8 +35,8 @@
 
 MetadataTable::MetadataTable(UnlinkedMetadataTable& unlinkedMetadata)
 {
-    linkingData() = UnlinkedMetadataTable::LinkingData {
-        &unlinkedMetadata,
+    new (&linkingData()) UnlinkedMetadataTable::LinkingData {
+        unlinkedMetadata,
         1,
     };
 }
@@ -55,7 +55,11 @@
 {
     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()

Modified: trunk/Source/_javascript_Core/bytecode/UnlinkedCodeBlock.cpp (240914 => 240915)


--- trunk/Source/_javascript_Core/bytecode/UnlinkedCodeBlock.cpp	2019-02-04 02:15:55 UTC (rev 240914)
+++ trunk/Source/_javascript_Core/bytecode/UnlinkedCodeBlock.cpp	2019-02-04 07:13:00 UTC (rev 240915)
@@ -57,6 +57,7 @@
 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 @@
     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 @@
 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 @@
     {
         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

Modified: trunk/Source/_javascript_Core/bytecode/UnlinkedCodeBlock.h (240914 => 240915)


--- trunk/Source/_javascript_Core/bytecode/UnlinkedCodeBlock.h	2019-02-04 02:15:55 UTC (rev 240914)
+++ trunk/Source/_javascript_Core/bytecode/UnlinkedCodeBlock.h	2019-02-04 07:13:00 UTC (rev 240915)
@@ -360,11 +360,11 @@
     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 @@
 
     String m_sourceURLDirective;
     String m_sourceMappingURLDirective;
-    UnlinkedMetadataTable m_metadata;
+    Ref<UnlinkedMetadataTable> m_metadata;
 
 #if ENABLE(DFG_JIT)
     DFG::ExitProfile m_exitProfile;

Modified: trunk/Source/_javascript_Core/bytecode/UnlinkedMetadataTable.h (240914 => 240915)


--- trunk/Source/_javascript_Core/bytecode/UnlinkedMetadataTable.h	2019-02-04 02:15:55 UTC (rev 240914)
+++ trunk/Source/_javascript_Core/bytecode/UnlinkedMetadataTable.h	2019-02-04 07:13:00 UTC (rev 240915)
@@ -32,7 +32,7 @@
 
 class MetadataTable;
 
-class UnlinkedMetadataTable {
+class UnlinkedMetadataTable : public RefCounted<UnlinkedMetadataTable> {
     friend class LLIntOffsetsExtractor;
     friend class MetadataTable;
     friend class CachedMetadataTable;
@@ -39,11 +39,10 @@
 
 public:
     struct LinkingData {
-        UnlinkedMetadataTable* unlinkedMetadata;
+        Ref<UnlinkedMetadataTable> unlinkedMetadata;
         unsigned refCount;
     };
 
-    UnlinkedMetadataTable();
     ~UnlinkedMetadataTable();
 
     unsigned addEntry(OpcodeID);
@@ -54,7 +53,14 @@
 
     RefPtr<MetadataTable> link();
 
+    static Ref<UnlinkedMetadataTable> create()
+    {
+        return adoptRef(*new UnlinkedMetadataTable);
+    }
+
 private:
+    UnlinkedMetadataTable();
+
     void unlink(MetadataTable&);
 
     size_t sizeInBytes(MetadataTable&);

Modified: trunk/Source/_javascript_Core/bytecode/UnlinkedMetadataTableInlines.h (240914 => 240915)


--- trunk/Source/_javascript_Core/bytecode/UnlinkedMetadataTableInlines.h	2019-02-04 02:15:55 UTC (rev 240914)
+++ trunk/Source/_javascript_Core/bytecode/UnlinkedMetadataTableInlines.h	2019-02-04 07:13:00 UTC (rev 240915)
@@ -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()

Modified: trunk/Source/_javascript_Core/runtime/CachedTypes.cpp (240914 => 240915)


--- trunk/Source/_javascript_Core/runtime/CachedTypes.cpp	2019-02-04 02:15:55 UTC (rev 240914)
+++ trunk/Source/_javascript_Core/runtime/CachedTypes.cpp	2019-02-04 07:13:00 UTC (rev 240915)
@@ -1170,13 +1170,15 @@
             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 @@
     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 @@
     , 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 @@
     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 @@
     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());

_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to