Bug 1416622 - Tighten up libpref's process checking. r=glandium draft
authorNicholas Nethercote <nnethercote@mozilla.com>
Mon, 13 Nov 2017 09:19:55 +1100
changeset 696940 424162e374107d1dd64312ca0e78353be84c063d
parent 696939 2475e791d8ca837e1cb44fd3665c0de6f9899e0b
child 739962 0c74d584ac0e9a2f755eeb793cab52d48477d07d
push id88828
push usernnethercote@mozilla.com
push dateMon, 13 Nov 2017 03:11:54 +0000
reviewersglandium
bugs1416622
milestone58.0a1
Bug 1416622 - Tighten up libpref's process checking. r=glandium libpref only allows pref modifications in the parent process. This patch tightens up the checking, which is a bit inconsistent. - It removes ENSURE_MAIN_PROCESS_WITH_WARNING, which does NS_WARNING on failure, and replaces its uses with ENSURE_MAIN_PROCESS, which does NS_ERROR on failure. This required adding an XRE_IsParentProcess() check to one place in editor/. - It converts XRE_IsContentProcess() tests to !XRE_IsParentProcess(), because we now have multiple kinds of non-parent process. - It uses ENSURE_MAIN_PROCESS to replace other checking code in a few places. - It improves a comment in HandleDirty(). MozReview-Commit-ID: D8znQWH7ery
editor/composer/nsEditorSpellCheck.cpp
modules/libpref/Preferences.cpp
--- a/editor/composer/nsEditorSpellCheck.cpp
+++ b/editor/composer/nsEditorSpellCheck.cpp
@@ -656,21 +656,27 @@ nsEditorSpellCheck::SetCurrentDictionary
         printf("***** Clearing content preferences for |%s|\n",
                NS_ConvertUTF16toUTF8(aDictionary).get());
 #endif
       }
 
       // Also store it in as a preference, so we can use it as a fallback.
       // We don't want this for mail composer because it uses
       // "spellchecker.dictionary" as a preference.
-      Preferences::SetString("spellchecker.dictionary", aDictionary);
+      //
+      // XXX: Prefs can only be set in the parent process, so this condition is
+      // necessary to stop libpref from throwing errors. But this should
+      // probably be handled in a better way.
+      if (XRE_IsParentProcess()) {
+        Preferences::SetString("spellchecker.dictionary", aDictionary);
 #ifdef DEBUG_DICT
-      printf("***** Storing spellchecker.dictionary |%s|\n",
-             NS_ConvertUTF16toUTF8(aDictionary).get());
+        printf("***** Possibly storing spellchecker.dictionary |%s|\n",
+               NS_ConvertUTF16toUTF8(aDictionary).get());
 #endif
+      }
     }
   }
   return mSpellChecker->SetCurrentDictionary(aDictionary);
 }
 
 NS_IMETHODIMP
 nsEditorSpellCheck::UninitSpellChecker()
 {
--- a/modules/libpref/Preferences.cpp
+++ b/modules/libpref/Preferences.cpp
@@ -100,40 +100,23 @@ using namespace mozilla;
         "ENSURE_MAIN_PROCESS: called %s on %s in a non-main process",          \
         func,                                                                  \
         pref);                                                                 \
       NS_ERROR(msg.get());                                                     \
       return NS_ERROR_NOT_AVAILABLE;                                           \
     }                                                                          \
   } while (0)
 
-#define ENSURE_MAIN_PROCESS_WITH_WARNING(func, pref)                           \
-  do {                                                                         \
-    if (MOZ_UNLIKELY(!XRE_IsParentProcess())) {                                \
-      nsPrintfCString msg(                                                     \
-        "ENSURE_MAIN_PROCESS: called %s on %s in a non-main process",          \
-        func,                                                                  \
-        pref);                                                                 \
-      NS_WARNING(msg.get());                                                   \
-      return NS_ERROR_NOT_AVAILABLE;                                           \
-    }                                                                          \
-  } while (0)
-
 #else // DEBUG
 
 #define ENSURE_MAIN_PROCESS(func, pref)                                        \
   if (MOZ_UNLIKELY(!XRE_IsParentProcess())) {                                  \
     return NS_ERROR_NOT_AVAILABLE;                                             \
   }
 
-#define ENSURE_MAIN_PROCESS_WITH_WARNING(func, pref)                           \
-  if (MOZ_UNLIKELY(!XRE_IsParentProcess())) {                                  \
-    return NS_ERROR_NOT_AVAILABLE;                                             \
-  }
-
 #endif // DEBUG
 
 //===========================================================================
 // The old low-level prefs API
 //===========================================================================
 
 struct PrefHashEntry;
 
