Bug 1867358 part 4: Add some release asserts (and upgrade an existing assert), to validate that NotificationController isn't still registered as a refresh observer when it's destroyed. r=eeejay

If these assertions somehow fail, then we are probably doomed to crash anyway.
The new release assertions just make it so we'll crash in a controlled way,
with a more actionable backtrace, closer to the problem-spot.

This patch also removes one mDocument null-check that's becoming redundant
since it follows a release-assert that would make us abort if the pointer is
null.

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

accessible/base/NotificationController.cpp

@@ -45,6 +45,8 @@ NotificationController::~NotificationController() {
   if (mDocument) {
     Shutdown();
   }
+  MOZ_RELEASE_ASSERT(mObservingState == eNotObservingRefresh,
+                     "Must unregister before being destroyed");
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -83,8 +85,13 @@ NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
 void NotificationController::Shutdown() {
   if (mObservingState != eNotObservingRefresh &&
       mPresShell->RemoveRefreshObserver(this, FlushType::Display)) {
+    // Note, this was our last chance to unregister, since we're about to
+    // clear mPresShell further down in this function.
     mObservingState = eNotObservingRefresh;
   }
+  MOZ_RELEASE_ASSERT(mObservingState == eNotObservingRefresh,
+                     "Must unregister before being destroyed (and we just "
+                     "passed our last change to unregister)");
 
   // Shutdown handling child documents.
   int32_t childDocCount = mHangingChildDocuments.Length();
@@ -674,12 +681,17 @@ void NotificationController::WillRefresh(mozilla::TimeStamp aTime) {
 
   AUTO_PROFILER_LABEL("NotificationController::WillRefresh", A11Y);
 
-  // If the document accessible that notification collector was created for is
+  // If mDocument is null, the document accessible that this notification
-  // now shut down, don't process notifications anymore.
+  // controller was created for is now shut down. This means we've lost our
-  NS_ASSERTION(
+  // ability to unregister ourselves, which is bad. (However, it also shouldn't
+  // be logically possible for us to get here with a null mDocument; the only
+  // thing that clears that pointer is our Shutdown() method, which first
+  // unregisters and fatally asserts if that fails).
+  MOZ_RELEASE_ASSERT(
       mDocument,
       "The document was shut down while refresh observer is attached!");
-  if (!mDocument || ipc::ProcessChild::ExpectingShutdown()) {
+
+  if (ipc::ProcessChild::ExpectingShutdown()) {
     return;
   }