Closed
Bug 1138079
Opened 9 years ago
Closed 9 years ago
Focus problem when starting browser-chrome tests
Categories
(Testing :: Mochitest, defect)
Tracking
(firefox38.0.5 fixed, firefox39 fixed, firefox40 fixed, firefox-esr38 fixed)
RESOLVED
FIXED
mozilla40
People
(Reporter: billm, Assigned: Gavin)
References
Details
Attachments
(1 file, 4 obsolete files)
2.41 KB,
patch
|
Details | Diff | Splinter Review |
I ran into a problem today where running a single test would fail because the harness window (the one with the "Run tests" button) was in focus while the test was running. We have code that raises the test window before the test starts, but it doesn't always seem to work. It looks like what's happening is that: 1. We open the test window and it's in focus. 2. We open the browser-harness.xul window. 3. The browser-harness.xul window call .focus() on the test window. This does nothing because the focus manager still considers the test window to be active. 4. Eventually the browser harness window comes into focus and stays that way. I don't know what the right way is to fix this, but I wrote a patch that waits a little while before doing step 3.
Reporter | ||
Comment 1•9 years ago
|
||
Attachment #8570938 -
Flags: review?(enndeakin)
Comment 2•9 years ago
|
||
How would I reproduce this?
Reporter | ||
Comment 3•9 years ago
|
||
Probably not easily. It happens to me most often when I run a single mochitest. As an example: mach mochitest-browser browser_522545.js When the test runs, the harness window (the gray window with the "Run" button at the top) is sometimes on top for me. That's the bug. However, it doesn't always happen.
Reporter | ||
Comment 4•9 years ago
|
||
I'm still seeing this Neil. If there's some way I can debug it further I'd be happy to. If not, I'd like to get this patch checked in.
Comment 5•9 years ago
|
||
I'm not clear I understand what this wants to do. Why don't you just wait until the browser-harness.xul window is focused before trying to anything with the test?
Reporter | ||
Comment 6•9 years ago
|
||
(In reply to Neil Deakin from comment #5) > I'm not clear I understand what this wants to do. Why don't you just wait > until the browser-harness.xul window is focused before trying to anything > with the test? The problem is that browser-harness.xul never comes into focus. I could click on it myself, but usually the test starts too quickly for that. Also, that would be silly. See comment 0 for why browser-harness.xul doesn't ever come into focus.
Comment 7•9 years ago
|
||
Comment on attachment 8570938 [details] [diff] [review] Fix focus problem in test harness >- testWin.focus(); >- var Tester = new testWin.Tester(links, gDumper, testsFinished); >- Tester.start(); >+ var windowWatcher = Cc['@mozilla.org/embedcomp/window-watcher;1']. >+ getService(Ci.nsIWindowWatcher); It would be more direct to get the active window from the focus manager instead. >+ // It's possible that the browser harness window hasn't gotten >+ // focus yet, in which case the main Firefox window is still in >+ // focus. In this situation, testWin.focus() will do >+ // nothing. So we wait until the testWin has lost focus before >+ // focusing it again. >+ function waitForLossOfFocus() { >+ if (windowWatcher.activeWindow === testWin) { >+ setTimeout(waitForLossOfFocus, 0); >+ return; >+ } >+ setStatus("Running..."); >+ testWin.focus(); >+ var Tester = new testWin.Tester(links, gDumper, testsFinished); >+ Tester.start(); >+ } >+ waitForLossOfFocus(); Why don't we just wait for the harness window to receive a focus event instead of using a timeout?
Assignee | ||
Comment 8•9 years ago
|
||
I think you could do essentially: if (Services.focus.focusedWindow != window) { // harness window is opened (because this code is running) but not focused yet // so wait for focus event on harness window waitForFocus(focusTestWindowAndStartTests, window); } else { focusTestWindowAndStartTests(); }
Assignee | ||
Updated•9 years ago
|
Component: Mochitest Chrome → BrowserTest
Assignee | ||
Comment 9•9 years ago
|
||
And you could use "waitForFocus(startTests, testWin)" instead of testWin.focus(), too.
Comment 10•9 years ago
|
||
I can reproduce this almost 100% with https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=ba26b2b79764 Both on tbox and locally on Linux x64. I have a run with PR logging Focus:5,FocusNavigation:5. Would that log help? I suspect that this is related to event loop shenanigans and asynchronous delivery of X focus events, but I really don't know enough to say for certain.
Comment 11•9 years ago
|
||
I've verified that this patch does fix the issues I'm seeing.
Blocks: 1140132
Assignee | ||
Comment 12•9 years ago
|
||
Actually it looks like relying on SimpleTest.waitForFocus in the harness itself is probably not a good idea, so I tried re-implementing it myself. For some reason this doesn't work, but I'm not sure why.
Comment 13•9 years ago
|
||
from comment #12) > Created attachment 8599968 [details] [diff] [review] > alternate patch > > Actually it looks like relying on SimpleTest.waitForFocus in the harness > itself is probably not a good idea, so I tried re-implementing it myself. > For some reason this doesn't work, but I'm not sure why. The 'focus' event can fire on the content page in the window, so you likely want a capturing listener here.
Reporter | ||
Comment 14•9 years ago
|
||
Do you want to take this bug over Gavin? That's fine with me. I'm a bit confused with all that's going on.
Flags: needinfo?(gavin.sharp)
Assignee | ||
Comment 15•9 years ago
|
||
I was just hoping to inspire you :) This patch seems to work. I'd love if you can drive this in, Bill.
Attachment #8570938 -
Attachment is obsolete: true
Attachment #8599968 -
Attachment is obsolete: true
Attachment #8570938 -
Flags: review?(enndeakin)
Flags: needinfo?(gavin.sharp)
Attachment #8600040 -
Flags: review?(enndeakin)
Assignee | ||
Comment 16•9 years ago
|
||
I guess it would be useful to confirm it fixes Bill/Benjamin's issues.
Flags: needinfo?(wmccloskey)
Flags: needinfo?(benjamin)
Comment 19•9 years ago
|
||
Comment on attachment 8600040 [details] [diff] [review] alternate patch This is ok, but you might want to do two changes: 1. Move the 'fm.focusedWindow != window' check inside the waitForFocus function so that it always makes this check. 2. Does Tester.start run the test right away or through an asynchronous call or later event? if the former, we should call callback() after an executeSoon so that we can ensure that focusing is actually finished.
Attachment #8600040 -
Flags: review?(enndeakin) → review+
Assignee | ||
Comment 20•9 years ago
|
||
Patch with comments addressed. https://treeherder.mozilla.org/#/jobs?repo=try&revision=907e9d5633fc
Assignee: wmccloskey → gavin.sharp
Attachment #8600040 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8602280 -
Flags: review?(enndeakin)
Updated•9 years ago
|
Attachment #8602280 -
Flags: review?(enndeakin) → review+
Assignee | ||
Comment 21•9 years ago
|
||
Attachment #8602280 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 22•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/d606bc66663f
Keywords: checkin-needed
Comment 23•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/e74afa4b00d4
status-firefox39:
--- → fixed
Comment 24•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-release/rev/96da8302e8a2
status-firefox38.0.5:
--- → fixed
Comment 25•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-esr38/rev/653b4c99d40a
status-firefox-esr38:
--- → fixed
https://hg.mozilla.org/mozilla-central/rev/d606bc66663f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Updated•6 years ago
|
Component: BrowserTest → Mochitest
You need to log in
before you can comment on or make changes to this bug.
Description
•