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

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

The methods I'm worried we're not supporting properly are forgetClosedWindow and forgetClosedTab. I'll see if I can write a testcase.

Comment 3

[Mike Conley (: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.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1225921 - Regression tests. r?billm

Bug 1225921 - Regression tests. r?billm

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Attachment: 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.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

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

> Why not just set dontRemove to false?

Yeah, I don't remember why I did that. Thanks.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Filed bug 1230636 to make the tab closing case more like the window closing one.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

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

Yep: https://dxr.mozilla.org/mozilla-central/rev/e02b17a2b5b8df7bb84f325fc08eedd2f3cab755/browser/base/content/sanitize.js#350

Comment 12

[Mike Conley (:mconley) (:⚙️)]

(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 13

[Mike Conley (:mconley) (:⚙️)]

https://hg.mozilla.org/integration/fx-team/rev/26865d807d138011f9fd83a85aaab761e89be259
Bug 1225921 - Regression tests. r=billm

https://hg.mozilla.org/integration/fx-team/rev/3d8926eef121ba1b6b003011e2cac09c00782657
Bug 1225921 - Have SessionStore keep a list of window data that might be saved during a flush. r=billm

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/26865d807d13
https://hg.mozilla.org/mozilla-central/rev/3d8926eef121

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bugnotes

http://people.mozilla.org/~mconley2/bugnotes/bug-1225921.html