554873 - SimpleTest.waitForFocus() is supposed to handle waiting for the load event but it doesn't really do it properly

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Henri Sivonen (:hsivonen)]

Steps to reproduce:
 1) Instrument SimpleTest.waitForFocus() with dump()s to see if it takes the branch that install a 'load' event listener.
 2) Call window.open() with a URL that won't load synchronously from XUL or from HTML in such circumstances that a real window is opened (not a tab!).
 3) Pass the object returned by window.open() to waitForFocus()

Actual results:
waitForFocus decides not to install a 'load' event listener, because the window has a parser-created about:blank with readyState "complete" when window.open() has returned but the event loop hasn't spun further.

Expected results:
Expected waitForFocus to install a 'load' event listener.

Additional info:
 * Making waitForFocus check if about:blank is loaded in the window doesn't work, because there are tests that accidentally depending on the current behavior. Also, ideally, it the SimpleTest library would be unsurprising even when opening a new window with about:blank as the destination URL.
 * waitForFocus appears to work with iframes and tabs, because they have a docshell-created (as opposed to parser-created) initial about:blank with readyState "uninitialized".
 * Checking for readyState is in general unsafe, because it doesn't tell if the document you are looking at is the document being navigated to or the document being navigated away from.

See also bug 546648, bug 543435 and 549539.

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #0)
> Additional info:
>  * Making waitForFocus check if about:blank is loaded in the window doesn't
> work, because there are tests that accidentally depending on the current
> behavior. Also, ideally, it the SimpleTest library would be unsurprising even
> when opening a new window with about:blank as the destination URL.

Are there a lot of tests depending on this behavior?

Maybe we can *add* an aggregate listener that is called when both waitForFocus *and* onload is called, and use it for the tests that are failing with html5 parser + waitForFocus?

Comment 2

[Henri Sivonen (:hsivonen)]

