Bug 1367619 - Use RWLock when accessing font prefs service off main thread; r=heycam

MozReview-Commit-ID: Dxdq6Etbwxa

--HG--
extra : rebase_source : ec9f847c7dce6a101db6a3d157395f5b73d87ba1

intl/locale/moz.build

@@ -50,12 +50,20 @@ UNIFIED_SOURCES += [
     'nsCollationFactory.cpp',
     'nsLanguageAtomService.cpp',
     'nsLocale.cpp',
-    'nsLocaleService.cpp',
     'nsScriptableDateFormat.cpp',
     'nsUConvPropertySearch.cpp',
     'OSPreferences.cpp',
 ]
 
+# This file includes Carbon.h, which pulls
+# in CoreServices on OSX. This requires
+# a TextRange struct, which clashes with mozilla::TextRange,
+# because of a `using namespace mozilla;` somewhere
+# in the unified build
+SOURCES += [
+    'nsLocaleService.cpp',
+]
+
 if CONFIG['ENABLE_INTL_API']:
     UNIFIED_SOURCES += [
         'DateTimeFormat.cpp',

intl/locale/nsLanguageAtomService.cpp

@@ -11,6 +11,7 @@
 #include "mozilla/intl/OSPreferences.h"
 #include "mozilla/dom/EncodingUtils.h"
 #include "mozilla/ClearOnShutdown.h"
+#include "mozilla/ServoBindings.h"
 
 using namespace mozilla;
 using mozilla::intl::OSPreferences;
@@ -66,15 +67,19 @@ nsLanguageAtomService::GetLocaleLanguage()
 }
 
 nsIAtom*
