Bug 1699298 - Ensure MatchPattern::Init and AtomSet::Get throw NS_ERROR_ILLEGAL_DURING_SHUTDOWN if called late on shutdown. draft
authorLuca Greco <lgreco@mozilla.com>
Mon, 19 Apr 2021 18:56:24 +0200
changeset 3667645 f492991efa15e0363cc909e5a90450be508b9d14
parent 3666865 8e850fd29a957f505e0355c1326279e06e9040bb
child 3667646 b1f5fabb4b1da773f1236b9a4671e13dfafe61d7
push id683130
push userluca.greco@alcacoop.it
push dateMon, 19 Apr 2021 16:57:35 +0000
treeherdertry@b1f5fabb4b1d [default view] [failures only]
bugs1699298
milestone89.0a1
Bug 1699298 - Ensure MatchPattern::Init and AtomSet::Get throw NS_ERROR_ILLEGAL_DURING_SHUTDOWN if called late on shutdown. 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).
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,29 @@ 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>();
+  // Fail earlier if we are late in the shutdown when we get to this call,
+  // the following AtomSet::Get would fail anyway.
+  if (PastShutdownPhase(ShutdownPhase::XPCOMShutdownFinal)) {
+    aRv.Throw(NS_ERROR_ILLEGAL_DURING_SHUTDOWN);
+    return;
+  }
+
+  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 +283,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 +480,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 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;
+    }
+
     if (MOZ_UNLIKELY(!sMatcher)) {
       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));
     }