Focus problem when starting browser-chrome tests

RESOLVED FIXED in Firefox 38.0.5

Status

defect
RESOLVED FIXED
4 years ago
a year ago

People

(Reporter: billm, Assigned: Gavin)

Tracking

unspecified
mozilla40
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox38.0.5 fixed, firefox39 fixed, firefox40 fixed, firefox-esr38 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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.

Comment 2

4 years ago
How would I reproduce this?
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.
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

4 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?
(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

4 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?
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();
}
Component: Mochitest Chrome → BrowserTest
And you could use "waitForFocus(startTests, testWin)" instead of testWin.focus(), too.
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.
I've verified that this patch does fix the issues I'm seeing.
Blocks: 1140132
Posted patch alternate patch (obsolete) — Splinter Review
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

4 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.
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)
Posted patch alternate patch (obsolete) — Splinter Review
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)
I guess it would be useful to confirm it fixes Bill/Benjamin's issues.
Flags: needinfo?(wmccloskey)
Flags: needinfo?(benjamin)
This patch appears to work as well.
Flags: needinfo?(benjamin)
This isn't reproducing for me today.
Flags: needinfo?(wmccloskey)

Comment 19

4 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+
Posted patch alternate patch, v2 (obsolete) — Splinter Review
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

4 years ago
Attachment #8602280 - Flags: review?(enndeakin) → review+
Posted patch patch to landSplinter Review
Attachment #8602280 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/d606bc66663f
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Component: BrowserTest → Mochitest
Product: Testing → Testing
You need to log in before you can comment on or make changes to this bug.