3 files changed, 64 insertions, 17 deletions

diff --git a/accessible/base/EventQueue.cpp b/accessible/base/EventQueue.cpp
index d64b358f5e..36787a44b1 100644
--- a/accessible/base/EventQueue.cpp
+++ b/accessible/base/EventQueue.cpp
@@ -348,7 +348,9 @@ void EventQueue::ProcessEventQueue() {
     AccEvent* event = events[idx];
     uint32_t eventType = event->mEventType;
     LocalAccessible* target = event->GetAccessible();
-    if (!target || target->IsDefunct()) continue;
+    if (!target || target->IsDefunct()) {
+      continue;
+    }
 
     // Collect select changes
     if (IPCAccessibilityActive()) {
@@ -413,7 +415,9 @@ void EventQueue::ProcessEventQueue() {
 
     nsEventShell::FireEvent(event);
 
-    if (!mDocument) return;
+    if (!mDocument) {
+      return;
+    }
   }
 
   if (mDocument && IPCAccessibilityActive() &&
diff --git a/accessible/base/EventQueue.h b/accessible/base/EventQueue.h
index 434acd3c09..87bcf89340 100644
--- a/accessible/base/EventQueue.h
+++ b/accessible/base/EventQueue.h
@@ -7,6 +7,7 @@
 #define mozilla_a11y_EventQueue_h_
 
 #include "AccEvent.h"
+#include "mozilla/Assertions.h"
 
 namespace mozilla {
 namespace a11y {
@@ -18,7 +19,10 @@ class DocAccessible;
  */
 class EventQueue {
  protected:
-  explicit EventQueue(DocAccessible* aDocument) : mDocument(aDocument) {}
+  explicit EventQueue(DocAccessible* aDocument) : mDocument(aDocument) {
+    MOZ_ASSERT(mDocument,
+               "There's no point creating an event queue for a null document");
+  }
 
   /**
    * Put an accessible event into the queue to process it later.
diff --git a/accessible/base/NotificationController.cpp b/accessible/base/NotificationController.cpp
index 6ac4223b7e..c7a702831f 100644
--- a/accessible/base/NotificationController.cpp
+++ b/accessible/base/NotificationController.cpp
@@ -41,7 +41,11 @@ NotificationController::NotificationController(DocAccessible* aDocument,
 
 NotificationController::~NotificationController() {
   NS_ASSERTION(!mDocument, "Controller wasn't shutdown properly!");
-  if (mDocument) Shutdown();
+  if (mDocument) {
+    Shutdown();
+  }
+  MOZ_RELEASE_ASSERT(mObservingState == eNotObservingRefresh,
+                     "Must unregister before being destroyed");
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -53,7 +57,9 @@ NS_IMPL_CYCLE_COLLECTING_NATIVE_RELEASE(NotificationController)
 NS_IMPL_CYCLE_COLLECTION_CLASS(NotificationController)
 
 NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(NotificationController)
-  if (tmp->mDocument) tmp->Shutdown();
+  if (tmp->mDocument) {
+    tmp->Shutdown();
+  }
 NS_IMPL_CYCLE_COLLECTION_UNLINK_END
 
 NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(NotificationController)
@@ -78,8 +84,16 @@ 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)");
+  // Immediately null out mPresShell, to prevent us from being registered as a
+  // refresh observer again.
+  mPresShell = nullptr;
 
   // Shutdown handling child documents.
   int32_t childDocCount = mHangingChildDocuments.Length();
@@ -92,7 +106,6 @@ void NotificationController::Shutdown() {
   mHangingChildDocuments.Clear();
 
   mDocument = nullptr;
-  mPresShell = nullptr;
 
   mTextHash.Clear();
   mContentInsertions.Clear();
@@ -434,9 +447,11 @@ void NotificationController::ScheduleContentInsertion(
 }
 
 void NotificationController::ScheduleProcessing() {
-  // If notification flush isn't planed yet start notification flush
+  // If notification flush isn't planned yet, start notification flush
   // asynchronously (after style and layout).
-  if (mObservingState == eNotObservingRefresh) {
+  // Note: the mPresShell null-check might be unnecessary; it's just to prevent
+  // a null-deref here, if we somehow get called after we've been shut down.
+  if (mObservingState == eNotObservingRefresh && mPresShell) {
     if (mPresShell->AddRefreshObserver(this, FlushType::Display,
                                        "Accessibility notifications")) {
       mObservingState = eRefreshObserving;
@@ -656,12 +671,17 @@ void NotificationController::WillRefresh(mozilla::TimeStamp aTime) {
 
   AUTO_PROFILER_LABEL("NotificationController::WillRefresh", A11Y);
 
-  // If the document accessible that notification collector was created for is
-  // now shut down, don't process notifications anymore.
-  NS_ASSERTION(
+  // If mDocument is null, the document accessible that this notification
+  // controller was created for is now shut down. This means we've lost our
+  // 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;
   }
 
@@ -835,7 +855,9 @@ void NotificationController::WillRefresh(mozilla::TimeStamp aTime) {
   nsTArray<RefPtr<DocAccessible>> newChildDocs;
   for (uint32_t idx = 0; idx < hangingDocCnt; idx++) {
     DocAccessible* childDoc = mHangingChildDocuments[idx];
-    if (childDoc->IsDefunct()) continue;
+    if (childDoc->IsDefunct()) {
+      continue;
+    }
 
     if (IPCAccessibilityActive() && !mDocument->IPCDoc()) {
       childDoc->Shutdown();
@@ -874,12 +896,16 @@ void NotificationController::WillRefresh(mozilla::TimeStamp aTime) {
     uint32_t childDocCnt = mDocument->ChildDocumentCount(), childDocIdx = 0;
     for (; childDocIdx < childDocCnt; childDocIdx++) {
       DocAccessible* childDoc = mDocument->GetChildDocumentAt(childDocIdx);
-      if (!childDoc->HasLoadState(DocAccessible::eCompletelyLoaded)) break;
+      if (!childDoc->HasLoadState(DocAccessible::eCompletelyLoaded)) {
+        break;
+      }
     }
 
     if (childDocIdx == childDocCnt) {
       mDocument->ProcessLoad();
-      if (!mDocument) return;
+      if (!mDocument) {
+        return;
+      }
     }
   }
 
@@ -909,7 +935,9 @@ void NotificationController::WillRefresh(mozilla::TimeStamp aTime) {
   uint32_t notificationCount = notifications.Length();
   for (uint32_t idx = 0; idx < notificationCount; idx++) {
     notifications[idx]->Process();
-    if (!mDocument) return;
+    if (!mDocument) {
+      return;
+    }
   }
 
   if (ipc::ProcessChild::ExpectingShutdown()) {
@@ -991,8 +1019,19 @@ void NotificationController::WillRefresh(mozilla::TimeStamp aTime) {
     }
   }
 
+  if (!mDocument) {
+    // A null mDocument means we've gotten a Shutdown() call (presumably via
+    // some script that we triggered above), and that means we're done here.
+    // Note: in this case, it's important that don't modify mObservingState;
+    // Shutdown() will have *unregistered* us as a refresh observer, and we
+    // don't want to mistakenly overwrite mObservingState and fool ourselves
+    // into thinking we've re-registered when we really haven't!
+    MOZ_ASSERT(mObservingState == eNotObservingRefresh,
+               "We've been shutdown, which means we should've been "
+               "unregistered as a refresh observer");
+    return;
+  }
   mObservingState = eRefreshObserving;
-  if (!mDocument) return;
 
   // Stop further processing if there are no new notifications of any kind or
   // events and document load is processed.