Closed
Bug 1368815
Opened 7 years ago
Closed 7 years ago
not all cookies being removed when deleting containers
Categories
(Core :: DOM: Security, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: kjozwiak, Assigned: baku)
References
(Blocks 1 open bug)
Details
(Whiteboard: [userContextId][domsecurity-backlog1])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•7 years ago
|
Whiteboard: [userContextId][domsecurity-backlog1 ] → [userContextId][domsecurity-backlog1]
Updated•7 years ago
|
Priority: P2 → P3
Assignee | ||
Comment 1•7 years ago
|
||
Assignee: nobody → amarchesini
Attachment #8887617 -
Flags: review?(mconley)
Comment 2•7 years ago
|
||
Shouldn't the ContextualIdentityService be closing the tabs? If the API for extensions deletes a container we won't see this change right?
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 3•7 years ago
|
||
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.
Flags: needinfo?(amarchesini)
Comment 4•7 years ago
|
||
Is there a bug ID for the extension API improval?
Comment 5•7 years ago
|
||
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().
Attachment #8887617 -
Flags: review?(mconley) → review-
Assignee | ||
Comment 6•7 years ago
|
||
Attachment #8887617 -
Attachment is obsolete: true
Attachment #8889506 -
Flags: review?(mconley)
Comment 7•7 years ago
|
||
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 ?
Attachment #8889506 -
Flags: review?(mconley) → review+
Comment 8•7 years ago
|
||
> 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();
}
});
Flags: needinfo?(mconley)
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 9•7 years ago
|
||
> 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?
Flags: needinfo?(amarchesini)
Comment 10•7 years ago
|
||
Pushed by amarchesini@mozilla.com:
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•7 years ago
|
||
Yeah I can do that in my bug if you like :) just wanted your opinion on it.
Comment 12•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment 13•7 years ago
|
||
(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
Flags: needinfo?(mconley)
Comment 14•7 years ago
|
||
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•7 years ago
|
||
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?
You need to log in
before you can comment on or make changes to this bug.
Description
•