Closed Bug 1005601 Opened 6 years ago Closed 5 years ago

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

Categories

(Firefox :: Menus, defect)

defect
Not set
Points:
5

Tracking

()

VERIFIED FIXED
Firefox 33
Iteration:
33.3
Tracking Status
firefox29 --- wontfix
firefox30 --- affected
firefox31 --- affected
firefox32 --- affected

People

(Reporter: ttaubert, Assigned: adw)

References

(Depends on 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

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
Could reproduce this on Mac and Linux. Didn't try Windows.
Another error before "gContextMenu is null" is "spellchecker is null". I suspect bug 856270 to be the cause? Building to confirm.
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
Blocks: 856270
I have no clue about the spellchecker code so I won't dive into this and just put it into the backlog :)
Flags: firefox-backlog+
Component: General → Menus
Whiteboard: p=5
Duplicate of this bug: 965676
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.
Attachment #8449422 - Flags: review?(adw)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Added to Iteration 33.2
Iteration: --- → 33.2
Points: --- → 5
QA Whiteboard: [qa?]
Whiteboard: p=5
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? :-\
QA Whiteboard: [qa?] → [qa+]
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.
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
Attachment #8449666 - Flags: review?(neil)
Attachment #8449422 - Flags: review?(adw)
(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!)
Assignee: gijskruitbosch+bugs → adw
Flags: needinfo?(mmucci)
Updated to Drew.
Flags: needinfo?(mmucci)
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
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
Hmm.  What do you think we should do then?  Add an InlineSpellChecker.isInitialSpellCheckPending getter or something?  canReallySpellCheckNoReally?
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
Attachment #8449666 - Attachment is obsolete: true
Attachment #8449666 - Flags: review?(neil)
Attachment #8450706 - Flags: review?(neil)
Iteration: 33.2 → 33.3
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
Attachment #8450706 - Attachment is obsolete: true
Attachment #8450706 - Flags: review?(neil)
Attachment #8452428 - Flags: review?(neil)
Attachment #8452428 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8452428 - Flags: review?(gijskruitbosch+bugs) → review+
(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 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. :-)
Attachment #8452428 - Flags: review?(neil)
(don't forget to update the commit message; it still has the try syntax in it)
(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.
https://hg.mozilla.org/mozilla-central/rev/8b0a999a23f9
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
(In reply to neil@parkwaycc.co.uk 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.
Hi Florin, can a QA contact be assigned for verification of this bug.
Flags: needinfo?(florin.mezei)
Flags: needinfo?(florin.mezei)
QA Contact: cornel.ionce
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.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa+] → [qa!]
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.
Previous "gContextMenu becomes null" Bug 632364
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"
(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!
See Also: → 1122906
Blocks: 1122906
Depends on: 1246296
You need to log in before you can comment on or make changes to this bug.