Open Bug 1073992 Opened 10 years ago Updated 2 years ago

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

Categories

(Firefox :: Session Restore, defect, P3)

defect
Points:
5

Tracking

()

Firefox 35

People

(Reporter: ttaubert, Unassigned)

References

(Depends on 1 open bug)

Details

Attachments

(3 files, 8 obsolete files)

18.04 KB, patch
Details | Diff | Splinter Review
2.48 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
51.47 KB, patch
ttaubert
: review-
Yoric
: review+
Details | Diff | Splinter Review
_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.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #8496604 - Flags: review?(dteller)
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?
Attachment #8496604 - Flags: review?(dteller) → review+
(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.
Merged the test from bug 1073513 and addressed review comments.
Attachment #8496604 - Attachment is obsolete: true
Attachment #8497082 - Flags: review?(dteller)
Iteration: --- → 35.2
Points: --- → 5
Flags: qe-verify+
Flags: firefox-backlog+
(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.
Iteration: 35.2 → 35.3
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?
Attachment #8497082 - Flags: review?(dteller)
(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.
QA Contact: cornel.ionce
Here's the debug check I added. Will squash commits after reviewing.
Attachment #8501879 - Flags: review?(dteller)
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.
Attachment #8501879 - Flags: review?(dteller) → review+
(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.
(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.
Pushed a tiny follow-up for bc1 bustage on PGO builds:

https://hg.mozilla.org/integration/fx-team/rev/82df8ad2c609
https://hg.mozilla.org/mozilla-central/rev/c98a31227412
https://hg.mozilla.org/mozilla-central/rev/82df8ad2c609
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Depends on: 1081135
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee: ttaubert → nobody
Status: REOPENED → NEW
Iteration: 35.3 → ---
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Iteration: --- → 35.3
I'm dropping this for now.
Assignee: ttaubert → nobody
Status: ASSIGNED → NEW
Iteration: 35.3 → ---
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 onemen.one@gmail.com)
Attachment #8506368 - Flags: review?(dteller)
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
Attachment #8506368 - Attachment is obsolete: true
Attachment #8506368 - Flags: review?(dteller)
Flags: needinfo?(dteller)
Attachment #8507402 - Flags: review?(dteller)
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.
Attachment #8507402 - Flags: review-
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.
Attachment #8507402 - Flags: review?(dteller) → feedback+
Flags: needinfo?(dteller)
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?
Tim,
let me know what you think before i ask another review
Attachment #8507402 - Attachment is obsolete: true
Attachment #8509286 - Flags: review?(ttaubert)
Assignee: nobody → tabmix.onemen
Status: NEW → ASSIGNED
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.
Attachment #8509286 - Flags: review?(ttaubert)
Depends on: 1088720
(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!
(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.
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
Attachment #8509286 - Attachment is obsolete: true
Attachment #8511619 - Flags: review?(ttaubert)
Attachment #8511619 - Flags: review?(dteller)
ClosedWindows.jsm was missing
Attachment #8511619 - Attachment is obsolete: true
Attachment #8511619 - Flags: review?(ttaubert)
Attachment #8511619 - Flags: review?(dteller)
Flags: needinfo?(dteller)
Attachment #8511621 - Flags: review?(ttaubert)
Attachment #8511621 - Flags: review?(dteller)
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`?
Attachment #8511621 - Flags: review?(dteller)
Flags: needinfo?(dteller)
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
Attachment #8511621 - Attachment is obsolete: true
Attachment #8511621 - Flags: review?(ttaubert)
Flags: needinfo?(dteller)
Attachment #8512623 - Flags: review?(ttaubert)
Attachment #8512623 - Flags: review?(dteller)
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.
Flags: needinfo?(dteller)
(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 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").
Attachment #8512623 - Flags: review?(dteller) → feedback+
(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.
Attachment #8512623 - Attachment is obsolete: true
Attachment #8512623 - Flags: review?(ttaubert)
Flags: needinfo?(dteller)
Attachment #8517948 - Flags: review?(ttaubert)
Attachment #8517948 - Flags: review?(dteller)
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?
Attachment #8517948 - Flags: review?(dteller) → review+
Flags: needinfo?(dteller)
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.
Attachment #8517948 - Attachment is obsolete: true
Attachment #8517948 - Flags: review?(ttaubert)
Attachment #8522384 - Flags: review?(ttaubert)
Attachment #8522384 - Flags: review?(dteller)
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.
Attachment #8522384 - Flags: review?(dteller) → review+
(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?
Flags: needinfo?(dteller)
(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.
(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.)
Flags: needinfo?(dteller)
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.
Attachment #8522384 - Flags: review?(ttaubert) → review-

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.

Assignee: tabmix.onemen → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(dao+bmo)
Severity: normal → S3
Flags: needinfo?(dao+bmo)
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.