521233 - waitForFocus(callback) times out in browser chrome tests

Status: REOPENED

(Testing :: Mochitest, defect)

Description

[Dão Gottwald [:dao]]

bug 516440 comment 21:
> Neither waitForFocus(callback) nor waitForFocus(callback, window) work. I guess
> it's because of these lines:
> 
>     var focusedWindow = { };
>     if (fm.activeWindow)
>       fm.getFocusedElementForWindow(fm.activeWindow, true, focusedWindow);
> 
>     // if this is a child frame, ensure that the frame is focused
>     SimpleTest.waitForFocus_focused = (focusedWindow.value == targetWindow);
> 
> waitForFocus(callback, content) works.

bug 516440 comment 23:
> It's not really a bug in the original code, because that code was designed for
> content windows. For waitForFocus(callback) in a browser chrome test, we really
> want (fm.activeWindow == targetWindow) instead of (focusedWindow.value ==
> targetWindow). That's basically what I've been doing in a few tests already:
> 
> function test() {
>   waitForExplicitFinish();      
> 
>   if (Cc["@mozilla.org/focus-manager;1"].getService(Ci.nsIFocusManager).activeWindow !=
>       window) {
>     setTimeout(test, 0);
>     window.focus();
>     return;
>   }
>   ...
> }

bug 516440 comment 24:
> please file a new bug, i suppose we could set what we need before calling
> SimpleTest.waitForFocus

Not sure what "set what we need before calling SimpleTest.waitForFocus" exactly means.

Comment 1

[Neil Deakin]

OK, I understand this bug now. The solution is probably to do at the beginning of waitForFocus:

var usedTargetWindow = { };
fm.getFocusedElementForWindow(targetWindow, true, usedTargetWindow)
targetWindow = usedTargetWindow.value;

Alternatively, tests could just ensure they focus gURLBar or something beforehand, but that requires per-test changes.

Comment 2

[Dão Gottwald [:dao]]

