Closed Bug 1391182 Opened 2 years ago Closed 2 years ago

Thunderbird's Mozmill test suite broken by bug 1388238 - TEST-UNEXPECTED-FAIL | Disconnect Error: Application unexpectedly closed (five times during Mozmill run)

Categories

(Thunderbird :: General, defect, blocker)

defect
Not set
blocker

Tracking

(thunderbird56 fixed, thunderbird57 fixed)

RESOLVED FIXED
Thunderbird 57.0
Tracking Status
thunderbird56 --- fixed
thunderbird57 --- fixed

People

(Reporter: jorgk, Unassigned)

References

Details

Attachments

(1 file)

First seen Thu Aug 17, 2017, 2:03:14

M-C last good: 1d38626ba9686d119e489db538ce84f8f9
M-C first bad: 63ca686c3f1e870649b6d9c559973d1005

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1d38626ba9686d119e489db538ce84f8f9&tochange=63ca686c3f1e870649b6d9c559973d1005

It crashes in five Mozmill runs (which are done by directory) in these tests:

On Linux:

account:        test-account-values.js | test_account_name
attachment:     test-attachment.js | test_delete_attachment_key
composition:    test-forward-headers.js | test_forward_inline
content-tabs:   test-content-tab.js | test_content_tab_onbeforeunload
folder-display: test-message-window.js | test_next_unread

One example below:

INFO -  TEST-START | /builds/slave/test/build/tests/mozmill/account/test-account-values.js | test_account_name
INFO -  JavaScript error: resource://mozmill/modules/frame.js -> file:///builds/slave/test/build/tests/mozmill/shared-modules/test-window-helpers.js, line 363: Error: Timeout while waiting for modal dialog.
INFO -  Timeout: bridge.execFunction("96669cfc-82e9-11e7-b557-0af0f5566296", bridge.registry["{e88f9f13-68f2-4e41-b90c-735f30af148c}"]["runTestDirectory"], ["/builds/slave/test/build/tests/mozmill/account"])
WARNING -  TEST-UNEXPECTED-FAIL | Disconnect Error: Application unexpectedly closed

On Windows and Mac other tests are crashing:

test-account-values.js | test_account_name
test-attachment.js | test_delete_attachment_key
test-attachment-reminder.js | test_attachment_reminder_appears_properly
test-content-tab.js | test_content_tab_onbeforeunload
test-columns.js | test_apply_to_folder_no_children

