1448200 - Context menu for a textarea can fail to appear on the first click due to apparent spell checking problem

Status: RESOLVED FIXED

(Firefox :: Menus, defect, P1)

Description

[Chris Siebenmann]

If I run Firefox Nightly in a completely clean environment on one of my Fedora 27 64-bit x86 machines, go to a web page with a multi-line textarea (one where spelling checking is or would be active), hover my mouse over the textarea without clicking on it, and then right-click to bring up the context menu, the first time I do this the context menu does not come up and the browser console will report:

  TypeError: realSpellChecker is null

(In the current Firefox Nightly this is reported as being from InlineSpellCheckerContent.jsm line '54:5'.)

If I right click a second time, the context menu appears. If I click in the textarea to select it before I right click to bring up the context menu, everything is fine; this includes clicking in it and starting to type text. In all cases spell checking appears to be active if I actually enter text.

My clean environment for this test is created with:

  export HOME=/tmp/randombit
  mkdir $HOME
  cd $HOME
  [run appropriate firefox version]

Now that I do more testing, this doesn't seem exclusive to Nightly. I can make it happen in the current Firefox Beta (60.0b5), and also in the current release version (Firefox 59.0.1). Some Internet searches for this specific message suggests that it was happening to some people at least as far back as last summer.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Chris, can you use the mozregression [1] tool to find when this started happening? I looked at the specific line of code that dereferences realSpellChecker and it was introduced in Firefox 35 [2]. I hope it hasn't been broken for that long! :)

[1] http://mozilla.github.io/mozregression/
[2] https://searchfox.org/mozilla-central/rev/003262ae12ce937950ffb8d3b0fa520d1cc38bff/toolkit/modules/InlineSpellCheckerContent.jsm#54

Comment 2

[Chris Siebenmann]

I've now bisected things both by hand (on release versions of Firefox) and with mozregression, which gave me drastically different results, and in the process I've noticed a behavior that may help narrow this down. So, here is what I see and what mozregression reports.

What I see with released versions is that (64-bit Linux) firefox-48.0.tar.bz2 from https://ftp.mozilla.org/pub/firefox/releases/ does not have this issue, while firefox-49.0.tar.bz2 does. However, the older version has a different issue, which is that if you immediately try to bring up the context menu in the way that provokes this bug, it appears but doesn't have the 'check spelling' tickbox. No errors are reported in the browser JS console. If I dismiss the context menu and then bring it back up, the second time around it has the 'check spelling' tickbox (with that ticked).

With mozregression, the result I get is:

   INFO: Last good revision: 2114ef80f6ae (2014-11-06)
   INFO: First bad revision: b62ccf3228ba (2014-11-07)
   INFO: Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2114ef80f6ae&tochange=b62ccf3228ba

(The old 'good' Nightly versions have the same behavior as Firefox 48.0, where the context menu appears immediately but doesn't have 'check spelling'.)

This is far older than the released versions, so there seems to be some configuration difference between Nightly and releases that delayed this bug appearing in release versions until several years later. Given that this change seems to have been made for bug #1026099 and that mentions e10s, perhaps the difference is e10s being on (by default) in Nightly but off until Firefox 49 (I think)? I tried to test with e10s in Firefox 48, but couldn't find a clear preference to turn it on.

(This may also explain why I haven't seen it in my own usage in my normal browsing, because I've forced e10s off in my browser through Firefox 56; only in 57+ has it become mandatory.)

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/mozilla-central/rev/a75897e664dd is included in your regression range which enabled e10s by default, so it sounds like you are right.

To enable e10s when running mozregression you can use the following command (using the command line):

`mozregression --good 2012-01-01 --bad 2014-11-07 --pref browser.tabs.remote.force-enable:true,browser.tabs.remote.autostart:true`

I've reduced your range so you won't have to test builds newer than the regression range above.

Comment 4

[Chris Siebenmann]

The command line options didn't reliably enable e10s, at least not all the time, but after I realized that I checked and enabled it by hand in every Nightly that supported it (as far as I could tell). This bisection narrowed things down to:

    INFO: Last good revision: b85c260821ab (2014-10-03)
    INFO: First bad revision: 229801d17f7e (2014-10-04)
    INFO: Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b85c260821ab&tochange=229801d17f7e

Unfortunately this is the range when bug #1026099 landed and created this code, so it seems to have been broken this way from the very start and no one noticed until now.

(I noticed because I'm hacking an 'Edit in external editor' context menu entry into a WebExtensions addon to duplicate the behavior of the old It's All Text addon. Once you have such a context menu entry, it's natural that the very first interaction with a textarea is to call up the context menu to start your external editor on it, especially when you're trying to test your new context menu entry.)

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Blake, can you take a look at this?

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

The old (non-e10s) code's behavior was to not show any spellchecking items if the spellchecker wasn't initialized when the context menu was being created. It's easy to replicate that here.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1448200 - Handle the creation of the spellchecker more gracefully.

If the user right-clicks on an input field and that click causes us to create
a spellchecker, then it won't be initialized in time to show the context menu.
Handle that case more gracefully by passing that state up to the parent.

Review commit: https://reviewboard.mozilla.org/r/232860/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/232860/

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8964073 [details]
Bug 1448200 - Handle the creation of the spellchecker more gracefully.

https://reviewboard.mozilla.org/r/232860/#review238492

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8fbafbc46da2
Handle the creation of the spellchecker more gracefully. r=Felipe

Comment 10

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/8fbafbc46da2

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider backporting to 60 ahead of the next ESR or should it ride the 61 train to release?

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #11)
> Is this something we should consider backporting to 60 ahead of the next ESR
> or should it ride the 61 train to release?

I guess it can't hurt to ask.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8964073 [details]
Bug 1448200 - Handle the creation of the spellchecker more gracefully.

Approval Request Comment
[Feature/Bug causing the regression]: e10s
[User impact if declined]: Right clicking on spell-checkable textareas that have never been focused before will fail to show the context menu (the second right click will show the context menu).
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: n/a
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: No, very low risk.
[Why is the change risky/not risky?]: In cases where the added code would trigger, we would have thrown an exception anyway.
[String changes made/needed]:

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8964073 [details]
Bug 1448200 - Handle the creation of the spellchecker more gracefully.

Good idea to fix this minor regression before the next major ESR.

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/c12a2478a5c0