940804 - Make session store tests use SSTabRestored when appropriate

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: test-changes

The session store tests often use the "load" event to mean "restoring the tab is done", which isn't really correct. It also won't work well in e10s, since the load event fires in the child. This patch modifies a bunch of tests to wait for the SSTabRestored event instead.

I also made a few other small changes:

- Used the whenBrowserLoaded function instead of waiting on "load" directly. This will make it easier to switch to e10s, since we can instead wait on a message or something.

- Took out some calls to stopPropagation() where the comment was "Call stopPropagation on the event so we won't fire the tabbrowser's SSTabRestored listeners." I don't think that the tabbrowser element has any SSTabRestored listeners, so I'm guessing that these are obsolete.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: tab-previews-fix

I found a case where a test was removing the tab during the SSTabRestored event. Then the event was handled by this tabPreview code, and it doesn't handle the possibility that the tab is no longer in the document. So I just added a check. I'm not sure what else we can do about it.

(Perhaps this is what the stopPropagation calls were for? I guess we could also solve the problem that way, but this seems more direct.)

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Bill McCloskey (:billm) from comment #0)
> - Took out some calls to stopPropagation() where the comment was "Call
> stopPropagation on the event so we won't fire the tabbrowser's SSTabRestored
> listeners." I don't think that the tabbrowser element has any SSTabRestored
> listeners, so I'm guessing that these are obsolete.

IIRC, these stopPropagation() calls were put there to make tests work that would call gBrowser.removeTab() in a SSTabRestored event handler. Not sure why we did that but I think the better solution would have been to use executeSoon() to continue on the next tick or something equivalent.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Bill McCloskey (:billm) from comment #1)
> I found a case where a test was removing the tab during the SSTabRestored
> event. Then the event was handled by this tabPreview code, and it doesn't
> handle the possibility that the tab is no longer in the document. So I just
> added a check. I'm not sure what else we can do about it.
> 
> (Perhaps this is what the stopPropagation calls were for? I guess we could
> also solve the problem that way, but this seems more direct.)

Oh, I just read that comment. Yes :)

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8335007 [details] [diff] [review]
test-changes

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

Thanks for the cleanup!

::: browser/base/content/test/general/browser_bug623893.js
@@ +32,3 @@
>  
> +  tab.addEventListener("SSTabRestored", function () {
> +    tab.removeEventListener("SSTabRestored", arguments.callee, false);

Would be better to have a function name than to use arguments.callee, which isn't available in strict mode.

::: browser/components/sessionstore/test/browser_367052.js
@@ +18,5 @@
>      let history = tab.linkedBrowser.webNavigation.sessionHistory;
>      ok(history.count >= 1, "the new tab does have at least one history entry");
>  
>      ss.setTabState(tab, JSON.stringify({ entries: [] }));
> +    whenBrowserLoaded(tab.linkedBrowser, function() {

Should be whenTabRestored(), no?

::: browser/components/sessionstore/test/browser_447951.js
@@ +33,4 @@
>          tabState = JSON.parse(ss.getTabState(tab));
> +        if (tabState.entries[tabState.entries.length - 1].url != baseURL + "end") {
> +          // It may take a few passes through the event loop before we
> +          // get the right URL.

Sorry I don't understand that change?

::: browser/components/sessionstore/test/browser_463206.js
@@ +30,5 @@
>      typeText(doc.getElementById("out1"), Date.now());
>      typeText(doc.getElementsByName("1|#out2")[0], Math.random());
>      typeText(doc.defaultView.frames[0].frames[1].document.getElementById("in1"), new Date());
>  
>      frameCount = 0;

Looks like we don't need that anymore.

::: browser/components/sessionstore/test/browser_465215.js
@@ +24,5 @@
>      isnot(ss.getTabValue(tab1, uniqueName), uniqueValue2, "tab values aren't sync'd");
>  
>      // overwrite the tab with the value which should remove it
>      ss.setTabState(tab1, JSON.stringify({ entries: [] }));
> +    whenBrowserLoaded(tab1.linkedBrowser, function() {

whenTabRestored()?

::: browser/components/sessionstore/test/browser_819510_perwindowpb.js
@@ +182,2 @@
>      executeSoon(aCallback);
> +  });

Nit: whenBrowserLoaded(aWin.gBrowser.selectedBrowser, aCallback);

::: browser/components/sessionstore/test/head.js
@@ +303,5 @@
>  
> +function whenTabRestored(aTab, aCallback = next) {
> +  aTab.addEventListener("SSTabRestored", function onRestored(aEvent) {
> +    aTab.removeEventListener("SSTabRestored", onRestored, true);
> +    aCallback();

That should be executeSoon(aCallback);

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8335008 [details] [diff] [review]
tab-previews-fix

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

::: browser/base/content/browser-tabPreviews.js
@@ +100,5 @@
> +        let tab = event.target;
> +        if (tab.parentNode) {
> +          // The tab could have been removed in a previous
> +          // SSTabRestored event handler. This happens a lot in tests.
> +          this.capture(tab, true);

By using executeSoon() in whenTabRestored() from the previous patch we will not need this.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I think I tried to run the SSTabRestored handler inside executeSoon and it didn't work. I'll try it when I get into the office. If it doesn't work, should I just put the removeTab calls inside executeSoon?

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Tim Taubert [:ttaubert] from comment #4)
> ::: browser/components/sessionstore/test/browser_367052.js
> @@ +18,5 @@
> >      let history = tab.linkedBrowser.webNavigation.sessionHistory;
> >      ok(history.count >= 1, "the new tab does have at least one history entry");
> >  
> >      ss.setTabState(tab, JSON.stringify({ entries: [] }));
> > +    whenBrowserLoaded(tab.linkedBrowser, function() {
> 
> Should be whenTabRestored(), no?

Originally this caused problems because SSTabRestored fires at a different time when tabState.entries.length == 0. However, the simplify-about-blank patch in bug 940777 fixes that. I've made the change and everything seems to work fine.

> 
> ::: browser/components/sessionstore/test/browser_447951.js
> @@ +33,4 @@
> >          tabState = JSON.parse(ss.getTabState(tab));
> > +        if (tabState.entries[tabState.entries.length - 1].url != baseURL + "end") {
> > +          // It may take a few passes through the event loop before we
> > +          // get the right URL.
> 
> Sorry I don't understand that change?

For whatever reason, if I change this test to use whenTabRestored, and if whenTabRestored uses executeSoon to run the callback, then this test fails. It doesn't see the new URL that the click event is supposed to add to the history. The way it uses executeSoon seemed pretty sketchy to me, so I changed it to wait until it got the expected data.

I could look into the problem more, but it didn't seem worth it at the time.

> ::: browser/components/sessionstore/test/head.js
> @@ +303,5 @@
> >  
> > +function whenTabRestored(aTab, aCallback = next) {
> > +  aTab.addEventListener("SSTabRestored", function onRestored(aEvent) {
> > +    aTab.removeEventListener("SSTabRestored", onRestored, true);
> > +    aCallback();
> 
> That should be executeSoon(aCallback);

I know I tried this before and it didn't work, but it seems to work now. Maybe I fixed something in between.

I've made all the other changes you requested. I'll land once bug 940777 lands, since this now depends on that.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4bc4fec5983f

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4bc4fec5983f