Bug 1959367 - Protect nsURLHelper singletons against unwanted initializations. r=necko-reviewers,jesup

Differential Revision: https://phabricator.services.mozilla.com/D244903

dom/media/webrtc/jsapi/MediaTransportHandler.cpp

@@ -360,9 +360,10 @@ static nsresult addNrIceServer(const nsString& aIceUrl,
       path.SetLength(questionmark);
     }
 
-    rv = net_GetAuthURLParser()->ParseAuthority(
+    nsCOMPtr<nsIURLParser> parser = net_GetAuthURLParser();
-        path.get(), static_cast<int>(path.Length()), nullptr, nullptr, nullptr,
+    rv = parser->ParseAuthority(path.get(), static_cast<int>(path.Length()),
-        nullptr, &hostPos, &hostLen, &port);
+                                nullptr, nullptr, nullptr, nullptr, &hostPos,
+                                &hostLen, &port);
     NS_ENSURE_SUCCESS(rv, rv);
     if (!hostLen) {
       return NS_ERROR_FAILURE;

netwerk/base/nsURLHelper.cpp

@@ -6,7 +6,9 @@
 
 #include "nsURLHelper.h"
 
+#include "mozilla/AppShutdown.h"
 #include "mozilla/Encoding.h"
+#include "mozilla/Mutex.h"
 #include "mozilla/RangedPtr.h"
 #include "mozilla/TextUtils.h"
 
@@ -34,61 +36,81 @@ using namespace mozilla;
 // Init/Shutdown
 //----------------------------------------------------------------------------
 
-static bool gInitialized = false;
+// We protect only the initialization with the mutex, such that we cannot
+// annotate the following static variables.
+static StaticMutex gInitLock MOZ_UNANNOTATED;
+// The relaxed memory ordering is fine here as we write this only when holding
+// gInitLock and only ever set it true once during EnsureGlobalsAreInited.
+static Atomic<bool, MemoryOrdering::Relaxed> gInitialized(false);
 static StaticRefPtr<nsIURLParser> gNoAuthURLParser;
 static StaticRefPtr<nsIURLParser> gAuthURLParser;
 static StaticRefPtr<nsIURLParser> gStdURLParser;
 
-static void InitGlobals() {
+static void EnsureGlobalsAreInited() {
-  nsCOMPtr<nsIURLParser> parser;
+  if (!gInitialized) {
+    StaticMutexAutoLock lock(gInitLock);
+    // Taking the lock will sync us with any other thread's write in case we
+    // saw a stale value above, thus we need to check again.
+    if (gInitialized) {
+      return;
+    }
 
-  parser = do_GetService(NS_NOAUTHURLPARSER_CONTRACTID);
+    nsCOMPtr<nsIURLParser> parser;
-  NS_ASSERTION(parser, "failed getting 'noauth' url parser");
+
-  if (parser) {
+    parser = do_GetService(NS_NOAUTHURLPARSER_CONTRACTID);
-    gNoAuthURLParser = parser;
+    NS_ASSERTION(parser, "failed getting 'noauth' url parser");
+    if (parser) {
+      gNoAuthURLParser = parser.forget();
+    }
+
+    parser = do_GetService(NS_AUTHURLPARSER_CONTRACTID);
+    NS_ASSERTION(parser, "failed getting 'auth' url parser");
+    if (parser) {
+      gAuthURLParser = parser.forget();
+    }
+
+    parser = do_GetService(NS_STDURLPARSER_CONTRACTID);
+    NS_ASSERTION(parser, "failed getting 'std' url parser");
+    if (parser) {
+      gStdURLParser = parser.forget();
+    }
+
+    gInitialized = true;
   }
-
-  parser = do_GetService(NS_AUTHURLPARSER_CONTRACTID);
-  NS_ASSERTION(parser, "failed getting 'auth' url parser");
-  if (parser) {
-    gAuthURLParser = parser;
-  }
-
-  parser = do_GetService(NS_STDURLPARSER_CONTRACTID);
-  NS_ASSERTION(parser, "failed getting 'std' url parser");
-  if (parser) {
-    gStdURLParser = parser;
-  }
-
-  gInitialized = true;
 }
 
 void net_ShutdownURLHelper() {
   if (gInitialized) {
-    gInitialized = false;
+    // We call this late in XPCOM shutdown when there is only the main thread
+    // left, so we can safely release the static pointers here.
+    MOZ_ASSERT(AppShutdown::IsInOrBeyond(ShutdownPhase::XPCOMShutdownFinal));
+    gNoAuthURLParser = nullptr;
+    gAuthURLParser = nullptr;
+    gStdURLParser = nullptr;
+    // We keep gInitialized true to protect us from resurrection.
   }
-  gNoAuthURLParser = nullptr;
-  gAuthURLParser = nullptr;
-  gStdURLParser = nullptr;
 }
 
 //----------------------------------------------------------------------------
 // nsIURLParser getters
 //----------------------------------------------------------------------------
 
-nsIURLParser* net_GetAuthURLParser() {
+already_AddRefed<nsIURLParser> net_GetAuthURLParser() {
-  if (!gInitialized) InitGlobals();
+  EnsureGlobalsAreInited();
-  return gAuthURLParser;
+  RefPtr<nsIURLParser> keepMe = gAuthURLParser;
+  return keepMe.forget();
 }
 
-nsIURLParser* net_GetNoAuthURLParser() {
+already_AddRefed<nsIURLParser> net_GetNoAuthURLParser() {
-  if (!gInitialized) InitGlobals();
+  EnsureGlobalsAreInited();
-  return gNoAuthURLParser;
+  RefPtr<nsIURLParser> keepMe = gNoAuthURLParser;
+  return keepMe.forget();
 }
 
-nsIURLParser* net_GetStdURLParser() {
+already_AddRefed<nsIURLParser> net_GetStdURLParser() {
-  if (!gInitialized) InitGlobals();
+  EnsureGlobalsAreInited();
-  return gStdURLParser;
+  RefPtr<nsIURLParser> keepMe = gStdURLParser;
+  return keepMe.forget();
 }
 
 //---------------------------------------------------------------------------
@@ -157,7 +179,7 @@ nsresult net_ParseFileURL(const nsACString& inURL, nsACString& outDirectory,
     return NS_ERROR_UNEXPECTED;
   }
 
-  nsIURLParser* parser = net_GetNoAuthURLParser();
+  nsCOMPtr<nsIURLParser> parser = net_GetNoAuthURLParser();
   NS_ENSURE_TRUE(parser, NS_ERROR_UNEXPECTED);
 
   uint32_t pathPos, filepathPos, directoryPos, basenamePos, extensionPos;

netwerk/base/nsURLHelper.h

@@ -26,9 +26,9 @@ void net_ShutdownURLHelperOSX();
 #endif
 
 /* access URL parsers */
-nsIURLParser* net_GetAuthURLParser();
+already_AddRefed<nsIURLParser> net_GetAuthURLParser();
-nsIURLParser* net_GetNoAuthURLParser();
+already_AddRefed<nsIURLParser> net_GetNoAuthURLParser();
-nsIURLParser* net_GetStdURLParser();
+already_AddRefed<nsIURLParser> net_GetStdURLParser();
 
 /* convert between nsIFile and file:// URL spec
  * net_GetURLSpecFromFile does an extra stat, so callers should