[JSC] Use LazyNeverDestroyed & std::call_once for complex singletons

ysuzuki@apple.com · ysuzuki@apple.com · commit dc8c3924428a · 2020-08-05T04:19:05.000Z
https://bugs.webkit.org/show_bug.cgi?id=215153 <rdar://problem/65718983> Reviewed by Mark Lam. Source/JavaScriptCore: We are getting some crashes in RemoteInspector and this speculatively fixes the crash. My guess is that NeverDestroyed<RemoteInspector> calls constructor twice in heavily contended situation: WebKit's static does not have thread-safety. If two threads come here at the same time, it is possible that constructor is invoked twice. In that case, later constructor will clear members, which involves clearing Lock m_mutex field. This makes Lock's invariant broken. This patch uses LazyNeverDestroyed and std::call_once to ensure invoking constructor only once. * API/glib/JSCVirtualMachine.cpp: * dfg/DFGCommonData.cpp: * disassembler/Disassembler.cpp: * inspector/remote/RemoteInspector.h: * inspector/remote/cocoa/RemoteInspectorCocoa.mm: (Inspector::RemoteInspector::singleton): * inspector/remote/glib/RemoteInspectorGlib.cpp: (Inspector::RemoteInspector::singleton): * inspector/remote/socket/RemoteInspectorServer.cpp: (Inspector::RemoteInspectorServer::singleton): * inspector/remote/socket/RemoteInspectorServer.h: * inspector/remote/socket/RemoteInspectorSocket.cpp: (Inspector::RemoteInspector::singleton): * inspector/remote/socket/RemoteInspectorSocketEndpoint.cpp: (Inspector::RemoteInspectorSocketEndpoint::singleton): * interpreter/Interpreter.cpp: (JSC::Interpreter::opcodeIDTable): * runtime/IntlObject.cpp: (JSC::intlAvailableLocales): (JSC::intlCollatorAvailableLocales): (JSC::defaultLocale): (JSC::numberingSystemsForLocale): Source/WTF: Add lock's bits in crash information to investigate if this speculative fix does not work. * wtf/LockAlgorithmInlines.h: (WTF::Hooks>::lockSlow): (WTF::Hooks>::unlockSlow): git-svn-id: http://svn.webkit.org/repository/webkit/trunk@265276 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/Source/JavaScriptCore/API/glib/JSCVirtualMachine.cpp b/Source/JavaScriptCore/API/glib/JSCVirtualMachine.cpp
@@ -51,8 +51,12 @@ static Lock wrapperCacheMutex;
 
 static HashMap<JSContextGroupRef, JSCVirtualMachine*>& wrapperMap()
 {
-    static NeverDestroyed<HashMap<JSContextGroupRef, JSCVirtualMachine*>> map;
-    return map;
+    static LazyNeverDestroyed<HashMap<JSContextGroupRef, JSCVirtualMachine*>> shared;
+    static std::once_flag onceKey;
+    std::call_once(onceKey, [&] {
+        shared.construct();
+    });
+    return shared;
 }
 
 static void addWrapper(JSContextGroupRef group, JSCVirtualMachine* vm)
diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,41 @@
+2020-08-04  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [JSC] Use LazyNeverDestroyed & std::call_once for complex singletons
+        https://bugs.webkit.org/show_bug.cgi?id=215153
+        <rdar://problem/65718983>
+
+        Reviewed by Mark Lam.
+
+        We are getting some crashes in RemoteInspector and this speculatively fixes the crash.
+        My guess is that NeverDestroyed<RemoteInspector> calls constructor twice in heavily contended situation:
+        WebKit's static does not have thread-safety. If two threads come here at the same time, it is possible that
+        constructor is invoked twice. In that case, later constructor will clear members, which involves clearing
+        Lock m_mutex field. This makes Lock's invariant broken.
+        This patch uses LazyNeverDestroyed and std::call_once to ensure invoking constructor only once.
+
+        * API/glib/JSCVirtualMachine.cpp:
+        * dfg/DFGCommonData.cpp:
+        * disassembler/Disassembler.cpp:
+        * inspector/remote/RemoteInspector.h:
+        * inspector/remote/cocoa/RemoteInspectorCocoa.mm:
+        (Inspector::RemoteInspector::singleton):
+        * inspector/remote/glib/RemoteInspectorGlib.cpp:
+        (Inspector::RemoteInspector::singleton):
+        * inspector/remote/socket/RemoteInspectorServer.cpp:
+        (Inspector::RemoteInspectorServer::singleton):
+        * inspector/remote/socket/RemoteInspectorServer.h:
+        * inspector/remote/socket/RemoteInspectorSocket.cpp:
+        (Inspector::RemoteInspector::singleton):
+        * inspector/remote/socket/RemoteInspectorSocketEndpoint.cpp:
+        (Inspector::RemoteInspectorSocketEndpoint::singleton):
+        * interpreter/Interpreter.cpp:
+        (JSC::Interpreter::opcodeIDTable):
+        * runtime/IntlObject.cpp:
+        (JSC::intlAvailableLocales):
+        (JSC::intlCollatorAvailableLocales):
+        (JSC::defaultLocale):
+        (JSC::numberingSystemsForLocale):
+
 2020-08-04  Keith Miller  <keith_miller@apple.com>
 
         CheckpointSideState shoud play nicely with StackOverflowException unwinding.
diff --git a/Source/JavaScriptCore/dfg/DFGCommonData.cpp b/Source/JavaScriptCore/dfg/DFGCommonData.cpp
@@ -61,7 +61,11 @@ void CommonData::shrinkToFit()
 static Lock pcCodeBlockMapLock;
 inline HashMap<void*, CodeBlock*>& pcCodeBlockMap(AbstractLocker&)
 {
-    static NeverDestroyed<HashMap<void*, CodeBlock*>> pcCodeBlockMap;
+    static LazyNeverDestroyed<HashMap<void*, CodeBlock*>> pcCodeBlockMap;
+    static std::once_flag onceKey;
+    std::call_once(onceKey, [&] {
+        pcCodeBlockMap.construct();
+    });
     return pcCodeBlockMap;
 }
 
diff --git a/Source/JavaScriptCore/disassembler/Disassembler.cpp b/Source/JavaScriptCore/disassembler/Disassembler.cpp
@@ -119,8 +119,12 @@ bool hadAnyAsynchronousDisassembly = false;
 
 AsynchronousDisassembler& asynchronousDisassembler()
 {
-    static NeverDestroyed<AsynchronousDisassembler> disassembler;
-    hadAnyAsynchronousDisassembly = true;
+    static LazyNeverDestroyed<AsynchronousDisassembler> disassembler;
+    static std::once_flag onceKey;
+    std::call_once(onceKey, [&] {
+        disassembler.construct();
+        hadAnyAsynchronousDisassembly = true;
+    });
     return disassembler.get();
 }
 
diff --git a/Source/JavaScriptCore/inspector/remote/RemoteInspector.h b/Source/JavaScriptCore/inspector/remote/RemoteInspector.h
@@ -119,7 +119,7 @@ class JS_EXPORT_PRIVATE RemoteInspector final
 #endif
     static void startDisabled();
     static RemoteInspector& singleton();
-    friend class NeverDestroyed<RemoteInspector>;
+    friend class LazyNeverDestroyed<RemoteInspector>;
 
     void registerTarget(RemoteControllableTarget*);
     void unregisterTarget(RemoteControllableTarget*);