(In reply to comment #1)
> (In reply to comment #0)
> > Additional info:
> >  * Making waitForFocus check if about:blank is loaded in the window doesn't
> > work, because there are tests that accidentally depending on the current
> > behavior. Also, ideally, it the SimpleTest library would be unsurprising even
> > when opening a new window with about:blank as the destination URL.
> 
> Are there a lot of tests depending on this behavior?

There are multiple, see bug 546648 comment 36 and bug 546648 comment 37.

> Maybe we can *add* an aggregate listener that is called when both waitForFocus
> *and* onload is called, and use it for the tests that are failing with html5
> parser + waitForFocus?

Adding another method was rejected in bug 546648. Adding another method would permit a gradual transition to the new method, though.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

what about fixing waitForFocus(), push the fix to a branch (We can still offer you Places branch for experimental landings if needed), fix test failures as they come out, when everything's green port all the fixes to central and revert the branch to a clean state.

Comment 4

[Henri Sivonen (:hsivonen)]

(In reply to comment #3)
> what about fixing waitForFocus(), push the fix to a branch (We can still offer
> you Places branch for experimental landings if needed), fix test failures as
> they come out, when everything's green port all the fixes to central and revert
> the branch to a clean state.

Part of the problem is that there's no agreed-upon way to fix waitForFocus(). As far as I can tell, there's no API for detecting if a window object has been asked to navigate to another URL but hasn't replaced the document object yet.

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

Henri what is your opinion on the right fix for waitForFocus?

Comment 6

[Neil Deakin]

Attachment: patch which seems to fix all tests

I added an extra argument to waitForFocus which indicates that a blank document is expected.

There might be a problem on windows, but I'm not sure if it's caused by this change yet.

Comment 7

[Neil Deakin]

Attachment: better patch

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 445011 [details] [diff] [review]
better patch

>diff --git a/testing/mochitest/browser-test.js b/testing/mochitest/browser-test.js

>-  this.waitForFocus = function test_waitForFocus(callback, targetWindow) {
>-    self.SimpleTest.waitForFocus(callback, targetWindow);
>+  this.waitForFocus = function test_waitForFocus(callback, targetWindow, skipLoadCheck) {
>+    self.SimpleTest.waitForFocus(callback, targetWindow, skipLoadCheck);
>   };

this could probably just be this.waitForFocus = self.SimpleTest.waitForFocus;


>diff --git a/testing/mochitest/tests/SimpleTest/SimpleTest.js b/testing/mochitest/tests/SimpleTest/SimpleTest.js
>--- a/testing/mochitest/tests/SimpleTest/SimpleTest.js
>+++ b/testing/mochitest/tests/SimpleTest/SimpleTest.js
>@@ -233,46 +233,49 @@ SimpleTest.requestLongerTimeout = functi
> 
> SimpleTest.waitForFocus_started = false;
> SimpleTest.waitForFocus_loaded = false;
> SimpleTest.waitForFocus_focused = false;
> 
> /**
>  * If the page is not yet loaded, waits for the load event. If the page is
>  * not yet focused, focuses and waits for the window to be focused. Calls

This is a bit unclear, sounds like if it is not yet loaded it just waits for the load event, but not for focus, while it waits for focus in both cases.


>- * the callback when completed.
>+ * the callback when completed. If the currently loaded page is 'about:blank',
>+ * then the page is assumed to not yet be loaded. Pass true for expectBlankPage
>+ * if you expect a blank page to be present.
>  *
>  * targetWindow should be specified if it is different than 'window'.
>  */

Please make this fake-javadoc a real javadoc with @param definitions


>-SimpleTest.waitForFocus = function (callback, targetWindow) {
>+SimpleTest.waitForFocus = function (callback, targetWindow, expectBlankPage) {

I guess you wanted to rename expectBlankPage to skipLoadCheck as you did in the browser part?
Especially since later there is an additional about:blank check.


>-    var usedTargetWindow = {};
>-    fm.getFocusedElementForWindow(targetWindow, true, usedTargetWindow);
>-    targetWindow = usedTargetWindow.value;
>+    var focusTargetWindow = {};
>+    fm.getFocusedElementForWindow(targetWindow, true, focusTargetWindow);
>+    focusTargetWindow = focusTargetWindow.value;

There is a lot of confusion between targetWindow and focusTargetWindow, since actually it could even be completely not focused, the "focus" prefix is playing me.
Can we name this childTargetWindow instead?


>     function debugFocusLog(prefix) {

>         SimpleTest.ok(true, prefix + " -- loaded: " + targetWindow.document.readyState +
>            " active window: " +
>                (fm.activeWindow ? "(" + fm.activeWindow + ") " + fm.activeWindow.location : "<no window active>") +
>            " focused window: " +
>                (fm.focusedWindow ? "(" + fm.focusedWindow + ") " + fm.focusedWindow.location : "<no window focused>") +
>            " desired window: (" + targetWindow + ") " + targetWindow.location +
>+           " child: (" + focusTargetWindow + ") " + focusTargetWindow.location +

"child window:" would be more consistent


>     function waitForEvent(event) {
>+        if (event.type == "load" && !expectBlankPage && event.target.location == "about:blank")
>+            return;

Actually, if skipLoadCheck (in this case still called expectBlankPage) is true, we don't even add a "load" listener, since waitForFocus_loaded is true, so the !expectBlankPage check looks pointless


>     // wait for the page to load if it hasn't already
>-    SimpleTest.waitForFocus_loaded = (targetWindow.document.readyState == "complete");
>+    SimpleTest.waitForFocus_loaded = expectBlankPage ||
>+        (targetWindow.location != "about:blank" && targetWindow.document.readyState == "complete");

please add a brief comment above regarding the "about:blank" check, add also a "See bug 554873 for details."


>     // check if the window is focused, and focus it if it is not

nit: uppercase-first and add punctation to comments.
This should probably be "Check if the desired window is focused..."


>     var focusedWindow = { };
>     if (fm.activeWindow)
>-      fm.getFocusedElementForWindow(fm.activeWindow, true, focusedWindow);
>+        fm.getFocusedElementForWindow(fm.activeWindow, true, focusedWindow);

what about renaming it to focusedChildWindow?

is it possible that fm.activeWindow is null? In such a case the next call to focusedWindow.value would error, you should probably:
var focusedChildWindow;
if (fm.activeWindow) {
   fm.getFocusedElementForWindow(fm.activeWindow, true, focusedChildWindow);
   focusedChildWindow = focusedChildWindow.value;
}

>     // if this is a child frame, ensure that the frame is focused
>-    SimpleTest.waitForFocus_focused = (focusedWindow.value == targetWindow);
>+    SimpleTest.waitForFocus_focused = (focusedWindow.value == focusTargetWindow);

and use focusedChildWindow directly here.


Apart the above comments, the approach looks good to me, so f+ on that, please get a tryserver pass to the next iteration.

Comment 9

[Neil Deakin]

Attachment: address comments and update tests

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 445795 [details] [diff] [review]
address comments and update tests

>diff --git a/testing/mochitest/tests/SimpleTest/SimpleTest.js b/testing/mochitest/tests/SimpleTest/SimpleTest.js

>+ * If the page is not yet loaded, waits for the load event. In addition, if
>+ * the page is not yet focused, focuses and waits for the window to be
>+ * focused. Calls the callback when completed. If the current page is
>+ * 'about:blank', then the page is assumed to not yet be loaded. Pass true for
>+ * expectBlankPage to not make this assumption if you expect a blank page to
>+ * be present.

nit: double spaces after periods help long comments readability.


>+ * @param callback function called when load and focus are complete

please indent these as (per previous newsgroup discussions, descriptions go on newline):

@param callback
       Function called when...
@param [optional] targetWindow
       Window to be...


>+SimpleTest.waitForFocus = function (callback, targetWindow, expectBlankPage) {

>+    // If the desired document is currently about:blank and we are not
>+    // expecting a blank page or vice versa

"desired document" fights with "we are not expecting", what we expect is probably what we desire.
I think this should read as:
"If the current document is about:blank and we are not expecting a blank page (or vice versa) and..."
But since i'm not English mother tongue, I leave this up to you.


>+    // Check if the desired window is focused, and focus it if it is not.

nit: s/if it is not/otherwise/


>+    var focusedChildWindow = { };

please use consistent style for empty objects, above you used {} without space in the middle.


>+    if (fm.activeWindow) {
>+        fm.getFocusedElementForWindow(fm.activeWindow, true, focusedChildWindow);
>+        focusedChildWindow = focusedChildWindow.value;
>+    }
>+  

trailing spaces here

I don't see show-stoppers here, and Ehsan confirmed this was helping his cases, thus I think you can land.
Thanks for having made this more readable/understandable.

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Neil, actually, I still have just one doubt about this check:
+    SimpleTest.waitForFocus_loaded =
+        (expectBlankPage == (targetWindow.location == "about:blank")) &&
+        targetWindow.document.readyState == "complete";

suppose that a browser-chrome test opens a window and runs some test in it, then the test closed the win and finishes. At that point browser-test would waitForFocus browser window just to ensure it is focused before starting next test. expectBlankPage is false based on latest patch from ehsan, and location is most likely about:blank. this is making waitForFocus_loaded = false, and I suppose we won't get any load event.

Comment 12

[(no longer active)]

(In reply to comment #11)
> Neil, actually, I still have just one doubt about this check:
> +    SimpleTest.waitForFocus_loaded =
> +        (expectBlankPage == (targetWindow.location == "about:blank")) &&
> +        targetWindow.document.readyState == "complete";
> 
> suppose that a browser-chrome test opens a window and runs some test in it,
> then the test closed the win and finishes. At that point browser-test would
> waitForFocus browser window just to ensure it is focused before starting next
> test. expectBlankPage is false based on latest patch from ehsan, and location
> is most likely about:blank. this is making waitForFocus_loaded = false, and I
> suppose we won't get any load event.

Is targetWindow here the window on which waitForFocus is called?  In that case, I don't think this is an issue, because in browser-test.js, I call waitForFocus on the chrome window object (a.k.a the js object for the OS level window.)

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Yeah, ok.

Comment 14

[Neil Deakin]

(In reply to comment #11)At that point browser-test would
> waitForFocus browser window just to ensure it is focused before starting next
> test.

The next test should be using waitForFocus if it needs to.

Comment 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #14)
> (In reply to comment #11)At that point browser-test would
> > waitForFocus browser window just to ensure it is focused before starting next
> > test.
> 
> The next test should be using waitForFocus if it needs to.

that was inserted as a way to reduce those oranges caused by previous tests forgetting (willing or not) to put back focus in place, and next test just arguing browser window is focused on start. asking each test to set focus on start does not look like a win if we can ensure that automatically.

Comment 16

[Neil Deakin]

http://hg.mozilla.org/mozilla-central/rev/eb49e81b9f4d

Comment 17

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

Pushed http://hg.mozilla.org/mozilla-central/rev/f87fbec86db6 to fix test ordering & remove a disabled test comment in the sessionstore makefile

Comment 18

[Serge Gautherie (:sgautherie)]

(In reply to Neil Deakin from comment #16)
> http://hg.mozilla.org/mozilla-central/rev/eb49e81b9f4d

Part of the m-c changeset landed in m-1.9.2b1:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/7915c3bcf85d

Comment 19

[Serge Gautherie (:sgautherie)]

(In reply to Serge Gautherie (:sgautherie) from comment #18)
> (In reply to Neil Deakin from comment #16)
> > http://hg.mozilla.org/mozilla-central/rev/eb49e81b9f4d
> 
> Part of the m-c changeset landed in m-1.9.2b1:
> http://hg.mozilla.org/releases/mozilla-1.9.2/rev/7915c3bcf85d

Ignore this comment.