waitForFocus(callback) times out in browser chrome tests

REOPENED
Unassigned

Status

defect
REOPENED
10 years ago
7 months ago

People

(Reporter: dao, Unassigned)

Tracking

unspecified
mozilla1.9.3a1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

10 years ago
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

10 years ago
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.
(Reporter)

Comment 2

10 years ago
(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

10 years ago
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.
What's the status here? The fix works to make my tests run.
Posted patch patch v1.0 (obsolete) — Splinter Review
Anything against this approach?
i tested it changing browser_bug304198.js that is actually a random orange (bug 511684), and looks like working.
Attachment #406706 - Flags: review?(enndeakin)
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)
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

10 years ago
I think we should put the code into SimpleTest.waitForFocus, as it would be good to have them work consistently.
sorry for late, i'm catching up, i'll post new patch that will put code in SimpleTest.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Attachment #406706 - Attachment is obsolete: true
Attachment #406706 - Flags: review?(enndeakin)
Posted patch patch v1.1 (obsolete) — Splinter Review
Attachment #408613 - Flags: review?(enndeakin)

Comment 11

10 years ago
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?
Posted patch patch v1.2 (obsolete) — Splinter Review
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?
Attachment #408613 - Attachment is obsolete: true
Attachment #408726 - Flags: review?(enndeakin)
Attachment #408613 - Flags: review?(enndeakin)
(Reporter)

Comment 13

10 years ago
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.
Posted patch patch v1.3Splinter Review
i see, it's calling targetWindow.focus()
Attachment #408726 - Attachment is obsolete: true
Attachment #408732 - Flags: review?
Attachment #408726 - Flags: review?(enndeakin)
Attachment #408732 - Flags: review? → review?(enndeakin)

Updated

10 years ago
Attachment #408732 - Flags: review?(enndeakin) → review+
http://hg.mozilla.org/mozilla-central/rev/032a9a176ee7
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
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.
(Reporter)

Comment 17

10 years ago
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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

10 years ago
(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.
(Reporter)

Updated

10 years ago
Blocks: 525635

Comment 20

10 years ago
(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.
Posted file 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?
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?
(Reporter)

Comment 23

10 years ago
Attachment #410224 - Flags: review?(enndeakin)

Comment 24

10 years ago
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.
(Reporter)

Comment 25

10 years ago
(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

10 years ago
(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.
(Reporter)

Comment 27

10 years ago
(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

10 years ago
> > > 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?
(Reporter)

Comment 29

10 years ago
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.
(Reporter)

Comment 30

10 years ago
Comment on attachment 410224 [details] [diff] [review]
second attempt

will try to do this without the activate event
Attachment #410224 - Flags: review?(enndeakin)
(Reporter)

Updated

10 years ago
Attachment #410224 - Flags: review?(enndeakin)
(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
(Reporter)

Comment 33

10 years ago
(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 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.
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.

Updated

10 years ago
Blocks: 513560
i'm not actively working on this, dao maybe wants to take it
Assignee: mak77 → nobody
(Reporter)

Updated

9 years ago
Blocks: 539002
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.
Depends on: 553248
bug 539002 is probably still the best way to reproduce this bug.

Comment 39

9 years ago
Comment on attachment 410224 [details] [diff] [review]
second attempt

Bug 554873 fixes some issues with waitForFocus. Is this bug still relevant?
Attachment #410224 - Flags: review?(enndeakin)
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

9 years ago
(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.
(Reporter)

Comment 42

9 years ago
(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.
(Reporter)

Updated

9 years ago
No longer depends on: 554873

Comment 43

9 years ago
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?
(Reporter)

Comment 44

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

Comment 45

9 years ago
(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?
(Reporter)

Comment 46

9 years ago
(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

9 years ago
(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.
(Reporter)

Comment 48

9 years ago
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

9 years ago
(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.
(Reporter)

Comment 50

9 years ago
(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

9 years ago
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.
(Reporter)

Comment 52

9 years ago
But I also said that these tests appear to work just fine with the activate event.
(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

9 years ago
(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?
(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

9 years ago
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.
(Reporter)

Comment 57

9 years ago
(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

9 years ago
(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.
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

9 years ago
(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

9 years ago
Sure, would any of you like to review?

Comment 62

9 years ago
(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.  :(
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").
No longer blocks: 525175
Component: BrowserTest → Mochitest
Product: Testing → Testing
(Reporter)

Updated

7 months ago
See Also: → 1492480
You need to log in before you can comment on or make changes to this bug.