1005601 - "gContextMenu is null" thrown for first textarea context menu in a window

Status: VERIFIED FIXED

(Firefox :: Menus, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

STR:

1) Open a new window
2) Focus the location bar
3) Type "lorem.de" and go
4) Right-click the textarea

Result: http://i.imgur.com/qkXU1rF.png

This is indeed a regression but not a recent one:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=204de5b7e0a6&tochange=dc8e78ed8c44

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Could reproduce this on Mac and Linux. Didn't try Windows.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Another error before "gContextMenu is null" is "spellchecker is null". I suspect bug 856270 to be the cause? Building to confirm.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

This call is failing with "spellchecker is null" and makes the context menu fail as well:

https://hg.mozilla.org/mozilla-central/annotate/3285e030d9c0/toolkit/modules/InlineSpellChecker.jsm#l157

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

I have no clue about the spellchecker code so I won't dive into this and just put it into the backlog :)

Comment 6

[:Gijs (he/him)]

Attachment: nullcheck spellchecker on first use,

This happens because we get initialized and ask the editor for the inline spell checker ( http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/InlineSpellChecker.jsm#21 ) which calls http://mxr.mozilla.org/mozilla-central/source/extensions/spellcheck/src/mozInlineSpellChecker.cpp#567 - which doesn't create the spellchecker. That only happens when http://mxr.mozilla.org/mozilla-central/source/extensions/spellcheck/src/mozInlineSpellChecker.cpp#795 is called, which as best I can tell is async from the call to http://mxr.mozilla.org/mozilla-central/source/extensions/spellcheck/src/mozInlineSpellChecker.cpp#742 , which originates somewhere in editor-land, outside our control (never mind being able to wait for the async call to complete). I think we should just be nullchecking here.

Comment 7

[Marco Mucci [:MarcoM]]

Added to Iteration 33.2

Comment 8

[:Gijs (he/him)]

Note that this leaves the language list empty, while keeping the submenu for it. That's already what happens if mInlineSpellChecker is null, but perhaps we should just hide the languages dropdown in that case? :-\

Comment 9

[Drew Willcoxon :adw]

I think this is a variant of bug 887010.  With the patch I posted there applied, after the steps in comment 0 the menu looks normalish, it just has Check Spelling unchecked if you open it before the async init has finished.  I don't think it's as simple as adding one null check, and I think I addressed that possibility in the other bug, but let me think about it.

Comment 10

[Drew Willcoxon :adw]

Attachment: update InlineSpellChecker.canSpellCheck

Neil, this borrows from my patch in bug 887010.  There, I added the disjunction to the enabled getter, but it belongs in canSpellCheck, which "returns false if there should be no spellchecking UI enabled at all," which is what we want when the spell checker is doing its initial check.

It's not necessary to take the entire patch in bug 887010 because as was the case with Thunderbird, Firefox doesn't actually need to know when the checker becomes usable because it simply rebuilds the relevant UI, the context menu, on demand.  So with this patch, when you follow the steps in comment 0, canSpellCheck is false, so nsContextMenu.js doesn't add any spell check items except Add Dictionaries.  When you then right-click a second time, canSpellCheck is true (assuming enough time has passed to allow the checker to finish), so nsContextMenu.js adds all the spell check items.

Given spell check's async nature, I think that's the best UX we can do in the specific case here of focusing a textbox by right-clicking it.  And I think it's much better to update InlineSpellChecker.jsm than to make all consumers do the check.

Gijs, sorry to steal your bug.

Relevant tests (that I remember) pass locally.

https://tbpl.mozilla.org/?tree=Try&rev=6e4f6435008f

Comment 11

[:Gijs (he/him)]

(In reply to Drew Willcoxon :adw from comment #10)
> Gijs, sorry to steal your bug.

No need to apologize! Here, you can have it. ;-)

(Marco, can you update the spreadsheet as well? Thanks!)

Comment 12

[Marco Mucci [:MarcoM]]

Updated to Drew.

Comment 13

[Drew Willcoxon :adw]

test_contextmenu_input.html failed on try.  I'm still looking at it, but I'm pretty it indicates a problem with the test, not the patch, so I'll leave the r? on the patch for now.

At first I didn't understand why that test is not currently failing with the same errors reported in this bug -- but it is!  The same errors are thrown three times during the test, but it still manages to pass, for example: https://tbpl.mozilla.org/php/getParsedLog.php?id=42979227&tree=Fx-Team&full=1

Comment 14

[[e-mail]]

Looks like this would break Thunderbird/SeaMonkey though, because they don't expect canSpellCheck to change, e.g. http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#4612

Comment 15

[Drew Willcoxon :adw]

Hmm.  What do you think we should do then?  Add an InlineSpellChecker.isInitialSpellCheckPending getter or something?  canReallySpellCheckNoReally?

Comment 16

[Drew Willcoxon :adw]

Attachment: InlineSpellChecker.initialSpellCheckPending

This adds InlineSpellChecker.initialSpellCheckPending and makes nsContextMenu factor it into its canSpell check.  It has the same UX in the same case that I described in comment 10.  If you disagree that it's a better UX than what happens now -- a weirdly empty dictionaries menu whose first item is a separator -- then let's just take Gijs's patch.

By the way, it looks like comm-central may be susceptible to the same bug.  It calls gSpellChecker.addDictionaryListToMenu on the msgComposeContext popup's popupshowing: http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#1080

Comment 17

[Drew Willcoxon :adw]

https://tbpl.mozilla.org/?tree=Try&rev=26f82cbf9154

Comment 18

[Drew Willcoxon :adw]

Attachment: InlineSpellChecker.initialSpellCheckPending, fix Windows failures

test_contextmenu_input.html failed on Windows.  The only difference between this patch and the previous is to stop special-casing Windows in three of the subtests in test_contextmenu_input.html.  (The test is disabled on Linux.)

https://tbpl.mozilla.org/?tree=Try&rev=4381b542cab1

Comment 19

[:Gijs (he/him)]

Comment on attachment 8452428 [details] [diff] [review]
InlineSpellChecker.initialSpellCheckPending, fix Windows failures

Review of attachment 8452428 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the test & nsContextMenu.js changes, although there's a question below. I'm assuming you're waiting for Neil to review the toolkit changes...

::: browser/base/content/test/general/test_contextmenu.html
@@ +82,5 @@
>                      "context-inspect", true];
>    }
>  
> +  var tests = [
> +    function () {

Was this switch necessary here? I half expected you to use task.jsm with all the functions, but you haven't, so I'm altogether not entirely sure why you switched the framework of the test.

Comment 20

[Drew Willcoxon :adw]

(In reply to :Gijs Kruitbosch from comment #19)
> I'm assuming you're waiting for Neil to review the toolkit changes...

Well, I'd prefer and respect Neil's review on InlineSpellChecker.jsm, but he's sometimes been a little slow to review my patches in the past, which is why I flagged someone else, too.  So if you're comfortable reviewing the InlineSpellChecker.jsm change too, please do.  Otherwise I'll wait for Neil.

> ::: browser/base/content/test/general/test_contextmenu.html
> @@ +82,5 @@
> >                      "context-inspect", true];
> >    }
> >  
> > +  var tests = [
> > +    function () {
> 
> Was this switch necessary here? I half expected you to use task.jsm with all
> the functions, but you haven't, so I'm altogether not entirely sure why you
> switched the framework of the test.

I needed to add a subtest in the middle of that giant `switch` statement, to test not waiting for the spell check init to complete.  I could have added a new `case` and updated all the subsequent cases, but I didn't like that idea and how unnecessarily hard it is to change this test.  A giant switch is just bad.  At the same time I didn't want to rewrite the test.  I can definitely go back to the switch if you'd prefer, no big deal.

Comment 21

[:Gijs (he/him)]

Comment on attachment 8452428 [details] [diff] [review]
InlineSpellChecker.initialSpellCheckPending, fix Windows failures

Review of attachment 8452428 [details] [diff] [review]:
-----------------------------------------------------------------

Alright, LGTM! :-)

::: browser/base/content/test/general/contextmenu_common.js
@@ +21,2 @@
>        onSpellCheck(element, actuallyOpenContextMenuFor);
> +    }

Nit, in the clear light of morning: braces around the if block means braces around the else block, IMO. :-)

