[JSC] Mitigate null UnlinkedMetadataTable pointer in CodeBlock destru…

…ctor

https://bugs.webkit.org/show_bug.cgi?id=272787
rdar://121747906

Reviewed by Yusuke Suzuki.

Attempts to fix a rare bug where the UnlinkedMetadataTable pointer accessed
in the CodeBlock destructor can become null. We think this may be due to a
series of thread-unsafe reference count operations that might allow the
destructor to happen twice, perhaps simultaneously on two threads. This
patch attempts to mitigate this by:

 1. Making UnlinkedMetadataTable and MetadataTable thread-safe refcounted.

 2. Checking for the presence of a null UnlinkedMetadataTable pointer in the
    appropriate functions, and attempting to handle it nonfatally. This means
    we skip updating the didOptimize state in the CodeBlock destructor, and
    that we intentionally leak MetadataTables if they have this null pointer.

* Source/JavaScriptCore/bytecode/CodeBlock.cpp:
(JSC::CodeBlock::~CodeBlock):
* Source/JavaScriptCore/bytecode/MetadataTable.cpp:
(JSC::MetadataTable::destroy):
(JSC::MetadataTable::sizeInBytesForGC):
* Source/JavaScriptCore/bytecode/MetadataTable.h:
(JSC::MetadataTable::forEachValueProfile):
(JSC::MetadataTable::valueProfileForOffset):
(JSC::MetadataTable::deref):
(JSC::MetadataTable::unlinkedMetadata const):
(JSC::MetadataTable::totalSize const):
* Source/JavaScriptCore/bytecode/UnlinkedMetadataTable.h:

Originally-landed-as: 4cac7925aca4. rdar://128091467
Canonical link: https://commits.webkit.org/278832@main

Source/JavaScriptCore/bytecode/CodeBlock.cpp

@@ -853,8 +853,12 @@ CodeBlock::~CodeBlock()
 
     if (LIKELY(!vm.heap.isShuttingDown())) {
         if (m_metadata) {
-            if (m_metadata->unlinkedMetadata().didOptimize() == TriState::Indeterminate)
-                m_metadata->unlinkedMetadata().setDidOptimize(TriState::False);
+            auto unlinkedMetadata = m_metadata->unlinkedMetadata();
+
+            // FIXME: This check should really not be necessary, see https://webkit.org/b/272787
+            ASSERT(unlinkedMetadata);
+            if (unlinkedMetadata && unlinkedMetadata->didOptimize() == TriState::Indeterminate)
+                unlinkedMetadata->setDidOptimize(TriState::False);
         }
     }
 

Source/JavaScriptCore/bytecode/MetadataTable.cpp

@@ -61,16 +61,24 @@ MetadataTable::~MetadataTable()
 
 void MetadataTable::destroy(MetadataTable* table)
 {
-    Ref<UnlinkedMetadataTable> unlinkedMetadata = WTFMove(table->linkingData().unlinkedMetadata);
+    RefPtr<UnlinkedMetadataTable> unlinkedMetadata = WTFMove(table->linkingData().unlinkedMetadata);
+
     table->~MetadataTable();
+
+    // FIXME: This check should really not be necessary, see https://webkit.org/b/272787
+    if (UNLIKELY(!unlinkedMetadata)) {
+        ASSERT_NOT_REACHED();
+        return;
+    }
+
     // Since UnlinkedMetadata::unlink frees the underlying memory of MetadataTable.
     // We need to destroy LinkingData before calling it.
     unlinkedMetadata->unlink(*table);
 }
 
 size_t MetadataTable::sizeInBytesForGC()
 {
-    return unlinkedMetadata().sizeInBytesForGC(*this);
+    return unlinkedMetadata()->sizeInBytesForGC(*this);
 }
 
 void MetadataTable::validate() const

Source/JavaScriptCore/bytecode/MetadataTable.h

@@ -71,7 +71,7 @@ class MetadataTable {
     ALWAYS_INLINE void forEachValueProfile(const Functor& func)
     {
         // We could do a checked multiply here but if it overflows we'd just not look at any value profiles so it's probably not worth it.
-        int lastValueProfileOffset = -unlinkedMetadata().m_numValueProfiles;
+        int lastValueProfileOffset = -unlinkedMetadata()->m_numValueProfiles;
         for (int i = -1; i >= lastValueProfileOffset; --i)
             func(valueProfilesEnd()[i]);
     }
@@ -83,7 +83,7 @@ class MetadataTable {
 
     ValueProfile& valueProfileForOffset(unsigned profileOffset)
     {
-        ASSERT(profileOffset <= unlinkedMetadata().m_numValueProfiles);
+        ASSERT(profileOffset <= unlinkedMetadata()->m_numValueProfiles);
         return valueProfilesEnd()[-static_cast<ptrdiff_t>(profileOffset)];
     }
 
@@ -96,12 +96,15 @@ class MetadataTable {
 
     void deref()
     {
-        unsigned tempRefCount = linkingData().refCount - 1;
-        if (!tempRefCount) {
+        if (!--linkingData().refCount) {
+            // Setting refCount to 1 here prevents double delete within the destructor but not from another thread
+            // since such a thread could have ref'ed this object long after it had been deleted. This is consistent
+            // with ThreadSafeRefCounted.h, see webkit.org/b/201576 for the reasoning.
+            linkingData().refCount = 1;
+
             MetadataTable::destroy(this);
             return;
         }
-        linkingData().refCount = tempRefCount;
     }
 
     unsigned refCount() const
@@ -124,7 +127,7 @@ class MetadataTable {
 
     void validate() const;
 
-    UnlinkedMetadataTable& unlinkedMetadata() const { return linkingData().unlinkedMetadata.get(); }
+    RefPtr<UnlinkedMetadataTable> unlinkedMetadata() const { return static_reference_cast<UnlinkedMetadataTable>(linkingData().unlinkedMetadata); }
 
 private:
     MetadataTable(UnlinkedMetadataTable&);
@@ -134,7 +137,7 @@ class MetadataTable {
 
     size_t totalSize() const
     {
-        return unlinkedMetadata().m_numValueProfiles * sizeof(ValueProfile) + sizeof(UnlinkedMetadataTable::LinkingData) + getOffset(UnlinkedMetadataTable::s_offsetTableEntries - 1);
+        return unlinkedMetadata()->m_numValueProfiles * sizeof(ValueProfile) + sizeof(UnlinkedMetadataTable::LinkingData) + getOffset(UnlinkedMetadataTable::s_offsetTableEntries - 1);
     }
 
     UnlinkedMetadataTable::LinkingData& linkingData() const

Source/JavaScriptCore/bytecode/UnlinkedMetadataTable.h

@@ -56,7 +56,7 @@ struct MetadataStatistics {
 #endif
 
 
-class UnlinkedMetadataTable : public RefCounted<UnlinkedMetadataTable> {
+class UnlinkedMetadataTable : public ThreadSafeRefCounted<UnlinkedMetadataTable> {
     WTF_MAKE_FAST_ALLOCATED_WITH_HEAP_IDENTIFIER(UnlinkedMetadataTable);
     friend class LLIntOffsetsExtractor;
     friend class MetadataTable;
@@ -69,7 +69,7 @@ class UnlinkedMetadataTable : public RefCounted<UnlinkedMetadataTable> {
 
     struct LinkingData {
         Ref<UnlinkedMetadataTable> unlinkedMetadata;
-        unsigned refCount;
+        std::atomic<unsigned> refCount;
     };
 
     ~UnlinkedMetadataTable();