Closed Bug 989393 Opened 6 years ago Closed 6 years ago

[Session Restore] Clean up old closed tabs and windows

Categories

(Firefox :: Session Restore, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 32

People

(Reporter: Yoric, Assigned: Yoric)

References

(Blocks 1 open bug)

Details

(Keywords: relnote, Whiteboard: p=0 s=it-32c-31a-30b.2 [qa!])

Attachments

(1 file, 6 obsolete files)

No description provided.
Whiteboard: p=0
Assignee: nobody → dteller
Attachment #8401525 - Flags: review?(ttaubert)
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?
Attachment #8401525 - Flags: review?(ttaubert) → feedback+
Applied feedback.
Attachment #8401525 - Attachment is obsolete: true
Attachment #8407472 - Flags: review?(ttaubert)
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));
Attachment #8407472 - Flags: review?(ttaubert) → feedback+
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Applied feedback.
Attachment #8407472 - Attachment is obsolete: true
Attachment #8408241 - Flags: review?(ttaubert)
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?
Attachment #8408241 - Flags: review?(ttaubert) → feedback+
I seem to remember that it ran successfully on Try.
In any case:  https://tbpl.mozilla.org/?tree=Try&rev=5463703e8f31
Attachment #8408241 - Attachment is obsolete: true
Attachment #8408969 - Flags: review?(ttaubert)
Comment on attachment 8408969 [details] [diff] [review]
Clean up old closed tabs/windows, v4

Your try push is orange :/
Attachment #8408969 - Flags: review?(ttaubert)
Sorry, too much stuff on my mq.
Attachment #8408969 - Attachment is obsolete: true
Attachment #8415149 - Flags: review?(ttaubert)
Ah, my test seems to interact with a previous test. Adding cleanup and waiting for Try/LDAP to resume their communications.
Same one with a little more cleanup at the start of the test.
Attachment #8415149 - Attachment is obsolete: true
Attachment #8415149 - Flags: review?(ttaubert)
Attachment #8417013 - Flags: review?(ttaubert)
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.
Attachment #8417013 - Flags: review?(ttaubert) → review+
https://hg.mozilla.org/integration/fx-team/rev/b9ebae6f81d9
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: p=0 → p=0[fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/b9ebae6f81d9
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: p=0[fixed-in-fx-team] → p=0
Target Milestone: --- → Firefox 32
Whiteboard: p=0 → p=0 s=it-32c-31a-30b.2 [qa?]
Whiteboard: p=0 s=it-32c-31a-30b.2 [qa?] → p=0 s=it-32c-31a-30b.2 [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?
Flags: needinfo?(ttaubert)
QA Contact: cornel.ionce
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.
Flags: needinfo?(ttaubert)
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.
Status: RESOLVED → VERIFIED
Whiteboard: p=0 s=it-32c-31a-30b.2 [qa+] → p=0 s=it-32c-31a-30b.2 [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 ???
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 32 → Firefox 31
Version: unspecified → 31 Branch
Don't reopen bugs just because you have a question. File a new bug, please.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 31 → Firefox 32
Version: 31 Branch → unspecified
Status: RESOLVED → VERIFIED
(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.
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.
This change is described in request bug 943352.
You need to log in before you can comment on or make changes to this bug.