Bug 1868456: Improve ManagedPostRefreshObserver invariants. r=emilio

Differential Revision: https://phabricator.services.mozilla.com/D195607

layout/base/nsRefreshObservers.cpp

@@ -6,6 +6,7 @@
 
 #include "nsRefreshObservers.h"
 #include "nsPresContext.h"
+#include "mozilla/Likely.h"
 
 namespace mozilla {
 
@@ -14,30 +15,40 @@ ManagedPostRefreshObserver::ManagedPostRefreshObserver(nsPresContext* aPc,
     : mPresContext(aPc), mAction(std::move(aAction)) {}
 
 ManagedPostRefreshObserver::ManagedPostRefreshObserver(nsPresContext* aPc)
-    : mPresContext(aPc) {}
+    : mPresContext(aPc) {
+  MOZ_ASSERT(mPresContext, "Can't observe without a nsPresContext");
+}
 
 ManagedPostRefreshObserver::~ManagedPostRefreshObserver() = default;
 
 void ManagedPostRefreshObserver::Cancel() {
   // Caller holds a strong reference, so no need to reference stuff from here.
-  mAction(true);
+  if (mAction) {
+    mAction(true);
+  }
   mAction = nullptr;
   mPresContext = nullptr;
 }
 
 void ManagedPostRefreshObserver::DidRefresh() {
-  RefPtr<ManagedPostRefreshObserver> thisObject = this;
+  if (MOZ_UNLIKELY(!mAction)) {
+    return;
+  }
+
+  RefPtr<ManagedPostRefreshObserver> thisObject = this;
+  auto action = std::move(mAction);
+  Unregister unregister = action(false);
 
-  Unregister unregister = mAction(false);
   if (unregister == Unregister::Yes) {
     if (RefPtr<nsPresContext> pc = std::move(mPresContext)) {
-      // In theory mAction could've ended up in `Cancel` being called. In which
-      // case we're already unregistered so no need to do anything.
-      mAction = nullptr;
+      // We have to null-check mPresContext here because in theory, mAction
+      // could've ended up in `Cancel` being called, which would clear
+      // mPresContext. In that case, we're already unregistered so no need to
+      // do anything.
       pc->UnregisterManagedPostRefreshObserver(this);
-    } else {
-      MOZ_DIAGNOSTIC_ASSERT(!mAction);
     }
+  } else {
+    mAction = std::move(action);
   }
 }