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

RESOLVED FIXED

Status

defect
--
major
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: hsivonen, Assigned: enndeakin)

Tracking

(Blocks 1 bug)

unspecified
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

9 years ago
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.
Blocks: 562328
Blocks: 521233
(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?
(Reporter)

Comment 2

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

Comment 4

9 years ago
(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.
Henri what is your opinion on the right fix for waitForFocus?
(Assignee)

Comment 6

9 years ago
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.
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
No longer blocks: 521233
(Assignee)

Comment 7

9 years ago
Posted patch better patch (obsolete) — Splinter Review
Attachment #443435 - Attachment is obsolete: true
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.
Attachment #445011 - Flags: feedback+
(Assignee)

Comment 9

9 years ago
Attachment #445011 - Attachment is obsolete: true
Attachment #445795 - Flags: review?(mak77)
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.
Attachment #445795 - Flags: review?(mak77) → review+
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.
(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.)
Yeah, ok.
(Assignee)

Comment 14

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

Comment 16

9 years ago
http://hg.mozilla.org/mozilla-central/rev/eb49e81b9f4d
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Pushed http://hg.mozilla.org/mozilla-central/rev/f87fbec86db6 to fix test ordering & remove a disabled test comment in the sessionstore makefile
(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.
You need to log in before you can comment on or make changes to this bug.