1400678 - quota storage should be cleared on exit if user has requested cookies to be cleared on exit

Status: RESOLVED FIXED

(Core :: Storage: Quota Manager, enhancement, P2)

Description

[Ben Kelly [:bkelly, not reviewing]]

We have a preference setting that allows cookies to be cleared at browser exit.  This does not currently clear quota storage, though.  This is a bit confusing.  We have improved the cookie-vs-quota-storage UX a lot with our new "Site Data" settings.  It would be nice to improve the UX around "clear on exit" as well.

See this stack overflow question for some context here:

https://superuser.com/questions/1250944/how-can-this-website-reidentify-me-even-after-deleting-all-of-my-browsers-histo/1251054

Hsin-Yi, do you know who the right people on the storage team are to triage this?  Not sure this is the right component either.

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

NI Mark (Storage UX) and Tina (Preferences UX) to see what they think about this. 
I also looped Cindy (PM) in CC to ensure this improvement request is on her radar for Storage UI v2 scope.

As this is an enhancement, setting P3 for now.

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

As covered in https://bugzilla.mozilla.org/show_bug.cgi?id=1413615#c8, I think we should actually be treating eSessionScoped like private-browsing mode for QuotaManager clients that don't have specific semantics for it.  Which is to say, when LocalStorage moves to QM, it can keep its special-casing of eSessionScoped, but no place else needs it.

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

At the most recent all-hands Storage UX meeting on the Wednesday morning at 8:30 (https://austinyallhands2017.sched.com/event/D5dI/next-steps-on-storage-persistence-and-permissions), I believe we reached consensus that we can treat eSessionOnly as "clear on shutdown".  So we can/should move ahead with this as proposed in comment 0. (And accordingly my comment 2 is moot and I'll tag it as such.)

Comment 4

[Johann Hofmann [:johannh]]

Yes, it sounds like that's what we want to do! :)

Comment 5

[Wennie]

Hi Andrew, I think we should implement this in 60, if possible. Can you assign this bug to someone in your team? Thanks!

Comment 6

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Shawn will be working on it this quarter, trying to target on 60. Thanks Shawn.

Comment 8

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

I don't have any chance to help on this bug further. ni Andrew.

Comment 10

[Marion Daly [:mdaly]]

:Baku can you take this over from Shawn?

Comment 11

[Andrea Marchesini [:baku]]

Attachment: session_pref2.patch

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8971301 [details] [diff] [review]
session_pref2.patch

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

The preference is a global default, but origins can have specific settings of their own.

Comment 13

[Andrew Sutherland [:asuth] (he/him)]

That is, nsContentUtils::GetCookieBehaviorForPrincipal[1] uses the the permission manager to check the "cookie" permission for the principal in question for nsContentUtils::StorageAllowedForPrincipal.  Per our IRC discussion, I think we should be checking nsContentUtils::StorageAllowedForPrincipal for each origin as part of a QuotaManager operation.  At startup this can be done during/after the initialization sweep, and at shutdown this can by walking the known quota origins in memory and scheduling specific wipes.  (A disk-based sweep at shutdown would be more thorough, but we frequently end up being I/O limited at shutdown, so using our in-memory state should be sufficient.)

https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/dom/base/nsContentUtils.cpp#8821

Comment 14

[Andrea Marchesini [:baku]]

Attachment: part 2 - Tests

Here a better approach with permission checks.

Comment 15

[Andrea Marchesini [:baku]]

Attachment: part 1 - Sanitizer.jsm

Comment 16

[Johann Hofmann [:johannh]]

Comment on attachment 8971485 [details] [diff] [review]
part 1 - Sanitizer.jsm

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

This looks okay to me, my main concern is that we're skipping the whole "pending sanitization" mechanism here. As far as I understand that gives us no guarantee that the cleanup actually happened. That might be fine (it's definitely better than the current situation), but I'd like to have mak take another look at this to be sure.

And I guess we're all in agreement that it's fine to not clear localStorage for now (because it's a pain in the butt and can't be tested) and just wait for bug 1286798 to happen instead.

::: browser/modules/Sanitizer.jsm
@@ +960,3 @@
>    }
> +
> +  // Let's retrieve the list of principals to clean up at shutdown.

I don't feel strongly about this, but it seems cleaner to put the whole block in a new function called sanitizeSessionPrincipals or something like that.

@@ +991,5 @@
> +
> +    await maybeSanitizeSessionPrincipals(principals);
> +  }
> +
> +  // Let's see if we have to forget some particular site.

Would it make sense to put this block in an "else" to the above if condition (or return early above) since we should only need this if we don't hit the if branch?

