Bug 1748450: If an Accessible is moved, reuse the RemoteAccessible even if the cache is disabled. r=eeejay

This fixes problems for Orca caused by object destruction when grabbing focus or setting the caret.

This patch also Fixes two bugs in the RemoteAccessible reuse code.
First, moved LocalAccessibles were previously being tracked even in the parent process if the cache was enabled.
While that was harmless, it was also unnecessary, since we only need to do that in order to send IPC notifications.
Second, when an OuterDocAccessible was moved, we were previously trying to shut down its child document, causing a crash.
Child documents must be left alone, since they are added ande removed differently to normal children.

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

accessible/generic/DocAccessible.cpp

@@ -1094,7 +1094,7 @@ void DocAccessible::BindToDocument(LocalAccessible* aAccessible,
     }
   }
 
-  if (StaticPrefs::accessibility_cache_enabled_AtStartup()) {
+  if (mIPCDoc) {
     mInsertedAccessibles.EnsureInserted(aAccessible);
   }
 }
@@ -1414,7 +1414,7 @@ void DocAccessible::ProcessBoundsChanged() {
 }
 
 void DocAccessible::SendAccessiblesWillMove() {
-  if (!mIPCDoc || !StaticPrefs::accessibility_cache_enabled_AtStartup()) {
+  if (!mIPCDoc) {
     return;
   }
   nsTArray<uint64_t> ids;
@@ -2345,7 +2345,7 @@ bool DocAccessible::MoveChild(LocalAccessible* aChild,
   if (curParent == aNewParent) {
     MOZ_ASSERT(aChild->IndexInParent() != aIdxInParent, "No move case");
     curParent->RelocateChild(aIdxInParent, aChild);
-    if (StaticPrefs::accessibility_cache_enabled_AtStartup()) {
+    if (mIPCDoc) {
       TrackMovedAccessible(aChild);
     }
 
@@ -2374,7 +2374,7 @@ bool DocAccessible::MoveChild(LocalAccessible* aChild,
 
   TreeMutation imut(aNewParent);
   aNewParent->InsertChildAt(aIdxInParent, aChild);
-  if (StaticPrefs::accessibility_cache_enabled_AtStartup()) {
+  if (mIPCDoc) {
     TrackMovedAccessible(aChild);
   }
   imut.AfterInsertion(aChild);

accessible/generic/DocAccessible.h

@@ -507,6 +507,9 @@ class DocAccessible : public HyperTextAccessibleWrap,
    */
   void ProcessBoundsChanged();
 
+  /**
+   * Only works in content process documents.
+   */
   bool IsAccessibleBeingMoved(LocalAccessible* aAcc) {
     return mMovedAccessibles.Contains(aAcc);
   }
@@ -744,8 +747,8 @@ class DocAccessible : public HyperTextAccessibleWrap,
   void SetRoleMapEntryForDoc(dom::Element* aElement);
 
   /**
-   * This must be called whenever an Accessible is moved if the cache is
-   * enabled. It keeps track of Accessibles moved during this tick.
+   * This must be called whenever an Accessible is moved in a content process.
+   * It keeps track of Accessibles moved during this tick.
    */
   void TrackMovedAccessible(LocalAccessible* aAcc);
 
@@ -755,11 +758,11 @@ class DocAccessible : public HyperTextAccessibleWrap,
   DocAccessibleChild* mIPCDoc;
 
   nsTHashSet<RefPtr<LocalAccessible>> mMaybeBoundsChanged;
-  // A set of Accessibles moved during this tick. Only used if the cache is
-  // enabled.
+  // A set of Accessibles moved during this tick. Only used in content
+  // processes.
   nsTHashSet<RefPtr<LocalAccessible>> mMovedAccessibles;
-  // A set of Accessibles inserted during this tick. Only used if the cache is
-  // enabled. This is needed to prevent insertions + moves of the same
+  // A set of Accessibles inserted during this tick. Only used in content
+  // processes. This is needed to prevent insertions + moves of the same
   // Accessible in the same tick from being tracked as moves.
   nsTHashSet<RefPtr<LocalAccessible>> mInsertedAccessibles;
 };

accessible/ipc/DocAccessibleParent.cpp

@@ -189,6 +189,11 @@ void DocAccessibleParent::ShutdownOrPrepareForMove(RemoteAccessible* aAcc) {
   // we want to keep the Accessible alive for reuse later.
   aAcc->SetParent(nullptr);
   mMovingIDs.EnsureRemoved(id);
+  if (aAcc->IsOuterDoc()) {
+    // Leave child documents alone. They are added and removed differently to
+    // normal children.
+    return;
+  }
   // Some children might be removed. Handle children the same way.
   for (RemoteAccessible* child : aAcc->mChildren) {
     ShutdownOrPrepareForMove(child);

accessible/tests/browser/e10s/browser_treeupdate_move.js

@@ -11,11 +11,6 @@ loadScripts(
   { name: "states.js", dir: MOCHITESTS_DIR }
 );
 
-const isCacheEnabled = Services.prefs.getBoolPref(
-  "accessibility.cache.enabled",
-  false
-);
-
 /**
  * Test moving Accessibles:
  * 1. A moved Accessible keeps the same Accessible.
@@ -30,12 +25,16 @@ addAccessibleTask(
     <h1 id="heading">Heading</h1>
     <p id="para">Para</p>
   </div>
+  <iframe id="iframe" src="https://example.com/"></iframe>
 </div>
   `,
   async function(browser, docAcc) {
     const textbox = findAccessibleChildByID(docAcc, "textbox");
     const heading = findAccessibleChildByID(docAcc, "heading");
     const para = findAccessibleChildByID(docAcc, "para");
+    const iframe = findAccessibleChildByID(docAcc, "iframe");
+    const iframeDoc = iframe.firstChild;
+    ok(iframeDoc, "iframe contains a document");
 
     let focused = waitForEvent(EVENT_FOCUS, textbox);
     textbox.takeFocus();
@@ -57,12 +56,9 @@ addAccessibleTask(
     // heading was a child of textbox, but was removed when textbox
     // was moved. Ensure it is dead.
     ok(isDefunct(heading), "heading is dead");
+    // Ensure the iframe and its embedded document are alive.
+    ok(!isDefunct(iframe), "iframe is alive");
+    ok(!isDefunct(iframeDoc), "iframeDoc is alive");
   },
-  {
-    chrome: true,
-    // Moves cause RemoteAccessible re-creation without the cache enabled.
-    topLevel: isCacheEnabled,
-    iframe: isCacheEnabled,
-    remoteIframe: isCacheEnabled,
-  }
+  { chrome: true, topLevel: true, iframe: true, remoteIframe: true }
 );