Closed Bug 1209414 Opened 9 years ago Closed 9 years ago

Implement test that simulates user right-click to select a different dictionary.

Categories

(Core :: Spelling checker, defect)

Product:
Core
Component:
Spelling checker
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla44
Tracking Flags:
Tracking Status
firefox44 --- fixed

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

Details

Attachments

(1 file, 8 obsolete files)

New test (v2b).
5.70 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review 
We need a decent test that
- simulates a user right-click to select a different dictionary and
- checks that the spell checking was updated.

All the tests in editor/composer/test and extensions/spellcheck/tests set the dictionary programmatically using SetCurrentDictionary(), and that does not sufficiently reflect the manual usage of Firefox.

(As a sample for simulating a right click we have test_bug1170484.html with helper_bug1170484.js.)
QA Contact: mozilla
Attached patch New test (v1) (obsolete) — — Splinter Review 
This should be it. Simple: context-click, Languages, English, done.
(I didn't follow the sample of bug 1170484 in the end).

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=51a025fe9134
Attached patch New test (v1a). (obsolete) — — Splinter Review 
Removed unintentional additional white-space.
Attachment #8667911 - Attachment is obsolete: true
Assignee: nobody → mozilla
QA Contact: mozilla
I forgot to say: With this test, bug 1208680 wouldn't have regressed.
Hmm, Linux x64 debug didn't like the simulated keystrokes. Trying again:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=de4afa75f569
Hmm, I fired up my Linux box, built Firefox and the test *does* work. So again, it's a problem of running the tests in bulk. Some test before will leave something unexpected behind.

On other platforms the test sometimes works, sometimes fails:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e770d5b77e22

Here's another test run with debug turned on:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d71734a37a4f
Flags: needinfo?(bugs)
Sorry Ehsan, I have to trouble you again. I've written this basic test, attachment 8668134 [details] [diff] [review], which displays a textarea with German/English content, initial spell checking in German. Then it does a context click, types "L" for "Language", followed by <enter> to select the first choice, English.

This test works fine locally (in a debug build) on Windows 7 and Linux (Mint 17.1). It works fine on the try servers in "opt" mode on Linux and Windows XP, 7 and 8. It fails on all debug builds and also Mac OS opt. For try runs see comment #8.

My conclusion is, that there is nothing wrong with the test, it has do do with how it's run on the try machines.

At the end I submitted a run with "DEBUG_DICT" in nsEditorSpellCheck.cpp turned on.

A good "opt" run shows this:
2561 INFO TEST-START | editor/composer/test/test_bug1209414.html
***** mPreferredLang (element) |de-DE|
***** Assigned from element/doc |de-DE|
***** Set |de-DE|.
***** mPreferredLang (element) |de-DE|
***** Assigned from element/doc |de-DE|
***** Set |de-DE|. <-- First load of the page sets initial dictionary German.
                   <-- Context menu is displayed, L, <enter>, English is selected.
***** Writing content preferences for |en-US| <-- Manual selection of language causes
***** Storing spellchecker.dictionary |en-US| <-- content preferences to be written.
                   <-- Spelling gets updated.
***** Clearing content preferences for || <-- Test does some clean-up.
***** Storing spellchecker.dictionary ||
MEMORY STAT | vsize 1018MB | residentFast 208MB | heapAllocated 98MB

A bad debug *try* run shows this:
2133 INFO TEST-START | editor/composer/test/test_bug1209414.html
++DOMWINDOW == 145 (0x7f652074e800) [pid = 1947] [serial = 4309] [outer = 0x7f6524978400]
***** mPreferredLang (element) |de-DE|
***** Assigned from element/doc |de-DE|
***** Set |de-DE|.
***** mPreferredLang (element) |de-DE|
***** Assigned from element/doc |de-DE|
***** Set |de-DE|. <-- First load of the page sets initial dictionary German.
TEST-INFO | started process screentopng <-- some funny thing happens, looks like some screen shot starts.
TEST-INFO | screentopng: exit 0
2134 INFO must wait for load
2135 INFO TEST-PASS | editor/composer/test/test_bug1209414.html | true expected (de_DE directory should exist)
2136 INFO TEST-PASS | editor/composer/test/test_bug1209414.html | expected de-DE
2137 INFO TEST-PASS | editor/composer/test/test_bug1209414.html | some misspelled words expected: today is a good day
                             <-- Looks like the typing of "L" and <enter> goes into
                                 the wrong window.
                             <-- English is never selected, so the subsequent test fail.
2138 INFO TEST-UNEXPECTED-FAIL | editor/composer/test/test_bug1209414.html | expected en-US - got "de-DE", expected "en-US"
SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:267:5
@chrome://mochitests/content/chrome/editor/composer/test/test_bug1209414.html:88:11
tick@resource://gre/modules/AsyncSpellCheckTestHelper.jsm:84:5
Not taking screenshot here: see the one that was previously logged
2139 INFO TEST-UNEXPECTED-FAIL | editor/composer/test/test_bug1209414.html | some misspelled words expected: heute ist ein guter - got "Ltodayisagoodday", expected "heuteisteinguter"
             ^^ <-- Look, "L" got typed into the textarea, not the context menu.
SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:267:5
@chrome://mochitests/content/chrome/editor/composer/test/test_bug1209414.html:89:1
tick@resource://gre/modules/AsyncSpellCheckTestHelper.jsm:84:5
***** Clearing content preferences for ||
***** Storing spellchecker.dictionary ||

A good debug *local* run shows:
0 INFO SimpleTest START
1 INFO TEST-START | editor/composer/test/test_bug1209414.html
2 INFO must wait for load
3 INFO TEST-PASS | editor/composer/test/test_bug1209414.html | true expected (de_DE directory sh ...
***** mPreferredLang (element) |de-DE|
***** Assigned from element/doc |de-DE|
***** Set |de-DE|.
4 INFO TEST-PASS | editor/composer/test/test_bug1209414.html | expected de-DE
5 INFO TEST-PASS | editor/composer/test/test_bug1209414.html | some misspelled words expected: t ...
***** Writing content preferences for |en-US|
***** Storing spellchecker.dictionary |en-US|
6 INFO TEST-PASS | editor/composer/test/test_bug1209414.html | expected en-US
7 INFO TEST-PASS | editor/composer/test/test_bug1209414.html | some misspelled words expected: h ...
***** Clearing content preferences for ||
***** Storing spellchecker.dictionary ||
MEMORY STAT | vsize 670MB | vsizeMaxContiguous 1806MB | residentFast 260MB | heapAllocated 53MB
8 INFO TEST-OK | editor/composer/test/test_bug1209414.html | took 5764ms
9 INFO TEST-START | Shutdown
10 INFO Passed:  5

So what should I do? Is there a way to turn those screenshots off during the test?

I think this test is necessary, since all other tests that deal with dictionaries set the language programatically (using SetCurrentDictionary()), there is not a single one that simulates the user actions. (Had this test existed before, bug 1208680 would not have regressed).
Flags: needinfo?(ehsan)
Mac OS "opt" takes screenshots as well, so that's why that non-debug build fails unlike the other non-debug builds.
Maybe just put:

skip-if = os=="mac" || debug # Snapshots make the test fail since they interfere with the context menu
I found a few test that deal with the context menu, but none types into it:
browser/base/content/test/general/test_contextmenu.html
dom/html/test/file_fullscreen-esc-context-menu.html
(and extensions/spellcheck/tests/mochitest/test_bug1170484.html).

So I'm inclided to just exclude the failing tests from the run and be done with it.
Attached patch New test (v1c). (obsolete) — — Splinter Review 
Same test as before, excluding environments that take screenshots, that is, debug and Mac.
Attachment #8667913 - Attachment is obsolete: true
Attachment #8668134 - Attachment is obsolete: true
Attachment #8669291 - Flags: feedback?(roc)
Attached patch New test (v1d). (obsolete) — — Splinter Review 
(oops, services not needed any more).
Attachment #8669291 - Attachment is obsolete: true
Attachment #8669291 - Flags: feedback?(roc)
Attachment #8669292 - Flags: feedback?(roc)
Hi James, I have a test here that fails on debug builds (although it works locally on debug compilations of Windows 7 and Linux). I believe that the screenshots interfere with the test.

My question is: Why are screenshots enabled on Mas OS X opt? There my test fails, too. See:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e770d5b77e22 and
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mozilla@jorgk.com-e770d5b77e22/try-macosx64/try_snowleopard_test-mochitest-other-bm106-tests1-macosx-build3234.txt.gz
Look at 05:36:14 - started process screencapture
Flags: needinfo?(james)
I don't really know anything about the reasons that mochitests are set up the way they are.
Flags: needinfo?(james) → needinfo?(ahalberstadt)
Screenshots are enabled on every platform, they only get taken *if* there is a failure. You see the screenshots because the test failed, not the other way around. Your problem is something else.
Flags: needinfo?(ahalberstadt)
Hmm, well, given the variety of failures as documented in comment #12, maybe it's a timing issue. Trying again with yet another setTimeout() added:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=03d4aa316d1c
https://treeherder.mozilla.org/#/jobs?repo=try&revision=03d4aa316d1c
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mozilla@jorgk.com-03d4aa316d1c/try-linux64-debug/try_ubuntu64_vm-debug_test-mochitest-other-bm51-tests1-linux64-build316.txt.gz
still didn't work any better.

(In reply to Andrew Halberstadt [:ahal] from comment #18)
> Your problem is something else.

Any idea what that "something else" could be?

The test claims to fail on Linux debug and Windows 7 debug, yet, locally, they work just fine. I have a Windows 7 and Linux 64 bit debug compile here.

Also, could you say something about the timing. Looking at the debug I see this:

2133 INFO TEST-START | editor/composer/test/test_bug1209414.html
++DOMWINDOW == 145 (0x7f652074e800) [pid = 1947] [serial = 4309] [outer = 0x7f6524978400]
***** mPreferredLang (element) |de-DE|
***** Assigned from element/doc |de-DE|
***** Set |de-DE|.
***** mPreferredLang (element) |de-DE|
***** Assigned from element/doc |de-DE|
***** Set |de-DE|. <-- First load of the page sets initial dictionary German.
TEST-INFO | started process screentopng <-- some funny thing happens, looks like some screen shot starts.
TEST-INFO | screentopng: exit 0
2134 INFO must wait for load
2135 INFO TEST-PASS | editor/composer/test/test_bug1209414.html | true expected (de_DE directory should exist)
2136 INFO TEST-PASS | editor/composer/test/test_bug1209414.html | expected de-DE
2137 INFO TEST-PASS | editor/composer/test/test_bug1209414.html | some misspelled words expected: today is a good day
                             <-- Looks like the typing of "L" and <enter> goes into
                                 the wrong window.
                             <-- English is never selected, so the subsequent test fail.
2138 INFO TEST-UNEXPECTED-FAIL | editor/composer/test/test_bug1209414.html | expected en-US - got "de-DE", expected "en-US"

So the screenshot process starts *before* the first test is even run and way before the fourth test fails.

Also, comparing the screenshots from comment #12, they don't show the same failure. In one case the expected context menu is on the screenshot, in some cases it's not, in one case the "L" that should go into the context menu gets typed into the page instead. That's really weird.

Here is another Mac screenshot
http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/11afbe826d90b37744262309720dd8f7f5128ccf320591ba2396fc4a174f7f9dedac77080e5ecf6369c0d4007684a888a88fbc2ee44c3c3e01611fd869d65385
where the "L" got typed into the main page, here on the other hand, also Mac
http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/b1aa4ecc930ba58fe2295cdd6e4434ddd95f5cc2d66d77f006d3ca8bde36e5649ffe85ebf8d1c4a96280918503863f9dbbb996aefceafd903b378ef0219c6de4
the expected menu shows.

How is the screen shot triggered? By the "test unexpected fail" or some other way? Does an unexpected popup window constitute a failure?

Perhaps you could grab the patch, it's just another test, no code change or build required, and try it on any version of Firefox, preferably on a Mac, since for Mac both "opt" and "debug" fail.

I don't know how to proceed with this. Everything I've tried so far works locally but not on try. I had cases like this before, where other tests had left settings behind, that made subsequent tests fail. But here, this is not the case. It was compiled with some debug and as far as I can see, all is well, only that the context menu doesn't work.
Flags: needinfo?(ahalberstadt)
Comment on attachment 8669292 [details] [diff] [review]
New test (v1d).

Ehsan suggested on IRC to use
document.getElementById("the id of the elem").doCommand()
while the context menu is showing.
Then, for the second level, wait for (another) popupshown event,
example here:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/test_contextmenu.html?force=1
Flags: needinfo?(ehsan)
Flags: needinfo?(ahalberstadt)
Attachment #8669292 - Flags: feedback?(roc)
I'll add that a common source of error is bad interaction with other tests (i.e other tests not cleaning up after themselves). Try running the test in automation by itself to rule out that possibility (see |mach try --help| for how to do this).
OK, here another attempt with an event listener to make sure the popup is really shown:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc6dc8876902
Attachment #8669292 - Attachment is obsolete: true
Did you look at test_contextmenu.html as I said on IRC yesterday?  This part of it more specifically? <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/contextmenu_common.js#3>  You're synthesizing the contextmenu mouse click without passing button: 2.  Looking at how that test does other things will help you fix your test here.
Attached patch New test (v2). (obsolete) — — Splinter Review 
Thanks for your interest.

I enclose the latest patch.
Try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=87bff03885e3

Yes, *of course* I followed *all* you said. I looked at
browser/base/content/test/general/test_contextmenu.html which uses
browser/base/content/test/general/contextmenu_common.js.

The new test here is 100% based on this example and follows your advice to the letter:
1) The context menu is displayed via a simulated context mouse-click,
   now I'm even passing button: 2
2) I added an event listener for "popupshown".
3) In the event handler I select the option I want with
   contextMenu.ownerDocument.getElementById("spell-check-dictionary-en-US").doCommand();
