662812 - Panorama isn't aware of the current SSWindowState when being initialized

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

When restoring a window, TabView.init() gets called by SS:

http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js#2555

SSWindowStateBusy is sent right before that and Panorama thinks the window state is ready and starts to do some crazy things. That might as well happen when starting the browser with automatic session restore and quickly opening Panorama. On my machine browser_tabview_bug597248.js is constantly timing out because it tries to call ss.getTabValue() on not yet restored tabs.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

>   init: function UI_init() {
>     try {
>       let self = this;
> 
>+      if (gWindow.__SS_windowStateBusy)
>+        this.storageBusy();

>   _sendWindowStateEvent: function sss__sendWindowStateEvent(aWindow, aType) {
>+    aWindow.__SS_windowStateBusy = (aType == "Busy");
>+
>     let event = aWindow.document.createEvent("Events");
>     event.initEvent("SSWindowState" + aType, true, false);
>     aWindow.dispatchEvent(event);
>   },

While that actually works it might be a bit too hacky. We could at least add a new method _setWindowState() that calls _sendWindowStateEvent(), but it seems not quite right to have a "private" member that is accessed by Panorama.

How about extending nsISessionStore to have a .isWindowStateBusy() method or the like?

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Actually we would even need a windowStateBusyCount because if I do

> for (let i = 0; i < 3; i++)
>   ss.undoCloseTab(0);

SS actually sends 3x SSWindowStateBusy and we would need to wait for 3x SSWindowStateReady to continue.

Comment 3

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

(In reply to comment #1)
> Created attachment 538032 [details] [diff] [review] [review]
> patch v1
> 
> >   init: function UI_init() {
> >     try {
> >       let self = this;
> > 
> >+      if (gWindow.__SS_windowStateBusy)
> >+        this.storageBusy();
> 
> >   _sendWindowStateEvent: function sss__sendWindowStateEvent(aWindow, aType) {
> >+    aWindow.__SS_windowStateBusy = (aType == "Busy");
> >+
> >     let event = aWindow.document.createEvent("Events");
> >     event.initEvent("SSWindowState" + aType, true, false);
> >     aWindow.dispatchEvent(event);
> >   },
> 
> While that actually works it might be a bit too hacky. We could at least add
> a new method _setWindowState() that calls _sendWindowStateEvent(), but it
> seems not quite right to have a "private" member that is accessed by
> Panorama.

I agree that this is a bit too hacky

> How about extending nsISessionStore to have a .isWindowStateBusy() method or
> the like?

What if we just include something in the JSON getWindowState returns? (state is definitely an overloaded term here). Then we can send any arbitrary bits of data we need to.

(In reply to comment #2)
> Actually we would even need a windowStateBusyCount because if I do
> 
> > for (let i = 0; i < 3; i++)
> >   ss.undoCloseTab(0);
> 
> SS actually sends 3x SSWindowStateBusy and we would need to wait for 3x
> SSWindowStateReady to continue.

Well, you'd have to call tabview.init right in the middle there, right? And really, if you get a ready event, then you'd know about the new tabs being added via undoclosetab, right?

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v3

Comment 6

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

Can you clarify (with a use case as it applies to panorama) why we need to count busy states? I don't see a need for that - a window is either ready or it's not. If we're sending out events when we're still busy, then we should fix that.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v4

(In reply to comment #6)
> Can you clarify (with a use case as it applies to panorama) why we need to
> count busy states? I don't see a need for that - a window is either ready or
> it's not. If we're sending out events when we're still busy, then we should
> fix that.

Yeah, I thought a bit about it and concur with you - we don't need that counter for any Panorama use case. The busyState is now a boolean and all tests pass locally.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

bugspam

(Fx7 was branched, removing open bugs from Fx7 meta bug, we won't create new meta bugs for upcoming Fx versions)

Comment 9

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

Comment on attachment 543114 [details] [diff] [review]
patch v4

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

Please add the session restore tests to make sure that window.busyState (or whatever you finally call it) is correct following the SSWindowState* events. r- for that & comments below, but f+ because the approach looks good.

::: browser/components/sessionstore/src/nsSessionStore.js
@@ +746,5 @@
>      // assign it a unique identifier (timestamp)
>      aWindow.__SSi = "window" + Date.now();
>  
>      // and create its data object
> +    this._windows[aWindow.__SSi] = { tabs: [], selected: 0, _closedTabs: [], busyState: false };

1. busyState as a property isn't driving me wild. Perhaps `isBusy` or just `busy` (like we do with pinned) since it's just a bool.
2. It doesn't make much sense to save that property to disk but we can't add it to INTERNAL_KEYS. Maybe we can delete it when saving to disk (probably not worth it to loop over windows though). Wouldn't be the first time we saved unnecessary data.
3. We can delete it when copying the window state to _closedWindows

@@ +3943,5 @@
> +   */
> +  _setWindowStateBusyValue:
> +    function sss__changeWindowStateBusyValue(aWindow, aValue) {
> +
> +    let state = this._windows[aWindow.__SSi].busyState = aValue;

You don't use state, so let's skip that.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v5

> Please add the session restore tests to make sure that window.busyState (or
> whatever you finally call it) is correct following the SSWindowState* events.

Done.

> 1. busyState as a property isn't driving me wild. Perhaps `isBusy` or just `busy`
> (like we do with pinned) since it's just a bool.

Renamed to 'busy'.

> 2. It doesn't make much sense to save that property to disk but we can't add it to
> INTERNAL_KEYS. Maybe we can delete it when saving to disk (probably not worth it
> to loop over windows though). Wouldn't be the first time we saved unnecessary data.

There is no code that loops over all windows when saving the ss state, yet. So I figured it might not be worth to do it just to delete this tiny flag.

> 3. We can delete it when copying the window state to _closedWindows

Done.

> You don't use state, so let's skip that.

Removed.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v6

Added fix for browser/browser_595601-restore_hidden.js expecting that tabview isn't loaded, yet. Updated it to run in a new window.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v7

When I pushed the patch to try again it revealed a pretty basic source of intermittent timeouts when switching between private browsing modes (and browser_tabview_bug624727.js) timed out.

Fixed in head.js: we need to use executeSoon() before calling afterAllTabsLoaded() to let Panorama do its work and put the tabs into groups and call gBrowser.showOnlyTheseTabsAndGroups(). If we don't do that it's possible that afterAllTabsLoaded() waits for hidden tabs to be restored (with restore_hidden_tabs=false).

Comment 13

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

Comment on attachment 552621 [details] [diff] [review]
patch v7

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

Just the one thing. Otherwise It looks good to me. I'm still not a reviewer there (though the changes seem to make sense to me).

::: browser/components/sessionstore/src/nsSessionStore.js
@@ +3997,5 @@
> +    // Keep the to-be-restored state in sync because that is returned by
> +    // getWindowState() as long as the window isn't loaded, yet.
> +    if (!this._isWindowLoaded(aWindow)) {
> +      let {windows} = this._statesToRestore[aWindow.__SS_restoreID];
> +      windows[0].busy = aValue;

I actually liked how you had this before. The destructuring is harder to read.

::: browser/components/sessionstore/test/browser/browser_595601-restore_hidden.js
@@ +116,5 @@
> +    TabsProgressListener.setCallback(callback);
> +
> +    ss.setWindowState(win, JSON.stringify(state), true);
> +  });
> +}

This is the 3rd test (I think) that we have this set of functions in. We should move that into head.js and make it a bit more robust, but don't worry about it here. If for some reason you run out of work and want to do it then, be my guest :)

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

http://hg.mozilla.org/integration/fx-team/rev/e68b6ce72fc3

Comment 15

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 552621 [details] [diff] [review]
patch v7

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

Comment 16

[Dão Gottwald [:dao]]

Backed out for regressing bug 658738 :/
http://hg.mozilla.org/mozilla-central/rev/6181ba4693f9

I'm not sure this patch did anything wrong. You might be hitting a platform bug, maybe you can narrow it down?

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

http://hg.mozilla.org/integration/fx-team/rev/c687cfaa9a68

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 19

[Virgil Dicu [:virgil] [QA]]

Hello.
Can this issue be verified using any known STRs?

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Virgil Dicu [QA] from comment #19)
> Can this issue be verified using any known STRs?

That's a very technical issue and alas can't be verified via the UI.

Comment 21

[Virgil Dicu [:virgil] [QA]]

Setting to [qa-] then. Thanks.