Bug 1699298 - Ensure MatchPattern::Init throws and AtomSet::Get returns NS_ERROR_ILLEGAL_DURING_SHUTDOWN if called late on shutdown. r=mccr8
authorLuca Greco <lgreco@mozilla.com>
Tue, 20 Apr 2021 19:43:33 +0000
changeset 576840 8a0a264d20c54414e4afd1ade2da39caf0b00829
parent 576839 a81431c752a6958cfb81a267b90fcffc31ec6671
child 576841 267e8925453b55cb1976318a44ae2ec2b80035f9
push id141570
push userluca.greco@alcacoop.it
push dateTue, 20 Apr 2021 19:45:57 +0000
treeherderautoland@8a0a264d20c5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmccr8
bugs1699298, 1620227, 1663315
milestone90.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1699298 - Ensure MatchPattern::Init throws and AtomSet::Get returns NS_ERROR_ILLEGAL_DURING_SHUTDOWN if called late on shutdown. r=mccr8 By looking a bit more into the pernosco session attached to this bug, I did notice that MatchPattern::Init is being called late in the xpcom shutdown (seems to be triggered by some pending Promise jobs related to loading the builtin search extension, which in that run got scheduled during the late shutdown). Under these conditions, AtomSet::Get does allocate again the static RefPtr (because the previous one was already destroyed as part of the xpcom shutdown), but it gets destroyed by ClearOnShutdown before the RefPtr gets to the caller: - https://searchfox.org/mozilla-central/rev/a8b75e4ba3f8ddf0e76b42681d0a7b7e78e67730/xpcom/base/ClearOnShutdown.cpp#19-24 This patch does: - check explicitly in MatchPattern::Init if we are get past the XPCOMShutdownFinal phase, and throws an explicit NS_ERROR_ILLEGAL_DURING_SHUTDOWN error if that is the case - change the signature for AtomSet::Get static method, to have an explicit non discardable nsresult as its result value, to track down other similar AtomSet::Get calls that may have to take this scenario into account (and to make it a bit more clear from the method signature that it can actually fail and the returned nsresult should be checked before actually using the refptr). As an additional side note: - I've looked into existing crash reports with this signature but I didn't find any (but I won't exclude that I may have not looked enough) - I've tried to reproduce conditions similar to the ones that I observed in the pernosco session (by cheating a bit and trying to force similar conditions with some temporary changes applied locally) but I wasn't able to trigger it in the exact same way Nevertheless by looking into some past bugzilla issues it looks that - it did already happen in the past that we would be still loading search extensions late in shutdown, (e.g. Bug 1620227) and so it doesn't seem totally surprising that there are other ways that we may be still in the progress. - we did fix some other crashes due to some C++ code being triggered by the js runtime late in shutdown (e.g. Bug 1663315 is one that I did notice in the middle of the bugs I looked), and so it doesn't seem that unexpected that we may still have a call to MatchPattern::Init that late in the shutdown. And so this seems to be a legit scenario to account for, even if I wasn't able to trigger it exactly like Grizzly triggered it during a fuzzy test run. Differential Revision: https://phabricator.services.mozilla.com/D112638
toolkit/components/extensions/MatchPattern.cpp
toolkit/components/extensions/MatchPattern.h
--- a/toolkit/components/extensions/MatchPattern.cpp
+++ b/toolkit/components/extensions/MatchPattern.cpp
@@ -245,17 +245,22 @@ already_AddRefed<MatchPattern> MatchPatt
     return nullptr;
   }
   return pattern.forget();
 }
 
 void MatchPattern::Init(JSContext* aCx, const nsAString& aPattern,
                         bool aIgnorePath, bool aRestrictSchemes,
                         ErrorResult& aRv) {
-  RefPtr<AtomSet> permittedSchemes = AtomSet::Get<PERMITTED_SCHEMES>();
+  RefPtr<AtomSet> permittedSchemes;
+  nsresult rv = AtomSet::Get<PERMITTED_SCHEMES>(permittedSchemes);
+  if (NS_FAILED(rv)) {
+    aRv.Throw(rv);
+    return;
+  }
 
   mPattern = aPattern;
 
   if (aPattern.EqualsLiteral("<all_urls>")) {
     mSchemes = permittedSchemes;
     mMatchSubdomain = true;
     return;
   }
@@ -271,21 +276,30 @@ void MatchPattern::Init(JSContext* aCx, 
   if (index <= 0) {
     aRv.Throw(NS_ERROR_INVALID_ARG);
     return;
   }
 
   RefPtr<nsAtom> scheme = NS_AtomizeMainThread(StringHead(aPattern, index));
   bool requireHostLocatorScheme = true;
   if (scheme == nsGkAtoms::_asterisk) {
-    mSchemes = AtomSet::Get<WILDCARD_SCHEMES>();
+    rv = AtomSet::Get<WILDCARD_SCHEMES>(mSchemes);
+    if (NS_FAILED(rv)) {
+      aRv.Throw(rv);
+      return;
+    }
   } else if (!aRestrictSchemes || permittedSchemes->Contains(scheme) ||
              scheme == nsGkAtoms::moz_extension) {
+    RefPtr<AtomSet> hostLocatorSchemes;
+    rv = AtomSet::Get<HOST_LOCATOR_SCHEMES>(hostLocatorSchemes);
+    if (NS_FAILED(rv)) {
+      aRv.Throw(rv);
+      return;
+    }
     mSchemes = new AtomSet({scheme});
-    RefPtr<AtomSet> hostLocatorSchemes = AtomSet::Get<HOST_LOCATOR_SCHEMES>();
     requireHostLocatorScheme = hostLocatorSchemes->Contains(scheme);
   } else {
     aRv.Throw(NS_ERROR_INVALID_ARG);
     return;
   }
 
   /***************************************************************************
    * Host
@@ -459,17 +473,22 @@ bool MatchPattern::Overlaps(const MatchP
 
 JSObject* MatchPattern::WrapObject(JSContext* aCx,
                                    JS::HandleObject aGivenProto) {
   return MatchPattern_Binding::Wrap(aCx, this, aGivenProto);
 }
 
 /* static */
 bool MatchPattern::MatchesAllURLs(const URLInfo& aURL) {
-  RefPtr<AtomSet> permittedSchemes = AtomSet::Get<PERMITTED_SCHEMES>();
+  RefPtr<AtomSet> permittedSchemes;
+  nsresult rv = AtomSet::Get<PERMITTED_SCHEMES>(permittedSchemes);
+  if (NS_FAILED(rv)) {
+    NS_WARNING("Failed to retrireve PERMITTED_SCHEMES AtomSet");
+    return false;
+  }
   return permittedSchemes->Contains(aURL.Scheme());
 }
 
 NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(MatchPattern, mPath, mParent)
 
 NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(MatchPattern)
   NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
   NS_INTERFACE_MAP_ENTRY(nsISupports)
--- a/toolkit/components/extensions/MatchPattern.h
+++ b/toolkit/components/extensions/MatchPattern.h
@@ -73,25 +73,37 @@ class AtomSet final : public RefCounted<
   void Remove(const nsAString& aElem) {
     RefPtr<nsAtom> atom = NS_AtomizeMainThread(aElem);
     return Remove(atom);
   }
 
   // Returns a cached, statically-allocated matcher for the given set of
   // literal strings.
   template <const char** schemes>
-  static already_AddRefed<AtomSet> Get() {
+  [[nodiscard]] static nsresult Get(RefPtr<AtomSet>& aMatcherOut) {
     static RefPtr<AtomSet> sMatcher;
 
     if (MOZ_UNLIKELY(!sMatcher)) {
+      // If this static method is called late during the shutdown,
+      // ClearOnShutdown would be destroying the instance before the
+      // RefPtr gets to the caller, let's make sure that this method
+      // signature does make it more clear by returning an explicit
+      // not discardable nsresult.
+      if (PastShutdownPhase(ShutdownPhase::XPCOMShutdownFinal)) {
+        aMatcherOut = nullptr;
+        return NS_ERROR_ILLEGAL_DURING_SHUTDOWN;
+      }
+
       sMatcher = new AtomSet(schemes);
       ClearOnShutdown(&sMatcher);
     }
 
-    return do_AddRef(sMatcher);
+    MOZ_ASSERT(sMatcher);
+    aMatcherOut = do_AddRef(sMatcher);
+    return NS_OK;
   }
 
   void Get(nsTArray<nsString>& aResult) const {
     aResult.SetCapacity(mElems.Length());
 
     for (const auto& atom : mElems) {
       aResult.AppendElement(nsDependentAtomString(atom));
     }