4) I close the context menu again.
Cool, let me know if this one does better on the try server.  :-)
Sadly this debug run times out just like the ones before:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mozilla@jorgk.com-87bff03885e3/try-linux64/try_ubuntu64_vm_test-mochitest-other-bm53-tests1-linux64-build702.txt.gz
Screenshot:
http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/77cb838a7c6e321c57c93dd41e2d141d2fb445de86fc3585e7c52ca460238bb58da0ef6e40aa06312d8e255369c96905a06e447b9b394275bacc30302ef672c1

Output:
2127 INFO TEST-START | editor/composer/test/test_bug1209414.html
++DOMWINDOW == 162 (0x7f359c69cc00) [pid = 2078] [serial = 4310] [outer = 0x7f35a5288400]
***** mPreferredLang (element) |de-DE|
***** Assigned from element/doc |de-DE|
***** Set |de-DE|.
+++++ mozInlineSpellChecker::SpellCheckRange
+++++ mozInlineSpellChecker::SpellCheckRange
***** mPreferredLang (element) |de-DE|
***** Assigned from element/doc |de-DE|
***** Set |de-DE|.
+++++ mozInlineSpellChecker::SpellCheckRange
--DOCSHELL 0x7f3590489500 == 13 [pid = 2078] [id = 1166]
--DOCSHELL 0x7f3593cfc700 == 12 [pid = 2078] [id = 1168]
--DOCSHELL 0x7f359123b100 == 11 [pid = 2078] [id = 1167]
--DOMWINDOW == 161 (0x7f35a547e800) [pid = 2078] [serial = 4188] [outer = (nil)] [url = chrome://mochitests/content/chrome/dom/xul/templates/tests/chrome/test_tmpl_xmlquerywithsort.xul]

[85 more DOM window messages deleted]

TEST-INFO | started process screentopng
TEST-INFO | screentopng: exit 0
2128 INFO must wait for load
2129 INFO TEST-PASS | editor/composer/test/test_bug1209414.html | Got context menu XUL
2130 INFO TEST-PASS | editor/composer/test/test_bug1209414.html | true expected (de_DE directory should exist)
2131 INFO TEST-PASS | editor/composer/test/test_bug1209414.html | expected de-DE
2132 INFO TEST-PASS | editor/composer/test/test_bug1209414.html | some misspelled words expected: today is a good day
2133 INFO TEST-UNEXPECTED-FAIL | editor/composer/test/test_bug1209414.html | Test timed out.
reportError@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:334:7

The fifth test would have been:
function handlePopup() {
  is(contextMenu.state, "open", "checking if popup is open");
but we never get there.

Frankly, I don't know what else to try.

Or maybe, we're barking up the wrong tree. Look what we have here:
[test_contextmenu.html]
skip-if = toolkit == "gtk2" || toolkit == "gtk3" || (os == 'mac' && os_version != '10.6') # disabled on Linux due to bug 513558, on Mac after 10.6 due to bug 792304

So all I could try is running the test on Windows and Mac OS 10.6.

What do you think?
Flags: needinfo?(ehsan)
With the new test, I have consistency across all platforms.
Optimised build works, debug build times out:

TEST-PASS | editor/composer/test/test_bug1209414.html | Got context menu XUL
TEST-PASS | editor/composer/test/test_bug1209414.html | true expected (de_DE directory should exist)
TEST-PASS | editor/composer/test/test_bug1209414.html | expected de-DE
TEST-PASS | editor/composer/test/test_bug1209414.html | some misspelled words expected: today is a good day
TEST-UNEXPECTED-FAIL | editor/composer/test/test_bug1209414.html | Test timed out.

After the forth test, the context menu should show up, but it never shows up, here the relevant code:

is(currentDictonary, "de-DE", "expected de-DE");
is(getMisspelledWords(editor_de), "today" + "is" + "a" + "good" + "day", "some misspelled words expected: today is a good day");
elem_de.focus();
synthesizeMouse(elem_de, 2, 2, { type : "contextmenu", button : 2 }, window);

Before, a listener was added:
contextMenu.addEventListener("popupshown", handlePopup, false);

And in the handler, we have:
function handlePopup() {
  is(contextMenu.state, "open", "checking if popup is open");

But in a debug run, the popup menu never shows up and the fifth test for the open menu is never executed. Here are sample screenshots:
http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/7431e10e12cdad8d2624114cefef50a4c553b2eeca6c3553d3693dbb3300f9f0e35885b93c64b1828030bbca5853494c1841189d042d083d3def09cc55be381b
http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/68c5151732e04a29e033bc6800e4a1887b8a07ba0aa3b5bf84af5c5d6103bf27ca3233c931f67855f394a6080ef32ebae6143a8139be6e93d388811d14b1d360
http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/6e0ce67269619c7555d9eaaf392a92a13fa9ee18b61c23c23f36a8144eff8c28247cd84af5035d93fe09a15315f18dd5853e02bcc9f624c6d703e1b9edd6f647
None shows a popup.

As noted in comment #28, at the point where the popup should show up, we get about 100 "DOMWINDOW" messages (I see 129 of them in one case I looked at in details). So maybe printing this debug takes so much time that the test times out.

So sadly, I don't have any other idea. I will submit the test to be run in non-debug only. Then we can use it like that, or forget about it altogether.
Flags: needinfo?(ehsan)
Attached patch New test (v2a). (obsolete) — — Splinter Review 
Sorry, I can't do any better. This works in non-debug only.
Attachment #8670293 - Attachment is obsolete: true
Attachment #8670650 - Flags: review?(roc)
Andrew, sorry to trouble you again. I cannot get the test to work in debug mode. At the point where test should display a popup window, it times out after displaying about 50-150 (upto 151) "DOMWINDOW" messages. If you want to take a look:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mozilla@jorgk.com-ff503b17b12a/try-win64-debug/try_win8_64-debug_test-mochitest-other-bm111-tests1-windows-build1622.txt.gz

Search for test_bug1209414.html.

I have suggested to run the test in non-debug builds only.
Flags: needinfo?(ahalberstadt)
Sorry, I'm more of a tools and infrastructure person. Probably not the guy you want for looking into js/gecko problems.

If you're running locally with a debug build, you can run |mach mochitest editor/composer/test/test_bug1209414.html --jsdebugger| to pop open the built-in debugger. You can set breakpoints then click the start button to start your test.
Flags: needinfo?(ahalberstadt)
(In reply to Jorg K (GMT+2) from comment #30)
> With the new test, I have consistency across all platforms.
> Optimised build works, debug build times out:

The most important difference between debug and opt builds is that debug builds are slower, which suggests that your test is relying on some race condition that happens to work out when the build is faster.  Do you see the error locally if you run all of the editor/composer tests on a debug build?  What if you make the machine artificially slow if you run something expensive in the background (such as building Firefox)?

> TEST-PASS | editor/composer/test/test_bug1209414.html | Got context menu XUL
> TEST-PASS | editor/composer/test/test_bug1209414.html | true expected (de_DE
> directory should exist)
> TEST-PASS | editor/composer/test/test_bug1209414.html | expected de-DE
> TEST-PASS | editor/composer/test/test_bug1209414.html | some misspelled
> words expected: today is a good day
> TEST-UNEXPECTED-FAIL | editor/composer/test/test_bug1209414.html | Test
> timed out.

You're seeing your test output log at the very end which is not really helpful.  To get the output inline with the normal stdout/stderr output, call SimpleTest.requestCompleteLog() early in your test.

> After the forth test, the context menu should show up, but it never shows
> up, here the relevant code:
> 
> is(currentDictonary, "de-DE", "expected de-DE");
> is(getMisspelledWords(editor_de), "today" + "is" + "a" + "good" + "day",
> "some misspelled words expected: today is a good day");
> elem_de.focus();
> synthesizeMouse(elem_de, 2, 2, { type : "contextmenu", button : 2 }, window);

Hmm, the code in your try push is different in that there is no call to .focus() <https://hg.mozilla.org/try/rev/6f13ad3f3114#l3.89>.  Maybe that matters?  (That is the only difference I can see with <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/contextmenu_common.js#3>).

> Before, a listener was added:
> contextMenu.addEventListener("popupshown", handlePopup, false);
> 
> And in the handler, we have:
> function handlePopup() {
>   is(contextMenu.state, "open", "checking if popup is open");
> 
> But in a debug run, the popup menu never shows up and the fifth test for the
> open menu is never executed. Here are sample screenshots:
> http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/
> 7431e10e12cdad8d2624114cefef50a4c553b2eeca6c3553d3693dbb3300f9f0e35885b93c64b
> 1828030bbca5853494c1841189d042d083d3def09cc55be381b
> http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/
> 68c5151732e04a29e033bc6800e4a1887b8a07ba0aa3b5bf84af5c5d6103bf27ca3233c931f67
> 855f394a6080ef32ebae6143a8139be6e93d388811d14b1d360
> http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/
> 6e0ce67269619c7555d9eaaf392a92a13fa9ee18b61c23c23f36a8144eff8c28247cd84af5035
> d93fe09a15315f18dd5853e02bcc9f624c6d703e1b9edd6f647
> None shows a popup.
> 
> As noted in comment #28, at the point where the popup should show up, we get
> about 100 "DOMWINDOW" messages (I see 129 of them in one case I looked at in
> details). So maybe printing this debug takes so much time that the test
> times out.

No, that's because you're getting the test output out of order.  What's happening here is that the handlePopup() function never gets called so the test just sits there and waits, and then we do some GC/CC which shows you those DOMWINDOW lines, indicating that we're destroying window objects from previous tests.  That part is normal.

> So sadly, I don't have any other idea. I will submit the test to be run in
> non-debug only. Then we can use it like that, or forget about it altogether.

Running the test in non-debug only is not a great choice.  How about trying to add the .focus() call?  If that doesn't work, the other option is to forget about opening a context menu and just do what clicking the context menu entry ultimately does, which is calling setCurrentDictionary() and spellCheckRange()? <http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/InlineSpellChecker.jsm#340>
Thanks, I did that just to see what happens. On the local build however, there is nothing to debug, since the test works as expected, like on *all* platforms with "opt" compilation.

Frankly, speaking like a layman, if I program the computer to popup a menu, and it just doesn't do it, or it's delayed so much that some other part of the system decides that it has timed out, there is little I can do.

I repeat: The program attaches an event listener for "popupshown", then simulates a right click, but the event listener is never triggered, so for all intents and purposes, the popup is never displayed, so the underlying framework/toolkit/library is not working.

My suggestion is to accept the test as it is and run it on "opt" builds, or abandon the idea of having a test at all, so we won't notice if spell checking breaks one day.

There are many tests in the system that only run on selected platforms or don't run in debug mode. As I said before, this test here would have avoided the regression in bug 1208680, so IMHO it's better to have it than not to have it at all.
Just double checking, but are you using a debug build locally (i.e 'ac_add_options --enable-debug' in your mozconfig)?

Adding it to opt only is a bad idea for two reasons:
1. If there is a timing issue in the test, it might just be luck that it's passing in opt and enabling it there could mean introducing a new intermittent failure.

2. Even if the failure isn't specifically the test code's fault, that means there is a real bug somewhere in the platform. Landing and calling it a day means the problem will get ignored.
(In reply to Ehsan Akhgari (don't ask for review please) from comment #34)
> Do you see the
> error locally if you run all of the editor/composer tests on a debug build? 
> What if you make the machine artificially slow if you run something
> expensive in the background (such as building Firefox)?
I haven't tried that.

> You're seeing your test output log at the very end which is not really
> helpful.  To get the output inline with the normal stdout/stderr output,
> call SimpleTest.requestCompleteLog() early in your test.
I can try that.

> Hmm, the code in your try push is different in that there is no call to
> .focus() <https://hg.mozilla.org/try/rev/6f13ad3f3114#l3.89>.  Maybe that
> matters?  (That is the only difference I can see with
> <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/
> general/contextmenu_common.js#3>).
Thanks for looking into it in such detail.
I noticed the missing focus call, too, so I added it:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8d81a1f755f
https://hg.mozilla.org/try/rev/6a7762b57c0c
It made no difference. (Frankly, why should I focus, when I right click onto it straight after.)

> Running the test in non-debug only is not a great choice.
I have no other choice. As far as I am concerned, the underlying framework just doesn't work. I say: context-click and nothing happens, why would that be my problem?

> How about trying to add the .focus() call?
Has already happened.

> If that doesn't work, the other option is to
> forget about opening a context menu and just do what clicking the context
> menu entry ultimately does, which is calling setCurrentDictionary() ...
We have plenty of tests like this already. I had to modify some them, since now that we killed the crazy listeners, the spell check doesn't update until you blur and focus again.

I repeat one more time: This test tries to simulate real user action. I would have preferred a right-click, type "L" (Languages), type "E" (English) or <return> to accept the first choice. That doesn't work, so I've already watered it down to grabbing the context menu and doing a doCommand(). If I water it down more, I don't need the test at all.

Sorry to sound frustrated, but I am. I've never made so many try submissions for any bug I have worked on.

I will do one last submission, this time with SimpleTest.requestCompleteLog(), as you suggested.
(In reply to Andrew Halberstadt [:ahal] from comment #36)
> Just double checking, but are you using a debug build locally (i.e
> 'ac_add_options --enable-debug' in your mozconfig)?
Yes:
ac_add_options --enable-debug
ac_add_options --disable-optimize
mk_add_options AUTOCLOBBER=1
I work in the debugger, so I assume that wouldn't work otherwise.

> Adding it to opt only is a bad idea for two reasons:
> 1. If there is a timing issue in the test, it might just be luck that it's
> passing in opt and enabling it there could mean introducing a new
> intermittent failure.
Agreed. But perhaps then someone with the right skill set could look into it.

> 2. Even if the failure isn't specifically the test code's fault, that means
> there is a real bug somewhere in the platform. Landing and calling it a day
> means the problem will get ignored.
Also agreed. But hey, there are plenty of tests "half"-enabled, and people have called it a day.

With my humble capabilities I can't fix the underlying platform, especially if the server is 10.000 km away somewhere on the west coast of the US and I can't reproduce the problem locally (although Ehsan made some suggestions).

I'll say it again: Had we had this test, bug 1208680 wouldn't have regressed.
Another try with SimpleTest.requestCompleteLog() and focus() (which was there before already on the last Linux try).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=38e1c2e26c1e
https://hg.mozilla.org/try/rev/900689f15fda
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mozilla@jorgk.com-38e1c2e26c1e/try-linux64-debug/try_ubuntu64_vm-debug_test-mochitest-other-bm116-tests1-linux64-build1013.txt.gz

I'm not sure that the debug is significantly different
(this time I didn't take out the front columns):

09:14:51     INFO -  ++DOMWINDOW == 163 (0x7f8c250bd000) [pid = 1963] [serial = 4309] [outer = 0x7f8c37025400]
09:14:51     INFO -  2132 INFO TEST-PASS | editor/composer/test/test_bug1209414.html | Got context menu XUL
09:14:51     INFO -  2133 INFO TEST-PASS | editor/composer/test/test_bug1209414.html | true expected (de_DE directory should exist)
09:14:51     INFO -  ***** mPreferredLang (element) |de-DE|
09:14:51     INFO -  ***** Assigned from element/doc |de-DE|
09:14:51     INFO -  ***** Set |de-DE|.
09:14:51     INFO -  +++++ mozInlineSpellChecker::SpellCheckRange
09:14:51     INFO -  2134 INFO TEST-PASS | editor/composer/test/test_bug1209414.html | expected de-DE
09:14:51     INFO -  2135 INFO TEST-PASS | editor/composer/test/test_bug1209414.html | some misspelled words expected: today is a good day
09:14:51     INFO -  +++++ mozInlineSpellChecker::SpellCheckRange
09:14:51     INFO -  ***** mPreferredLang (element) |de-DE|
09:14:51     INFO -  ***** Assigned from element/doc |de-DE|
09:14:51     INFO -  ***** Set |de-DE|.
09:14:51     INFO -  +++++ mozInlineSpellChecker::SpellCheckRange
[five minutes pass]
09:19:55     INFO -  TEST-INFO | started process screentopng
09:19:57     INFO -  TEST-INFO | screentopng: exit 0
09:19:57     INFO -  2136 INFO must wait for load
09:19:57     INFO -  2137 INFO TEST-UNEXPECTED-FAIL | editor/composer/test/test_bug1209414.html | Test timed out.

Looks like that after the forth test ("some misspelled words expected") another spell check is triggered, perhaps by the focus() or the right click. After that, nothing happens, the popup is never displayed and the listener for "popupshown" is not triggered. Therefore the test times out.

Sadly it's beyond the scope of this bug and beyond my capabilities to repair the underlying framework.

Even if I could reproduce the problem locally (Ehsan made suggestions in comment #34), I wouldn't know how to track it down.
With all due respect, "This is not my problem" is not the right attitude.  I understand that you're frustrated, but please understand that acting all frustrated on a bug doesn't help anyone.  Also I assure you that people don't just go and check in any test that doesn't work in some configurations with the appropriate skip-if annotations to just hide the issues.  That development approach doesn't scale to having hundreds of people working on a multi-million line code base.  :-)

Once you cool off, here are a couple of other ideas:

1. You can ask for a test slave so that you can run this test yourself on the actual test machine, that should let you debug the issue directly.  All you have to do is file a bug similar to bug 1208796.

2. You can also continue printf debugging on the try server (hopefully with more patience since wait times are longer if you post to the try server obviously.)  If you want to do that, I suggest starting by adding some printfs to nsContentUtils::SendMouseEvent() <http://mxr.mozilla.org/mozilla-central/source/dom/base/nsContentUtils.cpp#7805>.  That is the function that will ultimately get called from synthesizeMouse().  Make sure that the function is not returning early before dispatching the event (there are a few early returns there.)

Last but not least, this is a really tricky bug.  I have literally dealt with more than a thousand tests that failed on try but not locally and this one is a tricky one (as you have seen, I have not had many good ideas yet.)  So, don't lose hope, all you need is patience.  :-)
(In reply to Ehsan Akhgari (don't ask for review please) from comment #41)
> With all due respect, "This is not my problem" is not the right attitude.
Hmm. With all due respect, whose problem is it? There are interfaces, functions, Robert mentions "contracts", and there are modules which have an owner. It can't be that an (unpaid) volunteer goes around and fixes everything that's broken on his way. My test calls a defined interface, and that interface simply doesn't work. So I'd refer the matter to the owner of that interface/module.

This is really a bit off-topic, but let me say it anyway:
Since I started submitting patches in March 2015, I've picked-up a few abandoned patches:
1) Bug  730829 - WIP abandoned.
2) Bug 1140105 - patch abandoned.
3) Bug 1141017 - patch abandoned.
4) Bug  967494 - patch abandoned after review.
5) Bug  697981 - patch abandoned after review.
6) Bug  769604 - patch abandoned, I've just picked this up today.
7) Bug  728775 - patch abandoned, I'm considering picking this up one day.
I don't want to name the people who have abandoned their work, but let me just say that if I walked away from this bug here, I'd be in the company of the finest names. I've picked up bugs 1) to 5) in the list above and carried them through to completion/landing.

Realistically this bug is super-very-low priority. It doesn't fix anything, it would just add the icing to the cake of the dictionary clean-up, where I have already landed seven bugs and cleaned up the existing mess (bug 717433, bug 1200533, bug 697981, bug 1193293, bug 1204147, bug 1204540, bug 1205983).

I think I can really use my time more effectively than on this bug by fixing things that affect users.

I appreciate your technical comments and I will consider pursuing option 2 with some more debug prints. But then: Are we fixing code to make a test work?
(In reply to Jorg K (GMT+2) from comment #42)
> (In reply to Ehsan Akhgari (don't ask for review please) from comment #41)
> > With all due respect, "This is not my problem" is not the right attitude.
> Hmm. With all due respect, whose problem is it?

The health of our code base is the shared responsibility of all of the developers who work on it, including you and I.  :-)

> There are interfaces,
> functions, Robert mentions "contracts", and there are modules which have an
> owner. It can't be that an (unpaid) volunteer goes around and fixes
> everything that's broken on his way. My test calls a defined interface, and
> that interface simply doesn't work. So I'd refer the matter to the owner of
> that interface/module.

I understand that this model might be used in other projects, but it's really not how Mozilla works.  We have piles of code and therefore piles of bugs in that code.  We unfortunately don't have people who just take a non-working test and figure out why that's happening (believe me, I wish we did!)

(Also, the paid vs non-paid distinction doesn't really matter, you'd be in the exact same situation as an employee...)

> This is really a bit off-topic, but let me say it anyway:
> Since I started submitting patches in March 2015, I've picked-up a few
> abandoned patches:
> 1) Bug  730829 - WIP abandoned.
> 2) Bug 1140105 - patch abandoned.
> 3) Bug 1141017 - patch abandoned.
> 4) Bug  967494 - patch abandoned after review.
> 5) Bug  697981 - patch abandoned after review.
> 6) Bug  769604 - patch abandoned, I've just picked this up today.
> 7) Bug  728775 - patch abandoned, I'm considering picking this up one day.
> 
> I don't want to name the people who have abandoned their work, but let me
> just say that if I walked away from this bug here, I'd be in the company of
> the finest names. I've picked up bugs 1) to 5) in the list above and carried
> them through to completion/landing.

I know, and I appreciate it!

> Realistically this bug is super-very-low priority. It doesn't fix anything,
> it would just add the icing to the cake of the dictionary clean-up, where I
> have already landed seven bugs and cleaned up the existing mess (bug 717433,
> bug 1200533, bug 697981, bug 1193293, bug 1204147, bug 1204540, bug 1205983).
> 
> I think I can really use my time more effectively than on this bug by fixing
> things that affect users.

OK, that's fair.  But tests _do_ have a lot of value for our users.  I've been around since the days where we had no tests, and life is a lot better now because of the sheer number of tests that we have.  Spell checker is special in that it suffers from having too few tests.  Every single test matters in the long run.  :-)

