1635106 - Context menu on web page isn't recognising elements as editable so cannot paste / have spell suggestions

Status: RESOLVED FIXED

(Firefox :: Menus, defect, P1)

Description

[Ian Neal]

STR

1. Start Firefox
2. Use it without closing for 3-4 days
3. Copy some information from a web page
4. Try to paste to any editable element (textarea, input, etc) with context menu
5. Try to get a spelling suggestion for any word highlighted as spelt incorrectly.

Expected result

1. Option to paste in context menu
2. List of potential words to replace incorrectly spelt word.

Actual result

1. Varies sometimes context menu shows things like:
   Copy, Select All, Search Google for "<paste content>", Inspect Element
   or:
   Navigation icons, Save Page As..., etc
   It seems to depend on the last context menu outside of an editable element
2. As for 1.

Workaround:
Keyboard shortcuts still work for pasting.

Also to note:
Context menu still works correctly in browser's address bar, search and other UI dialogs.

Comment 1

[:Gijs (he/him)]

Any errors in the browser console when this happens?

Comment 2

[Ian Neal]

Sorry meant to include, there are two:
Error: Unexpected state InlineSpellChecker.jsm:30:13
initFromRemote resource://gre/modules/InlineSpellChecker.jsm:30
setContext chrome://browser/content/nsContextMenu.js:273
nsContextMenu chrome://browser/content/nsContextMenu.js:97
onpopupshowing chrome://browser/content/browser.xhtml:1
openContextMenu chrome://browser/content/nsContextMenu.js:91
receiveMessage resource:///actors/ContextMenuParent.jsm:35
InvalidStateError: An attempt was made to use an object that is not, or is no longer, usable InlineSpellCheckerParent.jsm:31
uninit resource://gre/actors/InlineSpellCheckerParent.jsm:31
uninit resource://gre/modules/InlineSpellChecker.jsm:574
uninit resource://gre/modules/InlineSpellChecker.jsm:48
hiding chrome://browser/content/nsContextMenu.js:298
onpopuphiding chrome://browser/content/browser.xhtml:1

Comment 3

[:Gijs (he/him)]

Hm. The errors suggest that we can get stuck in a state where when the context menu hides, we try to uninit the spellchecker instance, and that fails because we call:

InlineSpellCheckerUI.uninit() at https://searchfox.org/mozilla-central/rev/54f965e51e4f77866bec42737978d40d4c1fdfc5/browser/base/content/nsContextMenu.js#300
which calls

this.mRemote.uninit() at https://searchfox.org/mozilla-central/rev/54f965e51e4f77866bec42737978d40d4c1fdfc5/toolkit/modules/InlineSpellChecker.jsm#48

which calls this._actor.uninit() at https://searchfox.org/mozilla-central/rev/54f965e51e4f77866bec42737978d40d4c1fdfc5/toolkit/modules/InlineSpellChecker.jsm#574

which fails because the actor is dead. I can reproduce somewhat artificially by doing something like:

1. open a browser with an editable textfield, introduce some spelling mistakes into it. I used https://paste.mozilla.org/
2. open the browser console
3. run something like setTimeout(() => gBrowser.removeTab(gBrowser.selectedTab), 10000)
4. in the 10 seconds before the timer runs, open the context menu on the misspelled text
5. wait for the tab to close
6. make the context menu go away

I don't really know why/how this would happen organically - maybe if you open the context menu on a webpage that's already navigating somewhere else or closing?

When I follow these steps, the context menu seems to just miss some of the spellcheck items, but cut/copy/paste are all enabled. It's possible this is an OS difference or something...

I expect this regressed by bug 1505909, but that's quite old and I don't recall seeing this before. It's possible there's been a different, more recent change that has increased the likelihood of this happening, but I'm unsure what it would be.

Ian, do you have any ideas based on how you use the context menu in terms of how you get into this state?

Comment 4

[Ian Neal]

Most of my context menu use is within bugzilla, copying and pasting information from other windows / applications.
If I create a new browser window then that works fine. If I drag a misbehaving tab off to create a new window, that works fine too.
In theory another workaround is to drag one tab off to create a new window, then drag the other tabs onto that new window too - bit of a faff though.
I've noticed the issue for at least this version and the last but not sure it went as far back as 69/70 but might have done.

Comment 5

[André Monteiro]

The first time I saw this bug was on firefox 75.

Comment 6

[:Gijs (he/him)]