-nsLanguageAtomService::GetLanguageGroup(nsIAtom *aLanguage)
+nsLanguageAtomService::GetLanguageGroup(nsIAtom *aLanguage, bool* aNeedsToCache)
 {
   nsIAtom *retVal = mLangToGroup.GetWeak(aLanguage);
 
   if (!retVal) {
-    MOZ_ASSERT(NS_IsMainThread(), "Should not append to cache off main thread");
+    if (aNeedsToCache) {
+      *aNeedsToCache = true;
+      return nullptr;
+    }
     nsCOMPtr<nsIAtom> uncached = GetUncachedLanguageGroup(aLanguage);
     retVal = uncached.get();
 
+    AssertIsMainThreadOrServoLangFontPrefsCacheLocked();
     // The hashtable will keep an owning reference to the atom
     mLangToGroup.Put(aLanguage, uncached);
   }

intl/locale/nsLanguageAtomService.h

@@ -23,7 +23,23 @@ public:
   nsIAtom* LookupLanguage(const nsACString &aLanguage);
   already_AddRefed<nsIAtom> LookupCharSet(const nsACString& aCharSet);
   nsIAtom* GetLocaleLanguage();
-  nsIAtom* GetLanguageGroup(nsIAtom* aLanguage);
+
+  // Returns the language group that the specified language is a part of.
+  //
+  // aNeedsToCache is used for two things.  If null, it indicates that
+  // the nsLanguageAtomService is safe to cache the result of the
+  // language group lookup, either because we're on the main thread,
+  // or because we're on a style worker thread but the font lock has
+  // been acquired.  If non-null, it indicates that it's not safe to
+  // cache the result of the language group lookup (because we're on
+  // a style worker thread without the lock acquired).  In this case,
+  // GetLanguageGroup will store true in *aNeedsToCache true if we
+  // would have cached the result of a new lookup, and false if we
+  // were able to use an existing cached result.  Thus, callers that
+  // get a true *aNeedsToCache outparam value should make an effort
+  // to re-call GetLanguageGroup when it is safe to cache, to avoid
+  // recomputing the language group again later.
+  nsIAtom* GetLanguageGroup(nsIAtom* aLanguage, bool* aNeedsToCache = nullptr);
   already_AddRefed<nsIAtom> GetUncachedLanguageGroup(nsIAtom* aLanguage) const;
 
 private:

intl/locale/nsLocaleService.cpp

@@ -25,6 +25,8 @@
 #  include "nsPosixLocale.h"
 #endif
 
+using namespace mozilla;
+
 //
 // implementation constants
 const int LocaleListLength = 6;

intl/locale/nsScriptableDateFormat.cpp

@@ -8,6 +8,7 @@
 #include "nsIScriptableDateFormat.h"
 #include "nsCOMPtr.h"
 #include "nsServiceManagerUtils.h"
+#include "nsILocaleService.h"
 
 static NS_DEFINE_CID(kLocaleServiceCID, NS_LOCALESERVICE_CID);
 

layout/base/StaticPresData.cpp

@@ -7,8 +7,8 @@
 #include "mozilla/StaticPresData.h"
 
 #include "mozilla/Preferences.h"
+#include "mozilla/ServoBindings.h"
 #include "nsPresContext.h"
-
 namespace mozilla {
 
 static StaticPresData* sSingleton = nullptr;
@@ -236,10 +236,11 @@ LangGroupFontPrefs::Initialize(nsIAtom* aLangGroupAtom)
 }
 
 nsIAtom*
-StaticPresData::GetLangGroup(nsIAtom* aLanguage) const
+StaticPresData::GetLangGroup(nsIAtom* aLanguage,
+                             bool* aNeedsToCache) const
 {
   nsIAtom* langGroupAtom = nullptr;
-  langGroupAtom = mLangService->GetLanguageGroup(aLanguage);
+  langGroupAtom = mLangService->GetLanguageGroup(aLanguage, aNeedsToCache);
   if (!langGroupAtom) {
     langGroupAtom = nsGkAtoms::x_western; // Assume x-western is safe...
   }
@@ -258,14 +259,19 @@ StaticPresData::GetUncachedLangGroup(nsIAtom* aLanguage) const
 
 const LangGroupFontPrefs*
 StaticPresData::GetFontPrefsForLangHelper(nsIAtom* aLanguage,
-                                          const LangGroupFontPrefs* aPrefs) const
+                                          const LangGroupFontPrefs* aPrefs,
+                                          bool* aNeedsToCache) const
 {
   // Get language group for aLanguage:
   MOZ_ASSERT(aLanguage);
   MOZ_ASSERT(mLangService);
   MOZ_ASSERT(aPrefs);
 
-  nsIAtom* langGroupAtom = GetLangGroup(aLanguage);
+  nsIAtom* langGroupAtom = GetLangGroup(aLanguage, aNeedsToCache);
+
+  if (aNeedsToCache && *aNeedsToCache) {
+    return nullptr;
+  }
 
   LangGroupFontPrefs* prefs = const_cast<LangGroupFontPrefs*>(aPrefs);
   if (prefs->mLangGroup) { // if initialized
@@ -280,13 +286,21 @@ StaticPresData::GetFontPrefsForLangHelper(nsIAtom* aLanguage,
       }
       prefs = prefs->mNext;
     }
-    MOZ_ASSERT(NS_IsMainThread(), "Should not append to cache off main thread");
+    if (aNeedsToCache) {
+      *aNeedsToCache = true;
+      return nullptr;
+    }
+    AssertIsMainThreadOrServoLangFontPrefsCacheLocked();
     // nothing cached, so go on and fetch the prefs for this lang group:
     prefs = prefs->mNext = new LangGroupFontPrefs;
   }
 
-  MOZ_ASSERT(NS_IsMainThread(), "Should not append to cache off main thread");
+  if (aNeedsToCache) {
+    *aNeedsToCache = true;
+    return nullptr;
+  }
 
+  AssertIsMainThreadOrServoLangFontPrefsCacheLocked();
   prefs->Initialize(langGroupAtom);
 
   return prefs;

layout/base/StaticPresData.h

@@ -97,8 +97,21 @@ public:
    * Given a language, get the language group name, which can
    * be used as an argument to LangGroupFontPrefs::Initialize()
    *
+   * aNeedsToCache is used for two things.  If null, it indicates that
+   * the nsLanguageAtomService is safe to cache the result of the
+   * language group lookup, either because we're on the main thread,
+   * or because we're on a style worker thread but the font lock has
+   * been acquired.  If non-null, it indicates that it's not safe to
+   * cache the result of the language group lookup (because we're on
+   * a style worker thread without the lock acquired).  In this case,
+   * GetLanguageGroup will store true in *aNeedsToCache true if we
+   * would have cached the result of a new lookup, and false if we
+   * were able to use an existing cached result.  Thus, callers that
+   * get a true *aNeedsToCache outparam value should make an effort
+   * to re-call GetLanguageGroup when it is safe to cache, to avoid
+   * recomputing the language group again later.
    */
-  nsIAtom* GetLangGroup(nsIAtom* aLanguage) const;
+  nsIAtom* GetLangGroup(nsIAtom* aLanguage, bool* aNeedsToCache = nullptr) const;
 
   /**
    * Same as GetLangGroup, but will not cache the result
@@ -119,9 +132,12 @@ public:
    * be fine. But just to be on the safe side, we leave the old mechanism as-is,
    * with an additional per-session cache that new callers can use if they don't
    * have a PresContext.
+   *
+   * See comment on GetLangGroup for the usage of aNeedsToCache.
    */
   const LangGroupFontPrefs* GetFontPrefsForLangHelper(nsIAtom* aLanguage,
-                                                      const LangGroupFontPrefs* aPrefs) const;
+                                                      const LangGroupFontPrefs* aPrefs,
+                                                      bool* aNeedsToCache = nullptr) const;
   /**
    * Get the default font for the given language and generic font ID.
    * aLanguage may not be nullptr.
@@ -154,10 +170,10 @@ public:
     MOZ_ASSERT(aLanguage);
     return GetDefaultFontHelper(aFontID, aLanguage, GetFontPrefsForLang(aLanguage));
   }
-  const LangGroupFontPrefs* GetFontPrefsForLang(nsIAtom* aLanguage) const
+  const LangGroupFontPrefs* GetFontPrefsForLang(nsIAtom* aLanguage, bool* aNeedsToCache = nullptr) const
   {
     MOZ_ASSERT(aLanguage);
-    return GetFontPrefsForLangHelper(aLanguage, &mStaticLangGroupFontPrefs);
+    return GetFontPrefsForLangHelper(aLanguage, &mStaticLangGroupFontPrefs, aNeedsToCache);
   }
 
   void ResetCachedFontPrefs() { mStaticLangGroupFontPrefs.Reset(); }

layout/base/nsPresContext.h

@@ -352,11 +352,14 @@ public:
    * See the comment in StaticPresData::GetDefaultFont.
    */
   const nsFont* GetDefaultFont(uint8_t aFontID,
-                               nsIAtom *aLanguage) const
+                               nsIAtom *aLanguage, bool* aNeedsToCache = nullptr) const
   {
     nsIAtom* lang = aLanguage ? aLanguage : mLanguage.get();
-    return StaticPresData::Get()->GetDefaultFontHelper(aFontID, lang,
-                                                       GetFontPrefsForLang(lang));
+    const LangGroupFontPrefs* prefs = GetFontPrefsForLang(lang, aNeedsToCache);
+    if (aNeedsToCache && *aNeedsToCache) {
+      return nullptr;
+    }
+    return StaticPresData::Get()->GetDefaultFontHelper(aFontID, lang, prefs);
   }
 
   void ForceCacheLang(nsIAtom *aLanguage);
