The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 9

Status

Firefox Graveyard
Panorama
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

Trunk
Firefox 9
Dependency tree / graph

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

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

Comment 1

6 years ago
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?
Attachment #538032 - Flags: feedback?(paul)
(Assignee)

Updated

6 years ago
Blocks: 663049
(Assignee)

Comment 2

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

Comment 4

6 years ago
Created attachment 540114 [details] [diff] [review]
patch v2
Attachment #538032 - Attachment is obsolete: true
Attachment #538032 - Flags: feedback?(paul)
Attachment #540114 - Flags: review?(paul)
(Assignee)

Comment 5

6 years ago
Created attachment 540216 [details] [diff] [review]
patch v3
Attachment #540114 - Attachment is obsolete: true
Attachment #540114 - Flags: review?(paul)
Attachment #540216 - Flags: review?(paul)
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.
(Assignee)

Updated

6 years ago
Blocks: 666475
(Assignee)

Comment 7

6 years ago
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.
Attachment #540216 - Attachment is obsolete: true
Attachment #540216 - Flags: review?(paul)
Attachment #543114 - Flags: review?(paul)
(Assignee)

Comment 8

6 years ago
bugspam

(Fx7 was branched, removing open bugs from Fx7 meta bug, we won't create new meta bugs for upcoming Fx versions)
No longer blocks: 660175
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.
Attachment #543114 - Flags: review?(paul)
Attachment #543114 - Flags: review-
Attachment #543114 - Flags: feedback+
(Assignee)

Comment 10

6 years ago
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.
Attachment #543114 - Attachment is obsolete: true
Attachment #551443 - Flags: review?(paul)
(Assignee)

Comment 11

6 years ago
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.
Attachment #551443 - Attachment is obsolete: true
Attachment #551443 - Flags: review?(paul)
Attachment #551482 - Flags: review?(paul)
(Assignee)

Comment 12

6 years ago
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).
Attachment #551482 - Attachment is obsolete: true
Attachment #551482 - Flags: review?(paul)
Attachment #552621 - Flags: review?(paul)
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 :)
Attachment #552621 - Flags: review?(paul) → review+
(Assignee)

Comment 14

6 years ago
http://hg.mozilla.org/integration/fx-team/rev/e68b6ce72fc3
Whiteboard: [fixed-in-fx-team]
Comment on attachment 552621 [details] [diff] [review]
patch v7

http://hg.mozilla.org/mozilla-central/rev/e68b6ce72fc3
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 8
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?
Status: RESOLVED → REOPENED
status-firefox8: --- → fixed
Resolution: FIXED → ---
Whiteboard: [fixed-in-fx-team]
(Assignee)

Updated

6 years ago
Depends on: 680686
(Assignee)

Comment 17

6 years ago
http://hg.mozilla.org/integration/fx-team/rev/c687cfaa9a68
Whiteboard: [fixed-in-fx-team]
(Assignee)

Comment 18

6 years ago
http://hg.mozilla.org/mozilla-central/rev/c687cfaa9a68
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: Firefox 8 → Firefox 9
Hello.
Can this issue be verified using any known STRs?
Whiteboard: [qa?]
(Assignee)

Comment 20

6 years ago
(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.
Setting to [qa-] then. Thanks.
Whiteboard: [qa?] → [qa-]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.