989393 - [Session Restore] Clean up old closed tabs and windows

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Comment 1

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

Attachment: Clean up old closed tabs/windows, v1

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8401525 [details] [diff] [review]
Clean up old closed tabs/windows, v1

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

The approach looks good to me, thanks!

::: browser/components/sessionstore/src/SessionClosedData.jsm
@@ +21,5 @@
> +   * Start tracking a closed window.
> +   *
> +   * The window will remain tracked across restarts until untrackWindow is called.
> +   */
> +  trackWindow: function(window) {

The terms "tracking" and "untracking" seem very technical to me. I'm also not convinced that we really need to move this single line to a separate module. It would be sufficient IMO if we had a SessionCleanup.jsm that has method likes "purgeClosedWindows" and "purgeClosedTabs".

@@ +40,5 @@
> +   *
> +   * This should be called when restoring the window.
> +   */
> +  untrackWindow: function(window) {
> +    window.closedAt = undefined;

What exactly is the value of "untracking" windows or tabs? And if we do that shouldn't we delete the property?

@@ +69,5 @@
> +
> +    // Walk through closed windows, getting rid of windows that have been
> +    // closed a long time ago.
> +    let remove = [];
> +    for (let i in closedWindows) {

for (let i=0; i < closedWindows.length; i++) {

@@ +73,5 @@
> +    for (let i in closedWindows) {
> +      let winData = closedWindows[i];
> +      if (winData.closedAt || undefined) {
> +        if (now - winData.closedAt > TIME_TO_LIVE) {
> +          // That window was closed a very long time ago, just get rid of it

In terms of easier to read code, maybe:

let winData = closedWindows[i];
winData.closedAt = winData.closedAt || now;

if (now - winData.closedAt > TIME_TO_LIVE) {
  ...
}

@@ +87,5 @@
> +    }
> +    for (let i = remove.length - 1; i >= 0; --i) {
> +      closedWindows.splice(remove[i], 1);
> +    }
> +    return changed;

Nothing is using the return value so we can just get rid of that |changed| variable. Same for the method below.

@@ +110,5 @@
> +      let remove = [];
> +      for (let i in winData._closedTabs) {
> +        let tabData = winData._closedTabs[i];
> +        if (tabData.closedAt || undefined) {
> +          if (now - tabData.closedAt > TIME_TO_LIVE) {

Looks like we can share this code with the method above.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +3415,5 @@
> +      try {
> +        delete this._closedWindows[i]._shouldRestore;
> +      } catch (ex) {
> +        dump("_clearRestoringWindows error: " + ex + " at " + ex.stack + "\n");
> +      }

Why do we need that?

::: browser/components/sessionstore/src/moz.build
@@ +20,5 @@
>      'PageStyle.jsm',
>      'PrivacyFilter.jsm',
>      'PrivacyLevel.jsm',
>      'RecentlyClosedTabsAndWindowsMenuUtils.jsm',
> +    'SessionClosedData.jsm',

Should we call this SessionCleaner? SessionCleanup?

Comment 3

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

Attachment: Clean up old closed tabs/windows, v2

Applied feedback.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8407472 [details] [diff] [review]
Clean up old closed tabs/windows, v2

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

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +1349,5 @@
>          image: tabbrowser.getIcon(aTab),
>          pos: aTab._tPos,
> +	closedAt: Date.now()
> +      };
> +      this._windows[aWindow.__SSi]._closedTabs.unshift(data);

No need to touch that code and lose blame info here.

@@ +1412,5 @@
>      return SessionFile.gatherTelemetry(stateString);
>    },
>  
> +  onIdleDaily: function() {
> +    // Clean up data that has been closed a long time ago.

That could be a nice comment above the method definition.

@@ +1424,5 @@
> +    targets.push(this._closedWindows);
> +    // 2. Prepare to clean up the closed tabs of closed windows
> +    for (let winData of this._closedWindows) {
> +      targets.push(winData._closedTabs);
> +    }

It looks like we should have the cleanup code in a separate function and call that one after the other? You're pushing closed tabs from closed windows onto the stack here and we might iterate closed tabs from closed windows that would have been removed already if instead of using a stack we'd just call the function directly.

@@ +1447,5 @@
> +      for (let i = remove.length - 1; i >= 0; --i) {
> +	array.splice(remove[i], 1);
> +      }
> +    }
> +    

Nit: empty line with spaces

@@ +1450,5 @@
> +    }
> +    
> +
> +    // Do not reschedule a save. This will wait for the next regular
> +    // save.

This might also go into the comment above the method definition.

@@ +1705,5 @@
>      }
>  
>      // reopen the window
>      let state = { windows: this._closedWindows.splice(aIndex, 1) };
> +    state.windows[0].closedAt = undefined;

Shouldn't we |delete state.windows[0].closedAt| to save some bytes like we do in other places?

@@ +2585,5 @@
>           tab.__SS_extdata[key] = tabData.extData[key];
>        } else {
>          delete tab.__SS_extdata;
>        }
> +      tabData.closedAt = undefined;

Same here.

::: browser/components/sessionstore/test/browser_cleaner.js
@@ +2,5 @@
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +
> +/*
> + * This tests ensures that Session Restore eventually forgets about

Nit: This test

::: browser/components/sessionstore/test/head.js
@@ +495,5 @@
>  }
> +function promiseDelayedStartupFinished(aWindow) {
> +  let deferred = Promise.defer();
> +  whenDelayedStartupFinished(aWindow, deferred.resolve);
> +  return deferred.promise;

It recently occurred that I could/should start pushing for using the ES6 promise API.

return new Promise(resolve => whenDelayedStartupFinished(aWindow, resolve));

Comment 5

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

Attachment: Clean up old closed tabs/windows, v3

Applied feedback.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8408241 [details] [diff] [review]
Clean up old closed tabs/windows, v3

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

The patch looks good but I think the test still has a few problems. Did you push this to try yet?

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +1424,5 @@
> +
> +    // Remove closed tabs of open windows
> +    this._cleanupOldData([this._windows[key]._closedTabs for (key of Object.keys(this._windows))]);
> +  },
> +  _cleanupOldData: function(targets) {

Nit: please add an empty line above, give it some room to breathe :)

@@ +1425,5 @@
> +    // Remove closed tabs of open windows
> +    this._cleanupOldData([this._windows[key]._closedTabs for (key of Object.keys(this._windows))]);
> +  },
> +  _cleanupOldData: function(targets) {
> +    const TIME_TO_LIVE = Services.prefs.getIntPref("browser.sessionstore.cleanup.forget_closed_after");

const TIME_TO_LIVE = this._prefBranch.getIntPref("sessionstore.cleanup.forget_closed_after");

::: browser/components/sessionstore/test/browser_cleaner.js
@@ +24,5 @@
> +  return Date.now() - stamp <= 60000;
> +}
> +
> +function promiseCleanup() {
> +  return ss.setBrowserState("{\"windows\": []}");

setBrowserState() doesn't return a promise... You probably want waitForBrowserState(), or rather add promiseBrowserState().

@@ +34,5 @@
> +  let newTab1, newTab2;
> +  let newWin;
> +
> +    newTab1 = gBrowser.addTab(URL_TAB1);
> +    yield promiseBrowserLoaded(newTab1.linkedBrowser);

Indentation is off here.

@@ +82,5 @@
> +add_task(function* test_restore() {
> +  let newWin, newTab1, newTab2;
> +  try {
> +    info("3. Making sure that after restoring, we don't have closedAt");
> +    yield ss.setBrowserState(CLOSED_STATE);

This doesn't return a promise.

@@ +88,5 @@
> +    newWin = ss.undoCloseWindow(0);
> +    yield promiseDelayedStartupFinished(newWin);
> +
> +    newTab2 = ss.undoCloseTab(window, 0);
> +    yield promiseBrowserLoaded(newTab2.linkedBrowser);

promiseTabRestored()

@@ +91,5 @@
> +    newTab2 = ss.undoCloseTab(window, 0);
> +    yield promiseBrowserLoaded(newTab2.linkedBrowser);
> +
> +    newTab1 = ss.undoCloseTab(window, 0);
> +    yield promiseBrowserLoaded(newTab1.linkedBrowser);

promiseTabRestored()

@@ +148,5 @@
> +    is(state._closedWindows[0], undefined, "5. Second window was forgotten");
> +    is(state.windows[0]._closedTabs.length, 1, "5. Only one closed tab left");
> +    is(state.windows[0]._closedTabs[0].state.entries[0].url, url, "5. The second tab is still here");
> +  } finally {
> +    delete window.sidebar;

What's this for?

Comment 7

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

Attachment: Clean up old closed tabs/windows, v4

I seem to remember that it ran successfully on Try.
In any case:  https://tbpl.mozilla.org/?tree=Try&rev=5463703e8f31

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8408969 [details] [diff] [review]
Clean up old closed tabs/windows, v4

Your try push is orange :/

Comment 9

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

Attachment: Clean up old closed tabs/windows, v5

Sorry, too much stuff on my mq.

Comment 10

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

Ah, my test seems to interact with a previous test. Adding cleanup and waiting for Try/LDAP to resume their communications.

Comment 11

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

Attachment: Clean up old closed tabs/windows, v6

Same one with a little more cleanup at the start of the test.

Comment 12

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

Try: https://tbpl.mozilla.org/?tree=Try&rev=160b987098d9

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8417013 [details] [diff] [review]
Clean up old closed tabs/windows, v6

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

r=me with all comments addressed.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +1462,5 @@
> +
> +    for (let array of targets) {
> +      let remove = [];
> +      for (let i = 0; i < array.length; ++i) {
> +        let data = array[i];

If we iterated |array| backwards we wouldn't need |remove| and could remove entries right away.

::: browser/components/sessionstore/test/browser_cleaner.js
@@ +27,5 @@
> +let promiseCleanup = Task.async(function* () {
> +  info("Cleaning up browser");
> +
> +  yield promiseBrowserState(getClosedState());
> +});

Looks like this should just be:

function promiseCleanup() {
  info("Cleaning up browser");
  return promiseBrowserState(getClosedState());
}

@@ +46,5 @@
> +});
> +
> +add_task(function* test_open_and_close() {
> +  let newTab1, newTab2;
> +  let newWin;

Please move the declarations to where the variables are defined.

@@ +81,5 @@
> +  gBrowser.removeTab(newTab1);
> +  newTab1 = null;
> +
> +  gBrowser.removeTab(newTab2);
> +  newTab2 = null;

Nit: I don't think we actually need to null out variables anymore.

@@ +93,5 @@
> +});
> +
> +
> +add_task(function* test_restore() {
> +  let newWin, newTab1, newTab2;

Please declare variables with their definition (that works without try/finally).

@@ +113,5 @@
> +    is(state.windows[0].closedAt || false, false, "3. Main window doesn't have closedAt");
> +    is(state.windows[1].closedAt || false, false, "3. Second window doesn't have closedAt");
> +    is(state.windows[0].tabs[0].closedAt || false, false, "3. First tab doesn't have closedAt");
> +    is(state.windows[0].tabs[1].closedAt || false, false, "3. Second tab doesn't have closedAt");
> +  } finally {

Please remove the try/finally, we don't really do that in other tests either. Wrapping everything in try/catch/finally is really an anti-pattern. The initial task could add a cleanup function that removes superfluous windows and tabs but I'd rather not do that as it would just hide problems.

@@ +161,5 @@
> +    Services.obs.notifyObservers(null, "idle-daily", "");
> +    info("Sent idle-daily");
> +
> +    state = JSON.parse(ss.getBrowserState());
> +    info("Closed windows: " + JSON.stringify(state._closedWindows[0], null, "\t"));

I don't think we should clutter the output with that.

@@ +163,5 @@
> +
> +    state = JSON.parse(ss.getBrowserState());
> +    info("Closed windows: " + JSON.stringify(state._closedWindows[0], null, "\t"));
> +    is(state._closedWindows[0], undefined, "5. Second window was forgotten");
> +    info("Closed tabs: " + JSON.stringify(state.windows[0]._closedTabs[0], null, "\t"));

... and that.

@@ +169,5 @@
> +    is(state.windows[0]._closedTabs.length, 1, "5. Only one closed tab left");
> +    is(state.windows[0]._closedTabs[0].state.entries[0].url, url, "5. The second tab is still here");
> +  } finally {
> +    yield promiseCleanup();
> +  }

Same comment about try/finally.

::: browser/components/sessionstore/test/head.js
@@ +89,5 @@
>  // This assumes that tests will at least have some state/entries
>  function waitForBrowserState(aState, aSetStateCallback) {
> +  if (typeof aState != "object") {
> +    throw new TypeError();
> +  }

Can't you just use JSON.parse() here instead of throwing an error? That API drives me nuts and I can never remember whether it expects an object or string.

Comment 14

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

Attachment: Clean up old closed tabs/windows, v7

Applied feedback.
try: https://tbpl.mozilla.org/?tree=Try&rev=6f342f38f102

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/b9ebae6f81d9

Comment 17

[Cornel Ionce [:noni] [Hubs QA]]

Does this also needs manual verification given its in-testsuite coverage?

I've verified that the browser.sessionstore.cleanup.forget_closed_after pref is shown but the closed windows/tabs were not removed after two weeks (by manually changing the computer date forward with 15 days). Is there any better way of testing this?

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

Setting the computer forward is a good idea but we only cleanup history when the computer has been idle for a while. You might want to try leaving your computer idle for 5-10 minutes and go fetch a coffee in between :)

Another note: we only clean once per day when the computer is idle. Resetting the preference |idle.lastDailyNotification| and then restarting and leaving the computer idle should work hopefully.

Comment 19

[Cornel Ionce [:noni] [Hubs QA]]

Thank you Tim!

The closed windows/tabs (two weeks) were cleaned up using latest Nightly (build ID: 20140519030202) by also resetting the "idle.lastDailyNotification" pref, restarting and leaving the computer idle for several minutes.

Comment 20

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

Apparently, this has no discernable effect :/
http://raluca-elena.github.io/telemetry-dashboard/#filter=nightly%2F32%2FFX_SESSION_RESTORE_FILE_SIZE_BYTES!nightly%2F31%2FFX_SESSION_RESTORE_FILE_SIZE_BYTES!nightly%2F30%2FFX_SESSION_RESTORE_FILE_SIZE_BYTES!nightly%2F29%2FFX_SESSION_RESTORE_FILE_SIZE_BYTES&aggregates=Mean&evoOver=Builds&locked=false&sanitize=true&renderhistogram=Table

Comment 21

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

Nor on FX_SESSION_RESTORE_TOTAL_CLOSED_WINDOWS.
http://raluca-elena.github.io/telemetry-dashboard/#filter=nightly%2F32%2FFX_SESSION_RESTORE_TOTAL_CLOSED_WINDOWS_SIZE_BYTES!nightly%2F31%2FFX_SESSION_RESTORE_TOTAL_CLOSED_WINDOWS_SIZE_BYTES!nightly%2F30%2FFX_SESSION_RESTORE_FILE_SIZE_BYTES!nightly%2F29%2FFX_SESSION_RESTORE_FILE_SIZE_BYTES&aggregates=Mean&evoOver=Builds&locked=false&sanitize=true&renderhistogram=Table

Comment 22

[Tobias B. Besemer [:BesTo] (QA)]

OK, do I understand this right, that this should be in FF31b ???

I can't open the Telemetry Pages ... :-/
They don't load well ...

I also can't find the setting "browser.sessionstore.cleanup.forget_closed_after" in "about:config" ...

Can somebody document the idle-time ???
https://wiki.mozilla.org/Firefox/session_restore#Forgetting_closed_tabs.2Fwindows_after_a_time

How long have a tab/window be closed before it is cleaned up while the browser/computer is idle?

Have the browser be idle or the computer?

Is the idle-time really needed ???
The update shouldn't be a big amount of CPU & I/O, or?

If there is a idle-time needed, this wouldn't bring power-user maybe much ...
I use my browser for work, not for waiting. ;-)

What's about forgetting closed tabs/windows when the browser is closed normal ???

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

Don't reopen bugs just because you have a question. File a new bug, please.

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tobias B. Besemer from comment #22)
> OK, do I understand this right, that this should be in FF31b ???

No. The target milestone is 32.

> How long have a tab/window be closed before it is cleaned up while the
> browser/computer is idle?

2 weeks is the default.

> Have the browser be idle or the computer?

Both.

Comment 25

[Håkon A. Hjortland]

I use the closed windows list to store browser windows which I'm currently not using but which I will get back to later. I think I tried tab groups a while back, but I preferred this solution. My "stored" windows now keep disappearing, which is quite annoying. A couple of issues:

1) It was hard to find out what was going on. Exposing the browser.sessionstore.cleanup.forget_closed_after setting in e.g. the Session Manager add-on might help here (but that's outside the scope of this bug report, of course).

2) browser.sessionstore.cleanup.forget_closed_after is in milliseconds. The two-week default is already almost at the upper limit of int32, so I'm unable to set it to e.g. 100 years. I would very much like to have the option to disable the closed window cleanup. Maybe a forget_closed = true/false setting could be added?

Some keywords to help others find this bug report (it took me a while to find it myself): Recently closed windows list, old windows, expire, expiration, deleted, removed, cleaned, purged, gone, empty, trimmed, truncated, vanished.

Comment 26

[Nicolas Barbulesco]

This change is described in request bug 943352.