Bug 1635106 - fix spellchecker lifetime handling vs. the context menu, r=nika

This changes both the spellchecker parent code that interfaces with the
InlineSpellCheckerParent actor, and the child code interfacing with the
ContextMenuChild actor, to ensure they get notified when either actor
goes away.

It maintains the "uninit" messages to clear out spellcheck data when the
context menu goes away (while the window / actors remain intact).

It also adds some belts-and-suspenders type checks that allow us to
recover if we ever get in a bad state again, instead of stubbornly
throwing exceptions and breaking the UI for users.

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

browser/actors/ContextMenuChild.jsm

@@ -1227,4 +1227,20 @@ class ContextMenuChild extends JSWindowActorChild {
       }
     }
   }
+
+  _destructionObservers = new Set();
+  registerDestructionObserver(obj) {
+    this._destructionObservers.add(obj);
+  }
+
+  unregisterDestructionObserver(obj) {
+    this._destructionObservers.delete(obj);
+  }
+
+  didDestroy() {
+    for (let obs of this._destructionObservers) {
+      obs.actorDestroyed(this);
+    }
+    this._destructionObservers = null;
+  }
 }

browser/base/content/test/contextMenu/browser.ini

@@ -16,6 +16,8 @@ support-files =
 
 [browser_contextmenu_loadblobinnewtab.js]
 support-files = browser_contextmenu_loadblobinnewtab.html
+[browser_contextmenu_spellcheck.js]
+skip-if = toolkit == "gtk" || (os == "win" && processor == "aarch64") # disabled on Linux due to bug 513558, aarch64 due to 1533161
 [browser_view_image.js]
 support-files =
   test_view_image_revoked_cached_blob.html

browser/base/content/test/contextMenu/browser_contextmenu_input.js

@@ -45,146 +45,6 @@ add_task(async function test_text_input() {
   ]);
 });
 