diff --git a/Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm b/Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm
@@ -85,7 +85,11 @@ static bool globalAutomaticInspectionState()
 
 RemoteInspector& RemoteInspector::singleton()
 {
-    static NeverDestroyed<RemoteInspector> shared;
+    static LazyNeverDestroyed<RemoteInspector> shared;
+    static dispatch_once_t onceConstructKey;
+    dispatch_once(&onceConstructKey, ^{
+        shared.construct();
+    });
 
 #if PLATFORM(COCOA)
     if (needMachSandboxExtension)
diff --git a/Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp b/Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp
@@ -40,7 +40,11 @@ namespace Inspector {
 
 RemoteInspector& RemoteInspector::singleton()
 {
-    static NeverDestroyed<RemoteInspector> shared;
+    static LazyNeverDestroyed<RemoteInspector> shared;
+    static std::once_flag onceKey;
+    std::call_once(onceKey, [&] {
+        shared.construct();
+    });
     return shared;
 }
 
diff --git a/Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorServer.cpp b/Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorServer.cpp
@@ -35,8 +35,12 @@ namespace Inspector {
 
 RemoteInspectorServer& RemoteInspectorServer::singleton()
 {
-    static NeverDestroyed<RemoteInspectorServer> server;
-    return server;
+    static LazyNeverDestroyed<RemoteInspectorServer> shared;
+    static std::once_flag onceKey;
+    std::call_once(onceKey, [&] {
+        shared.construct();
+    });
+    return shared;
 }
 
 RemoteInspectorServer::~RemoteInspectorServer()
diff --git a/Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorServer.h b/Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorServer.h
@@ -43,7 +43,7 @@ class RemoteInspectorServer final : public RemoteInspectorSocketEndpoint::Listen
     bool isRunning() const { return !!m_server; }
 
 private:
-    friend class NeverDestroyed<RemoteInspectorServer>;
+    friend class LazyNeverDestroyed<RemoteInspectorServer>;
     RemoteInspectorServer() { Socket::init(); }
 
     bool didAccept(ConnectionID acceptedID, ConnectionID listenerID, Socket::Domain) final;
diff --git a/Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorSocket.cpp b/Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorSocket.cpp
@@ -41,7 +41,11 @@ namespace Inspector {
 
 RemoteInspector& RemoteInspector::singleton()
 {
-    static NeverDestroyed<RemoteInspector> shared;
+    static LazyNeverDestroyed<RemoteInspector> shared;
+    static std::once_flag onceKey;
+    std::call_once(onceKey, [&] {
+        shared.construct();
+    });
     return shared;
 }
 
diff --git a/Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorSocketEndpoint.cpp b/Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorSocketEndpoint.cpp
@@ -37,7 +37,11 @@ namespace Inspector {
 
 RemoteInspectorSocketEndpoint& RemoteInspectorSocketEndpoint::singleton()
 {
-    static NeverDestroyed<RemoteInspectorSocketEndpoint> shared;
+    static LazyNeverDestroyed<RemoteInspectorSocketEndpoint> shared;
+    static std::once_flag onceKey;
+    std::call_once(onceKey, [&] {
+        shared.construct();
+    });
     return shared;
 }
 
diff --git a/Source/JavaScriptCore/interpreter/Interpreter.cpp b/Source/JavaScriptCore/interpreter/Interpreter.cpp
@@ -336,13 +336,14 @@ Interpreter::~Interpreter()
 #if !ENABLE(LLINT_EMBEDDED_OPCODE_ID) || ASSERT_ENABLED
 HashMap<Opcode, OpcodeID>& Interpreter::opcodeIDTable()
 {
-    static NeverDestroyed<HashMap<Opcode, OpcodeID>> opcodeIDTable;
+    static LazyNeverDestroyed<HashMap<Opcode, OpcodeID>> opcodeIDTable;
 
     static std::once_flag initializeKey;
     std::call_once(initializeKey, [&] {
+        opcodeIDTable.construct();
         const Opcode* opcodeTable = LLInt::opcodeMap();
         for (unsigned i = 0; i < NUMBER_OF_BYTECODE_IDS; ++i)
-            opcodeIDTable.get().add(opcodeTable[i], static_cast<OpcodeID>(i));
+            opcodeIDTable->add(opcodeTable[i], static_cast<OpcodeID>(i));
     });
 
     return opcodeIDTable;
diff --git a/Source/JavaScriptCore/runtime/IntlObject.cpp b/Source/JavaScriptCore/runtime/IntlObject.cpp
@@ -241,41 +241,39 @@ static void addScriptlessLocaleIfNeeded(HashSet<String>& availableLocales, Strin
 
 const HashSet<String>& intlAvailableLocales()
 {
-    static NeverDestroyed<HashSet<String>> cachedAvailableLocales;
-    HashSet<String>& availableLocales = cachedAvailableLocales.get();
-
+    static LazyNeverDestroyed<HashSet<String>> availableLocales;
     static std::once_flag initializeOnce;
     std::call_once(initializeOnce, [&] {
-        ASSERT(availableLocales.isEmpty());
+        availableLocales.construct();
+        ASSERT(availableLocales->isEmpty());
         constexpr bool isImmortal = true;
         int32_t count = uloc_countAvailable();
         for (int32_t i = 0; i < count; ++i) {
             String locale = languageTagForLocaleID(uloc_getAvailable(i), isImmortal);
             if (locale.isEmpty())
                 continue;
-            availableLocales.add(locale);
-            addScriptlessLocaleIfNeeded(availableLocales, locale);
+            availableLocales->add(locale);
+            addScriptlessLocaleIfNeeded(availableLocales.get(), locale);
         }
     });
     return availableLocales;
 }
 
 const HashSet<String>& intlCollatorAvailableLocales()
 {
-    static NeverDestroyed<HashSet<String>> cachedAvailableLocales;
-    HashSet<String>& availableLocales = cachedAvailableLocales.get();
-
+    static LazyNeverDestroyed<HashSet<String>> availableLocales;
     static std::once_flag initializeOnce;
     std::call_once(initializeOnce, [&] {
-        ASSERT(availableLocales.isEmpty());
+        availableLocales.construct();
+        ASSERT(availableLocales->isEmpty());
         constexpr bool isImmortal = true;
         int32_t count = ucol_countAvailable();
         for (int32_t i = 0; i < count; ++i) {
             String locale = languageTagForLocaleID(ucol_getAvailable(i), isImmortal);
             if (locale.isEmpty())
                 continue;
-            availableLocales.add(locale);
-            addScriptlessLocaleIfNeeded(availableLocales, locale);
+            availableLocales->add(locale);
+            addScriptlessLocaleIfNeeded(availableLocales.get(), locale);
         }
     });
     return availableLocales;
@@ -521,11 +519,11 @@ String defaultLocale(JSGlobalObject* globalObject)
 
     // If all else fails, ask ICU. It will probably say something bogus like en_us even if the user
     // has configured some other language, but being wrong is better than crashing.
-    static NeverDestroyed<String> icuDefaultLocalString;
+    static LazyNeverDestroyed<String> icuDefaultLocalString;
     static std::once_flag initializeOnce;
     std::call_once(initializeOnce, [&] {
         constexpr bool isImmortal = true;
-        icuDefaultLocalString.get() = languageTagForLocaleID(uloc_getDefault(), isImmortal);
+        icuDefaultLocalString.construct(languageTagForLocaleID(uloc_getDefault(), isImmortal));
     });
     if (!icuDefaultLocalString->isEmpty())
         return icuDefaultLocalString.get();
@@ -778,12 +776,11 @@ JSValue supportedLocales(JSGlobalObject* globalObject, const HashSet<String>& av
 
 Vector<String> numberingSystemsForLocale(const String& locale)
 {
-    static NeverDestroyed<Vector<String>> cachedNumberingSystems;
-    Vector<String>& availableNumberingSystems = cachedNumberingSystems.get();
-
+    static LazyNeverDestroyed<Vector<String>> availableNumberingSystems;
     static std::once_flag initializeOnce;
     std::call_once(initializeOnce, [&] {
-        ASSERT(availableNumberingSystems.isEmpty());
+        availableNumberingSystems.construct();
+        ASSERT(availableNumberingSystems->isEmpty());
         UErrorCode status = U_ZERO_ERROR;
         UEnumeration* numberingSystemNames = unumsys_openAvailableNames(&status);
         ASSERT(U_SUCCESS(status));
@@ -796,7 +793,7 @@ Vector<String> numberingSystemsForLocale(const String& locale)
             ASSERT(U_SUCCESS(status));
             // Only support algorithmic if it is the default fot the locale, handled below.
             if (!unumsys_isAlgorithmic(numsys))
-                availableNumberingSystems.append(String(StringImpl::createStaticStringImpl(result, resultLength)));
+                availableNumberingSystems->append(String(StringImpl::createStaticStringImpl(result, resultLength)));
             unumsys_close(numsys);
         }
         uenum_close(numberingSystemNames);
@@ -809,7 +806,7 @@ Vector<String> numberingSystemsForLocale(const String& locale)
     unumsys_close(defaultSystem);
 
     Vector<String> numberingSystems({ defaultSystemName });
-    numberingSystems.appendVector(availableNumberingSystems);
+    numberingSystems.appendVector(availableNumberingSystems.get());
     return numberingSystems;
 }
 
diff --git a/Source/WTF/ChangeLog b/Source/WTF/ChangeLog
@@ -1,3 +1,17 @@
+2020-08-04  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [JSC] Use LazyNeverDestroyed & std::call_once for complex singletons
+        https://bugs.webkit.org/show_bug.cgi?id=215153
+        <rdar://problem/65718983>
+
+        Reviewed by Mark Lam.
+
+        Add lock's bits in crash information to investigate if this speculative fix does not work.
+
+        * wtf/LockAlgorithmInlines.h:
+        (WTF::Hooks>::lockSlow):
+        (WTF::Hooks>::unlockSlow):
+
 2020-08-04  Keith Miller  <keith_miller@apple.com>
 
         CheckpointSideState shoud play nicely with StackOverflowException unwinding.
diff --git a/Source/WTF/wtf/LockAlgorithmInlines.h b/Source/WTF/wtf/LockAlgorithmInlines.h
@@ -72,11 +72,11 @@ void LockAlgorithm<LockType, isHeldBit, hasParkedBit, Hooks>::lockSlow(Atomic<Lo
         
         if (!(currentValue & isHeldBit)) {
             dataLog("Lock not held!\n");
-            RELEASE_ASSERT_NOT_REACHED();
+            CRASH_WITH_INFO(currentValue);
         }
         if (!(currentValue & hasParkedBit)) {
             dataLog("Lock not parked!\n");
-            RELEASE_ASSERT_NOT_REACHED();
+            CRASH_WITH_INFO(currentValue);
         }
 
         // We now expect the value to be isHeld|hasParked. So long as that's the case, we can park.
@@ -113,7 +113,7 @@ void LockAlgorithm<LockType, isHeldBit, hasParkedBit, Hooks>::unlockSlow(Atomic<
         if ((oldByteValue & mask) != isHeldBit
             && (oldByteValue & mask) != (isHeldBit | hasParkedBit)) {
             dataLog("Invalid value for lock: ", oldByteValue, "\n");
-            RELEASE_ASSERT_NOT_REACHED();
+            CRASH_WITH_INFO(oldByteValue);
         }
         
         if ((oldByteValue & mask) == isHeldBit) {

Original file line number	Diff line number	Diff line change
`@@ -51,8 +51,12 @@ static Lock wrapperCacheMutex;`
`51`	`51`
`52`	`52`	`static HashMap<JSContextGroupRef, JSCVirtualMachine*>& wrapperMap()`
`53`	`53`	`{`
`54`		`- static NeverDestroyed<HashMap<JSContextGroupRef, JSCVirtualMachine*>> map;`
`55`		`- return map;`
	`54`	`+ static LazyNeverDestroyed<HashMap<JSContextGroupRef, JSCVirtualMachine*>> shared;`
	`55`	`+ static std::once_flag onceKey;`
	`56`	`+ std::call_once(onceKey, [&] {`
	`57`	`+ shared.construct();`
	`58`	`+ });`
	`59`	`+ return shared;`
`56`	`60`	`}`
`57`	`61`
`58`	`62`	`static void addWrapper(JSContextGroupRef group, JSCVirtualMachine* vm)`
Original file line number	Diff line number	Diff line change
`@@ -61,7 +61,11 @@ void CommonData::shrinkToFit()`
`61`	`61`	`static Lock pcCodeBlockMapLock;`
`62`	`62`	`inline HashMap<void, CodeBlock>& pcCodeBlockMap(AbstractLocker&)`
`63`	`63`	`{`
`64`		`- static NeverDestroyed<HashMap<void, CodeBlock>> pcCodeBlockMap;`
	`64`	`+ static LazyNeverDestroyed<HashMap<void, CodeBlock>> pcCodeBlockMap;`
	`65`	`+ static std::once_flag onceKey;`
	`66`	`+ std::call_once(onceKey, [&] {`
	`67`	`+ pcCodeBlockMap.construct();`
	`68`	`+ });`
`65`	`69`	`return pcCodeBlockMap;`
`66`	`70`	`}`
`67`	`71`
Original file line number	Diff line number	Diff line change
`@@ -119,8 +119,12 @@ bool hadAnyAsynchronousDisassembly = false;`
`119`	`119`
`120`	`120`	`AsynchronousDisassembler& asynchronousDisassembler()`
`121`	`121`	`{`
`122`		`- static NeverDestroyed<AsynchronousDisassembler> disassembler;`
`123`		`- hadAnyAsynchronousDisassembly = true;`
	`122`	`+ static LazyNeverDestroyed<AsynchronousDisassembler> disassembler;`
	`123`	`+ static std::once_flag onceKey;`
	`124`	`+ std::call_once(onceKey, [&] {`
	`125`	`+ disassembler.construct();`
	`126`	`+ hadAnyAsynchronousDisassembly = true;`
	`127`	`+ });`
`124`	`128`	`return disassembler.get();`
`125`	`129`	`}`
`126`	`130`
Original file line number	Diff line number	Diff line change
`@@ -40,7 +40,11 @@ namespace Inspector {`
`40`	`40`
`41`	`41`	`RemoteInspector& RemoteInspector::singleton()`
`42`	`42`	`{`
`43`		`- static NeverDestroyed<RemoteInspector> shared;`
	`43`	`+ static LazyNeverDestroyed<RemoteInspector> shared;`
	`44`	`+ static std::once_flag onceKey;`
	`45`	`+ std::call_once(onceKey, [&] {`
	`46`	`+ shared.construct();`
	`47`	`+ });`
`44`	`48`	`return shared;`
`45`	`49`	`}`
`46`	`50`
Original file line number	Diff line number	Diff line change
`@@ -35,8 +35,12 @@ namespace Inspector {`
`35`	`35`
`36`	`36`	`RemoteInspectorServer& RemoteInspectorServer::singleton()`
`37`	`37`	`{`
`38`		`- static NeverDestroyed<RemoteInspectorServer> server;`
`39`		`- return server;`
	`38`	`+ static LazyNeverDestroyed<RemoteInspectorServer> shared;`
	`39`	`+ static std::once_flag onceKey;`
	`40`	`+ std::call_once(onceKey, [&] {`
	`41`	`+ shared.construct();`
	`42`	`+ });`
	`43`	`+ return shared;`
`40`	`44`	`}`
`41`	`45`
`42`	`46`	`RemoteInspectorServer::~RemoteInspectorServer()`
Original file line number	Diff line number	Diff line change
`@@ -41,7 +41,11 @@ namespace Inspector {`
`41`	`41`
`42`	`42`	`RemoteInspector& RemoteInspector::singleton()`
`43`	`43`	`{`
`44`		`- static NeverDestroyed<RemoteInspector> shared;`
	`44`	`+ static LazyNeverDestroyed<RemoteInspector> shared;`
	`45`	`+ static std::once_flag onceKey;`
	`46`	`+ std::call_once(onceKey, [&] {`
	`47`	`+ shared.construct();`
	`48`	`+ });`
`45`	`49`	`return shared;`
`46`	`50`	`}`
`47`	`51`