1246296 - Fix spell checking subtests in browser_contextmenu.js

Status: NEW

(Firefox :: Menus, task, P3)

Description

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

Bug 1243838 is rewriting test_contextmenu.html to browser_contextmenu.js, but the spell checking menus aren't testing correctly.

This bug is a follow-up to get those working.

Comment 1

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

Note, I think the issue here is with the timing of the remote spell checker.

Comment 2

[(no longer active)]

I ended up fixing these when working on bug 908570.

Comment 3

[(no longer active)]

Attachment: Fix spell checking subtests in browser_contextmenu.js

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d40e7403de46
Fix spell checking subtests in browser_contextmenu.js; r=dao

Comment 5

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

Backed bug 908570 and bug 1246296 for failing the modified test browser_contextmenu.js on OSX and Windows (in non-e10s mode):

https://hg.mozilla.org/integration/mozilla-inbound/rev/a7281f6c576a8980975037278f51b11f5eaba15a
https://hg.mozilla.org/integration/mozilla-inbound/rev/69974770e8a63a412561f0490622c6717f749194

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=6b3dc11025136983d14bff43dc3b483fc25832d9
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=37980591&repo=mozilla-inbound

08:49:14     INFO -  107 INFO TEST-PASS | browser/base/content/test/general/browser_contextmenu.js | checking item #6 (---) name -
08:49:14     INFO -  108 INFO TEST-PASS | browser/base/content/test/general/browser_contextmenu.js | checking item #7 (context-selectall) name -
08:49:14     INFO -  109 INFO TEST-PASS | browser/base/content/test/general/browser_contextmenu.js | checking item #7 (context-selectall) enabled state -
08:49:14     INFO -  110 INFO TEST-PASS | browser/base/content/test/general/browser_contextmenu.js | checking item #8 (---) name -
08:49:14     INFO -  111 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_contextmenu.js | checking item #9 (spell-check-enabled) name - Got context-inspect, expected spell-check-enabled
08:49:14     INFO -  Stack trace:
08:49:14     INFO -      chrome://mochikit/content/browser-test.js:test_is:909
08:49:14     INFO -      chrome://mochitests/content/browser/browser/base/content/test/general/contextmenu_common.js:checkMenuItem:138
08:49:14     INFO -      chrome://mochitests/content/browser/browser/base/content/test/general/contextmenu_common.js:checkMenu:215
08:49:14     INFO -      chrome://mochitests/content/browser/browser/base/content/test/general/contextmenu_common.js:checkContextMenu:134
08:49:14     INFO -      chrome://mochitests/content/browser/browser/base/content/test/general/contextmenu_common.js:test_contextmenu:312
08:49:14     INFO -      Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:937:23

Comment 6

[(no longer active)]

I don't know what's up with the previous landing of this patch.  I pushed it to try and it's fully green sans intermittent oranges: https://treeherder.mozilla.org/#/jobs?repo=try&revision=407ec5e34fb9d6dbf73e91e31538f73109136423

I'll reland.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5118162c1a82
Fix spell checking subtests in browser_contextmenu.js; r=dao

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Still failing. Note that this test runs in the mochitest-clipboard suite, not the regular mochitest-bc like your Try push ran.

Comment 9

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a98b5d3a6c0
Backed out changeset 5118162c1a82 for browser_contextmenu.js failures on a CLOSED TREE.

Comment 10

[(no longer active)]

OK, I finally understand what's happening here.

This failure is due to the presence of InlineSpellCheckerUI.initialSpellCheckPending here <http://searchfox.org/mozilla-central/rev/3079f45ce59e105e2490b1c8b3a5a52cd47a5ac0/browser/base/content/nsContextMenu.js#369> added in bug 1005601.

Comment 11

[(no longer active)]

Ugh, pressed Send too early.

In the cases where the test passes, initialSpellCheckPending is false, so the nsContextMenu.js code adds the spell checking menu items which we expect it to, and the test passes.  But in some cases (such as our test machines for some reason, as I never managed to reproduce this locally) when we get to this code when running the test, initialSpellCheckPending is true, so the nsContextMenu code doesn't add any spell checking entries to the menu and the test fails.

Reading bug 1005601, it seems like this setup is intentional.  We should probably redo the test to expect this to happen...

Comment 12

[(no longer active)]

I've been debugging this on try, but it doesn't look like I have any chance to get this done before I leave.  Need to look at it again when I come back.

Comment 13

[:Gijs (out for now; he/him)]

We should probably try re-enabling these, they're still disabled (cf. https://searchfox.org/mozilla-central/rev/70b32246fce5ca1f53af573a21c1939df58cb969/browser/base/content/test/contextMenu/browser_contextmenu.js#1031-1058 ).

Comment 14

[:Gijs (out for now; he/him)]

Dan, is there any chance you're able to help get these tests back to running, with your context from the multi-lingual spellcheck work? AFAICT they currently get stuck trying to wait for spellcheck to be ready. I'm also, in practice, seeing no suggestions the first time I open the context menu on an input that has misspellings - the second time, the suggestions have been populated.

Comment 15

[:Gijs (out for now; he/him)]

(I came across this issue again in bug 923514)

Comment 16

[Dan Minor [:dminor]]

I'm afraid I don't have the time or much context to dig into this right now, my multi-lingual spellchecking work was mostly on the C++ side. This is at least partly using functions in AsyncSpellCheckTestHelper.jsm that caused problems for me with other mochitests. I changed the onSpellCheck function to block indefinitely waiting for spellchecking [1], the old behaviour was to spin the event loop an arbitrary number of times waiting for spellchecking to begin, this is now called maybeOnSpellCheck [2]. We have problems with intermittent failures in other tests as well, I think cleaning up these helper functions would be a good step forward, I tried to do that in Bug 1402822, but with limited success.

[1] https://searchfox.org/mozilla-central/rev/6ec440e105c2b75d5cae9d34f957a2f85a106d54/editor/AsyncSpellCheckTestHelper.jsm#106
[2] https://searchfox.org/mozilla-central/rev/6ec440e105c2b75d5cae9d34f957a2f85a106d54/editor/AsyncSpellCheckTestHelper.jsm#35