From 6fa5c7eb3ece7665e4fb0874c9273658b557401d Mon Sep 17 00:00:00 2001 From: Gijs Kruitbosch Date: Fri, 22 May 2020 08:35:57 +0000 Subject: [PATCH] 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 | 16 ++ .../base/content/test/contextMenu/browser.ini | 2 + .../contextMenu/browser_contextmenu_input.js | 142 +--------- .../browser_contextmenu_spellcheck.js | 251 ++++++++++++++++++ .../test/contextMenu/contextmenu_common.js | 8 +- toolkit/actors/InlineSpellCheckerParent.jsm | 19 ++ toolkit/modules/InlineSpellChecker.jsm | 22 +- toolkit/modules/InlineSpellCheckerContent.jsm | 8 + 8 files changed, 323 insertions(+), 145 deletions(-) create mode 100644 browser/base/content/test/contextMenu/browser_contextmenu_spellcheck.js diff --git a/browser/actors/ContextMenuChild.jsm b/browser/actors/ContextMenuChild.jsm index 26e2c90daf02..16a3f8ecbc81 100644 --- a/browser/actors/ContextMenuChild.jsm +++ b/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; + } } diff --git a/browser/base/content/test/contextMenu/browser.ini b/browser/base/content/test/contextMenu/browser.ini index f715e426c39b..f0a208793d63 100644 --- a/browser/base/content/test/contextMenu/browser.ini +++ b/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 diff --git a/browser/base/content/test/contextMenu/browser_contextmenu_input.js b/browser/base/content/test/contextMenu/browser_contextmenu_input.js index 6e30b0580f1b..94e9465fd58c 100644 --- a/browser/base/content/test/contextMenu/browser_contextmenu_input.js +++ b/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", diff --git a/browser/base/content/test/contextMenu/browser_contextmenu_spellcheck.js b/browser/base/content/test/contextMenu/browser_contextmenu_spellcheck.js new file mode 100644 index 000000000000..3a150fecffad --- /dev/null +++ b/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); +}); diff --git a/browser/base/content/test/contextMenu/contextmenu_common.js b/browser/base/content/test/contextMenu/contextmenu_common.js index e881ba5ef6dd..5ffb84f84931 100644 --- a/browser/base/content/test/contextMenu/contextmenu_common.js +++ b/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; + } } diff --git a/toolkit/actors/InlineSpellCheckerParent.jsm b/toolkit/actors/InlineSpellCheckerParent.jsm index 755a9215ede9..84b8a616cc43 100644 --- a/toolkit/actors/InlineSpellCheckerParent.jsm +++ b/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; + } } diff --git a/toolkit/modules/InlineSpellChecker.jsm b/toolkit/modules/InlineSpellChecker.jsm index 23dff64056a7..ffc58714b0d9 100644 --- a/toolkit/modules/InlineSpellChecker.jsm +++ b/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; }, }; diff --git a/toolkit/modules/InlineSpellCheckerContent.jsm b/toolkit/modules/InlineSpellCheckerContent.jsm index 7284dca69003..205585c4543f 100644 --- a/toolkit/modules/InlineSpellCheckerContent.jsm +++ b/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 {