::: browser/base/content/test/general/test_contextmenu.html
@@ +82,5 @@
>                      "context-inspect", true];
>    }
>  
> +  var tests = [
> +    function () {

Thanks for the explanation. Yes, let's switch to functions. :-)

Comment 22

[:Gijs (he/him)]

(don't forget to update the commit message; it still has the try syntax in it)

Comment 23

[Drew Willcoxon :adw]

Thanks, Gijs.

https://hg.mozilla.org/integration/fx-team/rev/8b0a999a23f9

Comment 24

[[e-mail]]

(In reply to Drew Willcoxon from comment #16)
> By the way, it looks like comm-central may be susceptible to the same bug. 
> It calls gSpellChecker.addDictionaryListToMenu on the msgComposeContext
> popup's popupshowing:
> http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/
> MsgComposeCommands.js#1080

I thought that the Firefox bug was due to the spellcheck being initialised by the context menu, or have I misunderstood the bug?

(In reply to Drew Willcoxon from comment #20)
> Well, I'd prefer and respect Neil's review on InlineSpellChecker.jsm, but
> he's sometimes been a little slow to review my patches in the past

Not just yours, I just can't find the time to do as many reviews as I'd like.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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

Comment 26

[Drew Willcoxon :adw]

(In reply to [e-mail] from comment #24)
> I thought that the Firefox bug was due to the spellcheck being initialised
> by the context menu, or have I misunderstood the bug?

Well, the bug is that Firefox doesn't wait for initialization to finish before calling addDictionaryListToMenu.  Initialization happens when the editor is focused, and in this bug, the editor is focused by right-clicking, which also happens to bring up the context menu, which calls addDictionaryListToMenu.

If it's possible that initialization is not finished by the time the line in comment 16 is reached, then Thunderbird is susceptible, even if there's rarely or never a problem in practice.  I don't know if that's the case which is why I said "may be susceptible," but I do remember that it checks for mInlineSpellChecker.spellCheckPending itself at some point, so maybe it's fine.  Sorry to worry you if so.

Comment 27

[Marco Mucci [:MarcoM]]

Hi Florin, can a QA contact be assigned for verification of this bug.

Comment 28

[Cornel Ionce [:noni] [Hubs QA]]

User Agents:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0
Mozilla/5.0 (X11; Linux i686; rv:33.0) Gecko/20100101 Firefox/33.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:33.0) Gecko/20100101 Firefox/33.0

Reproduced this issue on 2014.07.10 Nightly.

Verified as fixed using latest Nightly, build ID: 20140713030204 on Windows 7 64bit, Ubuntu 14.04 and Mac OS X 10.9.3.

Comment 29

[erosman]

I have also noticed this error since the upgrade to FF31.0 (Win7).
I Have not fully investigated it to be able to post reproduce method, yet.
I noticed it on clicking facebook images (FB runs many scripts on their images which may have caused the issue)
Once the error is logged, it continues to fail until FF is restarted as if the module that produces the gContextMenu has crashed. Everything goes back to normal after a FF restart.

Comment 30

[erosman]

Previous "gContextMenu becomes null" Bug 632364

Comment 31

[erosman]

Additional Info: I also noticed after opening a new window and activate the contextmenu, then move back to the previous window and activate the context menu, it fails with "window.gContextMenu is null"

Comment 32

[:Gijs (he/him)]

(In reply to erosman from comment #31)
> Additional Info: I also noticed after opening a new window and activate the
> contextmenu, then move back to the previous window and activate the context
> menu, it fails with "window.gContextMenu is null"

This sounds like a very different bug. Please file a new bug with detailed steps to reproduce on a clean, new profile. Thank you!