Bug 1344751 - Make nsStandardURL and nsIDNService available on worker threads. r=valentin

netwerk/base/nsStandardURL.cpp

@@ -23,6 +23,7 @@
 #include "mozilla/MemoryReporting.h"
 #include "mozilla/ipc/URIUtils.h"
 #include <algorithm>
+#include "mozilla/SyncRunnable.h"
 #include "nsContentUtils.h"
 #include "prprf.h"
 #include "nsReadableUtils.h"
@@ -42,7 +43,9 @@ static LazyLogModule gStandardURLLog("nsStandardURL");
 #ifdef MOZ_RUST_URLPARSE
 
 #include "RustURL.h"
-bool nsStandardURL::gRustEnabled = false;
+
+// Modified on the main thread, read on both main thread and worker threads.
+Atomic<bool> nsStandardURL::gRustEnabled(false);
 
 // Fall back to CPP-parsed URLs if the Rust one doesn't match.
 #define MOZ_RUST_URLPARSE_FALLBACK
@@ -137,9 +140,16 @@ namespace net {
 static NS_DEFINE_CID(kThisImplCID, NS_THIS_STANDARDURL_IMPL_CID);
 static NS_DEFINE_CID(kStandardURLCID, NS_STANDARDURL_CID);
 
+// This will always be initialized and destroyed on the main thread, but
+// can be safely used on other threads.
 nsIIDNService *nsStandardURL::gIDN = nullptr;
+
+// This value will only be updated on the main thread once. Worker threads
+// may race when reading this values, but that's OK because in the worst
+// case we will just dispatch a noop runnable to the main thread.
 bool nsStandardURL::gInitialized = false;
-char nsStandardURL::gHostLimitDigits[] = { '/', '\\', '?', '#', 0 };
+
+const char nsStandardURL::gHostLimitDigits[] = { '/', '\\', '?', '#', 0 };
 
 // Invalid host characters
 // We still allow % because it is in the ID of addons.
@@ -183,6 +193,8 @@ nsPrefObserver::Observe(nsISupports *subject,
                         const char *topic,
                         const char16_t *data)
 {
+    MOZ_ASSERT(NS_IsMainThread());
+
     if (!strcmp(topic, NS_PREFBRANCH_PREFCHANGE_TOPIC_ID)) {
         nsCOMPtr<nsIPrefBranch> prefBranch( do_QueryInterface(subject) );
         if (prefBranch) {
@@ -301,8 +313,10 @@ nsStandardURL::nsStandardURL(bool aSupportsFileURL, bool aTrackURL)
 {
     LOG(("Creating nsStandardURL @%p\n", this));
 
+    // gInitialized changes value only once (false->true) on the main thread.
+    // It's OK to race here because in the worst case we'll just
+    // dispatch a noop runnable to the main thread.
     if (!gInitialized) {
-        gInitialized = true;
         InitGlobalObjects();
     }
 
@@ -355,6 +369,19 @@ DumpLeakedURLs::~DumpLeakedURLs()
 void
 nsStandardURL::InitGlobalObjects()
 {
+    if (!NS_IsMainThread()) {
+        RefPtr<Runnable> r = NS_NewRunnableFunction(&nsStandardURL::InitGlobalObjects);
+        SyncRunnable::DispatchToThread(GetMainThreadEventTarget(), r, false);
+        return;
+    }
+
+    if (gInitialized) {
+        return;
+    }
+
+    MOZ_ASSERT(NS_IsMainThread());
+    gInitialized = true;
+
     nsCOMPtr<nsIPrefBranch> prefBranch( do_GetService(NS_PREFSERVICE_CONTRACTID) );
     if (prefBranch) {
         nsCOMPtr<nsIObserver> obs( new nsPrefObserver() );
@@ -363,11 +390,18 @@ nsStandardURL::InitGlobalObjects()
 #endif
         PrefsChanged(prefBranch, nullptr);
     }
+
+    nsCOMPtr<nsIIDNService> serv(do_GetService(NS_IDNSERVICE_CONTRACTID));
+    if (serv) {
+        NS_ADDREF(gIDN = serv.get());
+        MOZ_ASSERT(gIDN);
+    }
 }
 
 void
 nsStandardURL::ShutdownGlobalObjects()
 {
+    MOZ_ASSERT(NS_IsMainThread());
     NS_IF_RELEASE(gIDN);
 
 #ifdef DEBUG_DUMP_URLS_AT_SHUTDOWN
@@ -649,13 +683,6 @@ nsStandardURL::NormalizeIDN(const nsACString& host, nsCString& result)
     NS_ASSERTION(mHostEncoding == eEncoding_ASCII, "unexpected default encoding");
 
     bool isASCII;
-    if (!gIDN) {
-        nsCOMPtr<nsIIDNService> serv(do_GetService(NS_IDNSERVICE_CONTRACTID));
-        if (serv) {
-            NS_ADDREF(gIDN = serv.get());
-        }
-    }
-
     result.Truncate();
     nsresult rv = NS_ERROR_UNEXPECTED;
     if (gIDN) {
@@ -1249,6 +1276,8 @@ nsStandardURL::WriteSegment(nsIBinaryOutputStream *stream, const URLSegment &seg
 /* static */ void
 nsStandardURL::PrefsChanged(nsIPrefBranch *prefs, const char *pref)
 {
+    MOZ_ASSERT(NS_IsMainThread());
+
     LOG(("nsStandardURL::PrefsChanged [pref=%s]\n", pref));
 
 #define PREF_CHANGED(p) ((pref == nullptr) || !strcmp(pref, p))

netwerk/base/nsStandardURL.h

@@ -298,7 +298,7 @@ private:
     // global objects.  don't use COMPtr as its destructor will cause a
     // coredump if we leak it.
     static nsIIDNService               *gIDN;
-    static char                         gHostLimitDigits[];
+    static const char                   gHostLimitDigits[];
     static bool                         gInitialized;
 
 public:
@@ -307,7 +307,7 @@ public:
 #endif
 
 #ifdef MOZ_RUST_URLPARSE
-    static bool                        gRustEnabled;
+    static Atomic<bool>                gRustEnabled;
     RefPtr<RustURL>                    mRustURL;
 #endif
 };

netwerk/dns/nsIDNService.cpp

@@ -63,6 +63,9 @@ NS_IMPL_ISUPPORTS(nsIDNService,
 
 nsresult nsIDNService::Init()
 {
+  MOZ_ASSERT(NS_IsMainThread());
+  MutexAutoLock lock(mLock);
+
   nsCOMPtr<nsIPrefService> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID));
   if (prefs)
     prefs->GetBranch(NS_NET_PREF_IDNWHITELIST, getter_AddRefs(mIDNWhitelistPrefBranch));
@@ -83,6 +86,9 @@ NS_IMETHODIMP nsIDNService::Observe(nsISupports *aSubject,
                                     const char *aTopic,
                                     const char16_t *aData)
 {
+  MOZ_ASSERT(NS_IsMainThread());
+  MutexAutoLock lock(mLock);
+
   if (!strcmp(aTopic, NS_PREFBRANCH_PREFCHANGE_TOPIC_ID)) {
     nsCOMPtr<nsIPrefBranch> prefBranch( do_QueryInterface(aSubject) );
     if (prefBranch)
@@ -93,6 +99,9 @@ NS_IMETHODIMP nsIDNService::Observe(nsISupports *aSubject,
 
 void nsIDNService::prefsChanged(nsIPrefBranch *prefBranch, const char16_t *pref)
 {
+  MOZ_ASSERT(NS_IsMainThread());
+  mLock.AssertCurrentThreadOwns();
+
   if (!pref || NS_LITERAL_STRING(NS_NET_PREF_IDNBLACKLIST).Equals(pref)) {
     nsCOMPtr<nsISupportsString> blacklist;
     nsresult rv = prefBranch->GetComplexValue(NS_NET_PREF_IDNBLACKLIST,
@@ -131,9 +140,12 @@ void nsIDNService::prefsChanged(nsIPrefBranch *prefBranch, const char16_t *pref)
 }
 
 nsIDNService::nsIDNService()
-  : mShowPunycode(false)
+  : mLock("DNService pref value lock")
+  , mShowPunycode(false)
   , mIDNUseWhitelist(false)
 {
+  MOZ_ASSERT(NS_IsMainThread());
+
 #ifdef IDNA2008
   uint32_t IDNAOptions = UIDNA_CHECK_BIDI | UIDNA_CHECK_CONTEXTJ;
   if (!kIDNA2008_TransitionalProcessing) {
@@ -152,6 +164,8 @@ nsIDNService::nsIDNService()
 
 nsIDNService::~nsIDNService()
 {
+  MOZ_ASSERT(NS_IsMainThread());
+
 #ifdef IDNA2008
   uidna_close(mIDNA);
 #else
@@ -381,8 +395,41 @@ NS_IMETHODIMP nsIDNService::Normalize(const nsACString & input,
   return NS_OK;
 }
 
+namespace {
+
+class MOZ_STACK_CLASS MutexSettableAutoUnlock final
+{
+  Mutex* mMutex;
+public:
+  MutexSettableAutoUnlock()
+    : mMutex(nullptr)
+  { }
+
+  void
+  Acquire(mozilla::Mutex& aMutex)
+  {
+    MOZ_ASSERT(!mMutex);
+    mMutex = &aMutex;
+    mMutex->Lock();
+  }
+
+  ~MutexSettableAutoUnlock()
+  {
+    if (mMutex) {
+      mMutex->Unlock();
+    }
+  }
+};
+
+} // anonymous namespace
+
 NS_IMETHODIMP nsIDNService::ConvertToDisplayIDN(const nsACString & input, bool * _isASCII, nsACString & _retval)
 {
+  MutexSettableAutoUnlock lock;
+  if (!NS_IsMainThread()) {
+    lock.Acquire(mLock);
+  }
+
   // If host is ACE, then convert to UTF-8 if the host is in the IDN whitelist.
   // Else, if host is already UTF-8, then make sure it is normalized per IDN.
 
@@ -754,6 +801,10 @@ nsresult nsIDNService::decodeACE(const nsACString& in, nsACString& out,
 
 bool nsIDNService::isInWhitelist(const nsACString &host)
 {
+  if (!NS_IsMainThread()) {
+    mLock.AssertCurrentThreadOwns();
+  }
+
   if (mIDNUseWhitelist && mIDNWhitelistPrefBranch) {
     nsAutoCString tld(host);
     // make sure the host is ACE for lookup and check that there are no
@@ -780,6 +831,10 @@ bool nsIDNService::isInWhitelist(const nsACString &host)
 
 bool nsIDNService::isLabelSafe(const nsAString &label)
 {
+  if (!NS_IsMainThread()) {
+    mLock.AssertCurrentThreadOwns();
+  }
+
   if (!isOnlySafeChars(PromiseFlatString(label), mIDNBlacklist)) {
     return false;
   }
@@ -923,6 +978,10 @@ static const int32_t scriptComboTable[13][9] = {
 
 bool nsIDNService::illegalScriptCombo(Script script, int32_t& savedScript)
 {
+  if (!NS_IsMainThread()) {
+    mLock.AssertCurrentThreadOwns();
+  }
+
   if (savedScript == -1) {
     savedScript = findScriptIndex(script);
     return false;

netwerk/dns/nsIDNService.h

@@ -168,12 +168,25 @@ private:
   idn_nameprep_t mNamePrepHandle;
   nsCOMPtr<nsIUnicodeNormalizer> mNormalizer;
 #endif
+
+  // We use this mutex to guard access to:
+  // |mIDNBlacklist|, |mShowPunycode|, |mRestrictionProfile|,
+  // |mIDNUseWhitelist|.
+  //
+  // These members can only be updated on the main thread and
+  // read on any thread. Therefore, acquiring the mutex is required
+  // only for threads other than the main thread.
+  mozilla::Mutex mLock;
+
+  // guarded by mLock
   nsXPIDLString mIDNBlacklist;
 
   /**
    * Flag set by the pref network.IDN_show_punycode. When it is true,
    * IDNs containing non-ASCII characters are always displayed to the
    * user in punycode
+   *
+   * guarded by mLock
    */
   bool mShowPunycode;
 
@@ -187,8 +200,11 @@ private:
     eHighlyRestrictiveProfile,
     eModeratelyRestrictiveProfile
   };
+  // guarded by mLock;
   restrictionProfile mRestrictionProfile;
+  // guarded by mLock;
   nsCOMPtr<nsIPrefBranch> mIDNWhitelistPrefBranch;
+  // guarded by mLock
   bool mIDNUseWhitelist;
 };