@@ -901,24 +884,24 @@ public:
 #define WATCHING_PREF_RAII()
 
 #endif // DEBUG
 
 static PrefHashEntry*
 pref_HashTableLookup(const char* aKey)
 {
   MOZ_ASSERT(NS_IsMainThread() || mozilla::ServoStyleSet::IsInServoTraversal());
-  MOZ_ASSERT((!XRE_IsContentProcess() || gPhase != START),
+  MOZ_ASSERT((XRE_IsParentProcess() || gPhase != START),
              "pref access before commandline prefs set");
 
   // If you're hitting this assertion, you've added a pref access to start up.
   // Consider moving it later or add it to the whitelist in ContentPrefs.cpp
-  // and get review from a DOM peer
+  // and get review from a DOM peer.
 #ifdef DEBUG
-  if (XRE_IsContentProcess() && gPhase <= END_INIT_PREFS && !gWatchingPref &&
+  if (!XRE_IsParentProcess() && gPhase <= END_INIT_PREFS && !gWatchingPref &&
       !InInitArray(aKey)) {
     MOZ_CRASH_UNSAFE_PRINTF(
       "accessing non-init pref %s before the rest of the prefs are sent", aKey);
   }
 #endif
 
   return static_cast<PrefHashEntry*>(gHashTable->Search(aKey));
 }
@@ -2379,17 +2362,17 @@ nsPrefBranch::GetComplexValue(const char
                               const nsIID& aType,
                               void** aRetVal)
 {
   NS_ENSURE_ARG(aPrefName);
 
   nsresult rv;
   nsAutoCString utf8String;
 
-  // we have to do this one first because it's different than all the rest
+  // We have to do this one first because it's different to all the rest.
   if (aType.Equals(NS_GET_IID(nsIPrefLocalizedString))) {
     nsCOMPtr<nsIPrefLocalizedString> theString(
       do_CreateInstance(NS_PREFLOCALIZEDSTRING_CONTRACTID, &rv));
     if (NS_FAILED(rv)) {
       return rv;
     }
 
     const PrefName& pref = GetPrefName(aPrefName);
@@ -2428,38 +2411,32 @@ nsPrefBranch::GetComplexValue(const char
 
   // if we can't get the pref, there's no point in being here
   rv = GetCharPref(aPrefName, utf8String);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
   if (aType.Equals(NS_GET_IID(nsIFile))) {
-    if (XRE_IsContentProcess()) {
-      NS_ERROR("cannot get nsIFile pref from content process");
-      return NS_ERROR_NOT_AVAILABLE;
-    }
+    ENSURE_MAIN_PROCESS("GetComplexValue(nsIFile)", aPrefName);
 
     nsCOMPtr<nsIFile> file(do_CreateInstance(NS_LOCAL_FILE_CONTRACTID, &rv));
 
     if (NS_SUCCEEDED(rv)) {
       rv = file->SetPersistentDescriptor(utf8String);
       if (NS_SUCCEEDED(rv)) {
         file.forget(reinterpret_cast<nsIFile**>(aRetVal));
         return NS_OK;
       }
     }
     return rv;
   }
 
   if (aType.Equals(NS_GET_IID(nsIRelativeFilePref))) {
-    if (XRE_IsContentProcess()) {
-      NS_ERROR("cannot get nsIRelativeFilePref from content process");
-      return NS_ERROR_NOT_AVAILABLE;
-    }
+    ENSURE_MAIN_PROCESS("GetComplexValue(nsIRelativeFilePref)", aPrefName);
 
     nsACString::const_iterator keyBegin, strEnd;
     utf8String.BeginReading(keyBegin);
     utf8String.EndReading(strEnd);
 
     // The pref has the format: [fromKey]a/b/c
     if (*keyBegin++ != '[') {
       return NS_ERROR_FAILURE;
@@ -3094,19 +3071,19 @@ namespace mozilla {
 #define INITIAL_PREF_FILES 10
 
 static NS_DEFINE_CID(kZipReaderCID, NS_ZIPREADER_CID);
 
 void
 Preferences::HandleDirty()
 {
   if (!XRE_IsParentProcess()) {
-    // TODO: this should really assert because you can't set prefs in a
-    // content process. But so much code currently does this that we just
-    // ignore it for now.
+    // This path is hit a lot when setting up prefs for content processes. Just
+    // ignore it in that case, because content processes aren't responsible for
+    // saving prefs.
     return;
   }
 
   if (!gHashTable || !sPreferences) {
     return;
   }
 
   if (sPreferences->mProfileShutdown) {
@@ -3563,17 +3540,17 @@ Preferences::GetInstanceForService()
 
   Result<Ok, const char*> res = pref_InitInitialObjects();
   if (res.isErr()) {
     sPreferences = nullptr;
     gCacheDataDesc = res.unwrapErr();
     return nullptr;
   }
 
-  if (XRE_IsContentProcess()) {
+  if (!XRE_IsParentProcess()) {
     MOZ_ASSERT(gInitPrefs);
     for (unsigned int i = 0; i < gInitPrefs->Length(); i++) {
       Preferences::SetPreference(gInitPrefs->ElementAt(i));
     }
     delete gInitPrefs;
     gInitPrefs = nullptr;
 
   } else {
@@ -3786,52 +3763,43 @@ Preferences::Observe(nsISupports* aSubje
   }
 
   return rv;
 }
 
 NS_IMETHODIMP
 Preferences::ReadUserPrefsFromFile(nsIFile* aFile)
 {
-  if (MOZ_UNLIKELY(!XRE_IsParentProcess())) {
-    NS_ERROR("must load prefs from parent process");
-    return NS_ERROR_NOT_AVAILABLE;
-  }
+  ENSURE_MAIN_PROCESS("Preferences::ReadUserPrefsFromFile", "all prefs");
 
   if (!aFile) {
     NS_ERROR("ReadUserPrefsFromFile requires a parameter");
     return NS_ERROR_INVALID_ARG;
   }
 
   return openPrefFile(aFile);
 }
 
 NS_IMETHODIMP
 Preferences::ResetPrefs()
 {
-  if (MOZ_UNLIKELY(!XRE_IsParentProcess())) {
-    NS_ERROR("must reset prefs from parent process");
-    return NS_ERROR_NOT_AVAILABLE;
-  }
+  ENSURE_MAIN_PROCESS("Preferences::ResetPrefs", "all prefs");
 
   NotifyServiceObservers(NS_PREFSERVICE_RESET_TOPIC_ID);
 
   gHashTable->ClearAndPrepareForLength(PREF_HASHTABLE_INITIAL_LENGTH);
   gPrefNameArena.Clear();
 
   return pref_InitInitialObjects().isOk() ? NS_OK : NS_ERROR_FAILURE;
 }
 
 NS_IMETHODIMP
 Preferences::ResetUserPrefs()
 {
-  if (MOZ_UNLIKELY(!XRE_IsParentProcess())) {
-    NS_ERROR("must reset user prefs from parent process");
-    return NS_ERROR_NOT_AVAILABLE;
-  }
+  ENSURE_MAIN_PROCESS("Preferences::ResetUserPrefs", "all prefs");
 
   PREF_ClearAllUserPrefs();
   return NS_OK;
 }
 
 bool
 Preferences::AllowOffMainThreadSave()
 {
@@ -4081,20 +4049,17 @@ Preferences::MakeBackupPrefFile(nsIFile*
   NS_ENSURE_SUCCESS(rv, rv);
 
   return rv;
 }
 
 nsresult
 Preferences::SavePrefFileInternal(nsIFile* aFile, SaveMethod aSaveMethod)
 {
-  if (MOZ_UNLIKELY(!XRE_IsParentProcess())) {
-    NS_ERROR("must save pref file from parent process");
-    return NS_ERROR_NOT_AVAILABLE;
-  }
+  ENSURE_MAIN_PROCESS("Preferences::SavePrefFileInternal", "all prefs");
 
   // We allow different behavior here when aFile argument is not null, but it
   // happens to be the same as the current file.  It is not clear that we
   // should, but it does give us a "force" save on the unmodified pref file
   // (see the original bug 160377 when we added this.)
 
   if (nullptr == aFile) {
     mSavePending = false;
@@ -4654,57 +4619,57 @@ Preferences::GetComplex(const char* aPre
 {
   NS_ENSURE_TRUE(InitStaticMembers(), NS_ERROR_NOT_AVAILABLE);
   return sPreferences->mRootBranch->GetComplexValue(aPref, aType, aResult);
 }
 
 /* static */ nsresult
 Preferences::SetCString(const char* aPref, const char* aValue)
 {
-  ENSURE_MAIN_PROCESS_WITH_WARNING("SetCString", aPref);
+  ENSURE_MAIN_PROCESS("SetCString", aPref);
   NS_ENSURE_TRUE(InitStaticMembers(), NS_ERROR_NOT_AVAILABLE);
   return PREF_SetCStringPref(aPref, nsDependentCString(aValue), false);
 }
 
 /* static */ nsresult
 Preferences::SetCString(const char* aPref, const nsACString& aValue)
 {
-  ENSURE_MAIN_PROCESS_WITH_WARNING("SetCString", aPref);
+  ENSURE_MAIN_PROCESS("SetCString", aPref);
   NS_ENSURE_TRUE(InitStaticMembers(), NS_ERROR_NOT_AVAILABLE);
   return PREF_SetCStringPref(aPref, aValue, false);
 }
 
 /* static */ nsresult
 Preferences::SetString(const char* aPref, const char16ptr_t aValue)
 {
-  ENSURE_MAIN_PROCESS_WITH_WARNING("SetString", aPref);
+  ENSURE_MAIN_PROCESS("SetString", aPref);
   NS_ENSURE_TRUE(InitStaticMembers(), NS_ERROR_NOT_AVAILABLE);
   return PREF_SetCStringPref(aPref, NS_ConvertUTF16toUTF8(aValue), false);
 }
 
 /* static */ nsresult
 Preferences::SetString(const char* aPref, const nsAString& aValue)
 {
-  ENSURE_MAIN_PROCESS_WITH_WARNING("SetString", aPref);
+  ENSURE_MAIN_PROCESS("SetString", aPref);
   NS_ENSURE_TRUE(InitStaticMembers(), NS_ERROR_NOT_AVAILABLE);
   return PREF_SetCStringPref(aPref, NS_ConvertUTF16toUTF8(aValue), false);
 }
 
 /* static */ nsresult
 Preferences::SetBool(const char* aPref, bool aValue)
 {
-  ENSURE_MAIN_PROCESS_WITH_WARNING("SetBool", aPref);
+  ENSURE_MAIN_PROCESS("SetBool", aPref);
   NS_ENSURE_TRUE(InitStaticMembers(), NS_ERROR_NOT_AVAILABLE);
   return PREF_SetBoolPref(aPref, aValue, false);
 }
 
 /* static */ nsresult
 Preferences::SetInt(const char* aPref, int32_t aValue)
 {
-  ENSURE_MAIN_PROCESS_WITH_WARNING("SetInt", aPref);
+  ENSURE_MAIN_PROCESS("SetInt", aPref);
   NS_ENSURE_TRUE(InitStaticMembers(), NS_ERROR_NOT_AVAILABLE);
   return PREF_SetIntPref(aPref, aValue, false);
 }
 
 /* static */ nsresult
 Preferences::SetFloat(const char* aPref, float aValue)
 {
   return SetCString(aPref, nsPrintfCString("%f", aValue).get());
@@ -4717,17 +4682,17 @@ Preferences::SetComplex(const char* aPre
 {
   NS_ENSURE_TRUE(InitStaticMembers(), NS_ERROR_NOT_AVAILABLE);
   return sPreferences->mRootBranch->SetComplexValue(aPref, aType, aValue);
 }
 
 /* static */ nsresult
 Preferences::ClearUser(const char* aPref)
 {
-  ENSURE_MAIN_PROCESS_WITH_WARNING("ClearUser", aPref);
+  ENSURE_MAIN_PROCESS("ClearUser", aPref);
   NS_ENSURE_TRUE(InitStaticMembers(), NS_ERROR_NOT_AVAILABLE);
   return PREF_ClearUserPref(aPref);
 }
 
 /* static */ bool
 Preferences::HasUserValue(const char* aPref)
 {
   NS_ENSURE_TRUE(InitStaticMembers(), false);
@@ -5100,17 +5065,16 @@ Preferences::GetDefaultType(const char* 
            sPreferences->mDefaultRootBranch->GetPrefType(aPref, &result))
            ? result
            : nsIPrefBranch::PREF_INVALID;
 }
 
 } // namespace mozilla
 
 #undef ENSURE_MAIN_PROCESS
-#undef ENSURE_MAIN_PROCESS_WITH_WARNING
 
 //===========================================================================
 // Module and factory stuff
 //===========================================================================
 
 NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR(Preferences,
                                          Preferences::GetInstanceForService)
 NS_GENERIC_FACTORY_CONSTRUCTOR_INIT(nsPrefLocalizedString, Init)