892766 - SessionStore tests should wait for delayed startup to be finished when opening new windows

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

I have no idea what changed lately but tests time out big time for me locally. Turns out lots of tests just open new windows and expect them to be ready. They need to wait for browser-delayed-startup-finished.

This blocks bug 891360.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: make tests wait for delayed startup to be finished when opening new windows

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: [without white space changes] make tests wait for delayed startup to be finished when opening new windows

That's the same patch just without the white space changes for easier reviewing. Flagging for feedback just for better visibility :)

Comment 3

[:Gavin Sharp [email: [e-mail]]]

This sounds related to Steven's promise bug - presumably this fixes some of his failing tests?

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Yes, indeed. I was about to notify him, thanks for doing that :)

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Looks good on try:

https://tbpl.mozilla.org/?tree=Try&rev=355dd4e7d8ec

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 774327 [details] [diff] [review]
[without white space changes] make tests wait for delayed startup to be finished when opening new windows

Review of attachment 774327 [details] [diff] [review]:
-----------------------------------------------------------------

I'm a bit unclear about why the tests pass. A little documentation in head.js might be useful, and I'm also interested in a few clarifications.

::: browser/components/sessionstore/test/browser_354894_perwindowpb.js
@@ +195,5 @@
>      if (aIsPrivateWindow) {
>        options = {private: true};
>      }
>  
> +    whenNewWindowLoaded(options, function (newWin) {

I don't understand why this one works. While the original version looks strange, it seems to wait for both the window and the browser to have received a "load" event. Does whenNewWindowLoaded do that, too?

::: browser/components/sessionstore/test/browser_819510_perwindowpb.js
@@ +173,5 @@
>    Services.prefs.setIntPref("browser.sessionstore.interval", 0);
>  }
>  
>  function testOnWindow(aIsPrivate, aCallback) {
> +  whenNewWindowLoaded({private: aIsPrivate}, aCallback);

So here, we're not waiting for the window to be activated?

::: browser/components/sessionstore/test/browser_dying_cache.js
@@ +18,4 @@
>  
>    // Load some URL in the current tab.
> +  let flags = Ci.nsIWebNavigation.LOAD_FLAGS_REPLACE_HISTORY;
> +  win.gBrowser.selectedBrowser.loadURIWithFlags("about:robots", flags);

Why do we need this flag?

::: browser/components/sessionstore/test/head.js
@@ +291,5 @@
>  
>  function whenNewWindowLoaded(aOptions, aCallback) {
>    let win = OpenBrowserWindow(aOptions);
> +  whenDelayedStartupFinished(win, () => aCallback(win));
> +  return win;

So how does whenDelayedStartupFinished actually implement ensure that the window is properly loaded?

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #6)
> > +    whenNewWindowLoaded(options, function (newWin) {
> 
> I don't understand why this one works. While the original version looks
> strange, it seems to wait for both the window and the browser to have
> received a "load" event. Does whenNewWindowLoaded do that, too?

No, we don't need to wait for the first tab to be loaded. I guess that was some kind of hackish way to fix failing tests back then. The first tab load is no indication of the window's state. We need to wait for browser-delayed-startup-finished.

> >  function testOnWindow(aIsPrivate, aCallback) {
> > +  whenNewWindowLoaded({private: aIsPrivate}, aCallback);
> 
> So here, we're not waiting for the window to be activated?

No we don't need that. I have no idea why it was in there and I haven't seen this before in tests that deal with opening new windows.

> >    // Load some URL in the current tab.
> > +  let flags = Ci.nsIWebNavigation.LOAD_FLAGS_REPLACE_HISTORY;
> > +  win.gBrowser.selectedBrowser.loadURIWithFlags("about:robots", flags);
> 
> Why do we need this flag?

The test re-uses the first tab in a window and checks its serialized history at the bottom. If there's about:home in there (because that's the first history entry) it would fail. So it's better to just replace the whole history when loading about:robots.

> >  function whenNewWindowLoaded(aOptions, aCallback) {
> >    let win = OpenBrowserWindow(aOptions);
> > +  whenDelayedStartupFinished(win, () => aCallback(win));
> > +  return win;
> 
> So how does whenDelayedStartupFinished actually implement ensure that the
> window is properly loaded?

It waits for the browser-delayed-startup-finished notification. delayedStartup() is called one tick after the window received its 'load' event. We definitely need to wait for this notification as bug 881049 might defer this even more.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

(In reply to Tim Taubert [:ttaubert] from comment #7)
> > So how does whenDelayedStartupFinished actually implement ensure that the
> > window is properly loaded?
> 
> It waits for the browser-delayed-startup-finished notification.
> delayedStartup() is called one tick after the window received its 'load'
> event. We definitely need to wait for this notification as bug 881049 might
> defer this even more.

In that case, could you please document this point?

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 774327 [details] [diff] [review]
[without white space changes] make tests wait for delayed startup to be finished when opening new windows

Review of attachment 774327 [details] [diff] [review]:
-----------------------------------------------------------------

I'm a bit unclear about why the tests pass. A little documentation in head.js might be useful, and I'm also interested in a few clarifications.

::: browser/components/sessionstore/test/browser_354894_perwindowpb.js
@@ +195,5 @@
>      if (aIsPrivateWindow) {
>        options = {private: true};
>      }
>  
> +    whenNewWindowLoaded(options, function (newWin) {

I don't understand why this one works. While the original version looks strange, it seems to wait for both the window and the browser to have received a "load" event. Does whenNewWindowLoaded do that, too?

::: browser/components/sessionstore/test/browser_819510_perwindowpb.js
@@ +173,5 @@
>    Services.prefs.setIntPref("browser.sessionstore.interval", 0);
>  }
>  
>  function testOnWindow(aIsPrivate, aCallback) {
> +  whenNewWindowLoaded({private: aIsPrivate}, aCallback);

So here, we're not waiting for the window to be activated?

::: browser/components/sessionstore/test/browser_dying_cache.js
@@ +18,4 @@
>  
>    // Load some URL in the current tab.
> +  let flags = Ci.nsIWebNavigation.LOAD_FLAGS_REPLACE_HISTORY;
> +  win.gBrowser.selectedBrowser.loadURIWithFlags("about:robots", flags);

Why do we need this flag?

::: browser/components/sessionstore/test/head.js
@@ +291,5 @@
>  
>  function whenNewWindowLoaded(aOptions, aCallback) {
>    let win = OpenBrowserWindow(aOptions);
> +  whenDelayedStartupFinished(win, () => aCallback(win));
> +  return win;

So how does whenDelayedStartupFinished actually implement ensure that the window is properly loaded?

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Ah, sorry, I just meant to change the flag to feedback+ and somehow it reprinted all my comments. Please ignore the above comments.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #8)
> (In reply to Tim Taubert [:ttaubert] from comment #7)
> > > So how does whenDelayedStartupFinished actually implement ensure that the
> > > window is properly loaded?
> > 
> > It waits for the browser-delayed-startup-finished notification.
> > delayedStartup() is called one tick after the window received its 'load'
> > event. We definitely need to wait for this notification as bug 881049 might
> > defer this even more.
> 
> In that case, could you please document this point?

Of course.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Attachment: make tests wait for delayed startup to be finished when opening new windows, v2

Added document for whenNewWindowLoaded() and whenDelayedStartupFinished() to head.js.

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 774545 [details] [diff] [review]
make tests wait for delayed startup to be finished when opening new windows, v2

Review of attachment 774545 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/0d144e4351b5

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0d144e4351b5