(In reply to comment #1)
> OK, I understand this bug now. The solution is probably to do at the beginning
> of waitForFocus:
> 
> var usedTargetWindow = { };
> fm.getFocusedElementForWindow(targetWindow, true, usedTargetWindow)
> targetWindow = usedTargetWindow.value;

That would make the content are a steal other chrome elements' focus. That's probably not quite what the waitForFocus(callback) caller expects. I think we should fix this in the browser test framework.

Comment 3

[Neil Deakin]

It shouldn't change the focus. If targetWindow is a toplevel window, it should just raise that toplevel window if it isn't already and focus whatever child would already be focused.

Comment 4

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

What's the status here? The fix works to make my tests run.

Comment 5

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Anything against this approach?
i tested it changing browser_bug304198.js that is actually a random orange (bug 511684), and looks like working.

Comment 6

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 406706 [details] [diff] [review]
patch v1.0

>+++ b/testing/mochitest/browser-test.js

Why not do it in SimpleTest.waitForFocus? It already does the target window early return & has the focus manager. Or might that mess up non-browser-chrome tests? (it worked for my test)

Comment 7

[Marco Bonardo [:mak]]

for comment 2 and i agree that usually is better to not mix up things, but the expert here is Neil, so i'm merely leaving him last call.

Comment 8

[Neil Deakin]

I think we should put the code into SimpleTest.waitForFocus, as it would be good to have them work consistently.

Comment 9

[Marco Bonardo [:mak]]

sorry for late, i'm catching up, i'll post new patch that will put code in SimpleTest.

Comment 10

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Comment 11

[Neil Deakin]

Comment on attachment 408613 [details] [diff] [review]
patch v1.1

>   function urlbarBackspace(cb) {
>+    gURLBar.addEventListener("input", function () {
>+      gURLBar.removeEventListener("input", arguments.callee, false);
>+      cb();
>+    }, false);
>     gBrowser.selectedBrowser.focus();
>-    gURLBar.addEventListener("focus", function () {
>-      gURLBar.removeEventListener("focus", arguments.callee, false);
>-      gURLBar.addEventListener("input", function () {
>-        gURLBar.removeEventListener("input", arguments.callee, false);
>-        cb();
>-      }, false);
>-      executeSoon(function () {
>-        EventUtils.synthesizeKey("VK_BACK_SPACE", {});
>-      });
>-    }, false);
>     gURLBar.focus();
>+    waitForFocus(function() EventUtils.synthesizeKey("VK_BACK_SPACE", {}));

This shouldn't need to use waitForFocus. waitForFocus should only be used when waiting for a window to be focused, yet the active window doesn't get focused here.

Or is there some interaction that happens here?

Comment 12

[Marco Bonardo [:mak]]

Attachment: patch v1.2

bah, you're right, dunno why but i thought this would have ensured window was still focused. btw, removed the change.
We could maybe consider a waitForFocus(callback, element) where if element is not a DOMWindow it will check if it's focused and if it's not add a focus listener and calls the callback when the focus event fires?

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 408726 [details] [diff] [review]
patch v1.2

>+  window.focus();

This is redundant.

>+  info("Window has been correctly focused, continuing test");

This too, waitForFocus already logs by itself.

Comment 14

[Marco Bonardo [:mak]]

Attachment: patch v1.3

i see, it's calling targetWindow.focus()

Comment 15

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/032a9a176ee7

Comment 16

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

So I'm having this problem on Linux where the callback never gets called if the window is already focused... I can fix it by wrapping it in waitForFocus(other window) first, but that doesn't seem ideal.

Comment 17

[Dão Gottwald [:dao]]

(In reply to comment #16)
> I can fix it by wrapping it in waitForFocus(other
> window) first, but that doesn't seem ideal.

Yeah, that's not a solution.

Comment 18

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #17)
> Yeah, that's not a solution.

Indeed. And that's why bug 525635 exists :(

Neil, could this be related to why you have the window switching tests disabled on Linux? http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/chrome/window_focus.xul#1121

Comment 19

[Neil Deakin]

(In reply to comment #18)
> Neil, could this be related to why you have the window switching tests disabled
> on Linux?
> http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/chrome/window_focus.xul#1121

The tests are disabled only because I didn't feel like making the test code more complicated.

Comment 20

[cmtalbert]

(In reply to comment #19)
> (In reply to comment #18)
> > Neil, could this be related to why you have the window switching tests disabled
> > on Linux?
> > http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/chrome/window_focus.xul#1121
> 
> The tests are disabled only because I didn't feel like making the test code
> more complicated.
Just a drive by comment here...I can understand completely why you'd make that decision for the window_focus.xul test, it's already long and complicated.  But please, let's either add in a standalone test for this behavior as part of this bug's fix, or let's open a new bug to get some stand alone tests for the window switching behavior.   I don't want things like this to fall through the cracks of the test framework.

Thanks guys.

Comment 21

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: focus log

Here's output log of running the tests. I made waitForFocus dump to console & added a couple extra outputs. We open a new window after the last TEST-PASS in there and waitForFocus on the new window (http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/browser_NetworkPrioritizer.js#109 - I got rid of the extraneous waitForFocus to do it more "correctly").

Neil, anything jump out as obviously wrong?

Comment 22

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

After talking with Neil and working on a fix for bug 525635, it seems like there can be some timing issues when calling waitForFocus on new windows. The particular problem is that waitForFocus was waiting for focus on about:blank, which had actually been replaced with a different url and different "window", so the focus event never got triggered. By making sure I got the load event for the real URL first the test is working, but it's still not a great solution.

Now I really want this on 1.9.2 so I can get bug 514490 onto branch as well. I'm not super thrilled to land an unreliable test framework on branch though. So is there a better fix we can get here?

Comment 23

[Dão Gottwald [:dao]]

Attachment: second attempt

Comment 24

[Neil Deakin]

Comment on attachment 410224 [details] [diff] [review]
second attempt

>+    if (targetWindow.top == targetWindow &&
>+        targetWindow.constructor == targetWindow.ChromeWindow) {

I'm not sure what this last bit means. Are you trying to do targetWindow instanceof ChromeWindow?

>+        SimpleTest.waitForFocus_pending_activate = (fm.activeWindow != targetWindow);
>+        SimpleTest.waitForFocus_pending_focus = false;

I don't follow the logic here. The code here seems to imply that the test would run only when the activate event has fired but not focused. Waiting for focus means you need to wait for the focus event, not the activate event. The window isn't properly focused until then, whether it needs activating or not.

As an aside, we may want to file a bug to disallow calling any focus manager methods during the activate/deactivate events.

Comment 25

[Dão Gottwald [:dao]]

(In reply to comment #24)
> >+    if (targetWindow.top == targetWindow &&
> >+        targetWindow.constructor == targetWindow.ChromeWindow) {
> 
> I'm not sure what this last bit means. Are you trying to do targetWindow
> instanceof ChromeWindow?

instanceof ChromeWindow is true for content windows.

> >+        SimpleTest.waitForFocus_pending_activate = (fm.activeWindow != targetWindow);
> >+        SimpleTest.waitForFocus_pending_focus = false;
> 
> I don't follow the logic here. The code here seems to imply that the test would
> run only when the activate event has fired but not focused. Waiting for focus
> means you need to wait for the focus event, not the activate event. The window
> isn't properly focused until then, whether it needs activating or not.

I think the focus event wasn't reliable there. Wasn't that the motivation for adding the activate event?
Last I tried, the focus event didn't work for the tests using this pattern:
-  if (Cc["@mozilla.org/focus-manager;1"].getService(Ci.nsIFocusManager).activeWindow !=
-      window) {
-    setTimeout(test, 0);
-    window.focus();
-    return;
-  }

Comment 26

[Neil Deakin]

(In reply to comment #25)
> (In reply to comment #24)
> > I'm not sure what this last bit means. Are you trying to do targetWindow
> > instanceof ChromeWindow?
> 
> instanceof ChromeWindow is true for content windows.

Is there a bug filed on that? I tested this and note that instanceof Components.interfaces.nsIDOMChromeWindow does work as expected.


> I think the focus event wasn't reliable there. Wasn't that the motivation for
> adding the activate event?

The activate event is fired when the window is raised. Various focus related things have not yet happened.


> Last I tried, the focus event didn't work for the tests using this pattern:
> -  if
> (Cc["@mozilla.org/focus-manager;1"].getService(Ci.nsIFocusManager).activeWindow
> !=
> -      window) {
> -    setTimeout(test, 0);
> -    window.focus();
> -    return;
> -  }

What were you trying to acheive that wasn't working? That code above is covered exactly by waitForFocus.

Comment 27

[Dão Gottwald [:dao]]

(In reply to comment #26)
> > instanceof ChromeWindow is true for content windows.
> 
> Is there a bug filed on that? I tested this and note that instanceof
> Components.interfaces.nsIDOMChromeWindow does work as expected.

bug 295460

> > I think the focus event wasn't reliable there. Wasn't that the motivation for
> > adding the activate event?
> 
> The activate event is fired when the window is raised. Various focus related
> things have not yet happened.

The code that I cited worked reliably through the past few month, though, and I assume that activeWindow and the activate event are closely tied together.

Also note that the patch passed the testsuite on the tryserver, on all platforms, but of course that doesn't rule out intermittent problems.

> > Last I tried, the focus event didn't work for the tests using this pattern:
> > -  if
> > (Cc["@mozilla.org/focus-manager;1"].getService(Ci.nsIFocusManager).activeWindow
> > !=
> > -      window) {
> > -    setTimeout(test, 0);
> > -    window.focus();
> > -    return;
> > -  }
> 
> What were you trying to acheive that wasn't working? That code above is covered
> exactly by waitForFocus.

I added a focus event listener if activeWindow wasn't window, and it timed out.

Comment 28

[Neil Deakin]

> > > Last I tried, the focus event didn't work for the tests using this pattern:
> > > -  if
> > > (Cc["@mozilla.org/focus-manager;1"].getService(Ci.nsIFocusManager).activeWindow
> > > !=
> > > -      window) {
> > > -    setTimeout(test, 0);
> > > -    window.focus();
> > > -    return;
> > > -  }
> > 

So if the changes to SimpleTest.js aren't made and only the above in the other two files changed in this patch are replaced, the test fails?

Comment 29

[Dão Gottwald [:dao]]

I think it fails if a chrome element like the location bar is focused and the window isn't raised. It should work if the content area has focus (except for bug 525635), but then it will also wait for the content window to load, which the caller didn't ask for.

Comment 30

[Dão Gottwald [:dao]]

Comment on attachment 410224 [details] [diff] [review]
second attempt

will try to do this without the activate event

Comment 31

[Dão Gottwald [:dao]]

(In reply to comment #30)
> (From update of attachment 410224 [details] [diff] [review])
> will try to do this without the activate event

Didn't work...

changeset: http://hg.mozilla.org/try/rev/8e7304082237

Linux: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1257531806.1257541316.3706.gz

Windows: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1257531802.1257542006.11334.gz

Comment 32

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #31)

Interestingly neither of these failures were at the same place as the one that's failing on m-c

> Linux:
> http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1257531806.1257541316.3706.gz

I had seen this one before. But not since I added a waitForFocus before running each test function.

> Windows:
> http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1257531802.1257542006.11334.gz

This is happening farther inside nested waitForFocus calls. I haven't seen this before. But by this point pages should be loaded, so it shouldn't be the same issue I was seeing

Comment 33

[Dão Gottwald [:dao]]

(In reply to comment #32)
> But by this point pages should be loaded, so it shouldn't be the same
> issue I was seeing

It shouldn't matter whether the pages are loaded. With my change waitForFocus didn't look at them at all, since window_A and window_B are chrome windows.

Comment 34

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 408732 [details] [diff] [review]
patch v1.3

Pushed this to 1.9.2 as http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8c8fd549db98

If we get a "more right" fix here, great, but until then I needed this as is.

Comment 35

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

So since this is definitely still causing problems now that it's on branch (and causing problems in both of the browser-chrome tests using it - bug 525635 and bug 511684), and Dao says his "second attempt" patch works, I say we get the review done so we can take it or something like it to fix this once and for all.

Comment 36

[Marco Bonardo [:mak]]

i'm not actively working on this, dao maybe wants to take it

Comment 37

[Marco Bonardo [:mak]]

Notice bug 553248. I discussed a bit with Neil during the work week about this bug and he correctly pointed out that the hang could not have been in waitForFocus since we were not even seeing the logs from it.
Due to bug 553248 the logs were just hidden, so this bug comes back strongly.

Neil, please could you take a look at what we can do here and review the above patch? if you don't like it just r- and suggest alternatives, but this should move on.

Comment 38

[Marco Bonardo [:mak]]

bug 539002 is probably still the best way to reproduce this bug.

Comment 39

[Neil Deakin]

Comment on attachment 410224 [details] [diff] [review]
second attempt

Bug 554873 fixes some issues with waitForFocus. Is this bug still relevant?

Comment 40

[Marco Bonardo [:mak]]

If we won't see anymore hangs in b-c tests once that one will be fixed, I'd say it's not relevant, otherwise there could be a different reason, but I'm prone to think that these 2 are the same thing.

So, once bug 554873 is fixed we should re-enable the test in bug 539002 and check if it does not fail anymore.

Comment 41

[Neil Deakin]

(In reply to comment #40)
> So, once bug 554873 is fixed we should re-enable the test in bug 539002 and
> check if it does not fail anymore.

The patch in bug 554873 fixes and reenables that test.

Comment 42

[Dão Gottwald [:dao]]

(In reply to comment #41)
> (In reply to comment #40)
> > So, once bug 554873 is fixed we should re-enable the test in bug 539002 and
> > check if it does not fail anymore.
> 
> The patch in bug 554873 fixes and reenables that test.

That patch makes browser_524745.js wait for the content window rather than the chrome window. It doesn't help waitForFocus calls with chrome windows, which this bug is about.

Comment 43

[Neil Deakin]

So what is this bug about then?

The description 'waitForFocus times out in browser chrome tests' isn't currently occuring is it? If so, which tests are a problem?

Comment 44

[Dão Gottwald [:dao]]

browser_524745.js apparently... You shouldn't have to pass a content window to make that work.

Comment 45

[Neil Deakin]

(In reply to comment #44)
> browser_524745.js apparently... You shouldn't have to pass a content window to
> make that work.

True, I should be able to just pass 'window'. The other option is to ensure that the urlbar or something in chrome is focused first.

Or were you thinking of a different fix?

Comment 46

[Dão Gottwald [:dao]]

(In reply to comment #45)
> (In reply to comment #44)
> > browser_524745.js apparently... You shouldn't have to pass a content window to
> > make that work.
> 
> True, I should be able to just pass 'window'. The other option is to ensure
> that the urlbar or something in chrome is focused first.

Other option compared to what? Passing 'window' has the same effect as omitting the argument which the test is doing already, and expectBlankPage doesn't seem to make sense if the window is a chrome window...

> Or were you thinking of a different fix?

The patch in this bug should fix browser_524745.js as is, I think.

Comment 47

[Neil Deakin]

(In reply to comment #46)
> The patch in this bug should fix browser_524745.js as is, I think.

The patch here appears to callback upon the activate event for toplevel windows rather than the focus event. Windows however aren't focused until the focus event is received.

Comment 48

[Dão Gottwald [:dao]]

Wasn't the activate event introduced because the focus event didn't quite work for chrome windows in the first place?

Browser chrome tests actually depending on the window having focus (e.g. those using synthesizeKey, see bug 497839) seem to work fine with this patch.

Comment 49

[Neil Deakin]

(In reply to comment #48)
> Wasn't the activate event introduced because the focus event didn't quite work
> for chrome windows in the first place?

Don't think it was, no. If your goal is to find our when a window is made active, it's certainly the right event to use.

> Browser chrome tests actually depending on the window having focus (e.g. those
> using synthesizeKey, see bug 497839) seem to work fine with this patch.

Tests will fail if they rely on behaviour that hasn't happened at the time the activate event fires, for instance :focus rules, ime, focusing plugins, etc.

Comment 50

[Dão Gottwald [:dao]]

(In reply to comment #49)
> If your goal is to find our when a window is made
> active, it's certainly the right event to use.

It's what browser chrome tests currently using waitForFocus are interested in, fwiw. If we can't align that with the current waitForFocus implementation, we should probably revert bug 516440 and roll out a specialized helper function.

Comment 51

[Neil Deakin]

But you just said in comment 48 that the tests depend on synthesizeKey, in which case they want to know when the focus is put in the window, not when it is made active.

Comment 52

[Dão Gottwald [:dao]]

But I also said that these tests appear to work just fine with the activate event.

Comment 53

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #48)
> Wasn't the activate event introduced because the focus event didn't quite work
> for chrome windows in the first place?

Pretty sure that's exactly why it was introduced. When content has focus, the focus event for the chrome window never happens. See bug 511503 comment 3.

Comment 54

[Neil Deakin]

(In reply to comment #53)
> Pretty sure that's exactly why it was introduced. When content has focus, the
> focus event for the chrome window never happens. See bug 511503 comment 3.

Did you use a capturing event listener? Focus events do not have a bubble phase and won't bubble up from a content window to chrome.

That would grab all focus events though. Does that mean you actually wanted an event which fires on the toplevel window when a content window is focused?

Comment 55

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #54)
> (In reply to comment #53)
> > Pretty sure that's exactly why it was introduced. When content has focus, the
> > focus event for the chrome window never happens. See bug 511503 comment 3.
> 
> Did you use a capturing event listener? Focus events do not have a bubble phase
> and won't bubble up from a content window to chrome.

I don't remember exactly what I was doing, but I'm pretty sure I tried capture/non-capture and attaching to different things.

> That would grab all focus events though. Does that mean you actually wanted an
> event which fires on the toplevel window when a content window is focused?

Well, not exactly. I wanted to know when a top level window was focused (technically, activated). I didn't care where the focus was in that window. I guess when switching windows, the only focus event would fire in the content and like you said, won't bubble up to chrome, so I couldn't reliably know when a top level window was switched to.

Comment 56

[Neil Deakin]

So what do you want here?

The 'activate' event as currently implemented does not indicate that any window has the focus, only that it is now the active window. Any code relying on this is wrong. (whether tests happen to work is irrelevant)

If my reading of your comments is correct, however, and you actually want the 'activate' event to fire on the top-level window when the child is focused and just before the 'focus' event is fired on it, please file a bug on changing this.

Comment 57

[Dão Gottwald [:dao]]

(In reply to comment #56)
> The 'activate' event as currently implemented does not indicate that any window
> has the focus, only that it is now the active window. Any code relying on this
> is wrong. (whether tests happen to work is irrelevant)

Why is this irrelevant? We had random oranges with synthesizeKey, and waiting for window activation fixed them. That's a fact. browser-test.js actually waits for the 'activate' event between tests nowadays.

Comment 58

[Neil Deakin]

(In reply to comment #57)

> browser-test.js actually waits for the 'activate' event between tests nowadays.

browser-test.js looks to wait for the activate event and then runs the test on a setTimeout. By the time the timeout fires, the focus would have already occurred. This is assuming current code. This may not be the case in the future.

I'm not sure what the issue is here. I'm telling you that when the 'activate' event fires, the window is not yet set up for focus. But I'm not clear why you think that is ok.

Comment 59

[Marco Bonardo [:mak]]

Can we please try to get a review and land bug 554873, then use waitForFocus in browser-test.js with skipLoadCheck = true (what ehsan was trying to do in bug 543278) and see what happens?
While the discussion is interesting, seeing what happens in practice could be even more interesting and useful.

Comment 60

[(no longer active)]

(In reply to comment #59)
> Can we please try to get a review and land bug 554873, then use waitForFocus in
> browser-test.js with skipLoadCheck = true (what ehsan was trying to do in bug
> 543278) and see what happens?
> While the discussion is interesting, seeing what happens in practice could be
> even more interesting and useful.

I agree with Marco.  That seems to be the best course of action I can think of.

Comment 61

[Neil Deakin]

Sure, would any of you like to review?

Comment 62

[(no longer active)]

(In reply to comment #61)
> Sure, would any of you like to review?

I would've, except that I don't have a clear understanding about how waitForFocus works.  :(

Comment 63

[Marco Bonardo [:mak]]

Sounds like we are still timing out after "must wait for focus", we don't receive the "focus" event, at least in browser-test.js check (before in the same position we were not receiving "activate").