@@ +995,5 @@
> +  // Let's see if we have to forget some particular site.
> +  let enumerator = Services.perms.enumerator;
> +  while (enumerator.hasMoreElements()) {
> +    let permission = enumerator.getNext().QueryInterface(Ci.nsIPermission);
> +    if (permission.type == "cookie" &&  permission.capability == Ci.nsICookiePermission.ACCESS_SESSION) {

nit: I think there's an extra space after the &&

@@ +1003,5 @@
> +}
> +
> +// This method receives a list of principals and it checks if some of them need
> +// to be sanitize.
> +async function maybeSanitizeSessionPrincipals(principals) {

nit: We don't usually mark functions that don't await as async and I thought we had an ESLint rule for this...

@@ +1022,5 @@
> +async function sanitizeSessionPrincipal(principal) {
> +  return Promise.all([
> +    new Promise(r => {
> +      let req = quotaManagerService.clearStoragesForPrincipal(principal, null, false);
> +      req.callback = () => { r(); };

nit: req.callback = r;

Comment 17

[Johann Hofmann [:johannh]]

Comment on attachment 8971484 [details] [diff] [review]
part 2 - Tests

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

Thanks!

::: browser/base/content/test/sanitize/browser_cookiePermission.js
@@ +63,5 @@
> +
> +  // Cleaning up.
> +  await Sanitizer.runSanitizeOnShutdown();
> +
> +  // All gone!

nit: maybe "all gone" is a bit confusing as a comment here, since not all will be gone, also in the test below

@@ +74,5 @@
> +  await Sanitizer.sanitize([ "cookies", "offlineApps" ]);
> +});
> +
> +add_task(async function deleteOnlyCustomPermission() {
> +  // Let's force the session-only cookie pref.

nit: this comment seems incorrect

Comment 18

[Marco Bonardo [:mak]]

Comment on attachment 8971485 [details] [diff] [review]
part 1 - Sanitizer.jsm

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

Just to expand a little bit on what Johann said, we have a mechanism that stores the current pending sanitization in a pref, and on the next startup if the sanitization failed, we rerun it. But this is a little bit special, since it has to run regardless the sanitize-on-shutdown prefs, so we'd need dedicated code to handle sanitize-on-session items.

The thing you must be aware is that, in its current shape, it's possible this won't run in some cases, that is in case the browser crashes (and sanitize on shutdown won't run at all) or if sanitize on shutdown takes a long time and the async shutdown forces a crash to close the browser.
In those cases your data won't be sanitized.

Since this data escapes the normal sanitize-on-shutdown behavior, and it's instead the first case of sanitize-on-session, it's your choice how you want to deal with that problem: ignore, special-case, implement a whole sanitize-on-session handler.
I agree that it's an improvement VS the de-facto situation and I'll surely not block it in any form.

Clearing my review because Johann's is enough, I have a few suggestions though.

::: browser/modules/Sanitizer.jsm
@@ +48,5 @@
>    /**
> +   * Cookie lifetime policy is currently used to cleanup on shutdown other
> +   * components such as QuotaManager, localStorage, ServiceWorkers.
> +   */
> +  PREF_COOKIE_LIFETIME: "network.cookie.lifetimePolicy",

I don't understand why the Sanitizer API should expose this pref as a property, is not related to it and it's only used internally. Just use a const in the module scope?
The reasons for the other prefs to be here is that they are related to the Sanitizer and tests use them.

@@ +960,3 @@
>    }
> +
> +  // Let's retrieve the list of principals to clean up at shutdown.

I agree with johann that all this code should move to its own helper function rather than being put as-is in sanitizeOnShutdown.

@@ +966,5 @@
> +      quotaManagerService.getUsage(request => {
> +        if (request.resultCode != Cr.NS_OK) {
> +          // We are probably shutting down. We don't want to propagate the
> +          // error, rejecting the promise.
> +          resolve();

though, if anything else throws here, we'll still reject the promise.
Maybe you should just reject and .catch() the promise chain, so it is not propagated.

Additionally, this resolves to undefined, but later you just do principals.push that will fail on undefined.

I suggest you do let principals = await new Promise((resolve => reject) => {...}).catch(() => []);

@@ +1015,5 @@
> +      promises.push(sanitizeSessionPrincipal(principals[i]));
> +    }
> +  }
> +
> +  return Promise.all(promises);

note that Promise.all stops at the first rejection, if you don't want that you'll have to loop on promise.catch.
And at that point you already have a loop above that could be reused.
Personally I'd just inline sanitizeSessionPrincipal in the above loop, with some .catch, at that point.

@@ +1019,5 @@
> +  return Promise.all(promises);
> +}
> +
> +async function sanitizeSessionPrincipal(principal) {
> +  return Promise.all([

ditto re: Promise.all

Comment 19

[Marco Bonardo [:mak]]

(In reply to Marco Bonardo [::mak] from comment #18)
> I suggest you do let principals = await new Promise((resolve => reject) =>
> {...}).catch(() => []);

this should have been (resolve, reject) obviously...

Comment 20

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8971485 [details] [diff] [review]
part 1 - Sanitizer.jsm

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

There's a potential shutdown phase issue here, but it may not matter.  I'm still looking into shutdown-related issues, so expect a report back in a day or two.

The summary is: sanitizeOnShutdown() is hooked up to Places' ClientsShutdownBlocker which blocks the "profile-change-teardown" phase, which is before "profile-before-change" when the Places database actually shuts down.  QuotaManager shutdown is hooked up to "profile-before-change-qm" which is after "profile-before-change".  Explicit teardown of content processes with (modal dialog and other) nested event loop busting only happens at "profile-before-change" which is also when content processes are shutdown along with PContent.  However, nsAppStartup::Quit/nsAppStartup::CloseAllWindows should be happening prior to any profile teardown occurring, so it's possible that we're just terminating nested event loops too late in content processes and things are otherwise fine content-wise.

The main concern would be that we initiate a session-clear wipe for QM but content is still alive during/after that and is able to issue new QM operations.  Ideally we want to be sure that all content pages are fully terminated before we being the clear sweep.  If we can't guarantee that (although I feel like we should), it could be possible to modify the QM clear operation so that it takes and keeps an exclusive lock that prevents content from re-acquiring QM locks after the clear operation triggers.


re :johannh's comment on clearing current localstorage, the current LocalStorage implementation never touches disk for session-only data storage.

::: browser/modules/Sanitizer.jsm
@@ +954,5 @@
> +    let itemsToClear = getItemsToClearFromPrefBranch(Sanitizer.PREF_SHUTDOWN_BRANCH);
> +    await Sanitizer.sanitize(itemsToClear, { progress });
> +    // We didn't crash during shutdown sanitization, so annotate it to avoid
> +    // sanitizing again on startup.
> +    removePendingSanitization("shutdown");

I think :johannh already addressed that adding the cleanup logic in here bypasses much of the existing sanitization coordination logic.  That should likely be addressed, but if you don't: It seems like you actually want this line and the next line to happen at the bottom of the method, not up here.  Otherwise we can crash before doing the logic below.

@@ +960,3 @@
>    }
> +
> +  // Let's retrieve the list of principals to clean up at shutdown.

Wherever this logic adds up, I'd suggest adding a large block comment.  Here's a possibility or starting point:

Clear out QuotaManager storage for principals that have been marked as session only.  The cookie service has special logic that avoids writing such cookies to disk, but QuotaManager always touches disk, so we need to wipe the data on shutdown (or startup if we failed to wipe it at shutdown).  (Note that some session cookies do survive Firefox restarts because the Session Store that remembers your tabs between sessions takes on the responsibility for persisting them through restarts.)

The default value is determined by the "Browser Privacy" preference tab's "Cookies and Site Data" "Accept cookies and site data from websites...Keep until" setting.  The default is "They expire" (ACCEPT_NORMALLY), but it can also be set to "PRODUCT is closed" (ACCEPT_SESSION).  A permission can also be explicitly set on a per-origin basis from the Page Info's "Set Cookies" row.

We can think of there being two groups that might need to be wiped.  First, when the default is set to ACCEPT_SESSION, then all origins that use QuotaManager storage but don't have a specific permission set to ACCEPT_NORMALLY need to be wiped.  Second, the set of origins that have the permission explicitly set to ACCEPT_SESSION need to be wiped.  There are also other ways to think about and accomplish this, but this is what the logic below currently does!

@@ +991,5 @@
> +
> +    await maybeSanitizeSessionPrincipals(principals);
> +  }
> +
> +  // Let's see if we have to forget some particular site.

Contradicting :johannh here, this block is necessary.  I think the proposed block comment helps make it clear why this logic is necessary, as it's not entirely obvious on its own.

@@ +1022,5 @@
> +async function sanitizeSessionPrincipal(principal) {
> +  return Promise.all([
> +    new Promise(r => {
> +      let req = quotaManagerService.clearStoragesForPrincipal(principal, null, false);
> +      req.callback = () => { r(); };

contradicting :johannh here, it does seem appropriate to avoid resolving the nsIQuotaUsageCallback value as is done here.  It might make sense for the callback to be defined as `(/* aRequest */) => ...` to clarify we're intentionally dropping the argument on the floor while also not making linters freak out about the unused argument.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f8cfd70cc18
quota storage should be cleared on exit if user has requested cookies to be cleared on exit, r=johannh, f=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab385bc7a3fd
quota storage should be cleared on exit if user has requested cookies to be cleared on exit - tests, r=johannh

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/754f3238cab9
quota storage should be cleared on exit if user has requested cookies to be cleared on exit - fix a ES failure, r=me CLOSED TREE

Comment 23

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/2f8cfd70cc18
https://hg.mozilla.org/mozilla-central/rev/ab385bc7a3fd
https://hg.mozilla.org/mozilla-central/rev/754f3238cab9

Comment 24

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/2f8cfd70cc18
https://hg.mozilla.org/mozilla-central/rev/ab385bc7a3fd
https://hg.mozilla.org/mozilla-central/rev/754f3238cab9