From 96f8fc64b9780bf684705c955f0ea5d1ac1415b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Bargull?= Date: Thu, 12 Oct 2017 09:20:07 -0700 Subject: [PATCH 01/22] 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()) - nobj = &obj->as(); RootedShape shape(cx); Rooted 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()) { + HandleNativeObject nobj = obj.as(); 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 Date: Wed, 25 Oct 2017 09:54:13 -0700 Subject: [PATCH 02/22] 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(NSS_CMSContentInfo_GetContent(cinfo)); From 529db19e62d8042af6552cf0d91129aa28509b44 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Wed, 25 Oct 2017 10:22:38 +1100 Subject: [PATCH 03/22] 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 Date: Wed, 25 Oct 2017 10:22:38 +1100 Subject: [PATCH 04/22] 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 Date: Wed, 25 Oct 2017 10:22:38 +1100 Subject: [PATCH 05/22] 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* 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 Date: Wed, 25 Oct 2017 10:22:38 +1100 Subject: [PATCH 06/22] 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 Date: Wed, 25 Oct 2017 10:22:38 +1100 Subject: [PATCH 07/22] 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* aPrefs) Result 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 Date: Wed, 25 Oct 2017 10:22:39 +1100 Subject: [PATCH 08/22] 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 Date: Wed, 25 Oct 2017 10:22:39 +1100 Subject: [PATCH 09/22] 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(iter.Get()); nsAutoCString prefValue; @@ -4411,7 +4411,7 @@ Preferences::WritePrefFile(nsIFile* aFile, SaveMethod aSaveMethod) nsresult rv = NS_OK; mozilla::UniquePtr prefs = - MakeUnique(pref_savePrefs(gHashTable)); + MakeUnique(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 Date: Wed, 25 Oct 2017 10:22:39 +1100 Subject: [PATCH 10/22] 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 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 Date: Wed, 25 Oct 2017 10:22:39 +1100 Subject: [PATCH 11/22] Bug 1411480 - Change PrefSaveData's element type. r=glandium. Because nsCString is nicer than UniqueFreePtr. 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> PrefSaveData; +typedef nsTArray 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(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& a, - const mozilla::UniqueFreePtr& 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& a, - const mozilla::UniqueFreePtr& 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 Date: Thu, 26 Oct 2017 12:29:34 +1100 Subject: [PATCH 12/22] 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(aEntry); if (pref->mPrefFlags.IsTypeString()) { - if (pref->mDefaultPref.mStringVal) { - free(pref->mDefaultPref.mStringVal); - } - if (pref->mUserPref.mStringVal) { - free(pref->mUserPref.mStringVal); - } + free(const_cast(pref->mDefaultPref.mStringVal)); + free(const_cast(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(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(aExistingValue->mStringVal)); } if (aNewType == PrefType::String) { From 14790e76d7aeb44d5e568b8e27d56db0b6a6d712 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Thu, 26 Oct 2017 12:29:41 +1100 Subject: [PATCH 13/22] 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(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(aNode->mDomain)); free(aNode); return next_node; } From 140ebef4af351be418ab5a2a41ba4b2f69945691 Mon Sep 17 00:00:00 2001 From: Ryan VanderMeulen Date: Wed, 25 Oct 2017 22:10:11 -0400 Subject: [PATCH 14/22] 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 releaseObservedTypes; - /* Whether the atoms table will be swept. */ - ActiveThreadOrGCTaskData shouldSweepAtomsZone_; - /* Singly linked list of zones to be swept in the background. */ ActiveThreadOrGCTaskData 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 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 Date: Wed, 25 Oct 2017 19:22:36 -0700 Subject: [PATCH 15/22] 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(NSS_CMSContentInfo_GetContent(cinfo)); From 535f8401d0e927a5d08936ee485cdb683fb410b2 Mon Sep 17 00:00:00 2001 From: Phil Ringnalda Date: Wed, 25 Oct 2017 20:03:25 -0700 Subject: [PATCH 16/22] 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 PrefSaveData; +typedef nsTArray> 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(aEntry); if (pref->mPrefFlags.IsTypeString()) { - free(const_cast(pref->mDefaultPref.mStringVal)); - free(const_cast(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(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(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(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(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(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 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& a, + const mozilla::UniqueFreePtr& 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& a, + const mozilla::UniqueFreePtr& 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* aPrefs) Result Preferences::Init() { + PREF_SetDirtyCallback(&DirtyCallback); PREF_Init(); MOZ_TRY(pref_InitInitialObjects()); @@ -4187,15 +4261,15 @@ Preferences::GetPreferences(InfallibleTArray* 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 prefs = - MakeUnique(pref_savePrefs()); + MakeUnique(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 Date: Mon, 16 Oct 2017 21:08:42 -0700 Subject: [PATCH 17/22] 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 nsScriptSecurityManager::SystemPrincipalSingletonConstructor() { - nsIPrincipal *sysprin = nullptr; if (gScriptSecMan) - NS_ADDREF(sysprin = gScriptSecMan->mSystemPrincipal); - return static_cast(sysprin); + return do_AddRef(gScriptSecMan->mSystemPrincipal.get()).downcast(); + 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 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 FactoryCreate() { - DOMRequestService* res = new DOMRequestService; - NS_ADDREF(res); - return res; + return MakeAndAddRef(); } }; 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::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 = 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 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 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(); + 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 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::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 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::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(); + 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 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::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 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::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 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 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::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 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 nsDNSService::GetXPCOMSingleton() { if (IsNeckoChild()) { @@ -509,25 +509,23 @@ nsDNSService::GetXPCOMSingleton() return GetSingleton(); } -nsDNSService* +already_AddRefed 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(); + 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 GetXPCOMSingleton(); size_t SizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf) const; @@ -51,7 +51,7 @@ protected: private: ~nsDNSService(); - static nsDNSService* GetSingleton(); + static already_AddRefed 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::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 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::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 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::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 = 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 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::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 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::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(); + 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 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::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 GetSingleton(); template already_AddRefed 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::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 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 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::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(); + 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 +struct RemoveAlreadyAddRefed +{ + using Type = T; +}; + +template +struct RemoveAlreadyAddRefed> +{ + 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::Type; \ + static_assert(mozilla::IsSame, 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 Date: Wed, 25 Oct 2017 20:56:41 -0700 Subject: [PATCH 18/22] 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::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 GetSingleton(); nsAndroidHistory(); From b3157c0147593e63994b709c0ed16c2cedbfad48 Mon Sep 17 00:00:00 2001 From: Edgar Chen Date: Fri, 13 Oct 2017 11:54:46 +0800 Subject: [PATCH 19/22] 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, 3> mReactionQueue; - RefPtr 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 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