1368815 - not all cookies being removed when deleting containers

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P3)

Description

[Kamil Jozwiak [:kjozwiak]]

When a user deletes a container from fx, sometimes some of the cookies that are associated with that specific userContextId are left behind and not correctly removed. We shouldn't be leaving behind any cookies once a container was deleted. The Containers test pilot implementaiton has this issue [1] as well.

[1] https://github.com/mozilla/testpilot-containers/issues/522

STR:

* install the latest version of m-c (should already have containers enabled by default)
* open a "Shopping" container via the "+" new tab button
* load ebay.com inside the "Shopping" container and login/sign-in
* open a new tab and visit about:preferences#containers
* remove the "Shopping" container while it's still opened (same result if the tab is closed)

Now if you view the cookie manager or the cookies.sqlite database, you'll notice that there's a cookie that was left behind, example:
* "163" "ebay.com" "^userContextId=4" "npii" "btguid/[generated #]^cguid/[generated #]^" ".ebay.com" "/" "1527712036" "1496176037060153" "1496176036924867" "0" "0"	"0"

I've managed to reproduce this using draftkings.com as well which left behind two different cookies rather than the one in the above example.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: tabClosed.patch

Comment 2

[Jonathan Kingston [:jkt] he/him]

Shouldn't the ContextualIdentityService be closing the tabs? If the API for extensions deletes a container we won't see this change right?

Comment 3

[Andrea Marchesini [:baku]]

Sure, but that is another bug. Here I just want to be sure that cookies are correctly removed.
Currently the web extension API is something meant to be used by Container addon. We need to improve it.

Comment 4

[Jonathan Kingston [:jkt] he/him]

Is there a bug ID for the extension API improval?

Comment 5

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

Comment on attachment 8887617 [details] [diff] [review]
tabClosed.patch

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

Hey baku, sorry for the delay. This looks good - just a few minor issues. Thanks!

::: browser/components/preferences/in-content-new/containers.js
@@ +62,5 @@
> +        if (rv != 0) {
> +          return;
> +        }
> +
> +        return ContextualIdentityService.closeContainerTabs(userContextId);

Same commentary as the other containers.js.

::: browser/components/preferences/in-content-new/privacy.js
@@ +133,5 @@
>      let rv = Services.prompt.confirmEx(window, title, message, buttonFlags,
>                                         okButton, cancelButton, null, null, {});
>      if (rv == 0) {
> +      ContextualIdentityService.closeContainerTabs().then(() => {
> +        Services.prefs.setBoolPref("privacy.userContext.enabled", false);

Same question as the other privacy.js script.

::: browser/components/preferences/in-content/containers.js
@@ +39,5 @@
>        this._list.appendChild(item);
>      }
>    },
>  
>    onRemoveClick(button) {

Alternatively, if you wanted to avoid some Promise gymnastics, you could make this an async function, like:

async onRemoveClick(button) {
  // And in here, await the Promises you need.
  let userContextId = parseInt...
  let count = ...

  // ...

  await ContextualIdentityService.closeContainerTabs(userContextId);
  ContextualIdentityService.remove(userContextId);
  this._rebuildView();
},

I'd probably prefer that over the Promise chaining, fwiw.

::: browser/components/preferences/in-content/privacy.js
@@ +106,5 @@
>      let rv = Services.prompt.confirmEx(window, title, message, buttonFlags,
>                                         okButton, cancelButton, null, null, {});
>      if (rv == 0) {
> +      ContextualIdentityService.closeContainerTabs().then(() => {
> +        Services.prefs.setBoolPref("privacy.userContext.enabled", false);

Is there a risk of the user opening more container tabs before this pref has been set, but after closeContainerTabs has called? Should we set the pref before calling closeContainerTabs?

::: toolkit/components/contextualidentity/ContextualIdentityService.jsm
@@ +39,5 @@
> +  Services.obs.addObserver(this, "ipc:browser-destroyed");
> +}
> +
> +_TabRemovalObserver.prototype = {
> +  _promise: null,

Should this be _resolver ?

@@ +46,5 @@
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver]),
> +
> +  observe(subject, topic, data) {
> +    let tabParent = subject.QueryInterface(Ci.nsITabParent);
> +    let pos = this._tabParentIds.indexOf(tabParent.tabId);

It might be simpler to store the _tabParentIds in a Set().

@@ +54,5 @@
> +
> +    this._tabParentIds.splice(pos, 1);
> +    if (this._tabParentIds.length == 0) {
> +      this._resolver();
> +      Services.obs.removeObserver(this, "ipc:browser-destroyed");

We should probably remove the observer before calling _resolver().

Comment 6

[Andrea Marchesini [:baku]]

Attachment: tabClosed.patch

Comment 7

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

Comment on attachment 8889506 [details] [diff] [review]
tabClosed.patch

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

Thanks!

::: toolkit/components/contextualidentity/ContextualIdentityService.jsm
@@ +40,5 @@
> +}
> +
> +_TabRemovalObserver.prototype = {
> +  _resolver: null,
> +  _tabParentIds: [],

Should perhaps initialize this to null or an empty Set so as to not confuse future hackers.

@@ +51,5 @@
> +      this._tabParentIds.delete(tabParent.tabId);
> +      if (this._tabParentIds.size == 0) {
> +        Services.obs.removeObserver(this, "ipc:browser-destroyed");
> +        this._resolver();
> +      r

Trailing r! :D

@@ +354,5 @@
> +      new _TabRemovalObserver(resolve, tabParentIds);
> +    });
> +  },
> +
> +  disableContainers() {

This isn't actually disabling the containers - just broadcasting that we're clearing the data. Maybe this should be renamed to reflect that. Maybe, notifyAllContainersCleared ?

Comment 8

[Jonathan Kingston [:jkt] he/him]

> This isn't actually disabling the containers - just broadcasting that we're clearing the data. Maybe this should be renamed to reflect that. Maybe, notifyAllContainersCleared ?

For my enabling containers patch, I acually need a method exposing on the ContextualIdentityService to do the following:
- Close all container tabs
- Delete custom containers
- Create new containers that match defaults

My plan was just to observe the pref in the service and call this method internally as we would have two places in the code that need to disable containers and potentially users could disable the pref manually anyway.

Is there a large cost to observing the pref in the service and doing this work there? Currently because of the way the extensions preference setting works I can't know when the preference has been changed to the user default so I would likely need an observer there anyway: https://bugzilla.mozilla.org/show_bug.cgi?id=1354602#c17

Essentially have something like this:

  this._prefObserver = new PrefObserver("privacy.userContext.enabled");
  this._prefObserver.on("privacy.userContext.enabled", () => {
    if (!containersEnabled) {
      this.containersRemove();
    }
  });

Comment 9

[Andrea Marchesini [:baku]]

> Essentially have something like this:
> 
>   this._prefObserver = new PrefObserver("privacy.userContext.enabled");
>   this._prefObserver.on("privacy.userContext.enabled", () => {
>     if (!containersEnabled) {
>       this.containersRemove();
>     }
>   });


This seems reasonable to me. But can we do this in a follow up bug?

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/423e07f7d2d4
ContextualIdentityService should remove containers only when all the tabs are completely closed, r=mconley

Comment 11

[Jonathan Kingston [:jkt] he/him]

Yeah I can do that in my bug if you like :) just wanted your opinion on it.

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/423e07f7d2d4

Comment 13

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

(In reply to Jonathan Kingston [:jkt] from comment #8)
> Is there a large cost to observing the pref in the service and doing this
> work there? Currently because of the way the extensions preference setting
> works I can't know when the preference has been changed to the user default
> so I would likely need an observer there anyway:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1354602#c17
> 

IMO, this is the right thing to do - a single pref observer in some component. Though, this pattern is probably sufficient:

Services.prefs.addObserver("pref.name", ...);

Example: http://searchfox.org/mozilla-central/rev/ad093e98f42338effe2e2513e26c3a311dd96422/browser/base/content/browser-places.js#1552

Comment 14

[Jonathan Kingston [:jkt] he/him]

FYI this patch doesn't seem to contain the required changes for the main.js in: http://searchfox.org/mozilla-central/source/browser/components/preferences/in-content-new/main.js#951

Little worried by the service not included in that page, my plan is that we will enable showing this checkbox in Bug 1354602 when someone installs a container addon.

So I will address that issue there with the pref observer solution.

Comment 15

[Luke Crouch [:groovecoder]]

FWIW, I couldn't reproduce this with the latest Nightly build. Has this bug been overcome-by-events? Could other changes have fixed it? Or is it just a flaky racy condition that will always be hard to reproduce?