Last Comment Bug 662812 - Panorama isn't aware of the current SSWindowState when being initialized
: Panorama isn't aware of the current SSWindowState when being initialized
Status: RESOLVED FIXED
[qa-]
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- normal
: Firefox 9
Assigned To: Tim Taubert [:ttaubert]
:
:
Mentors:
Depends on: 680686
Blocks: 663049 666475
  Show dependency treegraph
 
Reported: 2011-06-08 08:48 PDT by Tim Taubert [:ttaubert]
Modified: 2016-04-12 14:00 PDT (History)
5 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 (3.96 KB, patch)
2011-06-08 09:07 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
patch v2 (15.38 KB, patch)
2011-06-17 13:14 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
patch v3 (14.80 KB, patch)
2011-06-18 00:12 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
patch v4 (14.36 KB, patch)
2011-06-30 06:15 PDT, Tim Taubert [:ttaubert]
paul: review-
paul: feedback+
Details | Diff | Splinter Review
patch v5 (17.15 KB, patch)
2011-08-08 06:28 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
patch v6 (22.48 KB, patch)
2011-08-08 09:41 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
patch v7 (23.34 KB, patch)
2011-08-12 01:36 PDT, Tim Taubert [:ttaubert]
paul: review+
Details | Diff | Splinter Review

Description Tim Taubert [:ttaubert] 2011-06-08 08:48:13 PDT
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] 2011-06-08 09:07:47 PDT
Created attachment 538032 [details] [diff] [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.

How about extending nsISessionStore to have a .isWindowStateBusy() method or the like?
Comment 2 Tim Taubert [:ttaubert] 2011-06-09 05:05:16 PDT
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 reading much bugmail, email directly) 2011-06-14 15:11:57 PDT
(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] 2011-06-17 13:14:06 PDT
Created attachment 540114 [details] [diff] [review]
patch v2
Comment 5 Tim Taubert [:ttaubert] 2011-06-18 00:12:45 PDT
Created attachment 540216 [details] [diff] [review]
patch v3
Comment 6 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-06-21 12:45:52 PDT
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] 2011-06-30 06:15:23 PDT
Created attachment 543114 [details] [diff] [review]
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] 2011-07-24 18:38:12 PDT
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 reading much bugmail, email directly) 2011-08-03 09:40:57 PDT
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] 2011-08-08 06:28:32 PDT
Created attachment 551443 [details] [diff] [review]
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] 2011-08-08 09:41:16 PDT
Created attachment 551482 [details] [diff] [review]
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] 2011-08-12 01:36:38 PDT
Created attachment 552621 [details] [diff] [review]
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 reading much bugmail, email directly) 2011-08-12 10:48:02 PDT
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] 2011-08-16 03:02:27 PDT
http://hg.mozilla.org/integration/fx-team/rev/e68b6ce72fc3
Comment 15 Rob Campbell [:rc] (:robcee) 2011-08-16 08:37:01 PDT
Comment on attachment 552621 [details] [diff] [review]
patch v7

http://hg.mozilla.org/mozilla-central/rev/e68b6ce72fc3
Comment 16 Dão Gottwald [:dao] 2011-08-19 02:10:28 PDT
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] 2011-08-21 07:23:54 PDT
http://hg.mozilla.org/integration/fx-team/rev/c687cfaa9a68
Comment 18 Tim Taubert [:ttaubert] 2011-08-21 23:54:30 PDT
http://hg.mozilla.org/mozilla-central/rev/c687cfaa9a68
Comment 19 Virgil Dicu [:virgil] [QA] 2011-10-05 05:58:55 PDT
Hello.
Can this issue be verified using any known STRs?
Comment 20 Tim Taubert [:ttaubert] 2011-10-05 06:01:35 PDT
(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] 2011-10-05 06:20:16 PDT
Setting to [qa-] then. Thanks.

Note You need to log in before you can comment on or make changes to this bug.