Bug 1888298: If shutdown of the accessibility service is triggered by a document shutting down, shut down the service asynchronously. r=nlapre

When DocAccessibles shut down, they shut down their child documents.
However, shutting down a document can result in shutting down the accessibility service if XPCOM is the only consumer and there are no more cached XPCOM documents.
If this is triggered by a child document, the service will shut down while the parent document is still shutting down, resulting in reentry and a crash.
In bug 1690456, code was added to prevent shutting down the service for a document with a parent.
However, this doesn't address "hanging child documents"; i.e. documents which currently have no parent because their OuterDocAccessible is being destroyed/recreated.

Rather than trying to avoid this specific case and given that we've hit reentry problems here twice before now (bug 1330765 and bug 1690456), I've taken a completely different approach.
Whenever shutdown of the service is triggered by a document shutdown, we now shut down the service asynchronously by dispatching a runnable.
This ensures that the service can't be shut down while shutting down a document.
We do this for both local and remote documents, since although we've never seen this for remote documents, I think this is safer.

In addition:

1. DocAccessible::Shutdown now has a diagnostic assert to hopefully detect such reentry should it occur again in future, making it much easier to debug.
2. Functions related to this have been refactored slightly for readability and consistency.

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

accessible/base/DocManager.cpp

@@ -86,22 +86,20 @@ LocalAccessible* DocManager::FindAccessibleInCache(nsINode* aNode) const {
   return nullptr;
 }
 
