1073992 - When closing windows one by one until quitting we should be able to revive more windows than max_undo_windows

Status: NEW

(Firefox :: Session Restore, defect, P3)

Description

[Tim Taubert [:ttaubert] (inactive)]

_closedWindows[] is capped by the max_undo_windows preference. At a default of three this means that a user could never close four windows and get all of them restored at the next start. Getting rid of the _shouldRestore flag would be nice too.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0004-Bug-1073992-Keep-track-of-revivable-windows-separate.patch

Comment 2

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

Comment on attachment 8496604 [details] [diff] [review]
0004-Bug-1073992-Keep-track-of-revivable-windows-separate.patch

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

Nice cleanup.

::: browser/components/sessionstore/RevivableWindows.jsm
@@ +16,5 @@
> + * from state._closedWindows[] to state.windows[] so that they're opened
> + * automatically on next startup. This feature lets us properly support
> + * closing windows in succession until the browser quits.
> + *
> + * We have to keep track of revivable closed windows in a separate module to

Nit: That sentence is a bit weird. Can you just say that this list is not capped by preferences?

@@ +23,5 @@
> + * closing then the user might lose data upon next startup.
> + */
> +this.RevivableWindows = Object.freeze({
> +  // Returns whether there are windows to revive.
> +  get empty() {

Nit: per our coding conventions, this should be `isEmpty`.

@@ +28,5 @@
> +    return closedWindows.length == 0;
> +  },
> +
> +  // Add a window to the list.
> +  add(winState) {

Oh, so we finally have a reasonable syntax for methods in JS? \o/
When did we get that?

::: browser/components/sessionstore/SessionSaver.jsm
@@ +212,5 @@
> +    // the SessionSaver because we only want to revive when saving to disk.
> +    // On Mac OS X this list will always be empty.
> +    let windowsToRevive = RevivableWindows.get();
> +    state.windows.unshift(...windowsToRevive);
> +    state._closedWindows.splice(0, windowsToRevive.length);

Could we add a DEBUG-only check that the elements [0, windowsToReviveLength[ of `state._closedWindows` match `windowsToRevive`?

::: browser/components/sessionstore/test/browser.ini
@@ +83,5 @@
>  [browser_merge_closed_tabs.js]
>  [browser_pageStyle.js]
>  [browser_privatetabs.js]
> +[browser_revive_windows.js]
> +skip-if = os == "mac"

Shouldn't you rather make this test have a different behavior on Mac and non-Mac?

::: browser/components/sessionstore/test/browser_revive_windows.js
@@ +2,5 @@
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +const URL = "about:mozilla?r=" + Math.random();

Nit: Could you add the test name?

@@ +19,5 @@
> +    ss.forgetClosedWindow(0);
> +  }
> +});
> +
> +add_task(function* () {

Nit: Could you give a name to the generator?

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #2)
> > + * We have to keep track of revivable closed windows in a separate module to
> 
> Nit: That sentence is a bit weird. Can you just say that this list is not
> capped by preferences?

Done.

> > +  get empty() {
> 
> Nit: per our coding conventions, this should be `isEmpty`.

Done.

> > +  // Add a window to the list.
> > +  add(winState) {
> 
> Oh, so we finally have a reasonable syntax for methods in JS? \o/
> When did we get that?

Not too long ago, I really like it :)

> > +    let windowsToRevive = RevivableWindows.get();
> > +    state.windows.unshift(...windowsToRevive);
> > +    state._closedWindows.splice(0, windowsToRevive.length);
> 
> Could we add a DEBUG-only check that the elements [0, windowsToReviveLength[
> of `state._closedWindows` match `windowsToRevive`?

We could do that but I honestly don't quite see the value. The invariant here is covered by the test, no?

> > +[browser_revive_windows.js]
> > +skip-if = os == "mac"
> 
> Shouldn't you rather make this test have a different behavior on Mac and
> non-Mac?

Yeah, good idea. Did that.

> > +const URL = "about:mozilla?r=" + Math.random();
> 
> Nit: Could you add the test name?

Done.

> > +add_task(function* () {
> 
> Nit: Could you give a name to the generator?

Done.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1073992-Keep-track-of-revivable-windows-separate.patch, v2

Merged the test from bug 1073513 and addressed review comments.

Comment 6

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

(In reply to Tim Taubert [:ttaubert] from comment #4)
> > Could we add a DEBUG-only check that the elements [0, windowsToReviveLength[
> > of `state._closedWindows` match `windowsToRevive`?
> 
> We could do that but I honestly don't quite see the value. The invariant
> here is covered by the test, no?

I would sleep more easily, which is of high value to me :)
The code of Session Restore, and the codepath that leads here, is sophisticated enough that I believe defensive programming is a good idea wherever we can.

Comment 7

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

Comment on attachment 8497082 [details] [diff] [review]
0001-Bug-1073992-Keep-track-of-revivable-windows-separate.patch, v2

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

I thought you were planning to add a DEBUG assertion in SessionSaver.jsm?

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #7)
> I thought you were planning to add a DEBUG assertion in SessionSaver.jsm?

I am :) Sorry, the work week is keeping me busy. Will post a new patch soon.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-add-debug-check-for-windowsToRevive.patch

Here's the debug check I added. Will squash commits after reviewing.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=4ea70206f1e2

Comment 11

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

Comment on attachment 8501879 [details] [diff] [review]
0002-add-debug-check-for-windowsToRevive.patch

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

r=me, but please change the exception thrown.

::: browser/components/sessionstore/SessionSaver.jsm
@@ +218,5 @@
> +    // Check that the windows to revive equal the windows
> +    // that we removed from the list of closed windows.
> +    let match = revivedWindows.every((win, idx) => {
> +      return win == windowsToRevive[windowsToRevive.length - 1 - idx];
> +    });

You don't really need the brackets and the `return`, do you?

@@ +221,5 @@
> +      return win == windowsToRevive[windowsToRevive.length - 1 - idx];
> +    });
> +
> +    if (!match) {
> +      throw "SessionStore: revived windows didn't match closed windows";

I'd rather you threw an Error. Throwing strings loses the stack, among other things.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #11)
> > +    let match = revivedWindows.every((win, idx) => {
> > +      return win == windowsToRevive[windowsToRevive.length - 1 - idx];
> > +    });
> 
> You don't really need the brackets and the `return`, do you?

No, but it looks pretty ugly leaving those out if it's not a one-liner.

> > +    if (!match) {
> > +      throw "SessionStore: revived windows didn't match closed windows";
> 
> I'd rather you threw an Error. Throwing strings loses the stack, among other
> things.

Oh, right. I always forget about that.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tim Taubert [:ttaubert] from comment #12)
> (In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment
> #11)
> > > +    let match = revivedWindows.every((win, idx) => {
> > > +      return win == windowsToRevive[windowsToRevive.length - 1 - idx];
> > > +    });
> > 
> > You don't really need the brackets and the `return`, do you?
> 
> No, but it looks pretty ugly leaving those out if it's not a one-liner.

Don't get me wrong, I love the syntax without curly braces and return but I think that only makes sense if all of the arrow function fits on one line. Otherwise it looks rather more confusing than expressive.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/c98a31227412

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Pushed a tiny follow-up for bc1 bustage on PGO builds:

https://hg.mozilla.org/integration/fx-team/rev/82df8ad2c609

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c98a31227412
https://hg.mozilla.org/mozilla-central/rev/82df8ad2c609

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Backed out for too many regressions and edge cases (bug 1081135):

https://hg.mozilla.org/integration/fx-team/rev/5c865feae9f5
https://hg.mozilla.org/integration/fx-team/rev/2dda4e376d60

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/2dda4e376d60
https://hg.mozilla.org/mozilla-central/rev/5c865feae9f5

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

I'm dropping this for now.

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

Backed out from Aurora as well:

https://hg.mozilla.org/releases/mozilla-aurora/rev/a7da5ed1451d

Comment 21

[tabmix.onemen]

Attachment: Update capClosedWindows not to splice closed window with _shouldRestore

This approach look simpler to me, give it a try

(i don't have editbugs permission in this account i have it in my older account [e-mail])

Comment 22

[tabmix.onemen]

Attachment: var 2 - Update capClosedWindows not to splice closed window with _shouldRestore

1 .In _capClosedWindows, we should keep all closed windows with _shouldRestore = true plus up to _max_windows_undo of older closed windows.

2. In SessionSaver._saveState we should get closed windows from first to last. first window in the list is the last window we closed, if last window in the list was without _shouldRestore the old code did not restore any _shouldRestore window

3. see comment in SessionSaver._saveState regrding shallow copy of _closedWindows

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8507402 [details] [diff] [review]
var 2 - Update capClosedWindows not to splice closed window with _shouldRestore

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

Thanks for your patch, I don't think that's the right solution though. We should probably split the problem that we restore _shouldRestore=true windows in the wrong order off into a new bug and write a separate test for it.

Having _shouldRestore tacked onto the window state in general is something I would really like to get rid off instead of re-adding it after we're done sending the session state to the worker. I think we should aim for a proper solution and maintain all closed windows in a separate data structure so keeping the two lists in sync would be a lot easier and more obvious.

::: browser/components/sessionstore/SessionSaver.jsm
@@ +212,5 @@
> +    let windowsToRevive = state._closedWindows.filter(win => {
> +      let _shouldRestore = !!win._shouldRestore;
> +      delete win._shouldRestore;
> +      return _shouldRestore;
> +    }).reverse();

This seems a little complicated. We should simply iterate windows, stopping at the first window without _shouldRestore=true.

while (state._closedWindows.length) {
  if (!state._closedWindows[0]._shouldRestore) {
    break;
  }

  delete state._closedWindows[0]._shouldRestore;
  state.windows.unshift(state._closedWindows.shift());
}

So... keep the old code and switch to iterating forwards and use .shift().

@@ +225,5 @@
> +    // state._closedWindows is shallow copy of SessionStoreInternal._closedWindows
> +    // deleting _shouldRestore here also delete it from SessionStoreInternal
> +    // we only delete _shouldRestore after user action see 
> +    // SessionStoreInternal._clearRestoringWindows
> +    windowsToRevive.forEach(win => win._shouldRestore = true);

This is exactly why I think we should do it the proper way and store that information outside of a window state. If we have problems synchronizing that information like my previous patch then we should just move all closed windows to a separate data structure.

Comment 24

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

Comment on attachment 8507402 [details] [diff] [review]
var 2 - Update capClosedWindows not to splice closed window with _shouldRestore

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

::: browser/components/sessionstore/SessionSaver.jsm
@@ +212,5 @@
> +    let windowsToRevive = state._closedWindows.filter(win => {
> +      let _shouldRestore = !!win._shouldRestore;
> +      delete win._shouldRestore;
> +      return _shouldRestore;
> +    }).reverse();

Actually, Tim, what is the difference between filtering and iterating until `_shouldRestore` is false? Besides the fact that we are likely to iterate a little further on a short list, this should give the same results, shouldn't it?

@@ +225,5 @@
> +    // state._closedWindows is shallow copy of SessionStoreInternal._closedWindows
> +    // deleting _shouldRestore here also delete it from SessionStoreInternal
> +    // we only delete _shouldRestore after user action see 
> +    // SessionStoreInternal._clearRestoringWindows
> +    windowsToRevive.forEach(win => win._shouldRestore = true);

Agreed with Tim, we have bugs here because the data structure is too messy. We should make it cleaner.

Now, if only there was a patch that implemented this already...

::: browser/components/sessionstore/SessionStore.jsm
@@ +1672,5 @@
>  
>      // reopen the window
>      let state = { windows: this._closedWindows.splice(aIndex, 1) };
>      delete state.windows[0].closedAt; // Window is now open.
> +    delete state.windows[0]._shouldRestore;

Good catch.

@@ +2080,5 @@
>        // at startup we don't accidentally add them to a popup window
>        do {
> +        let winData = lastClosedWindowsCopy.shift();
> +        delete winData._shouldRestore;
> +        total.unshift(winData);

Good catch.

@@ +3342,5 @@
> +
> +    let windowsToRevive = this._closedWindows.filter(win => !!win._shouldRestore);
> +    let maxWindows = this._max_windows_undo + windowsToRevive.length;
> +    if (this._closedWindows.length <= maxWindows)
> +      return;

I think that we should rather move the _shouldRestore windows into their own data structure.

Comment 25

[tabmix.onemen]

Thank you both for your feedback, i will work on new patch

Tim can you assign this bug to me, and also open new bug regarding "restore _shouldRestore=true windows in the wrong order" and assign it to me?

Comment 26

[tabmix.onemen]

Attachment: ver 3 - set _shouldRestore flag outside of a closed window state

Tim,
let me know what you think before i ask another review

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8509286 [details] [diff] [review]
ver 3 - set _shouldRestore flag outside of a closed window state

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

Thanks for working on this! The approach is nice but I think it shows that we would indeed be better of with handling closed windows in a separate module. With a properly named API for all the modifications to the closed window array keeping track of "revivable windows" shouldn't be hard and avoid the mistakes I've done in the first approach. I would rather have us re-write closed window handling properly instead of applying a band-aid to fix something that no one noticed was broken.

::: browser/components/sessionstore/SessionStore.jsm
@@ +1668,5 @@
>    },
>  
>    getClosedWindowData: function ssi_getClosedWindowData() {
> +    let closedWindows = this._closedWindows.map(win => win.state);
> +    return this._toJSONString(closedWindows);

return this._toJSONString(this._closedWindows.map(win => win.state));

@@ +1677,5 @@
>        throw Components.Exception("Invalid index: not in the closed windows", Cr.NS_ERROR_INVALID_ARG);
>      }
>  
>      // reopen the window
> +    let winData = this._closedWindows.splice(aIndex, 1)[0];

let [winData] = this._closedWindows.splice(aIndex, 1);

@@ +1900,5 @@
>  
>      // Merge closed windows from this session with ones from last session
>      if (lastSessionState._closedWindows) {
> +      let closedWindows = lastSessionState._closedWindows.map(win => ({state: win}));
> +      this._closedWindows = this._closedWindows.concat(closedWindows);

this._closedWindows.push(...closedWindows);

@@ +3357,5 @@
>        return;
>      let spliceTo = this._max_windows_undo;
>  #ifndef XP_MACOSX
> +    let windowsToRevive = this._closedWindows.filter(win => !!win._shouldRestore);
> +    let maxWindows = spliceTo + windowsToRevive.length;

The problem here is that adjusting the cap (same problem with popup windows below) leads to reporting more than the configured number of windows to the UI. getClosedWindowData() and getClosedWindowCount() would have to re-apply the cap which isn't really ideal.

Comment 28

[Tim Taubert [:ttaubert] (inactive)]

(In reply to tabmix.onemen from comment #25)
> Tim can you assign this bug to me, and also open new bug regarding "restore
> _shouldRestore=true windows in the wrong order" and assign it to me?

Filed bug 1088720 and assigned it to you!

Comment 29

[tabmix.onemen]

(In reply to Tim Taubert [:ttaubert] from comment #27)
> @@ +3357,5 @@
> >        return;
> >      let spliceTo = this._max_windows_undo;
> >  #ifndef XP_MACOSX
> > +    let windowsToRevive = this._closedWindows.filter(win => !!win._shouldRestore);
> > +    let maxWindows = spliceTo + windowsToRevive.length;
> 
> The problem here is that adjusting the cap (same problem with popup windows
> below) leads to reporting more than the configured number of windows to the
> UI. getClosedWindowData() and getClosedWindowCount() would have to re-apply
> the cap which isn't really ideal.

consider this scenarios
1. shouldRestore windows are larger the max_windows_undo in this case we need to keep more windows then max_windows_undo.
2. lets say max_windows_undo is 3 and user have 3 old closed windows (without shouldRestore), and 4 more opened window. when user close all opened windows (one at a time) and later start Firefox again with my approach Firefox will open 4 windows and keep the older 3 closed windows, with out the fix to capClosedWindows, Firefox only save and open 4 windows and the user lost the 3 old windows. i think we should keep also the old closed windows in this case.

regarding the UI. getClosedWindowData() and getClosedWindowCount(), we need to decide between these option:
1. using the UI will trigger clearRestoringWindows
2. add different style in closed windows list to windows with shouldRestore = true
3. change getClosedWindowData() and getClosedWindowCount() to return no more then max_windows_undo (with the current code in capClosedWindows we can get extra closed window when normalWindowIndex >= maxWindows)
4. allow getClosedWindowData() and getClosedWindowCount() to report more than the configured number of windows to the UI.
5. don't update capClosedWindows and only save up to max_windows_undo closed windows, in this case if shouldRestore windows are more then max_windows_undo only windows up to the max number will report to the UI.

Comment 30

[tabmix.onemen]

Attachment: keep track of revivable closed windows in a separate module

All call to _closedWindows moved to new module ClosedWindows.jsm

This patch passed all test in sessionstore\test

The only use of ClosedWindows.data in SessionStore is by onIdleDaily, i can move _cleanupOldData to Utils.jsm call it from ClosedWindows and remove access to ClosedWindowsInternal.data outside ClosedWindows

Let me know what to do regarding comment 29

Comment 31

[tabmix.onemen]

Attachment: keep track of revivable closed windows in a separate module

ClosedWindows.jsm was missing

Comment 32

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

Comment on attachment 8511621 [details] [diff] [review]
keep track of revivable closed windows in a separate module

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

Sorry, I have a few difficulties reading the code in the current state. If this is possible, could you post a version with more documentation first?

::: browser/components/sessionstore/ClosedWindows.jsm
@@ +16,5 @@
> + *                         // split the list. see split function comments.
> + *  }
> + *
> + * Unlike _closedTabs all properties except state are for internal use and
> + * will not save to disk.

Can you move the documentation of `_data` outside of the header?

@@ +22,5 @@
> + */
> +
> +const Cu = Components.utils;
> +
> +Cu.import("resource://gre/modules/Services.jsm", this);

You can make it lazy, too.

@@ +31,5 @@
> +XPCOMUtils.defineLazyModuleGetter(this, "Utils",
> +  "resource:///modules/sessionstore/Utils.jsm");
> +
> +this.ClosedWindows = Object.freeze({
> +  get count() {

These methods need documentation. What does this count? All closed windows? All recently closed windows? etc.

@@ +43,5 @@
> +  get isRevivableEmpty() {
> +    return ClosedWindowsInternal.isRevivableEmpty;
> +  },
> +
> +  has(index) {

Please document `index`. How do you get it? Is it stable wrt `merge`, `update`, `remove`, `split`?

Comment 33

[tabmix.onemen]

Attachment: ver 5 - keep track of closed windows in a separate module

All ClosedWindows.jsm documentation are in ClosedWindowsInternal.

Added test from Tim first patch.
This patch passed all test in sessionstore\test

Let me know what to do regarding comment 29

Comment 34

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

Sorry about the delay, I'm currently rushing to fix topcrasher #1, I'll try and take a look at your patch by the end of the week.

Comment 35

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

(In reply to tabmix.onemen from comment #29)
> consider this scenarios
> 1. shouldRestore windows are larger the max_windows_undo in this case we
> need to keep more windows then max_windows_undo.
> 2. lets say max_windows_undo is 3 and user have 3 old closed windows
> (without shouldRestore), and 4 more opened window. when user close all
> opened windows (one at a time) and later start Firefox again with my
> approach Firefox will open 4 windows and keep the older 3 closed windows,
> with out the fix to capClosedWindows, Firefox only save and open 4 windows
> and the user lost the 3 old windows. i think we should keep also the old
> closed windows in this case.

I think that makes sense.

> regarding the UI. getClosedWindowData() and getClosedWindowCount(), we need
> to decide between these option:
> 1. using the UI will trigger clearRestoringWindows
> 2. add different style in closed windows list to windows with shouldRestore
> = true
> 3. change getClosedWindowData() and getClosedWindowCount() to return no more
> then max_windows_undo (with the current code in capClosedWindows we can get
> extra closed window when normalWindowIndex >= maxWindows)
> 4. allow getClosedWindowData() and getClosedWindowCount() to report more
> than the configured number of windows to the UI.

This sounds like the best option to me.

> 5. don't update capClosedWindows and only save up to max_windows_undo closed
> windows, in this case if shouldRestore windows are more then
> max_windows_undo only windows up to the max number will report to the UI.

Comment 36

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

Comment on attachment 8512623 [details] [diff] [review]
ver 5 - keep track of closed windows in a separate module

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

Sorry about the delay, I was on urgent stuff. I haven't put together all my nits, and I haven't had time to look at browser_revive_windows.js yet – I'll try and do this by tomorrow.

For the rest, I like your patch. Given the critical nature of Session Restore, I'd appreciate if Tim could also take a look at the algorithm, and if he also agrees, we can move on to cleaning up the nits.

::: browser/components/sessionstore/ClosedWindows.jsm
@@ +122,5 @@
> +  get count() {
> +    return this._data.length;
> +  },
> +
> +  // Returns whether there are no windows to revive.

Nit: Define "revive".

@@ +134,5 @@
> +   *        Object containing session data for the window
> +   */
> +  add(winData) {
> +    // we don't want to save the busy state
> +    delete winData.busy;

I'm not sure it's the right place for this `delete`.

@@ +270,5 @@
> +    return {index: -1, state: null};
> +  },
> +
> +  // Preference to control the max number of closed windows we keep
> +  get _max_windows_undo() {

That looks strange. Aren't you going to install an observer every time we fetch `this._max_windows_undo`?

@@ +290,5 @@
> +   * "revivable" closed windows, except in the case where we don't have
> +   * any non-popup windows on Windows and Linux. Then we must resize such that
> +   * we have at least one non-popup window.
> +   */
> +  _capClosedWindows() {

Nit: I think this code could be made a little nicer.

::: browser/components/sessionstore/SessionStore.jsm
@@ +273,5 @@
>    restoreLastSession: function ss_restoreLastSession() {
>      SessionStoreInternal.restoreLastSession();
>    },
>  
> +  getCurrentState: function (aUpdateAll, aPrepareToSave) {

For backwards compatibility, please add a default value for `aPrepareToSave`.

@@ -1091,3 @@
>          if (hasSaveableTabs || isLastWindow) {
> -          // we don't want to save the busy state
> -          delete winData.busy;

I would keep the `delete winData.busy` here.

@@ -1209,5 @@
>      } else if (RunState.isRunning) {
>        SessionSaver.run();
>      }
> -
> -    this._clearRestoringWindows();

Good catch.

@@ +1646,5 @@
>      this.windowToFocus = window;
>      return window;
>    },
>  
>    forgetClosedWindow: function ssi_forgetClosedWindow(aIndex) {

We might take the opportunity to make this `aIndex = 0`.

@@ +2006,3 @@
>     * @returns object
>     */
> +  getCurrentState: function (aUpdateAll, aPrepareToSave) {

Can you add a default value for `aPrepareToSave`? Also, please detail what it means in the doc.

::: browser/components/sessionstore/test/browser_revive_windows.js
@@ +2,5 @@
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +const IS_MAC = ("nsILocalFileMac" in Ci);

Use `OS.Constants.Sys.Name == "Darwin"` (you'll need to import osfile.jsm").

Comment 37

[tabmix.onemen]

Attachment: ver 6 - keep track of closed windows in a separate module

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #35)
> 
> This sounds like the best option to me.
> 
> > 5. don't update capClosedWindows and only save up to max_windows_undo closed
> > windows, in this case if shouldRestore windows are more then
> > max_windows_undo only windows up to the max number will report to the UI.

Done.

The new _getSpliceToNumber function (need better name) only add "revivable" windows to the max number of closed windows for internal use.
The public ClosedWindows.count and ClosedWindows.data return no more then _max_windows_undo, except in the case where we don't have any non-popup windows on Windows and Linux. Then we must resize such that we have at least one non-popup window.
The old code resized the max number even when all the closed windows where popups.

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #36)
> ::: browser/components/sessionstore/ClosedWindows.jsm
> @@ +122,5 @@
> > +  get count() {
> > +    return this._data.length;
> > +  },
> > +
> > +  // Returns whether there are no windows to revive.
> 
> Nit: Define "revive".

Done.

See new commnets at the top of ClosedWindows.jsm.
Please review all comments; I know that my English is not perfect.

> @@ +134,5 @@
> > +   *        Object containing session data for the window
> > +   */
> > +  add(winData) {
> > +    // we don't want to save the busy state
> > +    delete winData.busy;

Done.

> @@ +270,5 @@
> > +    return {index: -1, state: null};
> > +  },
> > +
> > +  // Preference to control the max number of closed windows we keep
> > +  get _max_windows_undo() {
> 
> That looks strange. Aren't you going to install an observer every time we
> fetch `this._max_windows_undo`?

No.
> Object.defineProperty(this, "_max_windows_undo", definition);
Object.defineProperty overide the getter, see the same code in TabRestoreQueue

> @@ +290,5 @@
> > +   * "revivable" closed windows, except in the case where we don't have
> > +   * any non-popup windows on Windows and Linux. Then we must resize such that
> > +   * we have at least one non-popup window.
> > +   */
> > +  _capClosedWindows() {
> 
> Nit: I think this code could be made a little nicer.

Done, hopefully see _getSpliceToNumber.

> ::: browser/components/sessionstore/SessionStore.jsm
> @@ +273,5 @@
> >    restoreLastSession: function ss_restoreLastSession() {
> >      SessionStoreInternal.restoreLastSession();
> >    },
> >  
> > +  getCurrentState: function (aUpdateAll, aPrepareToSave) {
> 
> For backwards compatibility, please add a default value for `aPrepareToSave`.

Done.

> @@ -1091,3 @@
> >          if (hasSaveableTabs || isLastWindow) {
> > -          // we don't want to save the busy state
> > -          delete winData.busy;
> 
> I would keep the `delete winData.busy` here.

Done.
But the comment refer to "save" and we do the save by adding the data to closed windows, also if we move this code back to ClosedWindows.jsm, then we can make ClosedWindows.isRevivableEmpty (and all "revive" references) private to ClosedWindows.jsm.

> @@ +1646,5 @@
> >      this.windowToFocus = window;
> >      return window;
> >    },
> >  
> >    forgetClosedWindow: function ssi_forgetClosedWindow(aIndex) {
> 
> We might take the opportunity to make this `aIndex = 0`.

Done.
I did'nt remove the "default to the most-recently closed window" comment

> @@ +2006,3 @@
> >     * @returns object
> >     */
> > +  getCurrentState: function (aUpdateAll, aPrepareToSave) {
> 
> Can you add a default value for `aPrepareToSave`? Also, please detail what
> it means in the doc.

Done.

> ::: browser/components/sessionstore/test/browser_revive_windows.js
> @@ +2,5 @@
> > + * http://creativecommons.org/publicdomain/zero/1.0/ */
> > +
> > +"use strict";
> > +
> > +const IS_MAC = ("nsILocalFileMac" in Ci);
> 
> Use `OS.Constants.Sys.Name == "Darwin"` (you'll need to import osfile.jsm").

Done.
Use navigator.platform.match(/Mac/) instead of osfile.jsm.

Comment 38

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

Comment on attachment 8517948 [details] [diff] [review]
ver 6 - keep track of closed windows in a separate module

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

Looks good to me, thanks for handling this.

This time, full review with nitpicking. I'd be interested in having a second review from Tim, if possible.

::: browser/components/sessionstore/ClosedWindows.jsm
@@ +11,5 @@
> + *
> + *
> + * Windows and Linux revive/revivable closed windows
> + * =================================================
> + * In this module the term "revive" closed window refer to the operation we do,

Nit: "refers"

@@ +105,5 @@
> +  }
> +});
> +
> +let ClosedWindowsInternal = {
> +  /*

Nit: /**

@@ +111,5 @@
> +   * _data is an array holding list of objects in the form:
> +   * {
> +   *   state: winData,      * Objects containing session data for the closed
> +   *                        * window.
> +   *   closedAt: timestamp, * copy of winData.closedAt, cleanupOldData use it.

Is this field really necessary?

@@ +114,5 @@
> +   *                        * window.
> +   *   closedAt: timestamp, * copy of winData.closedAt, cleanupOldData use it.
> +   *   _isRevivable: true   * Boolean flag, in Windows and Linux we use it to
> +   *                        * split the list. see split function comments.
> +   * }

Could you make this a constructor? Say `function ClosedWinData`, for instance? This will help you clarify some doc below, plus it will make it a tad easier to debug in case of problem.

@@ +124,5 @@
> +   */
> +  _data: [],
> +
> +  /**
> +   * @Return Array of Objects containing session data for the closed windows

Nit: @return

@@ +125,5 @@
> +  _data: [],
> +
> +  /**
> +   * @Return Array of Objects containing session data for the closed windows
> +   *         to be use by SessionStore

Could you mention `winData`?

@@ +132,5 @@
> +    return this._unwrap(this._data.slice(0, this.count));
> +  },
> +
> +  /**
> +   * Replace the entire data with array of windows data

Nit: Please add dots at the end of sentences.

@@ +142,5 @@
> +    this._capClosedWindows();
> +  },
> +
> +  // Number of closed windows that are accessible by the UI
> +  // see _getSpliceToNumber for more details

Nit: /**-style comment for getters, too.

@@ +175,5 @@
> +   * @param index
> +   *        Number index at which to start removing item(s)
> +   * @param howMany
> +   *        An integer indicating the number of old array elements to remove
> +   *        (optional, default to 1 if missing)

Since you are returning something, please document @return.

@@ +200,5 @@
> +    this._data[index] = closedWindowData;
> +  },
> +
> +  /**
> +   * Merge closed windows from other session into this session

Nit: Can you explain that this actually concatenates + trims data?

@@ +209,5 @@
> +    this._data.push(...this._wrap(windowsData));
> +    this._capClosedWindows();
> +  },
> +
> +  // Remove all closed windows data

/**

@@ +215,5 @@
> +    this._data.length = 0;
> +  },
> +
> +  /**
> +   * Split closed windows up into revivable windows and remaining windows.

"Split up closed windows into..."

@@ +219,5 @@
> +   * Split closed windows up into revivable windows and remaining windows.
> +   * The function uses shallow copy of this._data to preserve current state.
> +   *
> +   * @param reviveNonPopup
> +   *        Boolean, when true revive the first non-popup closed window

... and otherwise?

@@ +221,5 @@
> +   *
> +   * @param reviveNonPopup
> +   *        Boolean, when true revive the first non-popup closed window
> +   * @param prepareToSave
> +   *        Boolean, when prepare to save closed windows data

"when `true`, return values in a format adapted to saving closed windows data, i.e. [some explanations].
 Otherwise, return values in a format adapted to [what?], i.e. [some explanations]"

Also, mention somewhere that this is affected by `RunState.isQuitting`, and why/how.

@@ +222,5 @@
> +   * @param reviveNonPopup
> +   *        Boolean, when true revive the first non-popup closed window
> +   * @param prepareToSave
> +   *        Boolean, when prepare to save closed windows data
> +   * @returns [windowsToRevive, closedWindows]

Docnit: What's the type of these objects?

@@ +225,5 @@
> +   *        Boolean, when prepare to save closed windows data
> +   * @returns [windowsToRevive, closedWindows]
> +   */
> +  split(reviveNonPopup, prepareToSave) {
> +    let windowsToRevive = [], windowsData = this._data.slice();

Nit: Let's put this on two `let`.

@@ +233,5 @@
> +    // the session.
> +    //XXXzpao We should do this for _restoreLastWindow == true, but that has
> +    //        its own check for popups. c.f. bug 597619
> +    if (reviveNonPopup && windowsData.length > 0 &&
> +        RunState.isQuitting) {

Actually, I think that you should remove this call to `RunState.isQuitting` and rather have the caller merge it in `reviveNonPopup`.

@@ +252,5 @@
> +    return [windowsToRevive, this._unwrap(windowsData)];
> +  },
> +
> +  /**
> +   * Wrap array of windows state into array of objects. see comment in _data

Here, for instance, converting the "objects" into instances of `ClosedWinData` will make that documentation less ambiguous.

@@ +262,5 @@
> +    return windowsData.map(winData => {
> +      // Make sure that we have a timestamp
> +      winData.closedAt = winData.closedAt || now;
> +      return {state: winData,
> +              closedAt: winData.closedAt};

We could avoid a somewhat expensive call to `Date.now()` in many cases if we get rid of `closedAt` and only use `winData.closedAt`.

@@ +287,5 @@
> +   */
> +  get firstNonPopupClosedWindow() {
> +    for (let i = 0, l = this._data.length; i < l; i++) {
> +      if (!this._data[i].state.isPopup)
> +        return {index: i, state: this._data[i].state};

Nit: Don't forget curly braces, even if it's a one-line `if ()`. Also in other places.

@@ +299,5 @@
> +      let value = Services.prefs.getIntPref(PREF);
> +      if (value < 0)
> +        value = 0;
> +      let definition = {value: value, configurable: true};
> +      Object.defineProperty(this, "_max_windows_undo", definition);

Ah, right, my bad.

@@ +335,5 @@
> +   *   We don't resize the number if there is no non-popup closed window.
> +   *
> +   * @param internal
> +   *        Boolean, true for internal call, add the number of "revivable"
> +   *        closed windows.

That argument is not clear.

@@ +339,5 @@
> +   *        closed windows.
> +   * @Return The number we should splice our closed windows data to, or null if
> +   *         we don't need to splice our data.
> +   */
> +  _getSpliceToNumber(internal) {

Nit: Not a very good name. Maybe `_getMaxNumberOfClosedWindows(isInternal)`?

@@ +358,5 @@
> +  },
> +
> +  /**
> +   * Clears the set of windows that are "revivable" before writing to disk to
> +   * make closing windows one after the other until shutdown work as expected.

That explanation is not very clear. I believe that you should end your sentence after "revivable", and then add more details in another paragraph.

::: browser/components/sessionstore/moz.build
@@ +35,5 @@
>      'SessionCookies.jsm',
>      'SessionFile.jsm',
>      'SessionHistory.jsm',
>      'SessionMigration.jsm',
> +    'SessionSaver.jsm',

\o/ Stack line numbers will finally be correct :)

::: browser/components/sessionstore/test/browser_revive_windows.js
@@ +22,5 @@
> + * "Undo Close Window" feature.
> + */
> +add_task(function* test_revive_windows() {
> +  // We can restore a single window max.
> +  Services.prefs.setIntPref(PREF_MAX_UNDO, 1);

Perhaps you should run this several times with different constants:
- once with PREF_MAX_UNDO set to 1;
- once with PREF_MAX_UNDO set to 5.

@@ +43,5 @@
> +  for (let win of windows) {
> +    yield promiseWindowClosed(win);
> +  }
> +
> +  is(ss.getClosedWindowCount(), 1, "one window restorable");

Nit: These days, it's `Assert.equal` instead of `is`. Same below.

@@ +47,5 @@
> +  is(ss.getClosedWindowCount(), 1, "one window restorable");
> +
> +  // Save to disk and read.
> +  let state = JSON.parse(yield promiseRecoveryFileContents());
> +  

Nit: whitespace.

@@ +49,5 @@
> +  // Save to disk and read.
> +  let state = JSON.parse(yield promiseRecoveryFileContents());
> +  
> +  // Check number of windows.
> +  if (IS_MAC) {

Nit: A small comment explaining what we expect on Mac/non-Mac?

@@ +82,5 @@
> +  yield promiseWindowClosed(win0);
> +  let data = ss.getClosedWindowData();
> +  ok(data.contains(URL_CLOSED_WINDOW), "window is restorable");
> +
> +  let win1 = yield promiseNewWindow();

Can you comment somewhere that opening a new window should make win0 non-revivable?

Comment 39

[tabmix.onemen]

Attachment: ver 7 - keep track of closed windows in a separate module

Addressed review comments.

> ::: browser/components/sessionstore/test/browser_revive_windows.js
> @@ +22,5 @@
> > + * "Undo Close Window" feature.
> > + */
> > +add_task(function* test_revive_windows() {
> > +  // We can restore a single window max.
> > +  Services.prefs.setIntPref(PREF_MAX_UNDO, 1);
> 
> Perhaps you should run this several times with different constants:
> - once with PREF_MAX_UNDO set to 1;
> - once with PREF_MAX_UNDO set to 5.
> 

Let's wait for Tim to comments on this. the test browser_revive_windows.js was taken from his patch.

Comment 40

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

Comment on attachment 8522384 [details] [diff] [review]
ver 7 - keep track of closed windows in a separate module

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

As previously, r+ with a few minor nitpicks. Having a second pair of eyes on the code would be nice, though.

::: browser/components/sessionstore/ClosedWindows.jsm
@@ +110,5 @@
> + * ClosedWinData properties are:
> + *    state: Objects containing SessionStore data for the closed window
> + *    isRevivable: Boolean, 'true' indicates that the window is revivable
> + *    closedAt: timestamp, helper to get/set the state.closedAt,
> + *              Utils.cleanupOldData use it

Nit: "uses" it

@@ +132,5 @@
> +
> +let ClosedWindowsInternal = {
> +  /**
> +   * States for all recently closed windows that are tracked by SessionStore.
> +   * _data is for internal use and we don't expose it to other modules.

Nit: Mention that it's an array of `ClosedWinData`.

@@ +163,5 @@
> +    let maxWindows = this._getMaxNumberOfClosedWindows(false);
> +    return maxWindows != null ? maxWindows : this._data.length;
> +  },
> +
> +  // Returns whether there are no windows to revive.

Nit: /**

@@ +259,5 @@
> +    let windowsToRevive = [];
> +    let closedWindows = this._data.slice();
> +    // If no non-popup browser window remains open, return the state of the last
> +    // closed window(s). We only want to do this when we're actually "ending"
> +    // the session, i.e. when RunState.isQuitting.

Nit: This last sentence should be explained in the doc, rather than (only) in a comment.

@@ +318,5 @@
> +    }
> +    return {index: -1, state: null};
> +  },
> +
> +  // Preference to control the max number of closed windows we keep.

/**

@@ +362,5 @@
> +   *   that we have at least one non-popup window.
> +   *   We don't resize the number if there is no non-popup closed window.
> +   *
> +   * @param isInternal
> +   *        Boolean, 'true' indicates that the call is internal

Nit: I'm still not very happy about the name of this argument. I don't have a good idea, though.

Comment 41

[tabmix.onemen]

(In reply to David Rajchenbach-Teller [:Yoric] (away until November 17th - use "needinfo") from comment #40)
> 
> As previously, r+ with a few minor nitpicks. Having a second pair of eyes on
> the code would be nice, though.
>
How can we get a second reviewer? 
I'v Emailed Tim but did not get any response, can you ask someone else to review this bug?

Comment 42

[tabmix.onemen]

(In reply to David Rajchenbach-Teller [:Yoric] (away until November 17th - use "needinfo") from comment #40)
> Comment on attachment 8522384 [details] [diff] [review]

> @@ +259,5 @@
> > +    let windowsToRevive = [];
> > +    let closedWindows = this._data.slice();
> > +    // If no non-popup browser window remains open, return the state of the last
> > +    // closed window(s). We only want to do this when we're actually "ending"
> > +    // the session, i.e. when RunState.isQuitting.
> 
> Nit: This last sentence should be explained in the doc, rather than (only)
> in a comment.

Now that we have both revivable check and popup check in ClosedWindows.getCurrentState, I think that both check and moving closed windows to state.windows[] should happened only when we're actually "ending" the session and when that call was made from SessionSaver._saveState.

If we move revivable windows to state.windows[] before the session is "ending" (as the current code does), we can get closed windows reopen after crash.

I'm also not sure if when calling getCurrentState from SessionStore.getBrowserState we should move non-popup window to state.windows[] when there are no non-popup windows open and SessionStore.getBrowserState is called when session is "ending".

We have to deal with edge case that we don't have any non-popup windows open and all the closed windows are popup. In this case we can end up opening popup window as normal window. Also I don't think that in this situation user expecting its popup windows to 'revive' on next startup.

Comment 43

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

(In reply to tabmix.onemen from comment #42)
> Now that we have both revivable check and popup check in
> ClosedWindows.getCurrentState, I think that both check and moving closed
> windows to state.windows[] should happened only when we're actually "ending"
> the session and when that call was made from SessionSaver._saveState.
> 
> If we move revivable windows to state.windows[] before the session is
> "ending" (as the current code does), we can get closed windows reopen after
> crash.

Consider the following scenario:
- user attempts to close windows A, B and C manually to quit, but takes their time to do so;
- by chance, SessionRestore saves between A and B, then Firefox crashes before the quit sequence has started.

If we move leave revivable windows out of state.windows[], window A won't be reopened when we recover, won't it?

> 
> I'm also not sure if when calling getCurrentState from
> SessionStore.getBrowserState we should move non-popup window to
> state.windows[] when there are no non-popup windows open and
> SessionStore.getBrowserState is called when session is "ending".
> 
> We have to deal with edge case that we don't have any non-popup windows open
> and all the closed windows are popup. In this case we can end up opening
> popup window as normal window. Also I don't think that in this situation
> user expecting its popup windows to 'revive' on next startup.

Yes, if there are no non-popup windows, we should start with a fresh session (modulo scratchpads, etc.)

Comment 44

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8522384 [details] [diff] [review]
ver 7 - keep track of closed windows in a separate module

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

Sorry for never getting back to this. I think in favor of bug 1171708 we shouldn't touch this code right now and make things possibly more complex than they already are. Either as part of the aforementioned bug or as a follow-up we can then clean up the closed window storage.

Comment 45

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:dao, could you have a look please?
For more information, please visit auto_nag documentation.