Async tab and window flushing may make it possible to accidentally save tabs we want to forget

RESOLVED FIXED in Firefox 45

Status

()

Firefox
Session Restore
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: mconley, Unassigned)

Tracking

unspecified
Firefox 45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox45 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

Bug 1109875 and bug 1171708 introduced TabStateFlusher.flush and TabStateFlusher.flushWindow which asynchronously flush all pending messages from tabs and windows that are being closed.

While we're waiting for that async flush to complete, there is a small window of time where the user or some add-on could choose to forget a particular tab. Once the flush is finished, however, we don't seem to respect that choice, meaning that the tab will not necessarily be forgotten.

Note that it's a small window of time, but we should definitely make sure to not keep stuff around if the user has expressed a desire to get rid of it.
Nom'ing because we probably want to make sure there's not a window of time during which the user could request that SessionStore forgets a tab or window, and then we re-remember it after a flush finishes.
tracking-e10s: --- → ?
Flags: needinfo?(mconley)
The methods I'm worried we're not supporting properly are forgetClosedWindow and forgetClosedTab. I'll see if I can write a testcase.
Flags: needinfo?(mconley)
We also _might_ accidentally re-insert closed windows / tabs after purging history. I'll see if I can write a test case for that too.
Created attachment 8694868 [details]
MozReview Request: Bug 1225921 - Regression tests. r?billm

Bug 1225921 - Regression tests. r?billm
Attachment #8694868 - Flags: review?(wmccloskey)
Created attachment 8694869 [details]
MozReview Request: Bug 1225921 - Have SessionStore keep a list of window data that might be saved during a flush. r?billm

Bug 1225921 - Have SessionStore keep a list of window data that might be saved during a flush. r?billm

This helps us keep track of what windows we've chosen to forget, and helps
us avoid the problem of accidentally saving a window we've chosen to forget.
Attachment #8694869 - Flags: review?(wmccloskey)
The tests depend on the browser.sessionstore.debug.no_auto_updates pref I added in bug 1226333, so added dependency.

Hey billm, I'm not too jazzed about putting yet another global WeakMap/Set on the SessionStore object, but this felt simplest. I considered using the same "forget" model that we use for tabs (where we end up removing the browser.permanentKey from the _closedTabs set for the forgetClosedTab case, or where we end up changing the window's _closedTabs array reference so that any attempts to re-insert don't matter for the purge history case), but that seemed really confusing. This feels more straight-forward, to hold a set of the data that we hope to maybe save. I'm also wondering if we want to transition the tab closing model to fit this, as the way that forgetting tabs works here wasn't immediately obvious for the purge history case.

Anyhow, let me know what you think.
Depends on: 1226333
tracking-e10s: ? → +
Attachment #8694868 - Flags: review?(wmccloskey) → review+
Comment on attachment 8694868 [details]
MozReview Request: Bug 1225921 - Regression tests. r?billm

https://reviewboard.mozilla.org/r/26915/#review24553

Does this test cover what happens for "Forget Site"? I remember we discussed that one, but I don't remember the conclusion. Does it end up triggering the purge observer?

::: browser/components/sessionstore/test/browser_forget_async_closings.js:37
(Diff revision 1)
> +  let promise = BrowserTestUtils.removeTab(tab, {dontRemove: true});
> +  gBrowser.removeTab(tab);

Why not just set dontRemove to false?
Comment on attachment 8694869 [details]
MozReview Request: Bug 1225921 - Have SessionStore keep a list of window data that might be saved during a flush. r?billm

https://reviewboard.mozilla.org/r/26917/#review24555

I think it would be reasonable to make the tab case more like this one (I was also confused until I read your comment). But it's fine not to as well.

::: browser/components/sessionstore/SessionStore.jsm:403
(Diff revision 1)
> +  // array for the session.

Please add to the comment here explaining how the tab case works.
Attachment #8694869 - Flags: review?(wmccloskey) → review+
https://reviewboard.mozilla.org/r/26915/#review24553

> Why not just set dontRemove to false?

Yeah, I don't remember why I did that. Thanks.
Filed bug 1230636 to make the tab closing case more like the window closing one.
(Above comment in response to the below)

(In reply to Bill McCloskey (:billm) from comment #7)
> Comment on attachment 8694868 [details]
> MozReview Request: Bug 1225921 - Regression tests. r?billm
> 
> https://reviewboard.mozilla.org/r/26915/#review24553
> 
> Does this test cover what happens for "Forget Site"? I remember we discussed
> that one, but I don't remember the conclusion. Does it end up triggering the
> purge observer?
>

Comment 14

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/26865d807d13
https://hg.mozilla.org/mozilla-central/rev/3d8926eef121
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
You need to log in before you can comment on or make changes to this bug.