@@ -1232,10 +1235,10 @@ protected:
    * Fetch the user's font preferences for the given aLanguage's
    * langugage group.
    */
-  const LangGroupFontPrefs* GetFontPrefsForLang(nsIAtom *aLanguage) const
+  const LangGroupFontPrefs* GetFontPrefsForLang(nsIAtom *aLanguage, bool* aNeedsToCache = nullptr) const
   {
     nsIAtom* lang = aLanguage ? aLanguage : mLanguage.get();
-    return StaticPresData::Get()->GetFontPrefsForLangHelper(lang, &mLangGroupFontPrefs);
+    return StaticPresData::Get()->GetFontPrefsForLangHelper(lang, &mLangGroupFontPrefs, aNeedsToCache);
   }
 
   void UpdateCharSet(const nsCString& aCharSet);

layout/style/ServoBindings.cpp

@@ -57,6 +57,7 @@
 #include "mozilla/SystemGroup.h"
 #include "mozilla/ServoMediaList.h"
 #include "mozilla/ServoComputedValuesWithParent.h"
+#include "mozilla/RWLock.h"
 #include "mozilla/dom/Element.h"
 #include "mozilla/dom/ElementInlines.h"
 #include "mozilla/dom/HTMLTableCellElement.h"
@@ -83,6 +84,37 @@ using namespace mozilla::dom;
 
 
 static Mutex* sServoFontMetricsLock = nullptr;