> I appreciate your technical comments and I will consider pursuing option 2
> with some more debug prints. But then: Are we fixing code to make a test
> work?

Yes, we do that all the time.  :-)
(In reply to Ehsan Akhgari (don't ask for review please) from comment #43)
> But tests _do_ have a lot of value for our users.

Indeed, therefore: Why don't we switch from purism to pragmatism.

The test works in "opt" mode, if fails for unknown reasons in "debug" mode.

What's wrong with adding it now "as is" and keeping an eye on it. If it starts failing in "opt", it can easily be pulled out again. No trains, no nothing. Switch if off and done. If not, it brings a benefit, until someone comes along (or even myself) and figures out how to make it work in "debug" mode.

I repeat, there are many tests with are "half"-enabled; they don't run in debug, don't run on certain platforms, just look for "skip-if", you'll find many. Agreed, it's not ideal to start off that way, but given that we invested due care and followed everything that a once-working test does (test_contextmenu.html, now completely disabled on Linux and disabled on some Mac OS X versions), I think it's a fair approach which gives the "best bang for the buck".

I let you and Robert (still r?) decide. In bug 1204147 comment #9 Robert said:
"The test doesn't have to always show the problem on every platform. It just has to sometimes show the problem on at least one platform."

So if I interpret that freely: Some test is better than none.
Just to show my good will, I added debug to nsContentUtils::SendMouseEvent() as suggested in comment #41.

Local run shows:
5 INFO TEST-PASS | editor/composer/test/test_bug1209414.html | expected de-DE
6 INFO TEST-PASS | editor/composer/test/test_bug1209414.html | some misspelled words expected: ...
+++++ mozInlineSpellChecker::SpellCheckRange
----- nsContentUtils::SendMouseEvent - DispatchInputEvent (2)
----- nsContentUtils::SendMouseEvent - normal return

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2577055312cb

Can you please point me to the code that actually pops up the menu and notifies the listeners.
Flags: needinfo?(ehsan)
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mozilla@jorgk.com-2577055312cb/try-linux64-debug/try_ubuntu64_vm-debug_test-mochitest-other-bm122-tests1-linux64-build490.txt.gz

00:56:58     INFO -  2133 INFO TEST-START | editor/composer/test/test_bug1209414.html
00:56:58     INFO -  ++DOMWINDOW == 159 (0x7f7b7fab8400) [pid = 1976] [serial = 4309] [outer = 0x7f7b76e7a000]
00:56:58     INFO -  2134 INFO TEST-PASS | editor/composer/test/test_bug1209414.html | Got context menu XUL
00:56:58     INFO -  2135 INFO TEST-PASS | editor/composer/test/test_bug1209414.html | true expected (de_DE directory should exist)
00:56:58     INFO -  ***** mPreferredLang (element) |de-DE|
00:56:58     INFO -  ***** Assigned from element/doc |de-DE|
00:56:58     INFO -  ***** Set |de-DE|.
00:56:58     INFO -  +++++ mozInlineSpellChecker::SpellCheckRange
00:56:58     INFO -  2136 INFO TEST-PASS | editor/composer/test/test_bug1209414.html | expected de-DE
00:56:58     INFO -  2137 INFO TEST-PASS | editor/composer/test/test_bug1209414.html | some misspelled words expected: today is a good day
00:56:58     INFO -  +++++ mozInlineSpellChecker::SpellCheckRange
00:56:58     INFO -  ----- nsContentUtils::SendMouseEvent - DispatchInputEvent (2)
00:56:58     INFO -  ----- nsContentUtils::SendMouseEvent - normal return
00:56:59     INFO -  ***** mPreferredLang (element) |de-DE|
00:56:59     INFO -  ***** Assigned from element/doc |de-DE|
00:56:59     INFO -  ***** Set |de-DE|.
[five minutes pass]
01:02:22     INFO -  TEST-INFO | started process screentopng
01:02:23     INFO -  TEST-INFO | screentopng: exit 0
01:02:23     INFO -  2138 INFO must wait for load
01:02:23     INFO -  2139 INFO TEST-UNEXPECTED-FAIL | editor/composer/test/test_bug1209414.html | Test timed out.

nsContentUtils::SendMouseEvent works the same as on my local build.

Looks like the
  elem_de.focus();
  synthesizeMouse(elem_de, 2, 2, { type : "contextmenu", button : 2 }, window);
dispatched the event and triggered a re-spell-check in German, only that the menu didn't pop up and the listener wasn't called.

So I repeat my question:

Can you please point me to the code that actually pops up the menu and notifies the listeners.

If I do a try run per day, maybe we'll get to the bottom of this by Christmas ;-)
It occurred to me that the focus() and context-click could get mixed up here.

I'm now wrapping that up more:
    onSpellCheck(elem_de, function () {
      synthesizeMouse(elem_de, 2, 2, { type : "contextmenu", button : 2 }, window);
    });

Local debug now shows:
***** mPreferredLang (element) |de-DE|
***** Assigned from element/doc |de-DE|
***** Set |de-DE|.
+++++ mozInlineSpellChecker::SpellCheckRange
----- nsContentUtils::SendMouseEvent - DispatchInputEvent (2)
----- nsContentUtils::SendMouseEvent - normal return
7 INFO TEST-PASS | editor/composer/test/test_bug1209414.html | checking if popup is open

so the asynchronous spell checking action is done before the mouse event is dispatched.

Haha, for the umtieth try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6701837e8f0

Ehsan, should this work, you don't have to answer the question "point me to the code that actually pops up the menu and notifies the listeners".

My physics teacher used to say when an experiment failed and he tried again: "New game, new luck" ;-)
Nope:

06:15:35     INFO -  2133 INFO TEST-PASS | editor/composer/test/test_bug1209414.html | expected de-DE
06:15:35     INFO -  2134 INFO TEST-PASS | editor/composer/test/test_bug1209414.html | some misspelled words expected: today is a good day
06:15:35     INFO -  +++++ mozInlineSpellChecker::SpellCheckRange
06:15:35     INFO -  ***** mPreferredLang (element) |de-DE|
06:15:35     INFO -  ***** Assigned from element/doc |de-DE|
06:15:35     INFO -  ***** Set |de-DE|.
06:15:35     INFO -  +++++ mozInlineSpellChecker::SpellCheckRange
06:15:35     INFO -  ----- nsContentUtils::SendMouseEvent - DispatchInputEvent (2)
06:15:36     INFO -  ----- nsContentUtils::SendMouseEvent - normal return
[five minutes later]
06:20:39     INFO -  TEST-INFO | started process screentopng
06:20:40     INFO -  TEST-INFO | screentopng: exit 0
06:20:40     INFO -  2135 INFO must wait for load
06:20:40     INFO -  2136 INFO TEST-UNEXPECTED-FAIL | editor/composer/test/test_bug1209414.html | Test timed out.

Clearly the spell checking finished before we trigger the mouse event, but still no avail.

I think I need to dig onto the code that actually pops up the menu and notifies the listeners, or fails to do so :-(
Let me show you how to fish.  :-)  Here is how you would answer such a question.  In your test, you have:

contextMenu = chromeWin.document.getElementById("contentAreaContextMenu");

This gives you the ID of the context menu element.  The code opening it obviously needs to access the element in order to be able to open it.  So, you should grep for "contentAreaContextMenu".  Filtering out things such as tests and the add-on SDK, you will soon find <https://dxr.mozilla.org/mozilla-central/rev/c6ede6f30f3dc886543bb1c76fd7c8b5a151786b/browser/base/content/browser.xul#1143>.  This part requires a bit of familiarity with XUL, but some XUL elements use their attributes to change their behavior, and this looks suspect.  Especially, knowing that the <browser> element is the thing that renders web content makes the theory more likely.  Typically these attributes are acted upon in three places of the code base: toolkit/content/widgets which contains the definitions of these XUL elements, dom/xul and layout/xul which contain the DOM and Layout implementation of XUL controls for the ones that are not implemented in JS in toolkit/content/widgets.  Looking into those locations for "context", we get to <https://dxr.mozilla.org/mozilla-central/rev/c6ede6f30f3dc886543bb1c76fd7c8b5a151786b/dom/xul/nsXULElement.cpp#1828> which is where the listener for the contextmenu event is registered.  The corresponding event handler is here: <https://dxr.mozilla.org/mozilla-central/rev/c6ede6f30f3dc886543bb1c76fd7c8b5a151786b/dom/xul/nsXULPopupListener.cpp#100>.  Reading through that function, you'll see that towards the end, we call LaunchPopup <https://dxr.mozilla.org/mozilla-central/rev/c6ede6f30f3dc886543bb1c76fd7c8b5a151786b/dom/xul/nsXULPopupListener.cpp#214> to actually display the popup (which in this case is a context menu, but for future reference, it can also be things such as tooltips, the hanging UI elements such as the hamburger menu or the downloads UI, etc.)

Both HandleEvent and LaunchPopup have a number of early returns which may be suspect here.
Flags: needinfo?(ehsan)
Thanks. I have added more debug in nsXULPopupListener.cpp.

Local run says:
6 INFO TEST-PASS | editor/composer/test/test_bug1209414.html | some misspelled words expected: today is a good day
+++++ mozInlineSpellChecker::SpellCheckRange
***** mPreferredLang (element) |de-DE|
***** Assigned from element/doc |de-DE|
***** Set |de-DE|.
+++++ mozInlineSpellChecker::SpellCheckRange
----- nsContentUtils::SendMouseEvent - DispatchInputEvent (2)
===== nsXULPopupListener::LaunchPopup - calling pm->ShowPopupAtScreen
===== nsXULPopupListener::LaunchPopup - normal return
===== nsXULPopupListener::HandleEvent - normal return
----- nsContentUtils::SendMouseEvent - normal return
7 INFO TEST-PASS | editor/composer/test/test_bug1209414.html | checking if popup is open

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9fe98b0ea2c2
Added more debug in nsXULElement::AddPopupListener, however, Ehsan's fishing doesn't seem to have caught any fish there since none of this debug is printed.

Anyway, next try run (cancelled the previous one):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=74a8f8396e14
While waiting for the try results, I've been doing some "fishing" of my own.

I compared the raw log of a successful Linux "opt" run with the raw log of an unsuccessful Linux "debug" run. I just wanted to rule out the possibility of a prior test leaving stuff behind.

The result is, that the "opt" run seems to run a large number of
INFO TEST-START | Shutdown
The "debug" run doesn't run those. What are those? Shutdown the client and start again?
That would explain why in "opt", there is a lesser chance of stuff being left behind from a previous test.

<uninteresting>
Other than these "Shutdowns" I noticed that the following tests only run in "debug":
  dom/bindings/test/test_bug1123516_maplikesetlikechrome.xul - 258
    (my test runs here: editor/composer/test/test_bug1209414.html - 658)
  layout/base/tests/chrome/test_leaf_layers_partition_browser_window.xul - 838
  toolkit/content/tests/chrome/test_largemenu.xul - 1001
  toolkit/content/tests/chrome/test_popup_anchoratrect.xul - 1019
  (the numbers indicate the order).
I glanced at test_bug1123516_maplikesetlikechrome.xul and didn't see anything that could have carried over to my test.
</uninteresting>

More interestingly, in "opt", my test runs in this block between two shutdowns:

Shutdown
editor/composer/test/test_async_UpdateCurrentDictionary.html
editor/composer/test/test_bug1200533.html
editor/composer/test/test_bug1204147.html
editor/composer/test/test_bug1205983.html
editor/composer/test/test_bug1209414.html <--- my test
editor/composer/test/test_bug338427.html
editor/composer/test/test_bug434998.xul
editor/composer/test/test_bug678842.html
editor/composer/test/test_bug697981.html
editor/composer/test/test_bug717433.html
Shutdown

So the obvious question is: What is "Shutdown", what does it do? Does it do what the name implies?

If so, we'd be looking for a needle in a haystack. In "debug" my test runs in position 658, so any of the 657 preceding ones could have left stuff behind, for example, attached a listener to *my* ;-) context menu. Maybe just a fantasy, but I'd like to rule it out.
Flags: needinfo?(ahalberstadt)
we just turned run-by-dir on for chrome-debug on Monday the 5th, it made it to m-c the 6th.  I suspect this is the difference.
There's a mode called 'run-by-dir' where the browser gets restarted between each directory of tests specifically with the goal of reducing test bleed-through. Is this a chrome test? I think 'run-by-dir' was just recently enabled for debug mochitest-chrome (sometime last week, see bug 1110982). If you rebase and push again, you should also see the "Shutdown" messages in debug, and with luck your problem might even magically disappear.

If the test still fails after that, another thing you could do is run just that single test by itself in automation by using |mach try|:
http://chmanchester.github.io/blog/2015/07/26/introducing-mach-try/

That way you can either confirm that the failure is caused by bleed-through, or eliminate bleed-through altogether.
(In reply to Joel Maher (:jmaher) from comment #54)
> we just turned run-by-dir on for chrome-debug on Monday the 5th, it made it
> to m-c the 6th.  I suspect this is the difference.

Thanks for the answer, which I sadly don't understand.

I assume you are saying that on "opt" the tests were "run by directory" with real shutdowns of the client in between? This feature is now also on the "debug" runs??

Well, my latest complete try run on "debug" is this:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6701837e8f0 - Thu Oct 8, 13:53:00
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mozilla@jorgk.com-c6701837e8f0/try-linux64-debug/try_ubuntu64_vm-debug_test-mochitest-other-bm123-tests1-linux64-build747.txt.gz

Looking for "INFO TEST-START | Shutdown" I find two matches.

So whatever you're saying and I don't understand, all I can say is that as of today my "debug" runs don't run in a block between two Shutdowns, as the "opt" ones.
(In reply to Andrew Halberstadt [:ahal] from comment #55)
> There's a mode called 'run-by-dir' where the browser gets restarted between
> each directory of tests specifically with the goal of reducing test
> bleed-through. Is this a chrome test?
As in registered in a chrome.ini - yes.

> I think 'run-by-dir' was just recently
> enabled for debug mochitest-chrome (sometime last week, see bug 1110982). If
> you rebase and push again, you should also see the "Shutdown" messages in
> debug, and with luck your problem might even magically disappear.
I need to rebase? Really? OK, I will. I will just wait for the current run to finish. Oh, just finished.
OK, latest try from comment #52 with debug. Totally weird, since this shows errors that were never there before. I'll rebase and fix it locally first. But this time the pop-up showed up. Incredible.

09:35:05     INFO -  2133 INFO TEST-START | editor/composer/test/test_bug1209414.html
09:35:05     INFO -  ++DOMWINDOW == 142 (0x7fdc42a55800) [pid = 1939] [serial = 4307] [outer = 0x7fdc5d050000]
09:35:05     INFO -  2134 INFO TEST-PASS | editor/composer/test/test_bug1209414.html | Got context menu XUL
09:35:05     INFO -  2135 INFO TEST-PASS | editor/composer/test/test_bug1209414.html | true expected (de_DE directory should exist)
09:35:06     INFO -  ***** mPreferredLang (element) |de-DE|
09:35:06     INFO -  ***** Assigned from element/doc |de-DE|
09:35:06     INFO -  ***** Set |de-DE|.
09:35:06     INFO -  +++++ mozInlineSpellChecker::SpellCheckRange
09:35:06     INFO -  2136 INFO TEST-PASS | editor/composer/test/test_bug1209414.html | expected de-DE
09:35:06     INFO -  2137 INFO TEST-PASS | editor/composer/test/test_bug1209414.html | some misspelled words expected: today is a good day
09:35:06     INFO -  +++++ mozInlineSpellChecker::SpellCheckRange
09:35:06     INFO -  ***** mPreferredLang (element) |de-DE|
09:35:06     INFO -  ***** Assigned from element/doc |de-DE|
09:35:06     INFO -  ***** Set |de-DE|.
09:35:06     INFO -  +++++ mozInlineSpellChecker::SpellCheckRange
09:35:06     INFO -  ----- nsContentUtils::SendMouseEvent - DispatchEvent (2)
09:35:06     INFO -  ===== nsXULPopupListener::LaunchPopup - calling pm->ShowPopupAtScreen
09:35:06     INFO -  JavaScript error: chrome://browser/content/nsContextMenu.js, line 587: TypeError: aNode is null
09:35:06     INFO -  ===== nsXULPopupListener::LaunchPopup - normal return
09:35:06     INFO -  ===== nsXULPopupListener::HandleEvent - normal return
09:35:06     INFO -  ----- nsContentUtils::SendMouseEvent - normal return
09:35:06     INFO -  2138 INFO TEST-PASS | editor/composer/test/test_bug1209414.html | checking if popup is open
09:35:06     INFO -  TEST-INFO | started process screentopng
09:35:08     INFO -  TEST-INFO | screentopng: exit 0
09:35:08     INFO -  2139 INFO must wait for load
09:35:08     INFO -  2140 INFO TEST-UNEXPECTED-FAIL | editor/composer/test/test_bug1209414.html | uncaught exception - TypeError: contextMenu.ownerDocument.getElementById(...) is null at chrome://mochitests/content/chrome/editor/composer/test/test_bug1209414.html:106
Flags: needinfo?(ahalberstadt)
Here we go with a new try run. The only difference is that chromeWin, contextMenu are now declared, before they were not, silly mistake. However, I thought that undeclared variables become properties of the global object, hmm. That should cause a difference between "opt" and "debug", or should it?

Looks like the undeclared variables are unrelated to the errors raised above:

JavaScript error: chrome://browser/content/nsContextMenu.js, line 587: TypeError: aNode is null
(that's not my code, unless I passed something invalid. Anyway, something was just checked in there, so the line number is already invalid).

contextMenu.ownerDocument.getElementById(...) is null
(strange, that hasn't happened before).

"Opt" and "debug" run here, let's surprise ourselves:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0aafe42cb11
Attachment #8670650 - Attachment is obsolete: true
Attachment #8670650 - Flags: review?(roc)
(In reply to Joel Maher (:jmaher) from comment #54)
> we just turned run-by-dir on for chrome-debug on Monday the 5th, it made it
> to m-c the 6th.  I suspect this is the difference.

This suggests that you should rebase on top of the current mozilla-central before trying other things, Jorg.
I have already done so a few hours ago. Test still runs locally on Windows 7, so that's why I sent another one to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0aafe42cb11
Victory!  You were getting bitten by a previous test leaving something behind unfortunately.
BTW, Ehsan, can I ask you to review this, since you've already been through it with a fine comb ;-)
Attached patch New test (v2b). — — Splinter Review 

    
    

    
  
Comment on attachment 8671627 [details] [diff] [review]
New test (v2b).

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

Thanks a *lot* for your hard work here!  Much appreciated.

        Attachment #8671627 -
        Flags: review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        New test (v3). (obsolete)
        — 
          — Splinter Review
      

    


  
Thanks for the quick review.

Sorry to trouble you one more time. The version you approved is actually not my preferred version.
  contextMenu.ownerDocument.getElementById("spell-check-dictionary-en-US").doCommand();
is not a user action and at that stage, the secondary drop-out menu doesn't even show.

I'd much rather simulate everything by typing "L" and "E". This has already worked before, see here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc6dc8876902

So I'm putting this back in, and in the hopefully last try run for this bug I'd like to see whether this works: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cae12168e3fd

    
    

    
  
Interdiff
https://bugzilla.mozilla.org/attachment.cgi?oldid=8671627&action=interdiff&newid=8671744&headers=1
shows more changes than there really are since one block was indented.

    
    

    
  
Dear Sheriff,

please check in the attached patch "New test (v2b)."

This patch contains a new Mochitest, which runs in Mochitest(Other).
I have green try runs for all (desktop) platforms:
Linux:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0aafe42cb11
Win/Mac: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca65d386ccab

Since no code was changed, it doesn't make any sense to run the complete test suite.
Since I have green try runs for all (desktop) platforms, it also makes no sense to run these again.

I would therefore suggest that you land this patch together with other patches and then run the tests corresponding to the other patches. There is also no need to land this quickly, it can wait until you have something else to land where this can be added to.

I'm just mentioning it to avoid excessive testing with not benefit. Or perhaps you have your own rules for when new tests are added.
Keywords: checkin-needed

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/2bc41a7237cc
Status: NEW → RESOLVED
Closed: 9 years ago

          status-firefox44:
          affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: