1073513 - Closed windows are revived in the wrong order when closing them one after the other until quitting

Status: RESOLVED DUPLICATE of bug 1073992

(Firefox :: Session Restore, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

In SessionStore.onClose() we do the following:

> this._closedWindows.unshift(winData);

This means that the most-recently closed window will always be the first in the array. Now let's assume that we close a window and do something that calls _clearRestoringWindows() e.g. open a new tab and do some more surfing.

With two windows still open we now close one after the other. Both will be put in _closedWindows[] as the first two entries and have _shouldRestore=true set.

In SessionSaver.jsm we now want to revive those two entries. SessionSaver._saveState() however iteraters _closedWindows[] backwards until it hits a window with _shouldRestore=false. It's immediately hitting the window we closed a long time ago and doesn't restore any more windows.

When shutting down and hitting the race condition in bug 1020831 we would now end up saving a session with only closed windows. In normal conditions getCurrentState() would restore the first closed window but not the second.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1073513-Revive-windows-in-the-correct-order-when.patch

Iterating the array normally should do the trick. Putting them into the first slot of state.windows[] when reviving should also be the right thing because those first in the array will be created earlier on startup and thus be in the background.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1073513-Revive-windows-in-the-correct-order-when.patch, v2

Fix tiny typo.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8495925 [details] [diff] [review]
0002-Bug-1073513-Revive-windows-in-the-correct-order-when.patch, v2

I'm stupid and need to make a small fix.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1073513-Revive-windows-in-the-correct-order-when.patch, v3

A wild test appears.

I fixed the loop and use Utils.shallowCopy() now to get a copy of _closedWindows[]. We need to ensure that saving doesn't remove the _shouldRestore properties or otherwise closing windows one-by-one until quit could fail to revive windows if we have an async save in between.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8496160 [details] [diff] [review]
0002-Bug-1073513-Revive-windows-in-the-correct-order-when.patch, v3

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

Almost r+. I just want to understand why, in the test, we want to reopen the windows in that specific order.

::: browser/components/sessionstore/SessionStore.jsm
@@ +2080,5 @@
>        }
>      }
>  
>      // shallow copy this._closedWindows to preserve current state
> +    let lastClosedWindowsCopy = this._closedWindows.map(Utils.shallowCopy);

What's the point of this call to `Utils.shallowCopy`? Maintaining `_shouldRestore`? If so, could you please mention it?

::: browser/components/sessionstore/test/browser_empty_session.js
@@ +5,5 @@
> +
> +const URL_MAIN_WINDOW = "about:mozilla?r=" + Math.random();
> +const URL_ADD_WINDOW1 = "about:mozilla?r=" + Math.random();
> +const URL_ADD_WINDOW2 = "about:mozilla?r=" + Math.random();
> +const URL_CLOSED_WINDOW = "about:mozilla?r=" + Math.random();

Nit: Could you add the name of the test in the url? I have found this helpful when debugging sequences of tests that fail only when run together.

@@ +52,5 @@
> +
> +add_task(function* () {
> +  // Repeat the checks once.
> +  for (let i = 0; i < 2; i++) {
> +    info(`checking window data, iteration #${i}`);

I'm not familiar with that "here var" syntax in JS. Where does it come from?

@@ +74,5 @@
> +      "correct main window");
> +
> +    // Check that we removed _shouldRestore before saving.
> +    ok(windows.every(win => !win.hasOwnProperty("_shouldRestore")),
> +      "no window should have the _shouldRestore property");

Good idea.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #5)
> Almost r+. I just want to understand why, in the test, we want to reopen the
> windows in that specific order.

As described shortly in comment #1, the last window that was closed should also be the last window to be restored upon startup. When creating windows on startup one after the other they will be on top of the windows that were created before them. So if you close window 1, 2, 3 then we will create them in the order 3, 2, 1 so that we end up with 3 in the background, 2 in the middle and 1 as the topmost window.

> >      // shallow copy this._closedWindows to preserve current state
> > +    let lastClosedWindowsCopy = this._closedWindows.map(Utils.shallowCopy);
> 
> What's the point of this call to `Utils.shallowCopy`? Maintaining
> `_shouldRestore`? If so, could you please mention it?

With bug 1073992 we actually don't need to touch that. I'll merge the two bugs.

> > +const URL_CLOSED_WINDOW = "about:mozilla?r=" + Math.random();
> 
> Nit: Could you add the name of the test in the url? I have found this
> helpful when debugging sequences of tests that fail only when run together.

Sure.

> > +    info(`checking window data, iteration #${i}`);
> 
> I'm not familiar with that "here var" syntax in JS. Where does it come from?

ES6 template strings landed: http://tc39wiki.calculist.org/es6/template-strings/

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

> > What's the point of this call to `Utils.shallowCopy`? Maintaining
> > `_shouldRestore`? If so, could you please mention it?
> 
> With bug 1073992 we actually don't need to touch that. I'll merge the two
> bugs.

Duping. I'll move the test over.