+static RWLock* sServoLangFontPrefsLock = nullptr;
+
+
+static
+const nsFont*
+ThreadSafeGetDefaultFontHelper(const nsPresContext* aPresContext, nsIAtom* aLanguage)
+{
+  bool needsCache = false;
+  const nsFont* retval;
+
+  {
+    AutoReadLock guard(*sServoLangFontPrefsLock);
+    retval = aPresContext->GetDefaultFont(kPresContext_DefaultVariableFont_ID,
+                                          aLanguage, &needsCache);
+  }
+  if (!needsCache) {
+    return retval;
+  }
+  {
+    AutoWriteLock guard(*sServoLangFontPrefsLock);
+  retval = aPresContext->GetDefaultFont(kPresContext_DefaultVariableFont_ID,
+                                        aLanguage, nullptr);
+  }
+  return retval;
+}
+
+void
+AssertIsMainThreadOrServoLangFontPrefsCacheLocked()
+{
+  MOZ_ASSERT(NS_IsMainThread() || sServoLangFontPrefsLock->LockedForWritingByCurrentThread());
+}
 
 uint32_t
 Gecko_ChildrenCount(RawGeckoNodeBorrowed aNode)
@@ -1083,9 +1115,7 @@ Gecko_nsFont_InitSystem(nsFont* aDest, int32_t aFontId,
                         const nsStyleFont* aFont, RawGeckoPresContextBorrowed aPresContext)
 {
   MutexAutoLock lock(*sServoFontMetricsLock);
-  const nsFont* defaultVariableFont =
-    aPresContext->GetDefaultFont(kPresContext_DefaultVariableFont_ID,
-                                 aFont->mLanguage);
+  const nsFont* defaultVariableFont = ThreadSafeGetDefaultFontHelper(aPresContext, aFont->mLanguage);
 
   // We have passed uninitialized memory to this function,
   // initialize it. We can't simply return an nsFont because then
@@ -1954,9 +1984,7 @@ void
 Gecko_nsStyleFont_FixupNoneGeneric(nsStyleFont* aFont,
                                    RawGeckoPresContextBorrowed aPresContext)
 {
-  const nsFont* defaultVariableFont =
-    aPresContext->GetDefaultFont(kPresContext_DefaultVariableFont_ID,
-                                 aFont->mLanguage);
+  const nsFont* defaultVariableFont = ThreadSafeGetDefaultFontHelper(aPresContext, aFont->mLanguage);
   nsRuleNode::FixupNoneGeneric(&aFont->mFont, aPresContext,
                                aFont->mGenericID, defaultVariableFont);
 }
@@ -1993,12 +2021,14 @@ InitializeServo()
   Servo_Initialize(URLExtraData::Dummy());
 
   sServoFontMetricsLock = new Mutex("Gecko_GetFontMetrics");
+  sServoLangFontPrefsLock = new RWLock("nsPresContext::GetDefaultFont");
 }
 
 void
 ShutdownServo()
 {
   delete sServoFontMetricsLock;
+  delete sServoLangFontPrefsLock;
   Servo_Shutdown();
 }
 

layout/style/ServoBindings.h

@@ -513,6 +513,7 @@ GeckoFontMetrics Gecko_GetFontMetrics(RawGeckoPresContextBorrowed pres_context,
 int32_t Gecko_GetAppUnitsPerPhysicalInch(RawGeckoPresContextBorrowed pres_context);
 void InitializeServo();
 void ShutdownServo();
+void AssertIsMainThreadOrServoLangFontPrefsCacheLocked();
 
 const nsMediaFeature* Gecko_GetMediaFeatures();
 nsCSSKeyword Gecko_LookupCSSKeyword(const uint8_t* string, uint32_t len);

xpcom/threads/RWLock.cpp

@@ -32,6 +32,14 @@ RWLock::RWLock(const char* aName)
 #endif
 }
 
+#ifdef DEBUG
+bool
+RWLock::LockedForWritingByCurrentThread()
+{
+  return mOwningThread == PR_GetCurrentThread();
+}
+#endif
+
 #ifndef XP_WIN
 RWLock::~RWLock()
 {

xpcom/threads/RWLock.h

@@ -55,6 +55,7 @@ public:
 #endif
 
 #ifdef DEBUG
+  bool LockedForWritingByCurrentThread();
   void ReadLock();
   void ReadUnlock();
   void WriteLock();