-void DocManager::RemoveFromXPCDocumentCache(DocAccessible* aDocument,
-                                            bool aAllowServiceShutdown) {
+void DocManager::RemoveFromXPCDocumentCache(DocAccessible* aDocument) {
   xpcAccessibleDocument* xpcDoc = mXPCDocumentCache.GetWeak(aDocument);
-  if (xpcDoc) {
-    xpcDoc->Shutdown();
-    mXPCDocumentCache.Remove(aDocument);
-
-    if (aAllowServiceShutdown && !HasXPCDocuments()) {
-      MaybeShutdownAccService(nsAccessibilityService::eXPCOM);
-    }
+  if (!xpcDoc) {
+    return;
+  }
+  xpcDoc->Shutdown();
+  mXPCDocumentCache.Remove(aDocument);
+  if (!HasXPCDocuments()) {
+    MaybeShutdownAccService(nsAccessibilityService::eXPCOM, /* aAsync */ true);
   }
 }
 
 void DocManager::NotifyOfDocumentShutdown(DocAccessible* aDocument,
-                                          Document* aDOMDocument,
-                                          bool aAllowServiceShutdown) {
+                                          Document* aDOMDocument) {
   // 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);
@@ -112,19 +110,19 @@ void DocManager::NotifyOfDocumentShutdown(DocAccessible* aDocument,
     return;
   }
 
-  RemoveFromXPCDocumentCache(aDocument, aAllowServiceShutdown);
+  RemoveFromXPCDocumentCache(aDocument);
   mDocAccessibleCache.Remove(aDOMDocument);
 }
 
 void DocManager::RemoveFromRemoteXPCDocumentCache(DocAccessibleParent* aDoc) {
   xpcAccessibleDocument* doc = GetCachedXPCDocument(aDoc);
-  if (doc) {
-    doc->Shutdown();
-    sRemoteXPCDocumentCache->Remove(aDoc);
+  if (!doc) {
+    return;
   }
-
+  doc->Shutdown();
+  sRemoteXPCDocumentCache->Remove(aDoc);
   if (sRemoteXPCDocumentCache && sRemoteXPCDocumentCache->Count() == 0) {
-    MaybeShutdownAccService(nsAccessibilityService::eXPCOM);
+    MaybeShutdownAccService(nsAccessibilityService::eXPCOM, /* aAsync */ true);
   }
 }
 

accessible/base/DocManager.h

@@ -56,15 +56,11 @@ 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,
-                                bool aAllowServiceShutdown = true);
+                                dom::Document* aDOMDocument);
 
-  void RemoveFromXPCDocumentCache(DocAccessible* aDocument,
-                                  bool aAllowServiceShutdown = true);
+  void RemoveFromXPCDocumentCache(DocAccessible* aDocument);
 
   /**
    * Return XPCOM accessible document.

accessible/base/nsAccessibilityService.cpp

@@ -1960,7 +1960,7 @@ nsAccessibilityService* GetOrCreateAccService(uint32_t aNewConsumer,
   return nsAccessibilityService::gAccessibilityService;
 }
 
-void MaybeShutdownAccService(uint32_t aFormerConsumer) {
+void MaybeShutdownAccService(uint32_t aFormerConsumer, bool aAsync) {
   nsAccessibilityService* accService =
       nsAccessibilityService::gAccessibilityService;
 
@@ -1985,11 +1985,32 @@ void MaybeShutdownAccService(uint32_t aFormerConsumer) {
   }
 
   if (nsAccessibilityService::gConsumers & ~aFormerConsumer) {
+    // There are still other consumers of the accessibility service, so we
+    // can't shut down.
     accService->UnsetConsumers(aFormerConsumer);
-  } else {
+    return;
+  }
+
+  if (!aAsync) {
     accService
         ->Shutdown();  // Will unset all nsAccessibilityService::gConsumers
+    return;
   }
+
+  static bool sIsPending = false;
+  if (sIsPending) {
+    // An async shutdown runnable is pending. Don't dispatch another.
+    return;
+  }
+  NS_DispatchToMainThread(NS_NewRunnableFunction(
+      "a11y::MaybeShutdownAccService", [aFormerConsumer]() {
+        // It's possible (albeit very unlikely) that another accessibility
+        // service consumer arrived since this runnable was dispatched. Use
+        // MaybeShutdownAccService to be safe.
+        MaybeShutdownAccService(aFormerConsumer, false);
+        sIsPending = false;
+      }));
+  sIsPending = true;
 }
 
 ////////////////////////////////////////////////////////////////////////////////

accessible/base/nsAccessibilityService.h

@@ -442,7 +442,7 @@ class nsAccessibilityService final : public mozilla::a11y::DocManager,
 
   friend nsAccessibilityService* GetAccService();
   friend nsAccessibilityService* GetOrCreateAccService(uint32_t, uint64_t);
-  friend void MaybeShutdownAccService(uint32_t);
+  friend void MaybeShutdownAccService(uint32_t, bool);
   friend void mozilla::a11y::PrefChanged(const char*, void*);
   friend mozilla::a11y::FocusManager* mozilla::a11y::FocusMgr();
   friend mozilla::a11y::SelectionManager* mozilla::a11y::SelectionMgr();
@@ -468,8 +468,13 @@ nsAccessibilityService* GetOrCreateAccService(
 
 /**
  * Shutdown accessibility service if needed.
+ * @param aFormerConsumer The ServiceConsumer that is no longer using the
+ *        service.
+ * @param aAsync True to shut down the service asynchronously using a runnable.
+ *        This should be used to avoid reentry if this is called during the
+ *        shutdown of a document.
  */
-void MaybeShutdownAccService(uint32_t aFormerConsumer);
+void MaybeShutdownAccService(uint32_t aFormerConsumer, bool aAsync = false);
 
 /**
  * Return true if we're in a content process and not B2G.

accessible/generic/DocAccessible.cpp

@@ -508,6 +508,8 @@ void DocAccessible::Shutdown() {
   // Mark the document as shutdown before AT is notified about the document
   // removal from its container (valid for root documents on ATK and due to
   // some reason for MSAA, refer to bug 757392 for details).
+  MOZ_DIAGNOSTIC_ASSERT(!IsDefunct(),
+                        "Already marked defunct. Reentrant shutdown!");
   mStateFlags |= eIsDefunct;
 
   if (mNotificationController) {
@@ -517,9 +519,6 @@ 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);
@@ -589,12 +588,7 @@ void DocAccessible::Shutdown() {
   HyperTextAccessible::Shutdown();
 
   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);
+  GetAccService()->NotifyOfDocumentShutdown(this, mDocumentNode);
   mDocumentNode = nullptr;
 }