Closed Bug 1265077 Opened 5 years ago Closed 5 years ago

Linux focus issues in test_modal_prompts.html

Categories

(Toolkit :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
e10s + ---
firefox50 --- fixed

People

(Reporter: Dolske, Assigned: Dolske)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Spinning this off from bug 1263784.

This test already special-cases Linux (and has since it was written eons ago), when checking to see if the focused element in the prompt is as expected. On Linux we often seem to just get no focused element, so we ignore that.

This still happens after the E10S rewrite, but now in E10S mode the focused element is often the <browser> instead of an expected element in the prompt.

I suspect the same root cause is in play for both. I'm making the test ignore the E10S issue for now, but we should look at that.
Attached patch Patch v.1 (obsolete) — Splinter Review
It seems some recent changes (from bug 1230462) ended up causing a fairly frequent intermittent test failure (bug 1278418), again involving Linux, so I took some time to look into this.

I noticed in bug 1278418 comment 6 that there's some funky code in dialog.xml [postLoadInit()] that waits for the dialog's log event to fire, then does a setTimeout(0), and _then_ finally handles setting the defaultButton focus. [There's a further call to the window's vaguely-named notifyDefaultButtonLoaded(), but that just handles pointer snapping on Windows.]

That's pretty gross. I'm not sure exactly why bug 1230462 made the existing test start failing, but it seems pretty racy so it's surprising this didn't blow up long ago.

[Part of my bug 1278418 comment 6 was wrong -- handlePrompt() does wait for the prompt to have loaded, by way of getDialogDoc()'s |childDocShell.busyFlags != Ci.nsIDocShell.BUSY_FLAGS_NONE| check.]

My first try was to have postLoadInit() set an attribute on the dialog when it was done with its focus-twiddling, and have handlePrompt() wait for that attribute to show up. I did see this condition happen in local testing, but even then some of the focus checks were still failing.

So instead I just added a check to wait for the prompt to actually become the focused window... And that seems to make the test pass. It's surprising to me that this doesn't happen until sometime after the load event fires, but there you have it.

Doing a try push to see if this is a reliable fix to let us remove the Linux todo()s in this test.
Assignee: nobody → dolske
This effectively backs out the workaround that landed in bug 1278418.

Looking at https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1278418&startday=2016-06-01&endday=2016-06-25&tree=all, that failure was happening on about half the pushes (~0.52 oranges per push); and seemed to be about twice as likely to hit Linux64 as Linux. Basically only opt and PGO, with a handful of asan/debug.

I've done a bunch of retriggers of the relevant test chunks on those platforms, and seems to be all-green.
Attached patch Patch v.2Splinter Review
Attachment #8765202 - Attachment is obsolete: true
Attachment #8765214 - Flags: review?(MattN+bmo)
Attachment #8765214 - Flags: review?(MattN+bmo) → review+
https://hg.mozilla.org/integration/fx-team/rev/b6ad395da0e5c99a4f4b6a5554d7df03a047393b
Bug 1265077 - Fix Linux focus issues in test_modal_prompts.html. r=MattN
https://hg.mozilla.org/mozilla-central/rev/b6ad395da0e5
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.