-add_task(async function test_text_input_spellcheck() {
-  await test_contextmenu(
-    "#input_spellcheck_no_value",
-    [
-      "context-undo",
-      false,
-      "---",
-      null,
-      "context-cut",
-      true,
-      "context-copy",
-      true,
-      "context-paste",
-      null, // ignore clipboard state
-      "context-delete",
-      false,
-      "---",
-      null,
-      "context-selectall",
-      false,
-      "---",
-      null,
-      "spell-check-enabled",
-      true,
-      "spell-dictionaries",
-      true,
-      [
-        "spell-check-dictionary-en-US",
-        true,
-        "---",
-        null,
-        "spell-add-dictionaries",
-        true,
-      ],
-      null,
-    ],
-    {
-      waitForSpellCheck: true,
-      async preCheckContextMenuFn() {
-        await SpecialPowers.spawn(
-          gBrowser.selectedBrowser,
-          [],
-          async function() {
-            let doc = content.document;
-            let input = doc.getElementById("input_spellcheck_no_value");
-            input.setAttribute("spellcheck", "true");
-            input.clientTop; // force layout flush
-          }
-        );
-      },
-    }
-  );
-});
-
-add_task(async function test_text_input_spellcheckwrong() {
-  await test_contextmenu(
-    "#input_spellcheck_incorrect",
-    [
-      "*prodigality",
-      true, // spelling suggestion
-      "spell-add-to-dictionary",
-      true,
-      "---",
-      null,
-      "context-undo",
-      false,
-      "---",
-      null,
-      "context-cut",
-      true,
-      "context-copy",
-      true,
-      "context-paste",
-      null, // ignore clipboard state
-      "context-delete",
-      false,
-      "---",
-      null,
-      "context-selectall",
-      true,
-      "---",
-      null,
-      "spell-check-enabled",
-      true,
-      "spell-dictionaries",
-      true,
-      [
-        "spell-check-dictionary-en-US",
-        true,
-        "---",
-        null,
-        "spell-add-dictionaries",
-        true,
-      ],
-      null,
-    ],
-    { waitForSpellCheck: true }
-  );
-});
-
-add_task(async function test_text_input_spellcheckcorrect() {
-  await test_contextmenu(
-    "#input_spellcheck_correct",
-    [
-      "context-undo",
-      false,
-      "---",
-      null,
-      "context-cut",
-      true,
-      "context-copy",
-      true,
-      "context-paste",
-      null, // ignore clipboard state
-      "context-delete",
-      false,
-      "---",
-      null,
-      "context-selectall",
-      true,
-      "---",
-      null,
-      "spell-check-enabled",
-      true,
-      "spell-dictionaries",
-      true,
-      [
-        "spell-check-dictionary-en-US",
-        true,
-        "---",
-        null,
-        "spell-add-dictionaries",
-        true,
-      ],
-      null,
-    ],
-    { waitForSpellCheck: true }
-  );
-});
-
 add_task(async function test_text_input_disabled() {
   await test_contextmenu(
     "#input_disabled",
@@ -204,7 +64,7 @@ add_task(async function test_text_input_disabled() {
       "---",
       null,
       "context-selectall",
-      true,
+      false,
       "---",
       null,
       "spell-check-enabled",

browser/base/content/test/contextMenu/browser_contextmenu_spellcheck.js

@@ -0,0 +1,251 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+let contextMenu;
+
+const example_base =
+  "http://example.com/browser/browser/base/content/test/contextMenu/";
+const MAIN_URL = example_base + "subtst_contextmenu_input.html";
+
+add_task(async function test_setup() {
+  await BrowserTestUtils.openNewForegroundTab(gBrowser, MAIN_URL);
+
+  const chrome_base =
+    "chrome://mochitests/content/browser/browser/base/content/test/contextMenu/";
+  const contextmenu_common = chrome_base + "contextmenu_common.js";
+  /* import-globals-from contextmenu_common.js */
+  Services.scriptloader.loadSubScript(contextmenu_common, this);
+
+  // Ensure screenshots is really disabled (bug 1498738)
+  const addon = await AddonManager.getAddonByID("screenshots@mozilla.org");
+  await addon.disable({ allowSystemAddons: true });
+});
+
+add_task(async function test_text_input_spellcheck() {
+  await test_contextmenu(
+    "#input_spellcheck_no_value",
+    [
+      "context-undo",
+      false,
+      "---",
+      null,
+      "context-cut",
+      null, // ignore the enabled/disabled states; there are race conditions
+      // in the edit commands but they're not relevant for what we're testing.
+      "context-copy",
+      null,
+      "context-paste",
+      null, // ignore clipboard state
+      "context-delete",
+      null,
+      "---",
+      null,
+      "context-selectall",
+      null,
+      "---",
+      null,
+      "spell-check-enabled",
+      true,
+      "spell-dictionaries",
+      true,
+      [
+        "spell-check-dictionary-en-US",
+        true,
+        "---",
+        null,
+        "spell-add-dictionaries",
+        true,
+      ],
+      null,
+    ],
+    {
+      waitForSpellCheck: true,
+      async preCheckContextMenuFn() {
+        await SpecialPowers.spawn(
+          gBrowser.selectedBrowser,
+          [],
+          async function() {
+            let doc = content.document;
+            let input = doc.getElementById("input_spellcheck_no_value");
+            input.setAttribute("spellcheck", "true");
+            input.clientTop; // force layout flush
+          }
+        );
+      },
+    }
+  );
+});
+
+add_task(async function test_text_input_spellcheckwrong() {
+  await test_contextmenu(
+    "#input_spellcheck_incorrect",
+    [
+      "*prodigality",
+      true, // spelling suggestion
+      "spell-add-to-dictionary",
+      true,
+      "---",
+      null,
+      "context-undo",
+      null,
+      "---",
+      null,
+      "context-cut",
+      null,
+      "context-copy",
+      null,
+      "context-paste",
+      null, // ignore clipboard state
+      "context-delete",
+      null,
+      "---",
+      null,
+      "context-selectall",
+      null,
+      "---",
+      null,
+      "spell-check-enabled",
+      true,
+      "spell-dictionaries",
+      true,
+      [
+        "spell-check-dictionary-en-US",
+        true,
+        "---",
+        null,
+        "spell-add-dictionaries",
+        true,
+      ],
+      null,
+    ],
+    { waitForSpellCheck: true }
+  );
+});
+
+const kCorrectItems = [
+  "context-undo",
+  false,
+  "---",
+  null,
+  "context-cut",
+  null,
+  "context-copy",
+  null,
+  "context-paste",
+  null, // ignore clipboard state
+  "context-delete",
+  null,
+  "---",
+  null,
+  "context-selectall",
+  null,
+  "---",
+  null,
+  "spell-check-enabled",
+  true,
+  "spell-dictionaries",
+  true,
+  [
+    "spell-check-dictionary-en-US",
+    true,
+    "---",
+    null,
+    "spell-add-dictionaries",
+    true,
+  ],
+  null,
+];
+
+add_task(async function test_text_input_spellcheckcorrect() {
+  await test_contextmenu("#input_spellcheck_correct", kCorrectItems, {
+    waitForSpellCheck: true,
+  });
+});
+
+add_task(async function test_text_input_spellcheck_deadactor() {
+  await test_contextmenu("#input_spellcheck_correct", kCorrectItems, {
+    waitForSpellCheck: true,
+    keepMenuOpen: true,
+  });
+  let wgp = gBrowser.selectedBrowser.browsingContext.currentWindowGlobal;
+
+  // Now the menu is open, and spellcheck is running, switch to another tab and
+  // close the original:
+  let tab = gBrowser.selectedTab;
+  await BrowserTestUtils.openNewForegroundTab(gBrowser, "https://example.org");
+  BrowserTestUtils.removeTab(tab);
+  // Ensure we've invalidated the actor
+  await TestUtils.waitForCondition(
+    () => wgp.isClosed,
+    "Waiting for actor to be dead after tab closes"
+  );
+  contextMenu.hidePopup();
+
+  // Now go back to the input testcase:
+  BrowserTestUtils.loadURI(gBrowser.selectedBrowser, MAIN_URL);
+  await BrowserTestUtils.browserLoaded(
+    gBrowser.selectedBrowser,
+    false,
+    MAIN_URL
+  );
+
+  // Check the menu still looks the same, keep it open again:
+  await test_contextmenu("#input_spellcheck_correct", kCorrectItems, {
+    waitForSpellCheck: true,
+    keepMenuOpen: true,
+  });
+
+  // Now navigate the tab, after ensuring there's an unload listener, so
+  // we don't end up in bfcache:
+  await SpecialPowers.spawn(gBrowser.selectedBrowser, [], async function() {
+    content.document.body.setAttribute("onunload", "");
+  });
+  wgp = gBrowser.selectedBrowser.browsingContext.currentWindowGlobal;
+
+  const NEW_URL = MAIN_URL.replace(".com", ".org");
+  BrowserTestUtils.loadURI(gBrowser.selectedBrowser, NEW_URL);
+  await BrowserTestUtils.browserLoaded(
+    gBrowser.selectedBrowser,
+    false,
+    NEW_URL
+  );
+  // Ensure we've invalidated the actor
+  await TestUtils.waitForCondition(
+    () => wgp.isClosed,
+    "Waiting for actor to be dead after onunload"
+  );
+  contextMenu.hidePopup();
+
+  // Check the menu *still* looks the same (and keep it open again):
+  await test_contextmenu("#input_spellcheck_correct", kCorrectItems, {
+    waitForSpellCheck: true,
+    keepMenuOpen: true,
+  });
+
+  // Check what happens if the actor stays alive by loading the same page
+  // again; now the context menu stuff should be destroyed by the menu
+  // hiding, nothing else.
+  wgp = gBrowser.selectedBrowser.browsingContext.currentWindowGlobal;
+  BrowserTestUtils.loadURI(gBrowser.selectedBrowser, NEW_URL);
+  await BrowserTestUtils.browserLoaded(
+    gBrowser.selectedBrowser,
+    false,
+    NEW_URL
+  );
+  contextMenu.hidePopup();
+
+  // Check the menu still looks the same:
+  await test_contextmenu("#input_spellcheck_correct", kCorrectItems, {
+    waitForSpellCheck: true,
+  });
+  // And test it a last time without any navigation:
+  await test_contextmenu("#input_spellcheck_correct", kCorrectItems, {
+    waitForSpellCheck: true,
+  });
+});
+
+add_task(async function test_cleanup() {
+  BrowserTestUtils.removeTab(gBrowser.selectedTab);
+});

browser/base/content/test/contextMenu/contextmenu_common.js

@@ -352,6 +352,8 @@ let lastElementSelector = null;
  *        preCheckContextMenuFn: callback to run before opening menu
  *        onContextMenuShown: callback to run when the context menu is shown
  *        postCheckContextMenuFn: callback to run after opening menu
+ *        keepMenuOpen: if true, we do not call hidePopup, the consumer is
+ *                      responsible for calling it.
  * @return {Promise} resolved after the test finishes
  */
 async function test_contextmenu(selector, menuItems, options = {}) {
@@ -472,6 +474,8 @@ async function test_contextmenu(selector, menuItems, options = {}) {
     info("Completed postCheckContextMenuFn");
   }
 
-  contextMenu.hidePopup();
-  await awaitPopupHidden;
+  if (!options.keepMenuOpen) {
+    contextMenu.hidePopup();
+    await awaitPopupHidden;
+  }
 }

toolkit/actors/InlineSpellCheckerParent.jsm

@@ -28,6 +28,25 @@ class InlineSpellCheckerParent extends JSWindowActorParent {
   }
 
   uninit() {
+    // This method gets called by InlineSpellChecker when the context menu
+    // goes away and the InlineSpellChecker instance is still alive.
+    // Stop referencing it and tidy the child end of us.
     this.sendAsyncMessage("InlineSpellChecker:uninit", {});
   }
+
+  _destructionObservers = new Set();
+  registerDestructionObserver(obj) {
+    this._destructionObservers.add(obj);
+  }
+
+  unregisterDestructionObserver(obj) {
+    this._destructionObservers.delete(obj);
+  }
+
+  didDestroy() {
+    for (let obs of this._destructionObservers) {
+      obs.actorDestroyed(this);
+    }
+    this._destructionObservers = null;
+  }
 }

toolkit/modules/InlineSpellChecker.jsm

@@ -27,7 +27,15 @@ InlineSpellChecker.prototype = {
 
   initFromRemote(aSpellInfo, aWindowGlobalParent) {
     if (this.mRemote) {
-      throw new Error("Unexpected state");
+      // We shouldn't get here, but let's just recover instead of bricking the
+      // menu by throwing exceptions:
+      Cu.reportError(new Error("Unexpected remote spellchecker present!"));
+      try {
+        this.mRemote.uninit();
+      } catch (ex) {
+        Cu.reportError(ex);
+      }
+      this.mRemote = null;
     }
     this.uninit();
 
@@ -490,6 +498,7 @@ function RemoteSpellChecker(aSpellInfo, aWindowGlobalParent) {
   this._spellInfo = aSpellInfo;
   this._suggestionGenerator = null;
   this._actor = aWindowGlobalParent.getActor("InlineSpellChecker");
+  this._actor.registerDestructionObserver(this);
 }
 
 RemoteSpellChecker.prototype = {
@@ -571,6 +580,15 @@ RemoteSpellChecker.prototype = {
     this._actor.recheckSpelling();
   },
   uninit() {
-    this._actor.uninit();
+    if (this._actor) {
+      this._actor.uninit();
+      this._actor.unregisterDestructionObserver(this);
+    }
+  },
+
+  actorDestroyed() {
+    // The actor lets us know if it gets destroyed, so we don't
+    // later try to call `.uninit()` on it.
+    this._actor = null;
   },
 };

toolkit/modules/InlineSpellCheckerContent.jsm

@@ -17,6 +17,7 @@ var InlineSpellCheckerContent = {
 
   initContextMenu(event, editFlags, actor) {
     this._actor = actor;
+    this._actor.registerDestructionObserver(this);
 
     let spellChecker;
     if (!(editFlags & (SpellCheckHelper.TEXTAREA | SpellCheckHelper.INPUT))) {
@@ -75,10 +76,17 @@ var InlineSpellCheckerContent = {
   },
 
   uninitContextMenu() {
+    if (this._actor) {
+      this._actor.unregisterDestructionObserver(this);
+    }
     this._actor = null;
     this._spellChecker = null;
   },
 
+  actorDestroyed() {
+    this.uninitContextMenu();
+  },
+
   _generateSpellSuggestions() {
     let spellChecker = this._spellChecker.mInlineSpellChecker.spellChecker;
     try {