From 96f8fc64b9780bf684705c955f0ea5d1ac1415b3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Andr=C3=A9=20Bargull?= <andre.bargull@gmail.com>
Date: Thu, 12 Oct 2017 09:20:07 -0700
Subject: [PATCH 01/20] Bug 1406398 - Avoid rooting the object twice in
 EnumerableOwnProperties. r=jandem

---
 js/src/builtin/Object.cpp | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/js/src/builtin/Object.cpp b/js/src/builtin/Object.cpp
index 03a6273409bf..05b990f4da07 100644
--- a/js/src/builtin/Object.cpp
+++ b/js/src/builtin/Object.cpp
@@ -1142,9 +1142,6 @@ EnumerableOwnProperties(JSContext* cx, const JS::CallArgs& args, EnumerableOwnPr
     RootedId id(cx);
     RootedValue key(cx);
     RootedValue value(cx);
-    RootedNativeObject nobj(cx);
-    if (obj->is<NativeObject>())
-        nobj = &obj->as<NativeObject>();
     RootedShape shape(cx);
     Rooted<PropertyDescriptor> desc(cx);
     // Step 4.
@@ -1161,7 +1158,8 @@ EnumerableOwnProperties(JSContext* cx, const JS::CallArgs& args, EnumerableOwnPr
         }
 
         // Step 4.a.i.
-        if (nobj) {
+        if (obj->is<NativeObject>()) {
+            HandleNativeObject nobj = obj.as<NativeObject>();
             if (JSID_IS_INT(id) && nobj->containsDenseElement(JSID_TO_INT(id))) {
                 value = nobj->getDenseOrTypedArrayElement(JSID_TO_INT(id));
             } else {

From f81f2d20db3c04b95f35fedfc7ed12a7a192128c Mon Sep 17 00:00:00 2001
From: David Keeler <dkeeler@mozilla.com>
Date: Wed, 25 Oct 2017 09:54:13 -0700
Subject: [PATCH 02/20] Bug 1411458 - Confirm we actually have a PKCS#7
 signedData content info. r=jcj

MozReview-Commit-ID: GKfL1C0EPWt
---
 security/manager/ssl/nsDataSignatureVerifier.cpp | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/security/manager/ssl/nsDataSignatureVerifier.cpp b/security/manager/ssl/nsDataSignatureVerifier.cpp
index f78740365e1a..2527431dd9ae 100644
--- a/security/manager/ssl/nsDataSignatureVerifier.cpp
+++ b/security/manager/ssl/nsDataSignatureVerifier.cpp
@@ -170,6 +170,12 @@ VerifyCMSDetachedSignatureIncludingCertificate(
     return NS_ERROR_CMS_VERIFY_NO_CONTENT_INFO;
   }
 
+  // We're expecting this to be a PKCS#7 signedData content info.
+  if (NSS_CMSContentInfo_GetContentTypeTag(cinfo)
+        != SEC_OID_PKCS7_SIGNED_DATA) {
+    return NS_ERROR_CMS_VERIFY_NO_CONTENT_INFO;
+  }
+
   // signedData is non-owning
   NSSCMSSignedData* signedData =
     static_cast<NSSCMSSignedData*>(NSS_CMSContentInfo_GetContent(cinfo));

From 529db19e62d8042af6552cf0d91129aa28509b44 Mon Sep 17 00:00:00 2001
From: Nicholas Nethercote <nnethercote@mozilla.com>
Date: Wed, 25 Oct 2017 10:22:38 +1100
Subject: [PATCH 03/20] Bug 1411480 - Inline and remove the parser's
 pref_DoCallback() function. r=glandium.

Preferences.cpp has two functions named pref_DoCallback(). One of them has a
single use in the parser. This patch inlines that single use to remove the name
duplication.

MozReview-Commit-ID: HnyteQ0N5M1

--HG--
extra : rebase_source : 93f963d3ba445c1c3a482fa95dbab33e57dae188
---
 modules/libpref/Preferences.cpp | 78 ++++++++++++++-------------------
 1 file changed, 33 insertions(+), 45 deletions(-)

diff --git a/modules/libpref/Preferences.cpp b/modules/libpref/Preferences.cpp
index c0887cb87002..3a75ac32be37 100644
--- a/modules/libpref/Preferences.cpp
+++ b/modules/libpref/Preferences.cpp
@@ -1499,49 +1499,6 @@ pref_ReportParseProblem(PrefParseState& aPS,
   }
 }
 
-// This function is called when a complete pref name-value pair has been
-// extracted from the input data.
-//
-// @param aPS
-//        parse state instance
-//
-// @return false to indicate a fatal error.
-static bool
-pref_DoCallback(PrefParseState* aPS)
-{
-  PrefValue value;
-
-  switch (aPS->mVtype) {
-    case PrefType::String:
-      value.mStringVal = aPS->mVb;
-      break;
-
-    case PrefType::Int:
-      if ((aPS->mVb[0] == '-' || aPS->mVb[0] == '+') && aPS->mVb[1] == '\0') {
-        pref_ReportParseProblem(*aPS, "invalid integer value", 0, true);
-        NS_WARNING("malformed integer value");
-        return false;
-      }
-      value.mIntVal = atoi(aPS->mVb);
-      break;
-
-    case PrefType::Bool:
-      value.mBoolVal = (aPS->mVb == kTrue);
-      break;
-
-    default:
-      break;
-  }
-
-  (*aPS->mReader)(aPS->mClosure,
-                  aPS->mLb,
-                  value,
-                  aPS->mVtype,
-                  aPS->mIsDefault,
-                  aPS->mIsStickyDefault);
-  return true;
-}
-
 // Initialize a PrefParseState instance.
 //
 // |aPS| is the PrefParseState instance.
@@ -1986,9 +1943,40 @@ PREF_ParseBuf(PrefParseState* aPS, const char* aBuf, int aBufLen)
       case PREF_PARSE_UNTIL_SEMICOLON:
         // tolerate only whitespace and embedded comments
         if (c == ';') {
-          if (!pref_DoCallback(aPS)) {
-            return false;
+
+          PrefValue value;
+
+          switch (aPS->mVtype) {
+            case PrefType::String:
+              value.mStringVal = aPS->mVb;
+              break;
+
+            case PrefType::Int:
+              if ((aPS->mVb[0] == '-' || aPS->mVb[0] == '+') &&
+                  aPS->mVb[1] == '\0') {
+                pref_ReportParseProblem(*aPS, "invalid integer value", 0, true);
+                NS_WARNING("malformed integer value");
+                return false;
+              }
+              value.mIntVal = atoi(aPS->mVb);
+              break;
+
+            case PrefType::Bool:
+              value.mBoolVal = (aPS->mVb == kTrue);
+              break;
+
+            default:
+              break;
           }
+
+          // We've extracted a complete name/value pair.
+          aPS->mReader(aPS->mClosure,
+                       aPS->mLb,
+                       value,
+                       aPS->mVtype,
+                       aPS->mIsDefault,
+                       aPS->mIsStickyDefault);
+
           state = PREF_PARSE_INIT;
         } else if (c == '/') {
           aPS->mNextState = state; // return here when done with comment

From 49e302c18ea578176fdcae06fdcba411f6783472 Mon Sep 17 00:00:00 2001
From: Nicholas Nethercote <nnethercote@mozilla.com>
Date: Wed, 25 Oct 2017 10:22:38 +1100
Subject: [PATCH 04/20] Bug 1411480 - Inline and remove pref_SetWatchingPref().
 r=glandium.

It's simple enough that having a separate function isn't helpful.

MozReview-Commit-ID: Ke9BIfp9yHU

--HG--
extra : rebase_source : de1518544ddb28185635628c33d356ab07480c1c
---
 modules/libpref/Preferences.cpp | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/modules/libpref/Preferences.cpp b/modules/libpref/Preferences.cpp
index 3a75ac32be37..69801c55da17 100644
--- a/modules/libpref/Preferences.cpp
+++ b/modules/libpref/Preferences.cpp
@@ -999,8 +999,6 @@ pref_SetValue(PrefValue* aExistingValue,
 
 static pref_initPhase gPhase = START;
 
-static bool gWatchingPref = false;
-
 static void
 pref_SetInitPhase(pref_initPhase aPhase)
 {
@@ -1013,12 +1011,6 @@ pref_GetInitPhase()
   return gPhase;
 }
 
-static void
-pref_SetWatchingPref(bool aWatching)
-{
-  gWatchingPref = aWatching;
-}
-
 struct StringComparator
 {
   const char* mKey;
@@ -1038,11 +1030,13 @@ InInitArray(const char* aKey)
   return BinarySearchIf(list, 0, prefsLen, StringComparator(aKey), &found);
 }
 
+static bool gWatchingPref = false;
+
 class WatchingPrefRAII
 {
 public:
-  WatchingPrefRAII() { pref_SetWatchingPref(true); }
-  ~WatchingPrefRAII() { pref_SetWatchingPref(false); }
+  WatchingPrefRAII() { gWatchingPref = true; }
+  ~WatchingPrefRAII() { gWatchingPref = false; }
 };
 
 #define WATCHING_PREF_RAII() WatchingPrefRAII watchingPrefRAII

From 5eeb08fa635947992d81848c4c5df664dd17cb64 Mon Sep 17 00:00:00 2001
From: Nicholas Nethercote <nnethercote@mozilla.com>
Date: Wed, 25 Oct 2017 10:22:38 +1100
Subject: [PATCH 05/20] Bug 1411480 - Inline and remove pref_[SG]etInitPhase().
 r=glandium.

They both have a single use.

MozReview-Commit-ID: 4Jj64B6NV0o

--HG--
extra : rebase_source : 51297caa052281544383bc76f120b4b62b686015
---
 modules/libpref/Preferences.cpp | 18 +++---------------
 1 file changed, 3 insertions(+), 15 deletions(-)

diff --git a/modules/libpref/Preferences.cpp b/modules/libpref/Preferences.cpp
index 69801c55da17..edee00760333 100644
--- a/modules/libpref/Preferences.cpp
+++ b/modules/libpref/Preferences.cpp
@@ -999,18 +999,6 @@ pref_SetValue(PrefValue* aExistingValue,
 
 static pref_initPhase gPhase = START;
 
-static void
-pref_SetInitPhase(pref_initPhase aPhase)
-{
-  gPhase = aPhase;
-}
-
-static pref_initPhase
-pref_GetInitPhase()
-{
-  return gPhase;
-}
-
 struct StringComparator
 {
   const char* mKey;
@@ -4243,15 +4231,15 @@ Preferences::GetPreferences(InfallibleTArray<PrefSetting>* aPrefs)
 
 #ifdef DEBUG
 void
-Preferences::SetInitPhase(pref_initPhase phase)
+Preferences::SetInitPhase(pref_initPhase aPhase)
 {
-  pref_SetInitPhase(phase);
+  gPhase = aPhase;
 }
 
 pref_initPhase
 Preferences::InitPhase()
 {
-  return pref_GetInitPhase();
+  return gPhase;
 }
 #endif
 

From 21290f61598ca23764cd640300ac7e0c29f33a46 Mon Sep 17 00:00:00 2001
From: Nicholas Nethercote <nnethercote@mozilla.com>
Date: Wed, 25 Oct 2017 10:22:38 +1100
Subject: [PATCH 06/20] Bug 1411480 - Inline and remove PREF_GetPrefType().
 r=glandium.

It has a single call site.

MozReview-Commit-ID: o5CwR8Od7o

--HG--
extra : rebase_source : 2f24a0bc7fa65af8cb60b820982c8d6cb8ee5298
---
 modules/libpref/Preferences.cpp | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/modules/libpref/Preferences.cpp b/modules/libpref/Preferences.cpp
index edee00760333..d09ae15cd719 100644
--- a/modules/libpref/Preferences.cpp
+++ b/modules/libpref/Preferences.cpp
@@ -1158,18 +1158,6 @@ pref_SizeOfPrivateData(MallocSizeOf aMallocSizeOf)
   return n;
 }
 
-static PrefType
-PREF_GetPrefType(const char* aPrefName)
-{
-  if (gHashTable) {
-    PrefHashEntry* pref = pref_HashTableLookup(aPrefName);
-    if (pref) {
-      return pref->mPrefFlags.GetPrefType();
-    }
-  }
-  return PrefType::Invalid;
-}
-
 // Bool function that returns whether or not the preference is locked and
 // therefore cannot be changed.
 static bool
@@ -2315,8 +2303,17 @@ NS_IMETHODIMP
 nsPrefBranch::GetPrefType(const char* aPrefName, int32_t* aRetVal)
 {
   NS_ENSURE_ARG(aPrefName);
+
   const PrefName& pref = GetPrefName(aPrefName);
-  switch (PREF_GetPrefType(pref.get())) {
+  PrefType type = PrefType::Invalid;
+  if (gHashTable) {
+    PrefHashEntry* entry = pref_HashTableLookup(pref.get());
+    if (entry) {
+      type = entry->mPrefFlags.GetPrefType();
+    }
+  }
+
+  switch (type) {
     case PrefType::String:
       *aRetVal = PREF_STRING;
       break;

From 74915ed1709b689b2c8085b9827077687bee5f3a Mon Sep 17 00:00:00 2001
From: Nicholas Nethercote <nnethercote@mozilla.com>
Date: Wed, 25 Oct 2017 10:22:38 +1100
Subject: [PATCH 07/20] Bug 1411480 - Remove the machinery for choosing the
 dirty callback. r=glandium.

It's unnecessarily general, because we only ever use
Preferences::DirtyCallback() as the callback.

And because it's no longer a callback, the patch renames DirtyCallback() as
HandleDirty().

MozReview-Commit-ID: Hl50dcxfVQq

--HG--
extra : rebase_source : f453d31215de3fdb0c5b6176becf2669e7ad55f1
---
 modules/libpref/Preferences.cpp | 40 +++++++--------------------------
 modules/libpref/Preferences.h   |  2 +-
 2 files changed, 9 insertions(+), 33 deletions(-)

diff --git a/modules/libpref/Preferences.cpp b/modules/libpref/Preferences.cpp
index d09ae15cd719..21dd67353ed3 100644
--- a/modules/libpref/Preferences.cpp
+++ b/modules/libpref/Preferences.cpp
@@ -333,29 +333,6 @@ static PLDHashTableOps pref_HashTableOps = {
   nullptr,
 };
 
-typedef void (*PrefsDirtyFunc)();
-static PrefsDirtyFunc gDirtyCallback = nullptr;
-
-static inline void
-MakeDirtyCallback()
-{
-  // Right now the callback function is always set, so we don't need
-  // to complicate the code to cover the scenario where we set the callback
-  // after we've already tried to make it dirty.  If this assert triggers
-  // we will add that code.
-  MOZ_ASSERT(gDirtyCallback);
-  if (gDirtyCallback) {
-    gDirtyCallback();
-  }
-}
-
-// Callback for whenever we change a preference.
-static void
-PREF_SetDirtyCallback(PrefsDirtyFunc aFunc)
-{
-  gDirtyCallback = aFunc;
-}
-
 //---------------------------------------------------------------------------
 
 static bool
@@ -852,7 +829,7 @@ PREF_DeleteBranch(const char* aBranchName)
     }
   }
 
-  MakeDirtyCallback();
+  Preferences::HandleDirty();
   return NS_OK;
 }
 
@@ -873,7 +850,7 @@ PREF_ClearUserPref(const char* aPrefName)
     }
 
     pref_DoCallback(aPrefName);
-    MakeDirtyCallback();
+    Preferences::HandleDirty();
   }
   return NS_OK;
 }
@@ -906,7 +883,7 @@ PREF_ClearAllUserPrefs()
     pref_DoCallback(prefString.c_str());
   }
 
-  MakeDirtyCallback();
+  Preferences::HandleDirty();
   return NS_OK;
 }
 
@@ -1123,7 +1100,7 @@ pref_HashPref(const char* aKey,
         // XXX should we free a user-set string value if there is one?
         pref->mPrefFlags.SetHasUserValue(false);
         if (!pref->mPrefFlags.IsLocked()) {
-          MakeDirtyCallback();
+          Preferences::HandleDirty();
           valueChanged = true;
         }
       }
@@ -1134,7 +1111,7 @@ pref_HashPref(const char* aKey,
         pref_SetValue(&pref->mUserPref, pref->mPrefFlags, aValue, aType)
           .SetHasUserValue(true);
       if (!pref->mPrefFlags.IsLocked()) {
-        MakeDirtyCallback();
+        Preferences::HandleDirty();
         valueChanged = true;
       }
     }
@@ -3233,7 +3210,7 @@ namespace mozilla {
 static NS_DEFINE_CID(kZipReaderCID, NS_ZIPREADER_CID);
 
 void
-Preferences::DirtyCallback()
+Preferences::HandleDirty()
 {
   if (!XRE_IsParentProcess()) {
     // TODO: this should really assert because you can't set prefs in a
@@ -3541,7 +3518,7 @@ public:
                                [fileCopy, rvCopy] {
                                  MOZ_RELEASE_ASSERT(NS_IsMainThread());
                                  if (NS_FAILED(rvCopy)) {
-                                   Preferences::DirtyCallback();
+                                   Preferences::HandleDirty();
                                  }
                                }));
     }
@@ -3930,7 +3907,6 @@ Preferences::SetInitPreferences(nsTArray<PrefSetting>* aPrefs)
 Result<Ok, const char*>
 Preferences::Init()
 {
-  PREF_SetDirtyCallback(&DirtyCallback);
   PREF_Init();
 
   MOZ_TRY(pref_InitInitialObjects());
@@ -4404,7 +4380,7 @@ Preferences::SavePrefFileInternal(nsIFile* aFile, SaveMethod aSaveMethod)
     }
 
     // Check for profile shutdown after mDirty because the runnables from
-    // DirtyCallback can still be pending.
+    // HandleDirty() can still be pending.
     if (mProfileShutdown) {
       NS_WARNING("Cannot save pref file after profile shutdown.");
       return NS_ERROR_ILLEGAL_DURING_SHUTDOWN;
diff --git a/modules/libpref/Preferences.h b/modules/libpref/Preferences.h
index aee201b1e0a6..df29c62f6001 100644
--- a/modules/libpref/Preferences.h
+++ b/modules/libpref/Preferences.h
@@ -334,7 +334,7 @@ public:
   static int64_t SizeOfIncludingThisAndOtherStuff(
     mozilla::MallocSizeOf aMallocSizeOf);
 
-  static void DirtyCallback();
+  static void HandleDirty();
 
   // Explicitly choosing synchronous or asynchronous (if allowed) preferences
   // file write. Only for the default file.  The guarantee for the "blocking"

From f357e385870a29f0693342feb3c7da79f4b6b068 Mon Sep 17 00:00:00 2001
From: Nicholas Nethercote <nnethercote@mozilla.com>
Date: Wed, 25 Oct 2017 10:22:39 +1100
Subject: [PATCH 08/20] Bug 1411480 - Simplify pref_HashPref()'s workings.
 r=glandium.

First, the patch removes the return values from PrefTypeFlags::Set*(). They
allow chained calls, but they're barely used and they just make the code harder
to read.

Second, the patch changes pref_SetValue() to not modify and return the pref
flags. It's clearer if the flags changing is done separately. This requires
passing pref_SetValue() the old type instead of the flags.

MozReview-Commit-ID: HZVS2TIlBIY

--HG--
extra : rebase_source : 32950f00975f7d9df63fa0af1c36e82b58516245
---
 modules/libpref/Preferences.cpp | 48 +++++++++++++++------------------
 1 file changed, 21 insertions(+), 27 deletions(-)

diff --git a/modules/libpref/Preferences.cpp b/modules/libpref/Preferences.cpp
index 21dd67353ed3..4a8b41ce485b 100644
--- a/modules/libpref/Preferences.cpp
+++ b/modules/libpref/Preferences.cpp
@@ -190,10 +190,9 @@ public:
   bool IsTypeBool() const { return IsPrefType(PrefType::Bool); }
   bool IsPrefType(PrefType type) const { return GetPrefType() == type; }
 
-  PrefTypeFlags& SetPrefType(PrefType aType)
+  void SetPrefType(PrefType aType)
   {
     mValue = mValue - AsInt(GetPrefType()) + AsInt(aType);
-    return *this;
   }
 
   PrefType GetPrefType() const
@@ -204,39 +203,35 @@ public:
 
   bool HasDefault() const { return mValue & PREF_FLAG_HAS_DEFAULT; }
 
-  PrefTypeFlags& SetHasDefault(bool aSetOrUnset)
+  void SetHasDefault(bool aSetOrUnset)
   {
-    return SetFlag(PREF_FLAG_HAS_DEFAULT, aSetOrUnset);
+    SetFlag(PREF_FLAG_HAS_DEFAULT, aSetOrUnset);
   }
 
   bool HasStickyDefault() const { return mValue & PREF_FLAG_STICKY_DEFAULT; }
 
-  PrefTypeFlags& SetHasStickyDefault(bool aSetOrUnset)
+  void SetHasStickyDefault(bool aSetOrUnset)
   {
-    return SetFlag(PREF_FLAG_STICKY_DEFAULT, aSetOrUnset);
+    SetFlag(PREF_FLAG_STICKY_DEFAULT, aSetOrUnset);
   }
 
   bool IsLocked() const { return mValue & PREF_FLAG_LOCKED; }
 
-  PrefTypeFlags& SetLocked(bool aSetOrUnset)
-  {
-    return SetFlag(PREF_FLAG_LOCKED, aSetOrUnset);
-  }
+  void SetLocked(bool aSetOrUnset) { SetFlag(PREF_FLAG_LOCKED, aSetOrUnset); }
 
   bool HasUserValue() const { return mValue & PREF_FLAG_USERSET; }
 
-  PrefTypeFlags& SetHasUserValue(bool aSetOrUnset)
+  void SetHasUserValue(bool aSetOrUnset)
   {
-    return SetFlag(PREF_FLAG_USERSET, aSetOrUnset);
+    SetFlag(PREF_FLAG_USERSET, aSetOrUnset);
   }
 
 private:
   static uint16_t AsInt(PrefType aType) { return (uint16_t)aType; }
 
-  PrefTypeFlags& SetFlag(uint16_t aFlag, bool aSetOrUnset)
+  void SetFlag(uint16_t aFlag, bool aSetOrUnset)
   {
     mValue = aSetOrUnset ? mValue | aFlag : mValue & ~aFlag;
-    return *this;
   }
 
   // We pack both the value of type (PrefType) and flags into the same int. The
@@ -950,26 +945,23 @@ pref_ValueChanged(PrefValue aOldValue, PrefValue aNewValue, PrefType aType)
 // Overwrite the type and value of an existing preference. Caller must ensure
 // that they are not changing the type of a preference that has a default
 // value.
-static PrefTypeFlags
+static void
 pref_SetValue(PrefValue* aExistingValue,
-              PrefTypeFlags aFlags,
+              PrefType aExistingType,
               PrefValue aNewValue,
               PrefType aNewType)
 {
-  if (aFlags.IsTypeString() && aExistingValue->mStringVal) {
+  if (aExistingType == PrefType::String) {
     free(aExistingValue->mStringVal);
   }
 
-  aFlags.SetPrefType(aNewType);
-  if (aFlags.IsTypeString()) {
+  if (aNewType == PrefType::String) {
     MOZ_ASSERT(aNewValue.mStringVal);
     aExistingValue->mStringVal =
       aNewValue.mStringVal ? moz_xstrdup(aNewValue.mStringVal) : nullptr;
   } else {
     *aExistingValue = aNewValue;
   }
-
-  return aFlags;
 }
 
 #ifdef DEBUG
@@ -1075,9 +1067,10 @@ pref_HashPref(const char* aKey,
       // ?? change of semantics?
       if (pref_ValueChanged(pref->mDefaultPref, aValue, aType) ||
           !pref->mPrefFlags.HasDefault()) {
-        pref->mPrefFlags =
-          pref_SetValue(&pref->mDefaultPref, pref->mPrefFlags, aValue, aType)
-            .SetHasDefault(true);
+        pref_SetValue(
+          &pref->mDefaultPref, pref->mPrefFlags.GetPrefType(), aValue, aType);
+        pref->mPrefFlags.SetPrefType(aType);
+        pref->mPrefFlags.SetHasDefault(true);
         if (aFlags & kPrefStickyDefault) {
           pref->mPrefFlags.SetHasStickyDefault(true);
         }
@@ -1107,9 +1100,10 @@ pref_HashPref(const char* aKey,
     } else if (!pref->mPrefFlags.HasUserValue() ||
                !pref->mPrefFlags.IsPrefType(aType) ||
                pref_ValueChanged(pref->mUserPref, aValue, aType)) {
-      pref->mPrefFlags =
-        pref_SetValue(&pref->mUserPref, pref->mPrefFlags, aValue, aType)
-          .SetHasUserValue(true);
+      pref_SetValue(
+        &pref->mUserPref, pref->mPrefFlags.GetPrefType(), aValue, aType);
+      pref->mPrefFlags.SetPrefType(aType);
+      pref->mPrefFlags.SetHasUserValue(true);
       if (!pref->mPrefFlags.IsLocked()) {
         Preferences::HandleDirty();
         valueChanged = true;

From e52452101fef63352dcbdfafe38cf3f18787ece7 Mon Sep 17 00:00:00 2001
From: Nicholas Nethercote <nnethercote@mozilla.com>
Date: Wed, 25 Oct 2017 10:22:39 +1100
Subject: [PATCH 09/20] Bug 1411480 - Remove pref_savePrefs()'s argument.
 r=glandium.

It's always gHashTable, and all the other functions in this file work directly
on gHashTable rather than taking a parameter.

MozReview-Commit-ID: BDCEvcMlo8P

--HG--
extra : rebase_source : e5d10e42401136a9a82d665d9717cf71ecf7e4ae
---
 modules/libpref/Preferences.cpp | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/modules/libpref/Preferences.cpp b/modules/libpref/Preferences.cpp
index 4a8b41ce485b..f0ad956c5715 100644
--- a/modules/libpref/Preferences.cpp
+++ b/modules/libpref/Preferences.cpp
@@ -549,11 +549,11 @@ pref_SetPref(const dom::PrefSetting& aPref)
 }
 
 static PrefSaveData
-pref_savePrefs(PLDHashTable* aTable)
+pref_savePrefs()
 {
-  PrefSaveData savedPrefs(aTable->EntryCount());
+  PrefSaveData savedPrefs(gHashTable->EntryCount());
 
-  for (auto iter = aTable->Iter(); !iter.Done(); iter.Next()) {
+  for (auto iter = gHashTable->Iter(); !iter.Done(); iter.Next()) {
     auto pref = static_cast<PrefHashEntry*>(iter.Get());
 
     nsAutoCString prefValue;
@@ -4411,7 +4411,7 @@ Preferences::WritePrefFile(nsIFile* aFile, SaveMethod aSaveMethod)
 
     nsresult rv = NS_OK;
     mozilla::UniquePtr<PrefSaveData> prefs =
-      MakeUnique<PrefSaveData>(pref_savePrefs(gHashTable));
+      MakeUnique<PrefSaveData>(pref_savePrefs());
 
     // Put the newly constructed preference data into sPendingWriteData
     // for the next request to pick up
@@ -4446,7 +4446,7 @@ Preferences::WritePrefFile(nsIFile* aFile, SaveMethod aSaveMethod)
   // This will do a main thread write. It is safe to do it this way because
   // AllowOffMainThreadSave() returns a consistent value for the lifetime of
   // the parent process.
-  PrefSaveData prefsData = pref_savePrefs(gHashTable);
+  PrefSaveData prefsData = pref_savePrefs();
   return PreferencesWriter::Write(aFile, prefsData);
 }
 

From d7b5b4b246262804eb3defd345474d5c0b2b6351 Mon Sep 17 00:00:00 2001
From: Nicholas Nethercote <nnethercote@mozilla.com>
Date: Wed, 25 Oct 2017 10:22:39 +1100
Subject: [PATCH 10/20] Bug 1411480 - Use |private| instead of |protected| in
 nsPrefBranch. r=glandium.

There's not much use using |protected| in a |final| class.

MozReview-Commit-ID: IECyfNGZsL5

--HG--
extra : rebase_source : bc9597ee45583e809f89f7e558cc325460390f98
---
 modules/libpref/Preferences.cpp | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/modules/libpref/Preferences.cpp b/modules/libpref/Preferences.cpp
index f0ad956c5715..2705227583d7 100644
--- a/modules/libpref/Preferences.cpp
+++ b/modules/libpref/Preferences.cpp
@@ -2110,7 +2110,7 @@ public:
 
   static void ReportToConsole(const nsAString& aMessage);
 
-protected:
+private:
   // Helper class for either returning a raw cstring or nsCString.
   typedef mozilla::Variant<const char*, const nsCString> PrefNameBase;
   class PrefName : public PrefNameBase
@@ -2179,7 +2179,6 @@ protected:
 
   void FreeObserverList(void);
 
-private:
   const nsCString mPrefRoot;
   bool mIsDefault;
 

From 9702633811f4b0cb88beb550986ab0d54b724414 Mon Sep 17 00:00:00 2001
From: Nicholas Nethercote <nnethercote@mozilla.com>
Date: Wed, 25 Oct 2017 10:22:39 +1100
Subject: [PATCH 11/20] Bug 1411480 - Change PrefSaveData's element type.
 r=glandium.

Because nsCString is nicer than UniqueFreePtr<char>.

The patch also streamlines pref_savePrefs(). And also changes StrEscape() to
always put quotations around the result, because that's needed in both call
sites.

MozReview-Commit-ID: HT7HZz4QiSN

--HG--
extra : rebase_source : f8d26a8c9f7ea911272113efc56744df639c5ccf
---
 modules/libpref/Preferences.cpp | 52 +++++++++++++++------------------
 1 file changed, 23 insertions(+), 29 deletions(-)

diff --git a/modules/libpref/Preferences.cpp b/modules/libpref/Preferences.cpp
index 2705227583d7..833c9912b172 100644
--- a/modules/libpref/Preferences.cpp
+++ b/modules/libpref/Preferences.cpp
@@ -137,7 +137,7 @@ using namespace mozilla;
 
 struct PrefHashEntry;
 
-typedef nsTArray<mozilla::UniqueFreePtr<char>> PrefSaveData;
+typedef nsTArray<nsCString> PrefSaveData;
 
 static PrefHashEntry*
 pref_HashTableLookup(const char* aKey);
@@ -394,10 +394,15 @@ PREF_Cleanup()
   PREF_CleanupPrefs();
 }
 
-// Note that this appends to aResult, and does not assign!
+// Assign to aResult a quoted, escaped copy of aOriginal.
 static void
 StrEscape(const char* aOriginal, nsCString& aResult)
 {
+  if (aOriginal == nullptr) {
+    aResult.AssignLiteral("\"\"");
+    return;
+  }
+
   // JavaScript does not allow quotes, slashes, or line terminators inside
   // strings so we must escape them. ECMAScript defines four line terminators,
   // but we're only worrying about \r and \n here.  We currently feed our pref
@@ -410,9 +415,7 @@ StrEscape(const char* aOriginal, nsCString& aResult)
   // \u2029.
   const char* p;
 
-  if (aOriginal == nullptr) {
-    return;
-  }
+  aResult.Assign("'");
 
   // Paranoid worst case all slashes will free quickly.
   for (p = aOriginal; *p; ++p) {
@@ -438,6 +441,8 @@ StrEscape(const char* aOriginal, nsCString& aResult)
         break;
     }
   }
+
+  aResult.Append('"');
 }
 
 //
@@ -556,10 +561,6 @@ pref_savePrefs()
   for (auto iter = gHashTable->Iter(); !iter.Done(); iter.Next()) {
     auto pref = static_cast<PrefHashEntry*>(iter.Get());
 
-    nsAutoCString prefValue;
-    nsAutoCString prefPrefix;
-    prefPrefix.AssignLiteral("user_pref(\"");
-
     // where we're getting our pref from
     PrefValue* sourcePref;
 
@@ -575,25 +576,22 @@ pref_savePrefs()
       continue;
     }
 
-    // strings are in quotes!
+    nsAutoCString prefName;
+    StrEscape(pref->mKey, prefName);
+
+    nsAutoCString prefValue;
     if (pref->mPrefFlags.IsTypeString()) {
-      prefValue = '\"';
       StrEscape(sourcePref->mStringVal, prefValue);
-      prefValue += '\"';
 
     } else if (pref->mPrefFlags.IsTypeInt()) {
       prefValue.AppendInt(sourcePref->mIntVal);
 
     } else if (pref->mPrefFlags.IsTypeBool()) {
-      prefValue = (sourcePref->mBoolVal) ? "true" : "false";
+      prefValue = sourcePref->mBoolVal ? "true" : "false";
     }
 
-    nsAutoCString prefName;
-    StrEscape(pref->mKey, prefName);
-
-    savedPrefs.AppendElement()->reset(
-      ToNewCString(prefPrefix + prefName + NS_LITERAL_CSTRING("\", ") +
-                   prefValue + NS_LITERAL_CSTRING(");")));
+    nsPrintfCString str("user_pref(%s, %s);", prefName.get(), prefValue.get());
+    savedPrefs.AppendElement(str);
   }
 
   return savedPrefs;
@@ -3413,16 +3411,14 @@ public:
 
     struct CharComparator
     {
-      bool LessThan(const mozilla::UniqueFreePtr<char>& a,
-                    const mozilla::UniqueFreePtr<char>& b) const
+      bool LessThan(const nsCString& aA, const nsCString& aB) const
       {
-        return strcmp(a.get(), b.get()) < 0;
+        return aA < aB;
       }
 
-      bool Equals(const mozilla::UniqueFreePtr<char>& a,
-                  const mozilla::UniqueFreePtr<char>& b) const
+      bool Equals(const nsCString& aA, const nsCString& aB) const
       {
-        return strcmp(a.get(), b.get()) == 0;
+        return aA == aB;
       }
     };
 
@@ -3433,10 +3429,8 @@ public:
     outStream->Write(
       kPrefFileHeader, sizeof(kPrefFileHeader) - 1, &writeAmount);
 
-    for (auto& prefptr : aPrefs) {
-      char* pref = prefptr.get();
-      MOZ_ASSERT(pref);
-      outStream->Write(pref, strlen(pref), &writeAmount);
+    for (nsCString& pref : aPrefs) {
+      outStream->Write(pref.get(), pref.Length(), &writeAmount);
       outStream->Write(NS_LINEBREAK, NS_LINEBREAK_LEN, &writeAmount);
     }
 

From 9222ab2401b7711077d46419cb3e0a2e7ff45204 Mon Sep 17 00:00:00 2001
From: Nicholas Nethercote <nnethercote@mozilla.com>
Date: Thu, 26 Oct 2017 12:29:34 +1100
Subject: [PATCH 12/20] Bug 1411480 - Make PrefValue::mStringVal const.
 r=glandium.

This makes it clear that the stored string values are never modified.

It requires some const_casts to free the strings, but that feels like an ok
trade-off.

MozReview-Commit-ID: JikBT3uwpfq

--HG--
extra : rebase_source : e611e57de4f059275ae2908f7300065cf953461d
---
 modules/libpref/Preferences.cpp | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/modules/libpref/Preferences.cpp b/modules/libpref/Preferences.cpp
index 833c9912b172..256c2adfabaf 100644
--- a/modules/libpref/Preferences.cpp
+++ b/modules/libpref/Preferences.cpp
@@ -148,7 +148,7 @@ static const uint32_t MAX_PREF_LENGTH = 1 * 1024 * 1024;
 static const uint32_t MAX_ADVISABLE_PREF_LENGTH = 4 * 1024;
 
 union PrefValue {
-  char* mStringVal;
+  const char* mStringVal;
   int32_t mIntVal;
   bool mBoolVal;
 };
@@ -262,12 +262,8 @@ ClearPrefEntry(PLDHashTable* aTable, PLDHashEntryHdr* aEntry)
 {
   auto pref = static_cast<PrefHashEntry*>(aEntry);
   if (pref->mPrefFlags.IsTypeString()) {
-    if (pref->mDefaultPref.mStringVal) {
-      free(pref->mDefaultPref.mStringVal);
-    }
-    if (pref->mUserPref.mStringVal) {
-      free(pref->mUserPref.mStringVal);
-    }
+    free(const_cast<char*>(pref->mDefaultPref.mStringVal));
+    free(const_cast<char*>(pref->mUserPref.mStringVal));
   }
 
   // Don't need to free this because it's allocated in memory owned by
@@ -469,7 +465,7 @@ PREF_SetCharPref(const char* aPrefName, const char* aValue, bool aSetDefault)
   }
 
   PrefValue pref;
-  pref.mStringVal = const_cast<char*>(aValue);
+  pref.mStringVal = aValue;
 
   return pref_HashPref(
     aPrefName, pref, PrefType::String, aSetDefault ? kPrefSetDefault : 0);
@@ -604,7 +600,7 @@ pref_EntryHasAdvisablySizedValues(PrefHashEntry* aHashEntry)
     return true;
   }
 
-  char* stringVal;
+  const char* stringVal;
   if (aHashEntry->mPrefFlags.HasDefault()) {
     stringVal = aHashEntry->mDefaultPref.mStringVal;
     if (strlen(stringVal) > MAX_ADVISABLE_PREF_LENGTH) {
@@ -697,7 +693,7 @@ PREF_CopyCharPref(const char* aPrefName, char** aValueOut, bool aGetDefault)
   }
 
   nsresult rv = NS_ERROR_UNEXPECTED;
-  char* stringVal;
+  const char* stringVal;
   PrefHashEntry* pref = pref_HashTableLookup(aPrefName);
 
   if (pref && pref->mPrefFlags.IsTypeString()) {
@@ -950,7 +946,7 @@ pref_SetValue(PrefValue* aExistingValue,
               PrefType aNewType)
 {
   if (aExistingType == PrefType::String) {
-    free(aExistingValue->mStringVal);
+    free(const_cast<char*>(aExistingValue->mStringVal));
   }
 
   if (aNewType == PrefType::String) {

From 14790e76d7aeb44d5e568b8e27d56db0b6a6d712 Mon Sep 17 00:00:00 2001
From: Nicholas Nethercote <nnethercote@mozilla.com>
Date: Thu, 26 Oct 2017 12:29:41 +1100
Subject: [PATCH 13/20] Bug 1411480 - Make CallbackNode::mDomain const.
 r=glandium.

MozReview-Commit-ID: EvLC5FrMyKy

--HG--
extra : rebase_source : d5998326dd40365903554094b9038248095020dc
---
 modules/libpref/Preferences.cpp | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/modules/libpref/Preferences.cpp b/modules/libpref/Preferences.cpp
index 256c2adfabaf..7275bc2d0607 100644
--- a/modules/libpref/Preferences.cpp
+++ b/modules/libpref/Preferences.cpp
@@ -291,7 +291,7 @@ MatchPrefEntry(const PLDHashEntryHdr* aEntry, const void* aKey)
 
 struct CallbackNode
 {
-  char* mDomain;
+  const char* mDomain;
 
   // If someone attempts to remove the node from the callback list while
   // pref_DoCallback is running, |func| is set to nullptr. Such nodes will
@@ -381,7 +381,7 @@ PREF_Cleanup()
 
   while (node) {
     next_node = node->mNext;
-    free(node->mDomain);
+    free(const_cast<char*>(node->mDomain));
     free(node);
     node = next_node;
   }
@@ -1194,7 +1194,7 @@ pref_RemoveCallbackNode(CallbackNode* aNode, CallbackNode* aPrevNode)
   if (gLastPriorityNode == aNode) {
     gLastPriorityNode = aPrevNode;
   }
-  free(aNode->mDomain);
+  free(const_cast<char*>(aNode->mDomain));
   free(aNode);
   return next_node;
 }

From 140ebef4af351be418ab5a2a41ba4b2f69945691 Mon Sep 17 00:00:00 2001
From: Ryan VanderMeulen <ryanvm@gmail.com>
Date: Wed, 25 Oct 2017 22:10:11 -0400
Subject: [PATCH 14/20] Backed out changeset 2c36f41ed77c (bug 1410123) for
 causing frequent Windows mochitest-gl leaks.

---
 js/src/gc/GCRuntime.h                         |  5 ---
 js/src/jsapi-tests/testGCFinalizeCallback.cpp | 14 +++----
 js/src/jsgc.cpp                               | 39 ++++++-------------
 3 files changed, 19 insertions(+), 39 deletions(-)

diff --git a/js/src/gc/GCRuntime.h b/js/src/gc/GCRuntime.h
index e26fc78ee484..01f4733f95b4 100644
--- a/js/src/gc/GCRuntime.h
+++ b/js/src/gc/GCRuntime.h
@@ -909,8 +909,6 @@ class GCRuntime
 
     bool fullGCForAtomsRequested() const { return fullGCForAtomsRequested_; }
 
-    bool shouldSweepAtomsZone() { return shouldSweepAtomsZone_; }
-
     double computeHeapGrowthFactor(size_t lastBytes);
     size_t computeTriggerBytes(double growthFactor, size_t lastBytes);
 
@@ -1303,9 +1301,6 @@ class GCRuntime
     /* Whether observed type information is being released in the current GC. */
     ActiveThreadData<bool> releaseObservedTypes;
 
-    /* Whether the atoms table will be swept. */
-    ActiveThreadOrGCTaskData<bool> shouldSweepAtomsZone_;
-
     /* Singly linked list of zones to be swept in the background. */
     ActiveThreadOrGCTaskData<ZoneList> backgroundSweepZones;
 
diff --git a/js/src/jsapi-tests/testGCFinalizeCallback.cpp b/js/src/jsapi-tests/testGCFinalizeCallback.cpp
index f7b26649cd6d..14204b3923d0 100644
--- a/js/src/jsapi-tests/testGCFinalizeCallback.cpp
+++ b/js/src/jsapi-tests/testGCFinalizeCallback.cpp
@@ -4,7 +4,7 @@
 
 #include "jsapi-tests/tests.h"
 
-static const unsigned BufferSize = 32;
+static const unsigned BufferSize = 20;
 static unsigned FinalizeCalls = 0;
 static JSFinalizeStatus StatusBuffer[BufferSize];
 
@@ -16,7 +16,7 @@ BEGIN_TEST(testGCFinalizeCallback)
     FinalizeCalls = 0;
     JS_GC(cx);
     CHECK(cx->runtime()->gc.isFullGc());
-    CHECK(checkGroupCount(1));
+    CHECK(checkSingleGroup());
     CHECK(checkFinalizeStatus());
 
     /* Full GC, incremental. */
@@ -50,7 +50,7 @@ BEGIN_TEST(testGCFinalizeCallback)
     JS::PrepareZoneForGC(global1->zone());
     JS::GCForReason(cx, GC_NORMAL, JS::gcreason::API);
     CHECK(!cx->runtime()->gc.isFullGc());
-    CHECK(checkGroupCount(1));
+    CHECK(checkSingleGroup());
     CHECK(checkFinalizeStatus());
 
     /* Zone GC, non-incremental, multiple zones. */
@@ -60,7 +60,7 @@ BEGIN_TEST(testGCFinalizeCallback)
     JS::PrepareZoneForGC(global3->zone());
     JS::GCForReason(cx, GC_NORMAL, JS::gcreason::API);
     CHECK(!cx->runtime()->gc.isFullGc());
-    CHECK(checkGroupCount(1));
+    CHECK(checkSingleGroup());
     CHECK(checkFinalizeStatus());
 
     /* Zone GC, incremental, single zone. */
@@ -73,7 +73,7 @@ BEGIN_TEST(testGCFinalizeCallback)
     }
     CHECK(!cx->runtime()->gc.isIncrementalGCInProgress());
     CHECK(!cx->runtime()->gc.isFullGc());
-    CHECK(checkGroupCount(2)); // One for our zone, one for the atoms zone.
+    CHECK(checkSingleGroup());
     CHECK(checkFinalizeStatus());
 
     /* Zone GC, incremental, multiple zones. */
@@ -152,10 +152,10 @@ virtual void uninit() override
     JSAPITest::uninit();
 }
 
-bool checkGroupCount(size_t count)
+bool checkSingleGroup()
 {
     CHECK(FinalizeCalls < BufferSize);
-    CHECK(FinalizeCalls == count * 3 + 1);
+    CHECK(FinalizeCalls == 4);
     return true;
 }
 
diff --git a/js/src/jsgc.cpp b/js/src/jsgc.cpp
index 801465eeb234..e7e10293487e 100644
--- a/js/src/jsgc.cpp
+++ b/js/src/jsgc.cpp
@@ -4039,6 +4039,10 @@ ShouldCollectZone(Zone* zone, JS::gcreason::Reason reason)
     // Off-thread parsing is inhibited after the start of GC which prevents
     // races between creating atoms during parsing and sweeping atoms on the
     // active thread.
+    //
+    // Otherwise, we always schedule a GC in the atoms zone so that atoms which
+    // the other collected zones are using are marked, and we can update the
+    // set of atoms in use by the other collected zones at the end of the GC.
     if (zone->isAtomsZone())
         return TlsContext.get()->canCollectAtoms();
 
@@ -5233,11 +5237,9 @@ UpdateAtomsBitmap(JSRuntime* runtime)
 
     // For convenience sweep these tables non-incrementally as part of bitmap
     // sweeping; they are likely to be much smaller than the main atoms table.
-    if (runtime->gc.shouldSweepAtomsZone()) {
-        runtime->unsafeSymbolRegistry().sweep();
-        for (CompartmentsIter comp(runtime, SkipAtoms); !comp.done(); comp.next())
-            comp->sweepVarNames();
-    }
+    runtime->unsafeSymbolRegistry().sweep();
+    for (CompartmentsIter comp(runtime, SkipAtoms); !comp.done(); comp.next())
+        comp->sweepVarNames();
 }
 
 static void
@@ -5493,7 +5495,7 @@ GCRuntime::beginSweepingSweepGroup(FreeOp* fop, SliceBudget& budget)
 
     AutoSCC scc(stats(), sweepGroupIndex);
 
-    bool groupIncludesAtomsZone = false;
+    bool sweepingAtoms = false;
     for (SweepGroupZonesIter zone(rt); !zone.done(); zone.next()) {
         /* Set the GC state to sweeping. */
         zone->changeGCState(Zone::Mark, Zone::Sweep);
@@ -5502,7 +5504,7 @@ GCRuntime::beginSweepingSweepGroup(FreeOp* fop, SliceBudget& budget)
         zone->arenas.purge();
 
         if (zone->isAtomsZone())
-            groupIncludesAtomsZone = true;
+            sweepingAtoms = true;
 
 #ifdef DEBUG
         zone->gcLastSweepGroupIndex = sweepGroupIndex;
@@ -5534,7 +5536,7 @@ GCRuntime::beginSweepingSweepGroup(FreeOp* fop, SliceBudget& budget)
         AutoLockHelperThreadState lock;
 
         Maybe<AutoRunParallelTask> updateAtomsBitmap;
-        if (groupIncludesAtomsZone)
+        if (sweepingAtoms)
             updateAtomsBitmap.emplace(rt, UpdateAtomsBitmap, PhaseKind::UPDATE_ATOMS_BITMAP, lock);
 
         AutoPhase ap(stats(), PhaseKind::SWEEP_COMPARTMENTS);
@@ -5563,15 +5565,13 @@ GCRuntime::beginSweepingSweepGroup(FreeOp* fop, SliceBudget& budget)
             joinTask(task, PhaseKind::SWEEP_WEAK_CACHES, lock);
     }
 
-    if (groupIncludesAtomsZone && shouldSweepAtomsZone())
+    if (sweepingAtoms)
         startSweepingAtomsTable();
 
     // Queue all GC things in all zones for sweeping, either on the foreground
     // or on the background thread.
 
     for (SweepGroupZonesIter zone(rt); !zone.done(); zone.next()) {
-        if (zone->isAtomsZone() && !shouldSweepAtomsZone())
-            continue;
 
         zone->arenas.queueForForegroundSweep(fop, ForegroundObjectFinalizePhase);
         zone->arenas.queueForForegroundSweep(fop, ForegroundNonObjectFinalizePhase);
@@ -5655,9 +5655,6 @@ GCRuntime::beginSweepPhase(JS::gcreason::Reason reason, AutoLockForExclusiveAcce
 
     MOZ_ASSERT(!abortSweepAfterCurrentGroup);
 
-    MOZ_ASSERT(maybeAtomsToSweep.ref().isNothing());
-    MOZ_ASSERT(!rt->atomsAddedWhileSweeping());
-
     AutoSetThreadIsSweeping threadIsSweeping;
 
     releaseHeldRelocatedArenas();
@@ -5832,7 +5829,7 @@ GCRuntime::startSweepingAtomsTable()
 IncrementalProgress
 GCRuntime::sweepAtomsTable(FreeOp* fop, SliceBudget& budget)
 {
-    if (!atomsZone->isGCSweeping() || !shouldSweepAtomsZone())
+    if (!atomsZone->isGCSweeping())
         return Finished;
 
     gcstats::AutoPhase ap(stats(), gcstats::PhaseKind::SWEEP_ATOMS_TABLE);
@@ -6478,9 +6475,6 @@ GCRuntime::endSweepPhase(bool destroyingRuntime, AutoLockForExclusiveAccess& loc
 #endif
 
     AssertNoWrappersInGrayList(rt);
-
-    MOZ_ASSERT(maybeAtomsToSweep.ref().isNothing());
-    MOZ_ASSERT(!rt->atomsAddedWhileSweeping());
 }
 
 void
@@ -7244,15 +7238,6 @@ GCRuntime::gcCycle(bool nonincrementalByAPI, SliceBudget& budget, JS::gcreason::
     // Note that GC callbacks are allowed to re-enter GC.
     AutoCallGCCallbacks callCallbacks(*this);
 
-    // We always schedule a GC in the atoms zone so that atoms which the other
-    // collected zones are using are marked, and we can update the set of atoms
-    // in use by the other collected zones at the end of the GC.  However we
-    // only collect the atoms table if the atoms zone was actually scheduled.
-    if (!isIncrementalGCInProgress()) {
-        shouldSweepAtomsZone_ = atomsZone->isGCScheduled();
-        atomsZone->scheduleGC();
-    }
-
     gcstats::AutoGCSlice agc(stats(), scanZonesBeforeGC(), invocationKind, budget, reason);
 
     minorGC(reason, gcstats::PhaseKind::EVICT_NURSERY_FOR_MAJOR_GC);

From f1d9f1f3801bd3a3f5536677dafd936df898cad5 Mon Sep 17 00:00:00 2001
From: Phil Ringnalda <philringnalda@gmail.com>
Date: Wed, 25 Oct 2017 19:22:36 -0700
Subject: [PATCH 15/20] Backed out changeset c2bdce9b76f2 (bug 1411458) for Mac
 Windows and Android build bustage

MozReview-Commit-ID: 20DOPFcmRFH
---
 security/manager/ssl/nsDataSignatureVerifier.cpp | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/security/manager/ssl/nsDataSignatureVerifier.cpp b/security/manager/ssl/nsDataSignatureVerifier.cpp
index 2527431dd9ae..f78740365e1a 100644
--- a/security/manager/ssl/nsDataSignatureVerifier.cpp
+++ b/security/manager/ssl/nsDataSignatureVerifier.cpp
@@ -170,12 +170,6 @@ VerifyCMSDetachedSignatureIncludingCertificate(
     return NS_ERROR_CMS_VERIFY_NO_CONTENT_INFO;
   }
 
-  // We're expecting this to be a PKCS#7 signedData content info.
-  if (NSS_CMSContentInfo_GetContentTypeTag(cinfo)
-        != SEC_OID_PKCS7_SIGNED_DATA) {
-    return NS_ERROR_CMS_VERIFY_NO_CONTENT_INFO;
-  }
-
   // signedData is non-owning
   NSSCMSSignedData* signedData =
     static_cast<NSSCMSSignedData*>(NSS_CMSContentInfo_GetContent(cinfo));

From 535f8401d0e927a5d08936ee485cdb683fb410b2 Mon Sep 17 00:00:00 2001
From: Phil Ringnalda <philringnalda@gmail.com>
Date: Wed, 25 Oct 2017 20:03:25 -0700
Subject: [PATCH 16/20] Backed out 11 changesets (bug 1411480) for libpref
 xpcshell test failures

Backed out changeset 0f266ffacf0d (bug 1411480)
Backed out changeset 75212b4a8c0a (bug 1411480)
Backed out changeset 0c807a8e8b29 (bug 1411480)
Backed out changeset 21324f73db0c (bug 1411480)
Backed out changeset f7de6fa0ef2c (bug 1411480)
Backed out changeset b7cdbe5153fa (bug 1411480)
Backed out changeset 8a66ec3e8338 (bug 1411480)
Backed out changeset 3fdf2ac7762d (bug 1411480)
Backed out changeset eaa177ef5f60 (bug 1411480)
Backed out changeset e87ba9542cf8 (bug 1411480)
Backed out changeset f1cf84a50ebc (bug 1411480)

MozReview-Commit-ID: GEVRPZp5eSH
---
 modules/libpref/Preferences.cpp | 310 ++++++++++++++++++++------------
 modules/libpref/Preferences.h   |   2 +-
 2 files changed, 193 insertions(+), 119 deletions(-)

diff --git a/modules/libpref/Preferences.cpp b/modules/libpref/Preferences.cpp
index 7275bc2d0607..c0887cb87002 100644
--- a/modules/libpref/Preferences.cpp
+++ b/modules/libpref/Preferences.cpp
@@ -137,7 +137,7 @@ using namespace mozilla;
 
 struct PrefHashEntry;
 
-typedef nsTArray<nsCString> PrefSaveData;
+typedef nsTArray<mozilla::UniqueFreePtr<char>> PrefSaveData;
 
 static PrefHashEntry*
 pref_HashTableLookup(const char* aKey);
@@ -148,7 +148,7 @@ static const uint32_t MAX_PREF_LENGTH = 1 * 1024 * 1024;
 static const uint32_t MAX_ADVISABLE_PREF_LENGTH = 4 * 1024;
 
 union PrefValue {
-  const char* mStringVal;
+  char* mStringVal;
   int32_t mIntVal;
   bool mBoolVal;
 };
@@ -190,9 +190,10 @@ public:
   bool IsTypeBool() const { return IsPrefType(PrefType::Bool); }
   bool IsPrefType(PrefType type) const { return GetPrefType() == type; }
 
-  void SetPrefType(PrefType aType)
+  PrefTypeFlags& SetPrefType(PrefType aType)
   {
     mValue = mValue - AsInt(GetPrefType()) + AsInt(aType);
+    return *this;
   }
 
   PrefType GetPrefType() const
@@ -203,35 +204,39 @@ public:
 
   bool HasDefault() const { return mValue & PREF_FLAG_HAS_DEFAULT; }
 
-  void SetHasDefault(bool aSetOrUnset)
+  PrefTypeFlags& SetHasDefault(bool aSetOrUnset)
   {
-    SetFlag(PREF_FLAG_HAS_DEFAULT, aSetOrUnset);
+    return SetFlag(PREF_FLAG_HAS_DEFAULT, aSetOrUnset);
   }
 
   bool HasStickyDefault() const { return mValue & PREF_FLAG_STICKY_DEFAULT; }
 
-  void SetHasStickyDefault(bool aSetOrUnset)
+  PrefTypeFlags& SetHasStickyDefault(bool aSetOrUnset)
   {
-    SetFlag(PREF_FLAG_STICKY_DEFAULT, aSetOrUnset);
+    return SetFlag(PREF_FLAG_STICKY_DEFAULT, aSetOrUnset);
   }
 
   bool IsLocked() const { return mValue & PREF_FLAG_LOCKED; }
 
-  void SetLocked(bool aSetOrUnset) { SetFlag(PREF_FLAG_LOCKED, aSetOrUnset); }
+  PrefTypeFlags& SetLocked(bool aSetOrUnset)
+  {
+    return SetFlag(PREF_FLAG_LOCKED, aSetOrUnset);
+  }
 
   bool HasUserValue() const { return mValue & PREF_FLAG_USERSET; }
 
-  void SetHasUserValue(bool aSetOrUnset)
+  PrefTypeFlags& SetHasUserValue(bool aSetOrUnset)
   {
-    SetFlag(PREF_FLAG_USERSET, aSetOrUnset);
+    return SetFlag(PREF_FLAG_USERSET, aSetOrUnset);
   }
 
 private:
   static uint16_t AsInt(PrefType aType) { return (uint16_t)aType; }
 
-  void SetFlag(uint16_t aFlag, bool aSetOrUnset)
+  PrefTypeFlags& SetFlag(uint16_t aFlag, bool aSetOrUnset)
   {
     mValue = aSetOrUnset ? mValue | aFlag : mValue & ~aFlag;
+    return *this;
   }
 
   // We pack both the value of type (PrefType) and flags into the same int. The
@@ -262,8 +267,12 @@ ClearPrefEntry(PLDHashTable* aTable, PLDHashEntryHdr* aEntry)
 {
   auto pref = static_cast<PrefHashEntry*>(aEntry);
   if (pref->mPrefFlags.IsTypeString()) {
-    free(const_cast<char*>(pref->mDefaultPref.mStringVal));
-    free(const_cast<char*>(pref->mUserPref.mStringVal));
+    if (pref->mDefaultPref.mStringVal) {
+      free(pref->mDefaultPref.mStringVal);
+    }
+    if (pref->mUserPref.mStringVal) {
+      free(pref->mUserPref.mStringVal);
+    }
   }
 
   // Don't need to free this because it's allocated in memory owned by
@@ -291,7 +300,7 @@ MatchPrefEntry(const PLDHashEntryHdr* aEntry, const void* aKey)
 
 struct CallbackNode
 {
-  const char* mDomain;
+  char* mDomain;
 
   // If someone attempts to remove the node from the callback list while
   // pref_DoCallback is running, |func| is set to nullptr. Such nodes will
@@ -324,6 +333,29 @@ static PLDHashTableOps pref_HashTableOps = {
   nullptr,
 };
 
+typedef void (*PrefsDirtyFunc)();
+static PrefsDirtyFunc gDirtyCallback = nullptr;
+
+static inline void
+MakeDirtyCallback()
+{
+  // Right now the callback function is always set, so we don't need
+  // to complicate the code to cover the scenario where we set the callback
+  // after we've already tried to make it dirty.  If this assert triggers
+  // we will add that code.
+  MOZ_ASSERT(gDirtyCallback);
+  if (gDirtyCallback) {
+    gDirtyCallback();
+  }
+}
+
+// Callback for whenever we change a preference.
+static void
+PREF_SetDirtyCallback(PrefsDirtyFunc aFunc)
+{
+  gDirtyCallback = aFunc;
+}
+
 //---------------------------------------------------------------------------
 
 static bool
@@ -381,7 +413,7 @@ PREF_Cleanup()
 
   while (node) {
     next_node = node->mNext;
-    free(const_cast<char*>(node->mDomain));
+    free(node->mDomain);
     free(node);
     node = next_node;
   }
@@ -390,15 +422,10 @@ PREF_Cleanup()
   PREF_CleanupPrefs();
 }
 
-// Assign to aResult a quoted, escaped copy of aOriginal.
+// Note that this appends to aResult, and does not assign!
 static void
 StrEscape(const char* aOriginal, nsCString& aResult)
 {
-  if (aOriginal == nullptr) {
-    aResult.AssignLiteral("\"\"");
-    return;
-  }
-
   // JavaScript does not allow quotes, slashes, or line terminators inside
   // strings so we must escape them. ECMAScript defines four line terminators,
   // but we're only worrying about \r and \n here.  We currently feed our pref
@@ -411,7 +438,9 @@ StrEscape(const char* aOriginal, nsCString& aResult)
   // \u2029.
   const char* p;
 
-  aResult.Assign("'");
+  if (aOriginal == nullptr) {
+    return;
+  }
 
   // Paranoid worst case all slashes will free quickly.
   for (p = aOriginal; *p; ++p) {
@@ -437,8 +466,6 @@ StrEscape(const char* aOriginal, nsCString& aResult)
         break;
     }
   }
-
-  aResult.Append('"');
 }
 
 //
@@ -465,7 +492,7 @@ PREF_SetCharPref(const char* aPrefName, const char* aValue, bool aSetDefault)
   }
 
   PrefValue pref;
-  pref.mStringVal = aValue;
+  pref.mStringVal = const_cast<char*>(aValue);
 
   return pref_HashPref(
     aPrefName, pref, PrefType::String, aSetDefault ? kPrefSetDefault : 0);
@@ -550,13 +577,17 @@ pref_SetPref(const dom::PrefSetting& aPref)
 }
 
 static PrefSaveData
-pref_savePrefs()
+pref_savePrefs(PLDHashTable* aTable)
 {
-  PrefSaveData savedPrefs(gHashTable->EntryCount());
+  PrefSaveData savedPrefs(aTable->EntryCount());
 
-  for (auto iter = gHashTable->Iter(); !iter.Done(); iter.Next()) {
+  for (auto iter = aTable->Iter(); !iter.Done(); iter.Next()) {
     auto pref = static_cast<PrefHashEntry*>(iter.Get());
 
+    nsAutoCString prefValue;
+    nsAutoCString prefPrefix;
+    prefPrefix.AssignLiteral("user_pref(\"");
+
     // where we're getting our pref from
     PrefValue* sourcePref;
 
@@ -572,22 +603,25 @@ pref_savePrefs()
       continue;
     }
 
-    nsAutoCString prefName;
-    StrEscape(pref->mKey, prefName);
-
-    nsAutoCString prefValue;
+    // strings are in quotes!
     if (pref->mPrefFlags.IsTypeString()) {
+      prefValue = '\"';
       StrEscape(sourcePref->mStringVal, prefValue);
+      prefValue += '\"';
 
     } else if (pref->mPrefFlags.IsTypeInt()) {
       prefValue.AppendInt(sourcePref->mIntVal);
 
     } else if (pref->mPrefFlags.IsTypeBool()) {
-      prefValue = sourcePref->mBoolVal ? "true" : "false";
+      prefValue = (sourcePref->mBoolVal) ? "true" : "false";
     }
 
-    nsPrintfCString str("user_pref(%s, %s);", prefName.get(), prefValue.get());
-    savedPrefs.AppendElement(str);
+    nsAutoCString prefName;
+    StrEscape(pref->mKey, prefName);
+
+    savedPrefs.AppendElement()->reset(
+      ToNewCString(prefPrefix + prefName + NS_LITERAL_CSTRING("\", ") +
+                   prefValue + NS_LITERAL_CSTRING(");")));
   }
 
   return savedPrefs;
@@ -600,7 +634,7 @@ pref_EntryHasAdvisablySizedValues(PrefHashEntry* aHashEntry)
     return true;
   }
 
-  const char* stringVal;
+  char* stringVal;
   if (aHashEntry->mPrefFlags.HasDefault()) {
     stringVal = aHashEntry->mDefaultPref.mStringVal;
     if (strlen(stringVal) > MAX_ADVISABLE_PREF_LENGTH) {
@@ -693,7 +727,7 @@ PREF_CopyCharPref(const char* aPrefName, char** aValueOut, bool aGetDefault)
   }
 
   nsresult rv = NS_ERROR_UNEXPECTED;
-  const char* stringVal;
+  char* stringVal;
   PrefHashEntry* pref = pref_HashTableLookup(aPrefName);
 
   if (pref && pref->mPrefFlags.IsTypeString()) {
@@ -818,7 +852,7 @@ PREF_DeleteBranch(const char* aBranchName)
     }
   }
 
-  Preferences::HandleDirty();
+  MakeDirtyCallback();
   return NS_OK;
 }
 
@@ -839,7 +873,7 @@ PREF_ClearUserPref(const char* aPrefName)
     }
 
     pref_DoCallback(aPrefName);
-    Preferences::HandleDirty();
+    MakeDirtyCallback();
   }
   return NS_OK;
 }
@@ -872,7 +906,7 @@ PREF_ClearAllUserPrefs()
     pref_DoCallback(prefString.c_str());
   }
 
-  Preferences::HandleDirty();
+  MakeDirtyCallback();
   return NS_OK;
 }
 
@@ -939,29 +973,52 @@ pref_ValueChanged(PrefValue aOldValue, PrefValue aNewValue, PrefType aType)
 // Overwrite the type and value of an existing preference. Caller must ensure
 // that they are not changing the type of a preference that has a default
 // value.
-static void
+static PrefTypeFlags
 pref_SetValue(PrefValue* aExistingValue,
-              PrefType aExistingType,
+              PrefTypeFlags aFlags,
               PrefValue aNewValue,
               PrefType aNewType)
 {
-  if (aExistingType == PrefType::String) {
-    free(const_cast<char*>(aExistingValue->mStringVal));
+  if (aFlags.IsTypeString() && aExistingValue->mStringVal) {
+    free(aExistingValue->mStringVal);
   }
 
-  if (aNewType == PrefType::String) {
+  aFlags.SetPrefType(aNewType);
+  if (aFlags.IsTypeString()) {
     MOZ_ASSERT(aNewValue.mStringVal);
     aExistingValue->mStringVal =
       aNewValue.mStringVal ? moz_xstrdup(aNewValue.mStringVal) : nullptr;
   } else {
     *aExistingValue = aNewValue;
   }
+
+  return aFlags;
 }
 
 #ifdef DEBUG
 
 static pref_initPhase gPhase = START;
 
+static bool gWatchingPref = false;
+
+static void
+pref_SetInitPhase(pref_initPhase aPhase)
+{
+  gPhase = aPhase;
+}
+
+static pref_initPhase
+pref_GetInitPhase()
+{
+  return gPhase;
+}
+
+static void
+pref_SetWatchingPref(bool aWatching)
+{
+  gWatchingPref = aWatching;
+}
+
 struct StringComparator
 {
   const char* mKey;
@@ -981,13 +1038,11 @@ InInitArray(const char* aKey)
   return BinarySearchIf(list, 0, prefsLen, StringComparator(aKey), &found);
 }
 
-static bool gWatchingPref = false;
-
 class WatchingPrefRAII
 {
 public:
-  WatchingPrefRAII() { gWatchingPref = true; }
-  ~WatchingPrefRAII() { gWatchingPref = false; }
+  WatchingPrefRAII() { pref_SetWatchingPref(true); }
+  ~WatchingPrefRAII() { pref_SetWatchingPref(false); }
 };
 
 #define WATCHING_PREF_RAII() WatchingPrefRAII watchingPrefRAII
@@ -1061,10 +1116,9 @@ pref_HashPref(const char* aKey,
       // ?? change of semantics?
       if (pref_ValueChanged(pref->mDefaultPref, aValue, aType) ||
           !pref->mPrefFlags.HasDefault()) {
-        pref_SetValue(
-          &pref->mDefaultPref, pref->mPrefFlags.GetPrefType(), aValue, aType);
-        pref->mPrefFlags.SetPrefType(aType);
-        pref->mPrefFlags.SetHasDefault(true);
+        pref->mPrefFlags =
+          pref_SetValue(&pref->mDefaultPref, pref->mPrefFlags, aValue, aType)
+            .SetHasDefault(true);
         if (aFlags & kPrefStickyDefault) {
           pref->mPrefFlags.SetHasStickyDefault(true);
         }
@@ -1087,19 +1141,18 @@ pref_HashPref(const char* aKey,
         // XXX should we free a user-set string value if there is one?
         pref->mPrefFlags.SetHasUserValue(false);
         if (!pref->mPrefFlags.IsLocked()) {
-          Preferences::HandleDirty();
+          MakeDirtyCallback();
           valueChanged = true;
         }
       }
     } else if (!pref->mPrefFlags.HasUserValue() ||
                !pref->mPrefFlags.IsPrefType(aType) ||
                pref_ValueChanged(pref->mUserPref, aValue, aType)) {
-      pref_SetValue(
-        &pref->mUserPref, pref->mPrefFlags.GetPrefType(), aValue, aType);
-      pref->mPrefFlags.SetPrefType(aType);
-      pref->mPrefFlags.SetHasUserValue(true);
+      pref->mPrefFlags =
+        pref_SetValue(&pref->mUserPref, pref->mPrefFlags, aValue, aType)
+          .SetHasUserValue(true);
       if (!pref->mPrefFlags.IsLocked()) {
-        Preferences::HandleDirty();
+        MakeDirtyCallback();
         valueChanged = true;
       }
     }
@@ -1123,6 +1176,18 @@ pref_SizeOfPrivateData(MallocSizeOf aMallocSizeOf)
   return n;
 }
 
+static PrefType
+PREF_GetPrefType(const char* aPrefName)
+{
+  if (gHashTable) {
+    PrefHashEntry* pref = pref_HashTableLookup(aPrefName);
+    if (pref) {
+      return pref->mPrefFlags.GetPrefType();
+    }
+  }
+  return PrefType::Invalid;
+}
+
 // Bool function that returns whether or not the preference is locked and
 // therefore cannot be changed.
 static bool
@@ -1194,7 +1259,7 @@ pref_RemoveCallbackNode(CallbackNode* aNode, CallbackNode* aPrevNode)
   if (gLastPriorityNode == aNode) {
     gLastPriorityNode = aPrevNode;
   }
-  free(const_cast<char*>(aNode->mDomain));
+  free(aNode->mDomain);
   free(aNode);
   return next_node;
 }
@@ -1434,6 +1499,49 @@ pref_ReportParseProblem(PrefParseState& aPS,
   }
 }
 
+// This function is called when a complete pref name-value pair has been
+// extracted from the input data.
+//
+// @param aPS
+//        parse state instance
+//
+// @return false to indicate a fatal error.
+static bool
+pref_DoCallback(PrefParseState* aPS)
+{
+  PrefValue value;
+
+  switch (aPS->mVtype) {
+    case PrefType::String:
+      value.mStringVal = aPS->mVb;
+      break;
+
+    case PrefType::Int:
+      if ((aPS->mVb[0] == '-' || aPS->mVb[0] == '+') && aPS->mVb[1] == '\0') {
+        pref_ReportParseProblem(*aPS, "invalid integer value", 0, true);
+        NS_WARNING("malformed integer value");
+        return false;
+      }
+      value.mIntVal = atoi(aPS->mVb);
+      break;
+
+    case PrefType::Bool:
+      value.mBoolVal = (aPS->mVb == kTrue);
+      break;
+
+    default:
+      break;
+  }
+
+  (*aPS->mReader)(aPS->mClosure,
+                  aPS->mLb,
+                  value,
+                  aPS->mVtype,
+                  aPS->mIsDefault,
+                  aPS->mIsStickyDefault);
+  return true;
+}
+
 // Initialize a PrefParseState instance.
 //
 // |aPS| is the PrefParseState instance.
@@ -1878,40 +1986,9 @@ PREF_ParseBuf(PrefParseState* aPS, const char* aBuf, int aBufLen)
       case PREF_PARSE_UNTIL_SEMICOLON:
         // tolerate only whitespace and embedded comments
         if (c == ';') {
-
-          PrefValue value;
-
-          switch (aPS->mVtype) {
-            case PrefType::String:
-              value.mStringVal = aPS->mVb;
-              break;
-
-            case PrefType::Int:
-              if ((aPS->mVb[0] == '-' || aPS->mVb[0] == '+') &&
-                  aPS->mVb[1] == '\0') {
-                pref_ReportParseProblem(*aPS, "invalid integer value", 0, true);
-                NS_WARNING("malformed integer value");
-                return false;
-              }
-              value.mIntVal = atoi(aPS->mVb);
-              break;
-
-            case PrefType::Bool:
-              value.mBoolVal = (aPS->mVb == kTrue);
-              break;
-
-            default:
-              break;
+          if (!pref_DoCallback(aPS)) {
+            return false;
           }
-
-          // We've extracted a complete name/value pair.
-          aPS->mReader(aPS->mClosure,
-                       aPS->mLb,
-                       value,
-                       aPS->mVtype,
-                       aPS->mIsDefault,
-                       aPS->mIsStickyDefault);
-
           state = PREF_PARSE_INIT;
         } else if (c == '/') {
           aPS->mNextState = state; // return here when done with comment
@@ -2104,7 +2181,7 @@ public:
 
   static void ReportToConsole(const nsAString& aMessage);
 
-private:
+protected:
   // Helper class for either returning a raw cstring or nsCString.
   typedef mozilla::Variant<const char*, const nsCString> PrefNameBase;
   class PrefName : public PrefNameBase
@@ -2173,6 +2250,7 @@ private:
 
   void FreeObserverList(void);
 
+private:
   const nsCString mPrefRoot;
   bool mIsDefault;
 
@@ -2267,17 +2345,8 @@ NS_IMETHODIMP
 nsPrefBranch::GetPrefType(const char* aPrefName, int32_t* aRetVal)
 {
   NS_ENSURE_ARG(aPrefName);
-
   const PrefName& pref = GetPrefName(aPrefName);
-  PrefType type = PrefType::Invalid;
-  if (gHashTable) {
-    PrefHashEntry* entry = pref_HashTableLookup(pref.get());
-    if (entry) {
-      type = entry->mPrefFlags.GetPrefType();
-    }
-  }
-
-  switch (type) {
+  switch (PREF_GetPrefType(pref.get())) {
     case PrefType::String:
       *aRetVal = PREF_STRING;
       break;
@@ -3197,7 +3266,7 @@ namespace mozilla {
 static NS_DEFINE_CID(kZipReaderCID, NS_ZIPREADER_CID);
 
 void
-Preferences::HandleDirty()
+Preferences::DirtyCallback()
 {
   if (!XRE_IsParentProcess()) {
     // TODO: this should really assert because you can't set prefs in a
@@ -3407,14 +3476,16 @@ public:
 
     struct CharComparator
     {
-      bool LessThan(const nsCString& aA, const nsCString& aB) const
+      bool LessThan(const mozilla::UniqueFreePtr<char>& a,
+                    const mozilla::UniqueFreePtr<char>& b) const
       {
-        return aA < aB;
+        return strcmp(a.get(), b.get()) < 0;
       }
 
-      bool Equals(const nsCString& aA, const nsCString& aB) const
+      bool Equals(const mozilla::UniqueFreePtr<char>& a,
+                  const mozilla::UniqueFreePtr<char>& b) const
       {
-        return aA == aB;
+        return strcmp(a.get(), b.get()) == 0;
       }
     };
 
@@ -3425,8 +3496,10 @@ public:
     outStream->Write(
       kPrefFileHeader, sizeof(kPrefFileHeader) - 1, &writeAmount);
 
-    for (nsCString& pref : aPrefs) {
-      outStream->Write(pref.get(), pref.Length(), &writeAmount);
+    for (auto& prefptr : aPrefs) {
+      char* pref = prefptr.get();
+      MOZ_ASSERT(pref);
+      outStream->Write(pref, strlen(pref), &writeAmount);
       outStream->Write(NS_LINEBREAK, NS_LINEBREAK_LEN, &writeAmount);
     }
 
@@ -3501,7 +3574,7 @@ public:
                                [fileCopy, rvCopy] {
                                  MOZ_RELEASE_ASSERT(NS_IsMainThread());
                                  if (NS_FAILED(rvCopy)) {
-                                   Preferences::HandleDirty();
+                                   Preferences::DirtyCallback();
                                  }
                                }));
     }
@@ -3890,6 +3963,7 @@ Preferences::SetInitPreferences(nsTArray<PrefSetting>* aPrefs)
 Result<Ok, const char*>
 Preferences::Init()
 {
+  PREF_SetDirtyCallback(&DirtyCallback);
   PREF_Init();
 
   MOZ_TRY(pref_InitInitialObjects());
@@ -4187,15 +4261,15 @@ Preferences::GetPreferences(InfallibleTArray<PrefSetting>* aPrefs)
 
 #ifdef DEBUG
 void
-Preferences::SetInitPhase(pref_initPhase aPhase)
+Preferences::SetInitPhase(pref_initPhase phase)
 {
-  gPhase = aPhase;
+  pref_SetInitPhase(phase);
 }
 
 pref_initPhase
 Preferences::InitPhase()
 {
-  return gPhase;
+  return pref_GetInitPhase();
 }
 #endif
 
@@ -4363,7 +4437,7 @@ Preferences::SavePrefFileInternal(nsIFile* aFile, SaveMethod aSaveMethod)
     }
 
     // Check for profile shutdown after mDirty because the runnables from
-    // HandleDirty() can still be pending.
+    // DirtyCallback can still be pending.
     if (mProfileShutdown) {
       NS_WARNING("Cannot save pref file after profile shutdown.");
       return NS_ERROR_ILLEGAL_DURING_SHUTDOWN;
@@ -4400,7 +4474,7 @@ Preferences::WritePrefFile(nsIFile* aFile, SaveMethod aSaveMethod)
 
     nsresult rv = NS_OK;
     mozilla::UniquePtr<PrefSaveData> prefs =
-      MakeUnique<PrefSaveData>(pref_savePrefs());
+      MakeUnique<PrefSaveData>(pref_savePrefs(gHashTable));
 
     // Put the newly constructed preference data into sPendingWriteData
     // for the next request to pick up
@@ -4435,7 +4509,7 @@ Preferences::WritePrefFile(nsIFile* aFile, SaveMethod aSaveMethod)
   // This will do a main thread write. It is safe to do it this way because
   // AllowOffMainThreadSave() returns a consistent value for the lifetime of
   // the parent process.
-  PrefSaveData prefsData = pref_savePrefs();
+  PrefSaveData prefsData = pref_savePrefs(gHashTable);
   return PreferencesWriter::Write(aFile, prefsData);
 }
 
diff --git a/modules/libpref/Preferences.h b/modules/libpref/Preferences.h
index df29c62f6001..aee201b1e0a6 100644
--- a/modules/libpref/Preferences.h
+++ b/modules/libpref/Preferences.h
@@ -334,7 +334,7 @@ public:
   static int64_t SizeOfIncludingThisAndOtherStuff(
     mozilla::MallocSizeOf aMallocSizeOf);
 
-  static void HandleDirty();
+  static void DirtyCallback();
 
   // Explicitly choosing synchronous or asynchronous (if allowed) preferences
   // file write. Only for the default file.  The guarantee for the "blocking"

From 257d9118dc34d778b3d956962e980f7133645f62 Mon Sep 17 00:00:00 2001
From: Kris Maglione <maglione.k@gmail.com>
Date: Mon, 16 Oct 2017 21:08:42 -0700
Subject: [PATCH 17/20] Bug 1409249: Require singleton constructors to return
 explicit already_AddRefed. r=froydnj

Right now, NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR expects singleton
constructors to return already-addrefed raw pointers, and while it accepts
constructors that return already_AddRefed, most existing don't do so.

Meanwhile, the convention elsewhere is that a raw pointer return value is
owned by the callee, and that the caller needs to addref it if it wants to
keep its own reference to it.

The difference in convention makes it easy to leak (I've definitely caused
more than one shutdown leak this way), so it would be better if we required
the singleton getters to return an explicit already_AddRefed, which would
behave the same for all callers.


This also cleans up several singleton constructors that left a dangling
pointer to their singletons when their initialization methods failed, when
they released their references without clearing their global raw pointers.

MozReview-Commit-ID: 9peyG4pRYcr

--HG--
extra : rebase_source : 2f5bd89c17cb554541be38444672a827c1392f3f
---
 caps/nsScriptSecurityManager.cpp              |  7 +++--
 caps/nsScriptSecurityManager.h                |  2 +-
 dom/base/DOMRequest.h                         |  8 +++---
 dom/quota/QuotaManagerService.cpp             |  9 +++----
 dom/quota/QuotaManagerService.h               |  4 +--
 extensions/cookie/nsPermissionManager.cpp     | 17 +++++-------
 extensions/cookie/nsPermissionManager.h       |  2 +-
 js/xpconnect/src/nsXPConnect.cpp              |  6 ++---
 js/xpconnect/src/xpcprivate.h                 |  5 +---
 modules/libjar/nsJARProtocolHandler.cpp       | 18 +++++--------
 modules/libjar/nsJARProtocolHandler.h         |  2 +-
 modules/libpref/Preferences.cpp               |  8 +++---
 modules/libpref/Preferences.h                 |  2 +-
 netwerk/base/nsIOService.cpp                  | 20 ++++++--------
 netwerk/base/nsIOService.h                    |  3 +--
 netwerk/dns/ChildDNSService.cpp               |  5 ++--
 netwerk/dns/ChildDNSService.h                 |  2 +-
 netwerk/dns/nsDNSService2.cpp                 | 20 +++++++-------
 netwerk/dns/nsDNSService2.h                   |  4 +--
 startupcache/StartupCache.cpp                 |  5 ++--
 startupcache/StartupCache.h                   |  2 +-
 storage/VacuumManager.cpp                     | 13 +++-------
 storage/VacuumManager.h                       |  2 +-
 storage/mozStorageService.cpp                 | 17 ++++++------
 storage/mozStorageService.h                   |  2 +-
 .../downloads/ApplicationReputation.cpp       | 16 +++---------
 .../downloads/ApplicationReputation.h         |  2 +-
 .../downloads/nsDownloadManager.cpp           | 18 ++++++-------
 .../components/downloads/nsDownloadManager.h  |  2 +-
 toolkit/components/places/History.cpp         |  5 ++--
 toolkit/components/places/History.h           |  5 ++--
 .../mozapps/extensions/AddonPathService.cpp   |  5 ++--
 toolkit/mozapps/extensions/AddonPathService.h |  2 +-
 uriloader/prefetch/nsOfflineCacheUpdate.h     |  3 +--
 .../prefetch/nsOfflineCacheUpdateService.cpp  | 19 +++++---------
 xpcom/components/ModuleUtils.h                | 26 +++++++++++++++++--
 36 files changed, 128 insertions(+), 160 deletions(-)

diff --git a/caps/nsScriptSecurityManager.cpp b/caps/nsScriptSecurityManager.cpp
index a4d3c4ce43d5..cbbcf1dfc5e2 100644
--- a/caps/nsScriptSecurityManager.cpp
+++ b/caps/nsScriptSecurityManager.cpp
@@ -1437,13 +1437,12 @@ nsScriptSecurityManager::InitStatics()
 // Currently this nsGenericFactory constructor is used only from FastLoad
 // (XPCOM object deserialization) code, when "creating" the system principal
 // singleton.
-SystemPrincipal *
+already_AddRefed<SystemPrincipal>
 nsScriptSecurityManager::SystemPrincipalSingletonConstructor()
 {
-    nsIPrincipal *sysprin = nullptr;
     if (gScriptSecMan)
-        NS_ADDREF(sysprin = gScriptSecMan->mSystemPrincipal);
-    return static_cast<SystemPrincipal*>(sysprin);
+        return do_AddRef(gScriptSecMan->mSystemPrincipal.get()).downcast<SystemPrincipal>();
+    return nullptr;
 }
 
 struct IsWhitespace {
diff --git a/caps/nsScriptSecurityManager.h b/caps/nsScriptSecurityManager.h
index a6e7e86f1378..224d69a470f0 100644
--- a/caps/nsScriptSecurityManager.h
+++ b/caps/nsScriptSecurityManager.h
@@ -53,7 +53,7 @@ public:
     // Invoked exactly once, by XPConnect.
     static void InitStatics();
 
-    static SystemPrincipal*
+    static already_AddRefed<SystemPrincipal>
     SystemPrincipalSingletonConstructor();
 
     /**
diff --git a/dom/base/DOMRequest.h b/dom/base/DOMRequest.h
index 4f3b46a97148..e690e9a8b3ee 100644
--- a/dom/base/DOMRequest.h
+++ b/dom/base/DOMRequest.h
@@ -103,12 +103,10 @@ public:
   NS_DECL_ISUPPORTS
   NS_DECL_NSIDOMREQUESTSERVICE
 
-  // Returns an owning reference! No one should call this but the factory.
-  static DOMRequestService* FactoryCreate()
+  // No one should call this but the factory.
+  static already_AddRefed<DOMRequestService> FactoryCreate()
   {
-    DOMRequestService* res = new DOMRequestService;
-    NS_ADDREF(res);
-    return res;
+    return MakeAndAddRef<DOMRequestService>();
   }
 };
 
diff --git a/dom/quota/QuotaManagerService.cpp b/dom/quota/QuotaManagerService.cpp
index b0545c7182fc..899ef061f753 100644
--- a/dom/quota/QuotaManagerService.cpp
+++ b/dom/quota/QuotaManagerService.cpp
@@ -225,14 +225,11 @@ QuotaManagerService::Get()
 }
 
 // static
-QuotaManagerService*
+already_AddRefed<QuotaManagerService>
 QuotaManagerService::FactoryCreate()
 {
-  // Returns a raw pointer that carries an owning reference! Lame, but the
-  // singleton factory macros force this.
-  QuotaManagerService* quotaManagerService = GetOrCreate();
-  NS_IF_ADDREF(quotaManagerService);
-  return quotaManagerService;
+  RefPtr<QuotaManagerService> quotaManagerService = GetOrCreate();
+  return quotaManagerService.forget();
 }
 
 void
diff --git a/dom/quota/QuotaManagerService.h b/dom/quota/QuotaManagerService.h
index 91e9ec379d89..c0f65023816c 100644
--- a/dom/quota/QuotaManagerService.h
+++ b/dom/quota/QuotaManagerService.h
@@ -63,8 +63,8 @@ public:
   static QuotaManagerService*
   Get();
 
-  // Returns an owning reference! No one should call this but the factory.
-  static QuotaManagerService*
+  // No one should call this but the factory.
+  static already_AddRefed<QuotaManagerService>
   FactoryCreate();
 
   void
diff --git a/extensions/cookie/nsPermissionManager.cpp b/extensions/cookie/nsPermissionManager.cpp
index ee91c2304cd5..81af32dd8c80 100644
--- a/extensions/cookie/nsPermissionManager.cpp
+++ b/extensions/cookie/nsPermissionManager.cpp
@@ -926,12 +926,11 @@ nsPermissionManager::~nsPermissionManager()
 }
 
 // static
-nsIPermissionManager*
+already_AddRefed<nsIPermissionManager>
 nsPermissionManager::GetXPCOMSingleton()
 {
   if (gPermissionManager) {
-    NS_ADDREF(gPermissionManager);
-    return gPermissionManager;
+    return do_AddRef(gPermissionManager);
   }
 
   // Create a new singleton nsPermissionManager.
@@ -940,15 +939,13 @@ nsPermissionManager::GetXPCOMSingleton()
   // Release our members, since GC cycles have already been completed and
   // would result in serious leaks.
   // See bug 209571.
-  gPermissionManager = new nsPermissionManager();
-  if (gPermissionManager) {
-    NS_ADDREF(gPermissionManager);
-    if (NS_FAILED(gPermissionManager->Init())) {
-      NS_RELEASE(gPermissionManager);
-    }
+  auto permManager = MakeRefPtr<nsPermissionManager>();
+  if (NS_SUCCEEDED(permManager->Init())) {
+    gPermissionManager = permManager.get();
+    return permManager.forget();
   }
 
-  return gPermissionManager;
+  return nullptr;;
 }
 
 nsresult
diff --git a/extensions/cookie/nsPermissionManager.h b/extensions/cookie/nsPermissionManager.h
index 2839d724508d..03f4d6179b14 100644
--- a/extensions/cookie/nsPermissionManager.h
+++ b/extensions/cookie/nsPermissionManager.h
@@ -167,7 +167,7 @@ public:
   NS_DECL_NSIOBSERVER
 
   nsPermissionManager();
-  static nsIPermissionManager* GetXPCOMSingleton();
+  static already_AddRefed<nsIPermissionManager> GetXPCOMSingleton();
   nsresult Init();
 
   // enums for AddInternal()
diff --git a/js/xpconnect/src/nsXPConnect.cpp b/js/xpconnect/src/nsXPConnect.cpp
index fcac8350ee04..6f19e85ce975 100644
--- a/js/xpconnect/src/nsXPConnect.cpp
+++ b/js/xpconnect/src/nsXPConnect.cpp
@@ -141,12 +141,10 @@ nsXPConnect::InitStatics()
     gSelf->mRuntime->InitSingletonScopes();
 }
 
-nsXPConnect*
+already_AddRefed<nsXPConnect>
 nsXPConnect::GetSingleton()
 {
-    nsXPConnect* xpc = nsXPConnect::XPConnect();
-    NS_IF_ADDREF(xpc);
-    return xpc;
+    return do_AddRef(nsXPConnect::XPConnect());
 }
 
 // static
diff --git a/js/xpconnect/src/xpcprivate.h b/js/xpconnect/src/xpcprivate.h
index 2b58a1394f11..76a300cdbc9d 100644
--- a/js/xpconnect/src/xpcprivate.h
+++ b/js/xpconnect/src/xpcprivate.h
@@ -257,10 +257,7 @@ public:
         return gSystemPrincipal;
     }
 
-    // This returns an AddRef'd pointer. It does not do this with an 'out' param
-    // only because this form is required by the generic module macro:
-    // NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR
-    static nsXPConnect* GetSingleton();
+    static already_AddRefed<nsXPConnect> GetSingleton();
 
     // Called by module code in dll startup
     static void InitStatics();
diff --git a/modules/libjar/nsJARProtocolHandler.cpp b/modules/libjar/nsJARProtocolHandler.cpp
index 1e0dfb9bfdea..bda4312e1cc4 100644
--- a/modules/libjar/nsJARProtocolHandler.cpp
+++ b/modules/libjar/nsJARProtocolHandler.cpp
@@ -63,23 +63,19 @@ NS_IMPL_ISUPPORTS(nsJARProtocolHandler,
                   nsIProtocolHandler,
                   nsISupportsWeakReference)
 
-nsJARProtocolHandler*
+already_AddRefed<nsJARProtocolHandler>
 nsJARProtocolHandler::GetSingleton()
 {
     if (!gJarHandler) {
-        gJarHandler = new nsJARProtocolHandler();
-        if (!gJarHandler)
-            return nullptr;
-
-        NS_ADDREF(gJarHandler);
-        nsresult rv = gJarHandler->Init();
-        if (NS_FAILED(rv)) {
-            NS_RELEASE(gJarHandler);
+        auto jar = MakeRefPtr<nsJARProtocolHandler>();
+        gJarHandler = jar.get();
+        if (NS_FAILED(jar->Init())) {
+            gJarHandler = nullptr;
             return nullptr;
         }
+        return jar.forget();
     }
-    NS_ADDREF(gJarHandler);
-    return gJarHandler;
+    return do_AddRef(gJarHandler);
 }
 
 NS_IMETHODIMP
diff --git a/modules/libjar/nsJARProtocolHandler.h b/modules/libjar/nsJARProtocolHandler.h
index a3d6ba479a86..0a528f6d27f3 100644
--- a/modules/libjar/nsJARProtocolHandler.h
+++ b/modules/libjar/nsJARProtocolHandler.h
@@ -25,7 +25,7 @@ public:
     // nsJARProtocolHandler methods:
     nsJARProtocolHandler();
 
-    static nsJARProtocolHandler *GetSingleton();
+    static already_AddRefed<nsJARProtocolHandler> GetSingleton();
 
     nsresult Init();
 
diff --git a/modules/libpref/Preferences.cpp b/modules/libpref/Preferences.cpp
index c0887cb87002..0bd5b82d43fe 100644
--- a/modules/libpref/Preferences.cpp
+++ b/modules/libpref/Preferences.cpp
@@ -3826,12 +3826,11 @@ public:
 
 } // namespace
 
-/* static */ Preferences*
+/* static */ already_AddRefed<Preferences>
 Preferences::GetInstanceForService()
 {
   if (sPreferences) {
-    NS_ADDREF(sPreferences);
-    return sPreferences;
+    return do_AddRef(sPreferences);
   }
 
   if (sShutdown) {
@@ -3868,8 +3867,7 @@ Preferences::GetInstanceForService()
     new AddPreferencesMemoryReporterRunnable();
   NS_DispatchToMainThread(runnable);
 
-  NS_ADDREF(sPreferences);
-  return sPreferences;
+  return do_AddRef(sPreferences);
 }
 
 /* static */ bool
diff --git a/modules/libpref/Preferences.h b/modules/libpref/Preferences.h
index aee201b1e0a6..1c0da935eef8 100644
--- a/modules/libpref/Preferences.h
+++ b/modules/libpref/Preferences.h
@@ -78,7 +78,7 @@ public:
   static void InitializeUserPrefs();
 
   // Returns the singleton instance which is addreffed.
-  static Preferences* GetInstanceForService();
+  static already_AddRefed<Preferences> GetInstanceForService();
 
   // Finallizes global members.
   static void Shutdown();
diff --git a/netwerk/base/nsIOService.cpp b/netwerk/base/nsIOService.cpp
index 62889893e251..ef80467c444e 100644
--- a/netwerk/base/nsIOService.cpp
+++ b/netwerk/base/nsIOService.cpp
@@ -351,23 +351,19 @@ nsIOService::InitializeProtocolProxyService()
     return rv;
 }
 
-nsIOService*
+already_AddRefed<nsIOService>
 nsIOService::GetInstance() {
     if (!gIOService) {
-        gIOService = new nsIOService();
-        if (!gIOService)
-            return nullptr;
-        NS_ADDREF(gIOService);
-
-        nsresult rv = gIOService->Init();
-        if (NS_FAILED(rv)) {
-            NS_RELEASE(gIOService);
+        RefPtr<nsIOService> ios = new nsIOService();
+        gIOService = ios.get();
+        if (NS_FAILED(ios->Init())) {
+            gIOService = nullptr;
             return nullptr;
         }
-        return gIOService;
+
+        return ios.forget();
     }
-    NS_ADDREF(gIOService);
-    return gIOService;
+    return do_AddRef(gIOService);
 }
 
 NS_IMPL_ISUPPORTS(nsIOService,
diff --git a/netwerk/base/nsIOService.h b/netwerk/base/nsIOService.h
index a933695fa2e2..3447e9626250 100644
--- a/netwerk/base/nsIOService.h
+++ b/netwerk/base/nsIOService.h
@@ -64,8 +64,7 @@ public:
 
     // Gets the singleton instance of the IO Service, creating it as needed
     // Returns nullptr on out of memory or failure to initialize.
-    // Returns an addrefed pointer.
-    static nsIOService* GetInstance();
+    static already_AddRefed<nsIOService> GetInstance();
 
     nsresult Init();
     nsresult NewURI(const char* aSpec, nsIURI* aBaseURI,
diff --git a/netwerk/dns/ChildDNSService.cpp b/netwerk/dns/ChildDNSService.cpp
index 1cf9872db722..6c533d0d93d6 100644
--- a/netwerk/dns/ChildDNSService.cpp
+++ b/netwerk/dns/ChildDNSService.cpp
@@ -26,7 +26,7 @@ namespace net {
 static ChildDNSService *gChildDNSService;
 static const char kPrefNameDisablePrefetch[] = "network.dns.disablePrefetch";
 
-ChildDNSService* ChildDNSService::GetSingleton()
+already_AddRefed<ChildDNSService> ChildDNSService::GetSingleton()
 {
   MOZ_ASSERT(IsNeckoChild());
 
@@ -34,8 +34,7 @@ ChildDNSService* ChildDNSService::GetSingleton()
     gChildDNSService = new ChildDNSService();
   }
 
-  NS_ADDREF(gChildDNSService);
-  return gChildDNSService;
+  return do_AddRef(gChildDNSService);
 }
 
 NS_IMPL_ISUPPORTS(ChildDNSService,
diff --git a/netwerk/dns/ChildDNSService.h b/netwerk/dns/ChildDNSService.h
index e028d0c19cbc..39d4b4383d19 100644
--- a/netwerk/dns/ChildDNSService.h
+++ b/netwerk/dns/ChildDNSService.h
@@ -32,7 +32,7 @@ public:
 
   ChildDNSService();
 
-  static ChildDNSService* GetSingleton();
+  static already_AddRefed<ChildDNSService> GetSingleton();
 
   void NotifyRequestDone(DNSRequestChild *aDnsRequest);
 
diff --git a/netwerk/dns/nsDNSService2.cpp b/netwerk/dns/nsDNSService2.cpp
index 4ec481eba0cd..aee2be077843 100644
--- a/netwerk/dns/nsDNSService2.cpp
+++ b/netwerk/dns/nsDNSService2.cpp
@@ -499,7 +499,7 @@ NS_IMPL_ISUPPORTS(nsDNSService, nsIDNSService, nsPIDNSService, nsIObserver,
  ******************************************************************************/
 static nsDNSService *gDNSService;
 
-nsIDNSService*
+already_AddRefed<nsIDNSService>
 nsDNSService::GetXPCOMSingleton()
 {
     if (IsNeckoChild()) {
@@ -509,25 +509,23 @@ nsDNSService::GetXPCOMSingleton()
     return GetSingleton();
 }
 
-nsDNSService*
+already_AddRefed<nsDNSService>
 nsDNSService::GetSingleton()
 {
     NS_ASSERTION(!IsNeckoChild(), "not a parent process");
 
     if (gDNSService) {
-        NS_ADDREF(gDNSService);
-        return gDNSService;
+        return do_AddRef(gDNSService);
     }
 
-    gDNSService = new nsDNSService();
-    if (gDNSService) {
-        NS_ADDREF(gDNSService);
-        if (NS_FAILED(gDNSService->Init())) {
-              NS_RELEASE(gDNSService);
-        }
+    auto dns = MakeRefPtr<nsDNSService>();
+    gDNSService = dns.get();
+    if (NS_FAILED(dns->Init())) {
+        gDNSService = nullptr;
+        return nullptr;
     }
 
-    return gDNSService;
+    return dns.forget();
 }
 
 NS_IMETHODIMP
diff --git a/netwerk/dns/nsDNSService2.h b/netwerk/dns/nsDNSService2.h
index 53ec1955359d..9970ca2233b8 100644
--- a/netwerk/dns/nsDNSService2.h
+++ b/netwerk/dns/nsDNSService2.h
@@ -34,7 +34,7 @@ public:
 
     nsDNSService();
 
-    static nsIDNSService* GetXPCOMSingleton();
+    static already_AddRefed<nsIDNSService> GetXPCOMSingleton();
 
     size_t SizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf) const;
 
@@ -51,7 +51,7 @@ protected:
 private:
     ~nsDNSService();
 
-    static nsDNSService* GetSingleton();
+    static already_AddRefed<nsDNSService> GetSingleton();
 
     uint16_t GetAFForLookup(const nsACString &host, uint32_t flags);
 
diff --git a/startupcache/StartupCache.cpp b/startupcache/StartupCache.cpp
index 6650161ccb65..895696d854eb 100644
--- a/startupcache/StartupCache.cpp
+++ b/startupcache/StartupCache.cpp
@@ -700,13 +700,12 @@ StartupCacheWrapper* StartupCacheWrapper::gStartupCacheWrapper = nullptr;
 
 NS_IMPL_ISUPPORTS(StartupCacheWrapper, nsIStartupCache)
 
-StartupCacheWrapper* StartupCacheWrapper::GetSingleton()
+already_AddRefed<StartupCacheWrapper> StartupCacheWrapper::GetSingleton()
 {
   if (!gStartupCacheWrapper)
     gStartupCacheWrapper = new StartupCacheWrapper();
 
-  NS_ADDREF(gStartupCacheWrapper);
-  return gStartupCacheWrapper;
+  return do_AddRef(gStartupCacheWrapper);
 }
 
 nsresult
diff --git a/startupcache/StartupCache.h b/startupcache/StartupCache.h
index 28355957ef9b..a7618415dd1f 100644
--- a/startupcache/StartupCache.h
+++ b/startupcache/StartupCache.h
@@ -214,7 +214,7 @@ class StartupCacheWrapper final
   NS_DECL_THREADSAFE_ISUPPORTS
   NS_DECL_NSISTARTUPCACHE
 
-  static StartupCacheWrapper* GetSingleton();
+  static already_AddRefed<StartupCacheWrapper> GetSingleton();
   static StartupCacheWrapper *gStartupCacheWrapper;
 };
 
diff --git a/storage/VacuumManager.cpp b/storage/VacuumManager.cpp
index f4a2bbb0549c..39b903f831af 100644
--- a/storage/VacuumManager.cpp
+++ b/storage/VacuumManager.cpp
@@ -311,7 +311,7 @@ NS_IMPL_ISUPPORTS(
 VacuumManager *
 VacuumManager::gVacuumManager = nullptr;
 
-VacuumManager *
+already_AddRefed<VacuumManager>
 VacuumManager::getSingleton()
 {
   //Don't allocate it in the child Process.
@@ -319,15 +319,10 @@ VacuumManager::getSingleton()
     return nullptr;
   }
 
-  if (gVacuumManager) {
-    NS_ADDREF(gVacuumManager);
-    return gVacuumManager;
+  if (!gVacuumManager) {
+    gVacuumManager = new VacuumManager();
   }
-  gVacuumManager = new VacuumManager();
-  if (gVacuumManager) {
-    NS_ADDREF(gVacuumManager);
-  }
-  return gVacuumManager;
+  return do_AddRef(gVacuumManager);
 }
 
 VacuumManager::VacuumManager()
diff --git a/storage/VacuumManager.h b/storage/VacuumManager.h
index 12603deb6b83..4d4c60c15ea5 100644
--- a/storage/VacuumManager.h
+++ b/storage/VacuumManager.h
@@ -28,7 +28,7 @@ public:
   /**
    * Obtains the VacuumManager object.
    */
-  static VacuumManager * getSingleton();
+  static already_AddRefed<VacuumManager> getSingleton();
 
 private:
   ~VacuumManager();
diff --git a/storage/mozStorageService.cpp b/storage/mozStorageService.cpp
index ad2c1ebfd7b7..1e25133828a3 100644
--- a/storage/mozStorageService.cpp
+++ b/storage/mozStorageService.cpp
@@ -193,12 +193,11 @@ NS_IMPL_ISUPPORTS(
 
 Service *Service::gService = nullptr;
 
-Service *
+already_AddRefed<Service>
 Service::getSingleton()
 {
   if (gService) {
-    NS_ADDREF(gService);
-    return gService;
+    return do_AddRef(gService);
   }
 
   // Ensure that we are using the same version of SQLite that we compiled with
@@ -222,14 +221,14 @@ Service::getSingleton()
   // The first reference to the storage service must be obtained on the
   // main thread.
   NS_ENSURE_TRUE(NS_IsMainThread(), nullptr);
-  gService = new Service();
-  if (gService) {
-    NS_ADDREF(gService);
-    if (NS_FAILED(gService->initialize()))
-      NS_RELEASE(gService);
+  RefPtr<Service> service = new Service();
+  gService = service.get();
+  if (NS_FAILED(service->initialize())) {
+    gService = nullptr;
+    return nullptr;
   }
 
-  return gService;
+  return service.forget();
 }
 
 nsIXPConnect *Service::sXPConnect = nullptr;
diff --git a/storage/mozStorageService.h b/storage/mozStorageService.h
index 8c00eee034a0..9a134c8ffbf4 100644
--- a/storage/mozStorageService.h
+++ b/storage/mozStorageService.h
@@ -52,7 +52,7 @@ public:
                            const nsAString &aStr2,
                            int32_t aComparisonStrength);
 
-  static Service *getSingleton();
+  static already_AddRefed<Service> getSingleton();
 
   NS_DECL_THREADSAFE_ISUPPORTS
   NS_DECL_MOZISTORAGESERVICE
diff --git a/toolkit/components/downloads/ApplicationReputation.cpp b/toolkit/components/downloads/ApplicationReputation.cpp
index 70a4a1c5d359..74161592bb56 100644
--- a/toolkit/components/downloads/ApplicationReputation.cpp
+++ b/toolkit/components/downloads/ApplicationReputation.cpp
@@ -1552,21 +1552,13 @@ NS_IMPL_ISUPPORTS(ApplicationReputationService,
 ApplicationReputationService*
   ApplicationReputationService::gApplicationReputationService = nullptr;
 
-ApplicationReputationService*
+already_AddRefed<ApplicationReputationService>
 ApplicationReputationService::GetSingleton()
 {
-  if (gApplicationReputationService) {
-    NS_ADDREF(gApplicationReputationService);
-    return gApplicationReputationService;
+  if (!gApplicationReputationService) {
+    gApplicationReputationService = new ApplicationReputationService();
   }
-
-  // We're not initialized yet.
-  gApplicationReputationService = new ApplicationReputationService();
-  if (gApplicationReputationService) {
-    NS_ADDREF(gApplicationReputationService);
-  }
-
-  return gApplicationReputationService;
+  return do_AddRef(gApplicationReputationService);
 }
 
 ApplicationReputationService::ApplicationReputationService()
diff --git a/toolkit/components/downloads/ApplicationReputation.h b/toolkit/components/downloads/ApplicationReputation.h
index 0ed68d6163eb..d8a31b0f4198 100644
--- a/toolkit/components/downloads/ApplicationReputation.h
+++ b/toolkit/components/downloads/ApplicationReputation.h
@@ -27,7 +27,7 @@ public:
   NS_DECL_NSIAPPLICATIONREPUTATIONSERVICE
 
 public:
-  static ApplicationReputationService* GetSingleton();
+  static already_AddRefed<ApplicationReputationService> GetSingleton();
 
 private:
   friend class PendingLookup;
diff --git a/toolkit/components/downloads/nsDownloadManager.cpp b/toolkit/components/downloads/nsDownloadManager.cpp
index fb57e3c71000..e711ac7e2ebb 100644
--- a/toolkit/components/downloads/nsDownloadManager.cpp
+++ b/toolkit/components/downloads/nsDownloadManager.cpp
@@ -25,22 +25,20 @@ NS_IMPL_ISUPPORTS(
 
 nsDownloadManager *nsDownloadManager::gDownloadManagerService = nullptr;
 
-nsDownloadManager *
+already_AddRefed<nsDownloadManager>
 nsDownloadManager::GetSingleton()
 {
   if (gDownloadManagerService) {
-    NS_ADDREF(gDownloadManagerService);
-    return gDownloadManagerService;
+    return do_AddRef(gDownloadManagerService);
   }
 
-  gDownloadManagerService = new nsDownloadManager();
-  if (gDownloadManagerService) {
-    NS_ADDREF(gDownloadManagerService);
-    if (NS_FAILED(gDownloadManagerService->Init()))
-      NS_RELEASE(gDownloadManagerService);
+  auto serv = MakeRefPtr<nsDownloadManager>();
+  gDownloadManagerService = serv.get();
+  if (NS_FAILED(serv->Init())) {
+    gDownloadManagerService = nullptr;
+    return nullptr;
   }
-
-  return gDownloadManagerService;
+  return serv.forget();
 }
 
 nsDownloadManager::~nsDownloadManager()
diff --git a/toolkit/components/downloads/nsDownloadManager.h b/toolkit/components/downloads/nsDownloadManager.h
index f03df33d9484..260d4a4f66f3 100644
--- a/toolkit/components/downloads/nsDownloadManager.h
+++ b/toolkit/components/downloads/nsDownloadManager.h
@@ -21,7 +21,7 @@ public:
 
   nsresult Init();
 
-  static nsDownloadManager *GetSingleton();
+  static already_AddRefed<nsDownloadManager> GetSingleton();
 
   nsDownloadManager()
   {
diff --git a/toolkit/components/places/History.cpp b/toolkit/components/places/History.cpp
index 1e441e772269..554c99e9779c 100644
--- a/toolkit/components/places/History.cpp
+++ b/toolkit/components/places/History.cpp
@@ -2421,7 +2421,7 @@ History::GetService()
 }
 
 /* static */
-History*
+already_AddRefed<History>
 History::GetSingleton()
 {
   if (!gService) {
@@ -2430,8 +2430,7 @@ History::GetSingleton()
     gService->InitMemoryReporter();
   }
 
-  NS_ADDREF(gService);
-  return gService;
+  return do_AddRef(gService);
 }
 
 mozIStorageConnection*
diff --git a/toolkit/components/places/History.h b/toolkit/components/places/History.h
index 75438d559890..8bdde6038d74 100644
--- a/toolkit/components/places/History.h
+++ b/toolkit/components/places/History.h
@@ -107,10 +107,9 @@ public:
   static History* GetService();
 
   /**
-   * Obtains a pointer that has had AddRef called on it.  Used by the service
-   * manager only.
+   * Used by the service manager only.
    */
-  static History* GetSingleton();
+  static already_AddRefed<History> GetSingleton();
 
   template<int N>
   already_AddRefed<mozIStorageStatement>
diff --git a/toolkit/mozapps/extensions/AddonPathService.cpp b/toolkit/mozapps/extensions/AddonPathService.cpp
index 86608dca47bd..2006e8a0b62c 100644
--- a/toolkit/mozapps/extensions/AddonPathService.cpp
+++ b/toolkit/mozapps/extensions/AddonPathService.cpp
@@ -57,14 +57,13 @@ NS_IMPL_ISUPPORTS(AddonPathService, amIAddonPathService)
 
 AddonPathService *AddonPathService::sInstance;
 
-/* static */ AddonPathService*
+/* static */ already_AddRefed<AddonPathService>
 AddonPathService::GetInstance()
 {
   if (!sInstance) {
     sInstance = new AddonPathService();
   }
-  NS_ADDREF(sInstance);
-  return sInstance;
+  return do_AddRef(sInstance);
 }
 
 static JSAddonId*
diff --git a/toolkit/mozapps/extensions/AddonPathService.h b/toolkit/mozapps/extensions/AddonPathService.h
index f739b018f3c6..bbde891f05ce 100644
--- a/toolkit/mozapps/extensions/AddonPathService.h
+++ b/toolkit/mozapps/extensions/AddonPathService.h
@@ -23,7 +23,7 @@ class AddonPathService final : public amIAddonPathService
 public:
   AddonPathService();
 
-  static AddonPathService* GetInstance();
+  static already_AddRefed<AddonPathService> GetInstance();
 
   JSAddonId* Find(const nsAString& path);
   static JSAddonId* FindAddonId(const nsAString& path);
diff --git a/uriloader/prefetch/nsOfflineCacheUpdate.h b/uriloader/prefetch/nsOfflineCacheUpdate.h
index dbd6a1ab77ba..06cf843e2ab1 100644
--- a/uriloader/prefetch/nsOfflineCacheUpdate.h
+++ b/uriloader/prefetch/nsOfflineCacheUpdate.h
@@ -356,8 +356,7 @@ public:
      */
     static nsOfflineCacheUpdateService *EnsureService();
 
-    /** Addrefs and returns the singleton nsOfflineCacheUpdateService. */
-    static nsOfflineCacheUpdateService *GetInstance();
+    static already_AddRefed<nsOfflineCacheUpdateService> GetInstance();
 
     static nsresult OfflineAppPinnedForURI(nsIURI *aDocumentURI,
                                            nsIPrefBranch *aPrefBranch,
diff --git a/uriloader/prefetch/nsOfflineCacheUpdateService.cpp b/uriloader/prefetch/nsOfflineCacheUpdateService.cpp
index d70c36390064..31ce98226e0b 100644
--- a/uriloader/prefetch/nsOfflineCacheUpdateService.cpp
+++ b/uriloader/prefetch/nsOfflineCacheUpdateService.cpp
@@ -292,25 +292,20 @@ nsOfflineCacheUpdateService::Init()
 }
 
 /* static */
-nsOfflineCacheUpdateService *
+already_AddRefed<nsOfflineCacheUpdateService>
 nsOfflineCacheUpdateService::GetInstance()
 {
     if (!gOfflineCacheUpdateService) {
-        gOfflineCacheUpdateService = new nsOfflineCacheUpdateService();
-        if (!gOfflineCacheUpdateService)
-            return nullptr;
-        NS_ADDREF(gOfflineCacheUpdateService);
-        nsresult rv = gOfflineCacheUpdateService->Init();
-        if (NS_FAILED(rv)) {
-            NS_RELEASE(gOfflineCacheUpdateService);
+        auto serv = MakeRefPtr<nsOfflineCacheUpdateService>();
+        gOfflineCacheUpdateService = serv.get();
+        if (NS_FAILED(serv->Init())) {
+            gOfflineCacheUpdateService = nullptr;
             return nullptr;
         }
-        return gOfflineCacheUpdateService;
+        return serv.forget();
     }
 
-    NS_ADDREF(gOfflineCacheUpdateService);
-
-    return gOfflineCacheUpdateService;
+    return do_AddRef(gOfflineCacheUpdateService);
 }
 
 /* static */
diff --git a/xpcom/components/ModuleUtils.h b/xpcom/components/ModuleUtils.h
index f4c9ecd420a1..03663bd9078e 100644
--- a/xpcom/components/ModuleUtils.h
+++ b/xpcom/components/ModuleUtils.h
@@ -49,8 +49,25 @@ _InstanceClass##Constructor(nsISupports *aOuter, REFNSIID aIID,               \
   return rv;                                                                  \
 }
 
+namespace mozilla {
+namespace detail {
+
+template<typename T>
+struct RemoveAlreadyAddRefed
+{
+  using Type = T;
+};
+
+template<typename T>
+struct RemoveAlreadyAddRefed<already_AddRefed<T>>
+{
+  using Type = T;
+};
+
+} // namespace detail
+} // namespace mozilla
+
 // 'Constructor' that uses an existing getter function that gets a singleton.
-// NOTE: assumes that getter does an AddRef - so additional AddRef is not done.
 #define NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR(_InstanceClass, _GetterProc) \
 static nsresult                                                               \
 _InstanceClass##Constructor(nsISupports *aOuter, REFNSIID aIID,               \
@@ -63,7 +80,12 @@ _InstanceClass##Constructor(nsISupports *aOuter, REFNSIID aIID,               \
     return NS_ERROR_NO_AGGREGATION;                                           \
   }                                                                           \
                                                                               \
-  inst = already_AddRefed<_InstanceClass>(_GetterProc());                     \
+  using T = mozilla::detail::RemoveAlreadyAddRefed<decltype(_GetterProc())>::Type; \
+  static_assert(mozilla::IsSame<already_AddRefed<T>, decltype(_GetterProc())>::value, \
+                "Singleton constructor must return already_AddRefed");        \
+  static_assert(mozilla::IsBaseOf<_InstanceClass, T>::value,                  \
+                "Singleton constructor must return correct already_AddRefed");\
+  inst = _GetterProc();                                                       \
   if (nullptr == inst) {                                                      \
     return NS_ERROR_OUT_OF_MEMORY;                                            \
   }                                                                           \

From dee87d13ab1ccfdf7617544e9e9e90fd5cee3864 Mon Sep 17 00:00:00 2001
From: Kris Maglione <maglione.k@gmail.com>
Date: Wed, 25 Oct 2017 20:56:41 -0700
Subject: [PATCH 18/20] Bug 1409249: Follow-up: Fix Android build bustage.
 r=bustage

MozReview-Commit-ID: 1I3z8Oqz9Qm
---
 mobile/android/components/build/nsAndroidHistory.cpp | 5 ++---
 mobile/android/components/build/nsAndroidHistory.h   | 2 +-
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/mobile/android/components/build/nsAndroidHistory.cpp b/mobile/android/components/build/nsAndroidHistory.cpp
index 2b115005589b..0081f8ea3058 100644
--- a/mobile/android/components/build/nsAndroidHistory.cpp
+++ b/mobile/android/components/build/nsAndroidHistory.cpp
@@ -30,7 +30,7 @@ NS_IMPL_ISUPPORTS(nsAndroidHistory, IHistory, nsIRunnable, nsITimerCallback, nsI
 nsAndroidHistory* nsAndroidHistory::sHistory = nullptr;
 
 /*static*/
-nsAndroidHistory*
+already_AddRefed<nsAndroidHistory>
 nsAndroidHistory::GetSingleton()
 {
   if (!sHistory) {
@@ -38,8 +38,7 @@ nsAndroidHistory::GetSingleton()
     NS_ENSURE_TRUE(sHistory, nullptr);
   }
 
-  NS_ADDREF(sHistory);
-  return sHistory;
+  return do_AddRef(sHistory);
 }
 
 nsAndroidHistory::nsAndroidHistory()
diff --git a/mobile/android/components/build/nsAndroidHistory.h b/mobile/android/components/build/nsAndroidHistory.h
index 5ddc838b28fc..37417c21f420 100644
--- a/mobile/android/components/build/nsAndroidHistory.h
+++ b/mobile/android/components/build/nsAndroidHistory.h
@@ -40,7 +40,7 @@ public:
    * Obtains a pointer that has had AddRef called on it.  Used by the service
    * manager only.
    */
-  static nsAndroidHistory* GetSingleton();
+  static already_AddRefed<nsAndroidHistory> GetSingleton();
 
   nsAndroidHistory();
 

From b3157c0147593e63994b709c0ed16c2cedbfad48 Mon Sep 17 00:00:00 2001
From: Edgar Chen <echen@mozilla.com>
Date: Fri, 13 Oct 2017 11:54:46 +0800
Subject: [PATCH 19/20] Bug 1410790 - Add more assertion in
 CustomElementData::SetCustomElementDefinition and GetCustomElementDefinition;
 r=smaug

This is a follow-up patch for bug 1392970. Since we only set CustomElementDefinition on a custom element
which is "custom", we could add more assertion to ensure that.

MozReview-Commit-ID: 2sLP53bAYVV

--HG--
extra : rebase_source : 523761aa7312ddfaaf91f79e39c44ddce5cf9335
---
 dom/base/CustomElementRegistry.cpp | 42 ++++++++++++++++++++++++++++++
 dom/base/CustomElementRegistry.h   | 23 ++++++----------
 dom/base/FragmentOrElement.cpp     | 18 ++-----------
 3 files changed, 52 insertions(+), 31 deletions(-)

diff --git a/dom/base/CustomElementRegistry.cpp b/dom/base/CustomElementRegistry.cpp
index 1545eccf40c1..ddac1589cd41 100644
--- a/dom/base/CustomElementRegistry.cpp
+++ b/dom/base/CustomElementRegistry.cpp
@@ -138,6 +138,48 @@ CustomElementData::CustomElementData(nsAtom* aType, State aState)
 {
 }
 
+void
+CustomElementData::SetCustomElementDefinition(CustomElementDefinition* aDefinition)
+{
+  MOZ_ASSERT(mState == State::eCustom);
+  MOZ_ASSERT(!mCustomElementDefinition);
+  MOZ_ASSERT(aDefinition->mType == mType);
+
+  mCustomElementDefinition = aDefinition;
+}
+
+CustomElementDefinition*
+CustomElementData::GetCustomElementDefinition()
+{
+  MOZ_ASSERT(mCustomElementDefinition ? mState == State::eCustom
+                                      : mState != State::eCustom);
+
+  return mCustomElementDefinition;
+}
+
+void
+CustomElementData::Traverse(nsCycleCollectionTraversalCallback& aCb) const
+{
+  for (uint32_t i = 0; i < mReactionQueue.Length(); i++) {
+    if (mReactionQueue[i]) {
+      mReactionQueue[i]->Traverse(aCb);
+    }
+  }
+
+  if (mCustomElementDefinition) {
+    NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(aCb, "mCustomElementDefinition");
+    aCb.NoteNativeChild(mCustomElementDefinition,
+      NS_CYCLE_COLLECTION_PARTICIPANT(CustomElementDefinition));
+  }
+}
+
+void
+CustomElementData::Unlink()
+{
+  mReactionQueue.Clear();
+  mCustomElementDefinition = nullptr;
+}
+
 //-----------------------------------------------------
 // CustomElementRegistry
 
diff --git a/dom/base/CustomElementRegistry.h b/dom/base/CustomElementRegistry.h
index bb1fc1babae4..014d1f76efaf 100644
--- a/dom/base/CustomElementRegistry.h
+++ b/dom/base/CustomElementRegistry.h
@@ -132,24 +132,16 @@ struct CustomElementData
   // e.g., create an element, insert a node.
   AutoTArray<UniquePtr<CustomElementReaction>, 3> mReactionQueue;
 
-  RefPtr<CustomElementDefinition> mCustomElementDefinition;
+  void SetCustomElementDefinition(CustomElementDefinition* aDefinition);
+  CustomElementDefinition* GetCustomElementDefinition();
 
-  void
-  SetCustomElementDefinition(CustomElementDefinition* aDefinition)
-  {
-    MOZ_ASSERT(!mCustomElementDefinition);
-
-    mCustomElementDefinition = aDefinition;
-  }
-
-  CustomElementDefinition*
-  GetCustomElementDefinition()
-  {
-    return mCustomElementDefinition;
-  }
+  void Traverse(nsCycleCollectionTraversalCallback& aCb) const;
+  void Unlink();
 
 private:
   virtual ~CustomElementData() {}
+
+  RefPtr<CustomElementDefinition> mCustomElementDefinition;
 };
 
 #define ALEADY_CONSTRUCTED_MARKER nullptr
@@ -169,7 +161,8 @@ struct CustomElementDefinition
                           mozilla::dom::LifecycleCallbacks* aCallbacks,
                           uint32_t aDocOrder);
 
-  // The type (name) for this custom element.
+  // The type (name) for this custom element, for <button is="x-foo"> or <x-foo>
+  // this would be x-foo.
   RefPtr<nsAtom> mType;
 
   // The localname to (e.g. <button is=type> -- this would be button).
diff --git a/dom/base/FragmentOrElement.cpp b/dom/base/FragmentOrElement.cpp
index 2020ed160e29..14b29f5b6517 100644
--- a/dom/base/FragmentOrElement.cpp
+++ b/dom/base/FragmentOrElement.cpp
@@ -710,19 +710,7 @@ FragmentOrElement::nsDOMSlots::Traverse(nsCycleCollectionTraversalCallback &cb)
   cb.NoteXPCOMChild(mExtendedSlots->mXBLInsertionParent.get());
 
   if (mExtendedSlots->mCustomElementData) {
-    for (uint32_t i = 0;
-         i < mExtendedSlots->mCustomElementData->mReactionQueue.Length(); i++) {
-      if (mExtendedSlots->mCustomElementData->mReactionQueue[i]) {
-        mExtendedSlots->mCustomElementData->mReactionQueue[i]->Traverse(cb);
-      }
-    }
-
-    if (mExtendedSlots->mCustomElementData->mCustomElementDefinition) {
-      NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb,
-        "mExtendedSlots->mCustomElementData->mCustomElementDefinition");
-      cb.NoteNativeChild(mExtendedSlots->mCustomElementData->mCustomElementDefinition,
-        NS_CYCLE_COLLECTION_PARTICIPANT(CustomElementDefinition));
-    }
+    mExtendedSlots->mCustomElementData->Traverse(cb);
   }
 
   NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "mExtendedSlots->mFrameLoaderOrOpener");
@@ -753,9 +741,7 @@ FragmentOrElement::nsDOMSlots::Unlink()
   MOZ_ASSERT(!(mExtendedSlots->mXBLBinding));
   mExtendedSlots->mXBLInsertionParent = nullptr;
   if (mExtendedSlots->mCustomElementData) {
-    if (mExtendedSlots->mCustomElementData->mCustomElementDefinition) {
-      mExtendedSlots->mCustomElementData->mCustomElementDefinition = nullptr;
-    }
+    mExtendedSlots->mCustomElementData->Unlink();
     mExtendedSlots->mCustomElementData = nullptr;
   }
   nsCOMPtr<nsIFrameLoader> frameLoader =

From 0015c73286444bc290aa50cd6946dd5ec03aff98 Mon Sep 17 00:00:00 2001
From: Boris Zbarsky <bzbarsky@mit.edu>
Date: Thu, 26 Oct 2017 00:35:22 -0400
Subject: [PATCH 20/20] Bug 1411138.  Be consistent (and follow the spec) in
 terms of how we determine the element that propagates styles to the viewport.
  r=dholbert

MozReview-Commit-ID: 8tH3nCDQJID
---
 layout/base/crashtests/1411138.html           | 13 ++++
 layout/base/crashtests/crashtests.list        |  1 +
 layout/base/nsPresContext.cpp                 | 13 ++--
 testing/web-platform/meta/MANIFEST.json       | 67 +++++++++++++++++--
 .../visufx/overflow-propagation-001a.html     |  9 +++
 .../visufx/overflow-propagation-001b.html     | 14 ++++
 .../visufx/overflow-propagation-001c.html     | 16 +++++
 .../support/overflow-propagation-001-ref.html |  7 ++
 8 files changed, 130 insertions(+), 10 deletions(-)
 create mode 100644 layout/base/crashtests/1411138.html
 create mode 100644 testing/web-platform/tests/css/CSS2/visufx/overflow-propagation-001a.html
 create mode 100644 testing/web-platform/tests/css/CSS2/visufx/overflow-propagation-001b.html
 create mode 100644 testing/web-platform/tests/css/CSS2/visufx/overflow-propagation-001c.html
 create mode 100644 testing/web-platform/tests/css/CSS2/visufx/support/overflow-propagation-001-ref.html

diff --git a/layout/base/crashtests/1411138.html b/layout/base/crashtests/1411138.html
new file mode 100644
index 000000000000..028e8c9222c2
--- /dev/null
+++ b/layout/base/crashtests/1411138.html
@@ -0,0 +1,13 @@
+<!-- Needs to be in quirks mode -->
+<html>
+  <head>
+    <script>
+      try { o1 = document.createElement('frameset') } catch(e) { }
+      try { o2 = document.createElement('body') } catch(e) { }
+      try { o2.style.overflow = 'auto'; } catch (e) {}
+      try { document.documentElement.appendChild(o1) } catch(e) { }
+      try { document.documentElement.appendChild(o2) } catch(e) { }
+      try { o2.scrollLeft } catch(e) { }
+    </script>
+  </head>
+</html>
diff --git a/layout/base/crashtests/crashtests.list b/layout/base/crashtests/crashtests.list
index 8e90267ff857..9cda08791b7b 100644
--- a/layout/base/crashtests/crashtests.list
+++ b/layout/base/crashtests/crashtests.list
@@ -513,3 +513,4 @@ load 1404789-2.html
 load 1406562.html
 load 1409088.html
 load 1409147.html
+load 1411138.html
diff --git a/layout/base/nsPresContext.cpp b/layout/base/nsPresContext.cpp
index a884ea570adf..8276ee5464f7 100644
--- a/layout/base/nsPresContext.cpp
+++ b/layout/base/nsPresContext.cpp
@@ -25,6 +25,7 @@
 #include "nsIContent.h"
 #include "nsIFrame.h"
 #include "nsIDocument.h"
+#include "nsIDocumentInlines.h"
 #include "nsIPrintSettings.h"
 #include "nsLanguageAtomService.h"
 #include "mozilla/LookAndFeel.h"
@@ -56,6 +57,7 @@
 #include "mozilla/MemoryReporting.h"
 #include "mozilla/dom/Element.h"
 #include "nsIMessageManager.h"
+#include "mozilla/dom/HTMLBodyElement.h"
 #include "mozilla/dom/MediaQueryList.h"
 #include "nsSMILAnimationController.h"
 #include "mozilla/css/ImageLoader.h"
@@ -1516,14 +1518,15 @@ GetPropagatedScrollbarStylesForViewport(nsPresContext* aPresContext,
     return nullptr;
   }
 
-  Element* bodyElement = htmlDoc->GetBody();
-
-  if (!bodyElement ||
-      !bodyElement->NodeInfo()->Equals(nsGkAtoms::body)) {
-    // The body is not a <body> tag, it's a <frameset>.
+  Element* bodyElement = htmlDoc->GetBodyElement();
+  if (!bodyElement) {
+    // No body, nothing to do here.
     return nullptr;
   }
 
+  MOZ_ASSERT(bodyElement->IsHTMLElement(nsGkAtoms::body),
+             "GetBodyElement returned something bogus");
+
   RefPtr<nsStyleContext> bodyStyle =
     styleSet->ResolveStyleFor(bodyElement, rootStyle,
                               LazyComputeBehavior::Allow);
diff --git a/testing/web-platform/meta/MANIFEST.json b/testing/web-platform/meta/MANIFEST.json
index eb0d445eaa59..79696b2060bd 100644
--- a/testing/web-platform/meta/MANIFEST.json
+++ b/testing/web-platform/meta/MANIFEST.json
@@ -95709,6 +95709,42 @@
      {}
     ]
    ],
+   "css/CSS2/visufx/overflow-propagation-001a.html": [
+    [
+     "/css/CSS2/visufx/overflow-propagation-001a.html",
+     [
+      [
+       "/css/CSS2/visufx/support/overflow-propagation-001-ref.html",
+       "=="
+      ]
+     ],
+     {}
+    ]
+   ],
+   "css/CSS2/visufx/overflow-propagation-001b.html": [
+    [
+     "/css/CSS2/visufx/overflow-propagation-001b.html",
+     [
+      [
+       "/css/CSS2/visufx/support/overflow-propagation-001-ref.html",
+       "=="
+      ]
+     ],
+     {}
+    ]
+   ],
+   "css/CSS2/visufx/overflow-propagation-001c.html": [
+    [
+     "/css/CSS2/visufx/overflow-propagation-001c.html",
+     [
+      [
+       "/css/CSS2/visufx/support/overflow-propagation-001-ref.html",
+       "=="
+      ]
+     ],
+     {}
+    ]
+   ],
    "css/CSS2/visufx/visibility-005.xht": [
     [
      "/css/CSS2/visufx/visibility-005.xht",
@@ -224201,6 +224237,11 @@
      {}
     ]
    ],
+   "css/CSS2/visufx/support/overflow-propagation-001-ref.html": [
+    [
+     {}
+    ]
+   ],
    "css/CSS2/visuren/anonymous-boxes-001a-ref.xht": [
     [
      {}
@@ -391648,7 +391689,7 @@
    "support"
   ],
   "XMLHttpRequest/resources/auth2/corsenabled.py": [
-   "a70576e6ae82a030c6776923082e5aa50fad0078",
+   "4136b00b543096216f9f1ad1314c7062dda04179",
    "support"
   ],
   "XMLHttpRequest/resources/auth3/auth.py": [
@@ -391668,11 +391709,11 @@
    "support"
   ],
   "XMLHttpRequest/resources/auth7/corsenabled.py": [
-   "3f8fe382e3aa4ffe0e7c1ecb4571a55866bba27f",
+   "cbc1e4a39cc2e999ef7bd4053a600e8b0d515bb5",
    "support"
   ],
   "XMLHttpRequest/resources/auth8/corsenabled-no-authorize.py": [
-   "c3add811ecf33bf3452fe471d27756dc152db81f",
+   "523fbc134ecfee5b13e4ef93508712847fc4e396",
    "support"
   ],
   "XMLHttpRequest/resources/auth9/auth.py": [
@@ -469815,6 +469856,18 @@
    "d71b8aa84a63f7579a5f33b05960571e24417110",
    "reftest"
   ],
+  "css/CSS2/visufx/overflow-propagation-001a.html": [
+   "3aaba555d04ae9a0f6ee48aa7ac8fc4600554645",
+   "reftest"
+  ],
+  "css/CSS2/visufx/overflow-propagation-001b.html": [
+   "c582c21c5fb8079047d6a2350351fede33c0f6db",
+   "reftest"
+  ],
+  "css/CSS2/visufx/overflow-propagation-001c.html": [
+   "57062d2e0eb9077f965183aaed2a4c18675585a7",
+   "reftest"
+  ],
   "css/CSS2/visufx/shape-spaces-001.xht": [
    "2449b8843064386c9854436bbc59eccb695b9578",
    "visual"
@@ -469823,6 +469876,10 @@
    "e4843d42a26189132e1bdd53e8618521330baeca",
    "support"
   ],
+  "css/CSS2/visufx/support/overflow-propagation-001-ref.html": [
+   "b5df0a9c23e9556ade5179681452a40b913d392d",
+   "support"
+  ],
   "css/CSS2/visufx/visibility-001.xht": [
    "f75f3db564058f469756ec1398078fc76473d632",
    "visual"
@@ -585960,7 +586017,7 @@
    "wdspec"
   ],
   "webdriver/tests/element_retrieval/get_active_element.py": [
-   "9080770b60a988cb37e30700efb118d392a896c7",
+   "41dab8ecf11556f7b1490d515557de659813881e",
    "wdspec"
   ],
   "webdriver/tests/fullscreen_window.py": [
@@ -586088,7 +586145,7 @@
    "support"
   ],
   "webdriver/tests/support/asserts.py": [
-   "b02f45e99cdee49f12608e60333e566c8c0e04d0",
+   "4e4f29b9d305383e5bcfa01fa1ba789d0c12ef93",
    "support"
   ],
   "webdriver/tests/support/fixtures.py": [
diff --git a/testing/web-platform/tests/css/CSS2/visufx/overflow-propagation-001a.html b/testing/web-platform/tests/css/CSS2/visufx/overflow-propagation-001a.html
new file mode 100644
index 000000000000..77107d8faf50
--- /dev/null
+++ b/testing/web-platform/tests/css/CSS2/visufx/overflow-propagation-001a.html
@@ -0,0 +1,9 @@
+<!doctype html>
+<meta charset=utf-8>
+<title>Ensure that body propagates its scrollbars</title>
+<link rel=match href="support/overflow-propagation-001-ref.html">
+<link rel=help href="https://www.w3.org/TR/CSS21/visufx.html#overflow-clipping">
+<body style="overflow: hidden; margin: 100px; width: 100px; height: 100px; border: 1px solid green; position: absolute; top: 0; left: 0">
+  The body should have visible overflow of the text that totally doesn't fit
+  in the little box.
+</body>
diff --git a/testing/web-platform/tests/css/CSS2/visufx/overflow-propagation-001b.html b/testing/web-platform/tests/css/CSS2/visufx/overflow-propagation-001b.html
new file mode 100644
index 000000000000..c72994b8c44b
--- /dev/null
+++ b/testing/web-platform/tests/css/CSS2/visufx/overflow-propagation-001b.html
@@ -0,0 +1,14 @@
+<!doctype html>
+<meta charset=utf-8>
+<title>Ensure that body propagates its scrollbars</title>
+<link rel=match href="support/overflow-propagation-001-ref.html">
+<link rel=help href="https://www.w3.org/TR/CSS21/visufx.html#overflow-clipping">
+<script>
+  onload = function() {
+    document.body.remove();
+    var b = document.createElement("body");
+    b.style = "overflow: hidden; margin: 100px; width: 100px; height: 100px; border: 1px solid green; position: absolute; top: 0; left: 0";
+    b.textContent = "The body should have visible overflow of the text that totally doesn't fit in the little box.";
+    document.documentElement.appendChild(b);
+  }
+</script>
diff --git a/testing/web-platform/tests/css/CSS2/visufx/overflow-propagation-001c.html b/testing/web-platform/tests/css/CSS2/visufx/overflow-propagation-001c.html
new file mode 100644
index 000000000000..6956ff4143e3
--- /dev/null
+++ b/testing/web-platform/tests/css/CSS2/visufx/overflow-propagation-001c.html
@@ -0,0 +1,16 @@
+<!doctype html>
+<meta charset=utf-8>
+<title>Ensure that body propagates its scrollbars even if there is a preceding frameset</title>
+<link rel=match href="support/overflow-propagation-001-ref.html">
+<link rel=help href="https://www.w3.org/TR/CSS21/visufx.html#overflow-clipping">
+<script>
+  onload = function() {
+    document.body.remove();
+    var f = document.createElement("frameset");
+    document.documentElement.appendChild(f);
+    var b = document.createElement("body");
+    b.style = "overflow: hidden; margin: 100px; width: 100px; height: 100px; border: 1px solid green; position: absolute; top: 0; left: 0";
+    b.textContent = "The body should have visible overflow of the text that totally doesn't fit in the little box.";
+    document.documentElement.appendChild(b);
+  }
+</script>
diff --git a/testing/web-platform/tests/css/CSS2/visufx/support/overflow-propagation-001-ref.html b/testing/web-platform/tests/css/CSS2/visufx/support/overflow-propagation-001-ref.html
new file mode 100644
index 000000000000..f0693fbea29d
--- /dev/null
+++ b/testing/web-platform/tests/css/CSS2/visufx/support/overflow-propagation-001-ref.html
@@ -0,0 +1,7 @@
+<!doctype html>
+<html style="overflow: hidden">
+<meta charset=utf-8>
+<body style="margin: 100px; width: 100px; height: 100px; border: 1px solid green; position: absolute; top: 0; left: 0">
+    The body should have visible overflow of the text that totally doesn't fit
+    in the little box.
+</body>