528451 - sessionstore-browser-state-restored is not reliable regarding closed windows

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Marco Bonardo [:mak]]

sessionstore-browser-state-set fires after setBrowserState finishes, but at that point windows are still closing, so the browser state is only partially set.
PrivateBrowsing will wait for this notification before starting, and some test do the same, so i think it should not lie, and fire only when the state has been really set (all windows that should be open are open, all windows that should be closed are closed).

Comment 1

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 412178 [details] [diff] [review]
patch v1.0

paul is busy with blockers, removing request to him.

Comment 3

[Simon Bünzli]

(In reply to comment #0)
> (all windows that should be open are open, all windows that
> should be closed are closed).

Isn't that already the case (with the exception of the window mediator still keeping track of closed windows for a few further cycles)?

And in what cases is the difference between "some windows still closing" and "all closing windows gone" relevant? Could this affect Private Browsing or tests not using the window mediator?

Comment 4

[(no longer active)]

(In reply to comment #3)
> And in what cases is the difference between "some windows still closing" and
> "all closing windows gone" relevant? Could this affect Private Browsing or
> tests not using the window mediator?

If there are some windows that are still closing, and the sessionstore component keeps track of those windows, when we switch the private browsing mode, the state of those windows will be saved, and they will be restored later when exiting that mode.  The effect of this from a user's perspective is that they see a window that they had closed come back after exiting the private browsing mode, which is definitely not the behavior that we want.

Please note the correlation between when sessionstore-browser-state-set is fired and whether sessionstore keeps track of windows for which the closed property is true (bug 528440).

Comment 5

[(no longer active)]

This bug can affect the fix of bug 526194, which is a 3.6 blocker, so I think this should also block the release of Firefox 3.6.

Comment 6

[Dão Gottwald [:dao]]

(In reply to comment #4)
> If there are some windows that are still closing, and the sessionstore
> component keeps track of those windows, when we switch the private browsing
> mode, the state of those windows will be saved, and they will be restored later
> when exiting that mode.  The effect of this from a user's perspective is that
> they see a window that they had closed come back after exiting the private
> browsing mode, which is definitely not the behavior that we want.

I don't think we're talking about seconds here, so I don't think a user can close a window and enter private browsing while that window is still in the intermediate non-destroyed state.

Comment 7

[(no longer active)]

(In reply to comment #6)
> (In reply to comment #4)
> > If there are some windows that are still closing, and the sessionstore
> > component keeps track of those windows, when we switch the private browsing
> > mode, the state of those windows will be saved, and they will be restored later
> > when exiting that mode.  The effect of this from a user's perspective is that
> > they see a window that they had closed come back after exiting the private
> > browsing mode, which is definitely not the behavior that we want.
> 
> I don't think we're talking about seconds here, so I don't think a user can
> close a window and enter private browsing while that window is still in the
> intermediate non-destroyed state.

Oh, they can.  This is almost what bug 526194 is about...

Comment 8

[Dão Gottwald [:dao]]

Can you please CC me there?

Comment 9

[(no longer active)]

(In reply to comment #8)
> Can you please CC me there?

Of course.  Sorry I forgot that you can't see that bug...

Comment 10

[Simon Bünzli]

(In reply to comment #4)
> Please note the correlation between when sessionstore-browser-state-set is
> fired and whether sessionstore keeps track of windows for which the closed
> property is true (bug 528440).

So with bug 528440 fixed, this issue becomes moot, then? Else, please also CC me on bug 526194.

Comment 11

[(no longer active)]

(In reply to comment #10)
> (In reply to comment #4)
> > Please note the correlation between when sessionstore-browser-state-set is
> > fired and whether sessionstore keeps track of windows for which the closed
> > property is true (bug 528440).
> 
> So with bug 528440 fixed, this issue becomes moot, then? Else, please also CC
> me on bug 526194.

I guess not, but I'll CC you on that bug anyway.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Sigh, the ever growing blocker tree from bug 526194 ...

Comment 13

[Marco Bonardo [:mak]]

blockers without owners are insane, taking for now.

Comment 14

[Simon Bünzli]

Comment on attachment 412178 [details] [diff] [review]
patch v1.0

r+=me with one nit fixed: Please compare array lengths against numbers instead of evaluating them as boolean expressions (i.e. "!a.length" should be "a.length == 0" and "a.length" should be "a.length > 0").

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

Can we get this in and closed off?

Comment 16

[Marco Bonardo [:mak]]

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

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 412178 [details] [diff] [review]
patch v1.0

>+    var self = this;
>+    // close all other browser windows
>+    this._forEachBrowserWindow(function(aWindow) {
>+      if (aWindow != window) {
>+        self._closingWindows.push(aWindow);
>+        aWindow.close();
>+      }
>+    });

You can use 'this' inside of the callback.

Comment 18

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/982a57b405a9