(In reply to monterro from comment #5)

The first time I saw this bug was on firefox 75.

Thanks.

Nika, can you think of changes wrt actors in the 74/75 timeframe that'd explain something like comment #3 happening more frequently? And/or are there other ways that you can imagine the actor reference going away that aren't as artificial as what I'm suggesting in that comment?

The steps to fix this bug are more or less clear I suppose, but I wonder if whatever change regressed this would also have impacted other actor consumers that would similar fixes, so it would be nice to understand the root cause or regressor better...

Comment 7

[Nika Layzell [:nika] (ni? for response)]

I unfortunately don't have a super crisp memory of what's changed in the 74/75 timeframe, but I'd believe that something tweaked around the lifecycle of JSWindowActors to make something like this more common, but the underlying issue would still occur.

I do notice that the code in InlineSpellCheckerChild.jsm doesn't listen to the willDestroy() or didDestroy() notifications, and so it won't call uninitContextMenu if it happens to go away before it receives an InlineSpellChecker:uninit message from the parent process. It may be a good idea to make the code in this actor not rely on sending messages to the other side to perform teardown, and to rely on willDestroy() and didDestroy() to change between states.

Comment 8

[:Gijs (he/him)]

(In reply to Nika Layzell [:nika] (ni? for response) from comment #7)

I unfortunately don't have a super crisp memory of what's changed in the 74/75 timeframe, but I'd believe that something tweaked around the lifecycle of JSWindowActors to make something like this more common, but the underlying issue would still occur.

I do notice that the code in InlineSpellCheckerChild.jsm doesn't listen to the willDestroy() or didDestroy() notifications, and so it won't call uninitContextMenu if it happens to go away before it receives an InlineSpellChecker:uninit message from the parent process. It may be a good idea to make the code in this actor not rely on sending messages to the other side to perform teardown, and to rely on willDestroy() and didDestroy() to change between states.

This is sort of interesting, because it's an issue with the child actor, whereas the issue in comment #6 is in the parent. I also think that if the menu hides before the actor goes away, we still want to call uninitContextMenu - we don't want to only call it when the actor goes away, because users can switch between tabs and use spellchecking in several tabs at a time. And for that we'll need to continue to send messages ourselves, I believe. Finally, if a spellchecking or contextmenu actor gets destroyed, it may not currently be used by the currently open contextmenu, so we cannot unconditionally call uninit code from actor didDestroy handlers...

Comment 9

[:Gijs (he/him)]

Attachment: 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.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d75769547bb8
fix spellchecker lifetime handling vs. the context menu, r=nika

Comment 11

[Natalia Csoregi [:nataliaCs]]

Backed out for failures on browser_contextmenu_input.js

backout: https://hg.mozilla.org/integration/autoland/rev/3c8984a0c36ed65030157c78efb2a95009dd1f1e

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=d75769547bb8ec4a103f2fafbe96945a1d9187ca&group_state=expanded&selectedTaskRun=KllDIBTcQgC3kir-cCF33A-0

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=303164709&repo=autoland&lineNumber=1747

[task 2020-05-20T22:07:02.723Z] 22:07:02 INFO - TEST-PASS | browser/base/content/test/contextMenu/browser_contextmenu_input.js | checking item #6 (---) name -
[task 2020-05-20T22:07:02.723Z] 22:07:02 INFO - TEST-PASS | browser/base/content/test/contextMenu/browser_contextmenu_input.js | checking item #7 (context-selectall) name -
[task 2020-05-20T22:07:02.723Z] 22:07:02 INFO - Buffered messages finished
[task 2020-05-20T22:07:02.724Z] 22:07:02 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/contextMenu/browser_contextmenu_input.js | checking item #7 (context-selectall) enabled state - Got false, expected true
[task 2020-05-20T22:07:02.724Z] 22:07:02 INFO - Stack trace:
[task 2020-05-20T22:07:02.724Z] 22:07:02 INFO - chrome://mochikit/content/browser-test.js:test_is:1327
[task 2020-05-20T22:07:02.724Z] 22:07:02 INFO - chrome://mochitests/content/browser/browser/base/content/test/contextMenu/contextmenu_common.js:checkMenuItem:245
[task 2020-05-20T22:07:02.724Z] 22:07:02 INFO - chrome://mochitests/content/browser/browser/base/content/test/contextMenu/contextmenu_common.js:checkMenu:309
[task 2020-05-20T22:07:02.724Z] 22:07:02 INFO - chrome://mochitests/content/browser/browser/base/content/test/contextMenu/contextmenu_common.js:checkContextMenu:179
[task 2020-05-20T22:07:02.724Z] 22:07:02 INFO - chrome://mochitests/content/browser/browser/base/content/test/contextMenu/contextmenu_common.js:test_contextmenu:464
[task 2020-05-20T22:07:02.724Z] 22:07:02 INFO - chrome://mochitests/content/browser/browser/base/content/test/contextMenu/browser_contextmenu_input.js:test_text_input_disabled:49
[task 2020-05-20T22:07:02.724Z] 22:07:02 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1064
[task 2020-05-20T22:07:02.724Z] 22:07:02 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1104
[task 2020-05-20T22:07:02.724Z] 22:07:02 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:927
[task 2020-05-20T22:07:02.724Z] 22:07:02 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:918
[task 2020-05-20T22:07:02.724Z] 22:07:02 INFO - TEST-PASS | browser/base/content/test/contextMenu/browser_contextmenu_input.js | checking item #8 (---) name -

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0224aa78cb39
fix spellchecker lifetime handling vs. the context menu, r=nika

Comment 14

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/0224aa78cb39