From 1b4d4c6a323b145ba563454e18a210ade3306940 Mon Sep 17 00:00:00 2001
From: James Teh <jteh@mozilla.com>
Date: Thu, 4 Feb 2021 01:10:52 +0000
Subject: [PATCH] Bug 1690456: Don't allow nsAccessibilityService to be shut
 down while a parent document is still shutting down. r=yzen

DocAccessible::Shutdown calls DocManager::NotifyOfDocumentShutdown, which can shut down nsAccessibilityService if there are no more consumers.
Previously, this could happen even when shutting down child documents.
Since shutting down the service shuts down all documents, this resulted in shutting down the parent document within an outer call to shut down that same document.
Even if that reentry were prevented, the service would have been gone when returning to the outer Shutdown call, which still needs the service to complete its cleanup.

To fix this, DocManager::NotifyOfDocumentShutdown takes an argument specifying whether to allow service shutdown.
This is set to false when shutting down child documents.
The service is thus allowed to shut down when returning to the parent document Shutdown.

In addition, mPresShell is cleared before shutting down child documents to prevent reentry like this.
While this should no longer happen, this should safeguard against similar pain in future.

Differential Revision: https://phabricator.services.mozilla.com/D103966
---
 accessible/base/DocManager.cpp       | 10 ++++++----
 accessible/base/DocManager.h         |  8 ++++++--
 accessible/generic/DocAccessible.cpp | 17 +++++++++++++----
 3 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/accessible/base/DocManager.cpp b/accessible/base/DocManager.cpp
index 9011f20dad70..0c3fa5d3ba87 100644
--- a/accessible/base/DocManager.cpp
+++ b/accessible/base/DocManager.cpp
@@ -91,20 +91,22 @@ Accessible* DocManager::FindAccessibleInCache(nsINode* aNode) const {
   return nullptr;
 }
 
-void DocManager::RemoveFromXPCDocumentCache(DocAccessible* aDocument) {
+void DocManager::RemoveFromXPCDocumentCache(DocAccessible* aDocument,
+                                            bool aAllowServiceShutdown) {
   xpcAccessibleDocument* xpcDoc = mXPCDocumentCache.GetWeak(aDocument);
   if (xpcDoc) {
     xpcDoc->Shutdown();
     mXPCDocumentCache.Remove(aDocument);
 
-    if (!HasXPCDocuments()) {
+    if (aAllowServiceShutdown && !HasXPCDocuments()) {
       MaybeShutdownAccService(nsAccessibilityService::eXPCOM);
     }
   }
 }
 
 void DocManager::NotifyOfDocumentShutdown(DocAccessible* aDocument,
-                                          Document* aDOMDocument) {
+                                          Document* aDOMDocument,
+                                          bool aAllowServiceShutdown) {
   // We need to remove listeners in both cases, when document is being shutdown
   // or when accessibility service is being shut down as well.
   RemoveListeners(aDOMDocument);
@@ -115,7 +117,7 @@ void DocManager::NotifyOfDocumentShutdown(DocAccessible* aDocument,
     return;
   }
 
-  RemoveFromXPCDocumentCache(aDocument);
+  RemoveFromXPCDocumentCache(aDocument, aAllowServiceShutdown);
   mDocAccessibleCache.Remove(aDOMDocument);
 }
 
diff --git a/accessible/base/DocManager.h b/accessible/base/DocManager.h
index 5fd820ff46a8..81ff4e72b1ce 100644
--- a/accessible/base/DocManager.h
+++ b/accessible/base/DocManager.h
@@ -56,11 +56,15 @@ class DocManager : public nsIWebProgressListener,
 
   /**
    * Called by document accessible when it gets shutdown.
+   * @param aAllowServiceShutdown true to shut down nsAccessibilityService
+   *        if it is no longer required, false to prevent it.
    */
   void NotifyOfDocumentShutdown(DocAccessible* aDocument,
-                                dom::Document* aDOMDocument);
+                                dom::Document* aDOMDocument,
+                                bool aAllowServiceShutdown = true);
 
-  void RemoveFromXPCDocumentCache(DocAccessible* aDocument);
+  void RemoveFromXPCDocumentCache(DocAccessible* aDocument,
+                                  bool aAllowServiceShutdown = true);
 
   /**
    * Return XPCOM accessible document.
diff --git a/accessible/generic/DocAccessible.cpp b/accessible/generic/DocAccessible.cpp
index 42357d02cd87..43cab7396dc9 100644
--- a/accessible/generic/DocAccessible.cpp
+++ b/accessible/generic/DocAccessible.cpp
@@ -396,6 +396,9 @@ void DocAccessible::Shutdown() {
 
   RemoveEventListeners();
 
+  // mParent->RemoveChild clears mParent, but we need to know whether we were a
+  // child later, so use a flag.
+  const bool isChild = !!mParent;
   if (mParent) {
     DocAccessible* parentDocument = mParent->Document();
     if (parentDocument) parentDocument->RemoveChildDocument(this);
@@ -404,6 +407,9 @@ void DocAccessible::Shutdown() {
     MOZ_ASSERT(!mParent, "Parent has to be null!");
   }
 
+  mPresShell->SetDocAccessible(nullptr);
+  mPresShell = nullptr;  // Avoid reentrancy
+
   // Walk the array backwards because child documents remove themselves from the
   // array as they are shutdown.
   int32_t childDocCount = mChildDocuments.Length();
@@ -424,9 +430,6 @@ void DocAccessible::Shutdown() {
     mVirtualCursor = nullptr;
   }
 
-  mPresShell->SetDocAccessible(nullptr);
-  mPresShell = nullptr;  // Avoid reentrancy
-
   mDependentIDsHashes.Clear();
   mNodeToAccessibleMap.Clear();
 
@@ -446,7 +449,13 @@ void DocAccessible::Shutdown() {
 
   HyperTextAccessibleWrap::Shutdown();
 
-  GetAccService()->NotifyOfDocumentShutdown(this, mDocumentNode);
+  MOZ_ASSERT(GetAccService());
+  GetAccService()->NotifyOfDocumentShutdown(
+      this, mDocumentNode,
+      // Make sure we don't shut down AccService while a parent document is
+      // still shutting down. The parent will allow service shutdown when it
+      // reaches this point.
+      /* aAllowServiceShutdown */ !isChild);
   mDocumentNode = nullptr;
 }