New bustage every day :-(
Flags: needinfo?(acelists)
OK, I ran
mozmake SOLO_TEST=composition/test-attachment-reminder.js mozmill-one

It runs TEST-START | c:\mozilla-source\comm-central\mail\test\mozmill\composition\test-attachment-reminder.js | test_attachment_reminder_appears_properly

I see this the modal panel on the screen "Did you forget to add an attachment?"

Yet, in the console:
JavaScript error: resource://mozmill/modules/frame.js -> file:///c:/mozilla-source/comm-central/mail/test/mozmill/shared-modules/test-window-helpers.js, line 363: Error: Timeout while waiting for modal dialog.

And waits there for a while and then it's dead.

Similar issue in
mozmake SOLO_TEST=composition/test-attachment.js mozmill-one

I see a panel: Rename Attachment: New attachment name: ...

and then it hangs there. So windows that popped up are no longer detected.

In the rather short regression range
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1d38626ba9686d119e489db538ce84f8f9&tochange=63ca686c3f1e870649b6d9c559973d1005

nothing catches the eye, perhaps bug 1376895. I'll back it out locally to see.
After one hour of recompilation I can deny that bug 1376895 is at fault.

FRG, you have a good eye for regressions, where do you put your money in this case?
Flags: needinfo?(frgrahl)
This is a little urgent since we've basically lost Mozmill tests. The first test in the directory that fails will abort that directory and further tests don't run.
Really beats me this time. I am finding nothing too. You sure you got all pushes from autoland and m-i in the range?
Flags: needinfo?(frgrahl)
Yes, I click on the B's in the treeherder and get the M-C version. I tried backing out bug 1376754 but that don't do it either.
Senior regression spotter FRG pointed me to bug 1388238 (in a private message) and he is dead right, backing out
https://hg.mozilla.org/mozilla-central/rev/8c1ef3d6ed6d
https://hg.mozilla.org/mozilla-central/rev/9f15f3ee6cc1
makes the tests pass again.

That bug is mostly about the master password, but also modifies:
toolkit/components/prompts/src/CommonDialog.jsm
https://hg.mozilla.org/mozilla-central/rev/9f15f3ee6cc1#l2.12

Steve and Matt, why would those changes affect a prompt dialogue in Thunderbirds Mozmill tests?
Flags: needinfo?(schung)
Flags: needinfo?(MattN+bmo)
Just removing this hunk:
+        if (xulDialog) {
+            xulDialog.setAttribute("windowtype", "prompt:" + this.args.promptType);
+        }
+
makes the test pass again. Looks like this interferes with the windowtype handling in mail/test/mozmill/shared-modules/test-window-helpers.js, maybe here:

function wait_for_modal_dialog(aWindowType, aTimeout) {
  mark_action("fdh", "wait_for_modal_dialog", [aWindowType, aTimeout]);
  WindowWatcher.waitForModalDialog(aWindowType, aTimeout);
  mark_action("fdhb", "/wait_for_modal_dialog", [aWindowType, aTimeout]);
}

Not sure how to fix that.
I am not sure it is a good idea to just overwrite the windowtype in CommonDialog.jsm:

https://dxr.mozilla.org/comm-central/search?q=windowtype&redirect=false

May break a few other things too.
Blocks: 1388238
Severity: normal → blocker
Summary: TEST-UNEXPECTED-FAIL | Disconnect Error: Application unexpectedly closed (five times during Mozmill run) → Thunderbird's Mozmill test suite broken by bug 1388238 - TEST-UNEXPECTED-FAIL | Disconnect Error: Application unexpectedly closed (five times during Mozmill run)
What? Do they prepend "prompt:" prefix to the windowtype attribute in SOME windows? Yes, that hoses our tests that depend if a window with the proper windowtype appear.

Why is that change useful?

If it stays in m-c, we can adapt easily. Either wait for the "prompt:" prefixed types in the affected windows OR we can just make the tests check if window with windowtype OR "prompt:"+windowtype appeared.
Could be a one-line fix until the reason for the change is found.
Flags: needinfo?(acelists)
Let's have that one line change. I'll be doing a try push with the M-C change backed out since we don't have any Mozmill coverage right now since the first test with a prompt takes down the entire directory of tests. Not good.
Mozmill is actually green as can be seen on try with that problem resolved.
Attached patch 1391182.patchSplinter Review
OK, all the alert/prompt dialogs that had id=commonDialog now have also windowType=prompt:<promptType> .

In our tests we match dialogs by windowType OR id. Previously the id matched (we looked for "commonDialog", but the windowType now takes precedence and the dialogs no longer match so the tests time out.

So the code in m-c does not alter existing windowTypes, just adds some bogus ones we do not want, this patch ignores them and falls back to id again.
Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=72f28d6d5ce8039f42c4dcc12b8753aef72bd465

I suggest this to be a temporary hack. If the patch in bug 1388238 sticks and these windowTypes stay there, we can back this out and properly convert our tests to look for the new windowTypes in the form "prompt:alert" or "prompt:confirmEx" instead of the current "commonDialog". There are not that many occurences, just that for each case we need to find out which type of prompt is shown.
Attachment #8898494 - Flags: review?(jorgk)
The windowtype was added to make it easier to enumerate prompts of a specific type. That change wasn't hacky at all and I don't see a compelling reason to revert it as it will be useful for other prompts, not just the password ones.

I think you should take the patch in comment 12 for now and then fix the tests to handle the new windowtypes longer term.
Flags: needinfo?(schung)
Flags: needinfo?(MattN+bmo)
Comment on attachment 8898494 [details] [diff] [review]
1391182.patch

Thanks, I'll get this landed.
Attachment #8898494 - Flags: review?(jorgk) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/ee3ba3bf21dc
Port bug 1388238 - In mozmill tests, skip windowType attribute in dialogs if they start with "prompt:" prefix. r=jorgk
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Thanks again, Aceman. I took the liberty to improve the commit message and add a comment.
Target Milestone: --- → Thunderbird 57.0
Beta (TB 56):
https://hg.mozilla.org/releases/comm-beta/rev/52f5fb6fd50bb2121428c1fb9f9a6b30e158af4f

I had to uplift this since bug 1388238 got uplifted (bug 1388238 comment #30) and since that day, Mozmill fails on C-B.
You need to log in before you can comment on or make changes to this bug.