1283709 - Cookies are not restored correctly to container tabs after browser restart

Status: VERIFIED FIXED

(Core :: Security, defect, P1)

Description

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(follow-up from https://bugzilla.mozilla.org/show_bug.cgi?id=1280590#c7)

The session restore code for cookies has an restore algorithm which doesn't take containers into account. As a result if you have the same host open in two tabs with different containers, only ONE of the containers has its cookies restored (the last one to set the cookie).

STR:
1. Open two tabs, in seperate containers (e.g. work and bank)
2. Load https://misuse.co/containers/cookies.html in ALL tabs
3. Set the cookie to the container name (e.g. "work" in work and "bank" in bank) NOTING the order in which you set it
4. quit firefox and reopen

Result:
Only the last tab has a session cookie still set. IE if you did work then bank, only the bank tab has a cookie still.

Expected:
Both tabs still have a session cookie.

Comment 1

[Paul Theriault [:pauljt] (no longer reading bugmail)]

As linked bug, pretty sure the issue here is this code:

http://searchfox.org/mozilla-central/source/browser/components/sessionstore/SessionCookies.jsm#67

The cookie storing/restore logic doesn't have any concept of containers.IIUC it only stores one cookie per host, rather than one cookie per-host-per-container.

Comment 3

[Jonathan Hao (inactive) [:jhao]]

The CookieStore that SessionRestore uses is a map<host, map<path, map<name, cookie> > >. I think we should add origin suffix to the hosts.

Comment 4

[Tim Huang[:timhuang]]

(In reply to Jonathan Hao [:jhao] from comment #3)
> The CookieStore that SessionRestore uses is a map<host, map<path, map<name,
> cookie> > >. I think we should add origin suffix to the hosts.

I think we should add a new layer of the map which using the originAttributes suffix as the key, but not to only append suffix after the host.

Comment 5

[Tim Huang[:timhuang]]

(In reply to Tim Huang[:timhuang] from comment #4)
> I think we should add a new layer of the map which using the
> originAttributes suffix as the key, but not to only append suffix after the
> host.

Because we don't have to modify the CookieStore.getCookiesForHost() if we choose to add a new layer of the map for originAttributes since all cookies of different containers for the same host will under the map of this host. So we can get all cookies for this host without knowing the containers.

Comment 6

[Jonathan Hao (inactive) [:jhao]]

Attachment: Map SessionCookies by origin attributes.

Now it is map<host,map<originAttributes,map<path,map<name,cookie>>>>.

Comment 7

[Jonathan Hao (inactive) [:jhao]]

Attachment: Test if cookies are restored correctly to container tabs.

This is a test similar to Paul's STR. This test passes only if the other patch of mine is applied.

Mike, could you review these patches?

Comment 8

[Mike de Boer [:mikedeboer]]

Comment on attachment 8767719 [details] [diff] [review]
Test if cookies are restored correctly to container tabs.

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

Why not add this test to browser/components/sessionstore/test/browser_sessionStoreContainer.js? I prefer that you add tests close to the most common location of the files you changed (browser/components/sessionstore/SessionCookies.jsm in this case).

::: browser/components/contextualidentity/test/browser/browser_sessionRestore.js
@@ +7,5 @@
> +  "work",
> +];
> +
> +const BASE_URI = "http://mochi.test:8888/browser/browser/components/"
> +  + "contextualidentity/test/browser/empty_file.html";

nit: please put the `+` on the previous line.

@@ +12,5 @@
> +
> +const ss = Cc["@mozilla.org/browser/sessionstore;1"].getService(Ci.nsISessionStore);
> +const {TabStateFlusher} = Cu.import("resource:///modules/sessionstore/TabStateFlusher.jsm", {});
> +
> +// opens `uri' in a new tab with the provided userContextId and focuses it.

nit: start comments with a capitalized first character.
nit2: please be consistent which quotes you use.

@@ +13,5 @@
> +const ss = Cc["@mozilla.org/browser/sessionstore;1"].getService(Ci.nsISessionStore);
> +const {TabStateFlusher} = Cu.import("resource:///modules/sessionstore/TabStateFlusher.jsm", {});
> +
> +// opens `uri' in a new tab with the provided userContextId and focuses it.
> +// returns the newly opened tab

nit: please close your sentences with a dot.

@@ +15,5 @@
> +
> +// opens `uri' in a new tab with the provided userContextId and focuses it.
> +// returns the newly opened tab
> +function* openTabInUserContext(uri, userContextId) {
> +  // open the tab in the correct userContextId

nit: same as above.

@@ +24,5 @@
> +  tab.ownerDocument.defaultView.focus();
> +
> +  let browser = gBrowser.getBrowserForTab(tab);
> +  yield BrowserTestUtils.browserLoaded(browser);
> +  return {tab, browser};

nit: It's ok to let object literals breathe: `return { tab, browser };`

@@ +31,5 @@
> +add_task(function* setup() {
> +  // make sure userContext is enabled.
> +  yield SpecialPowers.pushPrefEnv({"set": [
> +      ["privacy.userContext.enabled", true]
> +    ]});

Please fold this task with the actual `test` task below.

nit: please format this object literal properly. I propose:
```js
yield SpecialPowers.pushPrefEnv({
  set: [ [ "privacy.userContext.enabled", true ] ]
});
```

@@ +50,5 @@
> +add_task(function* test() {
> +  let lastSessionRestore;
> +  for (let userContextId of Object.keys(USER_CONTEXTS)) {
> +    // load the page in 3 different contexts and set a cookie
> +    // which should only be visible in that context

nit: please format this comment properly.

@@ +53,5 @@
> +    // load the page in 3 different contexts and set a cookie
> +    // which should only be visible in that context
> +    let cookie = USER_CONTEXTS[userContextId];
> +
> +    // open our tab in the given user context

nit: same here.

@@ +54,5 @@
> +    // which should only be visible in that context
> +    let cookie = USER_CONTEXTS[userContextId];
> +
> +    // open our tab in the given user context
> +    let {tab, browser} = yield* openTabInUserContext(BASE_URI, userContextId);

nit: It's ok to let destructuring assignments breathe: `let { tab, browser } = yield ...`

@@ +60,5 @@
> +    yield Promise.all([
> +        waitForNewCookie(),
> +        ContentTask.spawn(browser, cookie, function (cookie) {
> +          content.document.cookie = cookie;
> +        })

nit: too much indentation.
nit2: `cookie => content.document.cookie = cookie`

@@ +74,5 @@
> +  }
> +
> +  let state = JSON.parse(lastSessionRestore);
> +  is(state.windows[0].cookies.length, USER_CONTEXTS.length,
> +      "session restore should have each container's cookie");

nit: please indent using two spaces.

Comment 9

[Mike de Boer [:mikedeboer]]

Comment on attachment 8767626 [details] [diff] [review]
Map SessionCookies by origin attributes.

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

I'm OK with this with an all-green try run for mochitest-browser.

::: browser/components/sessionstore/SessionCookies.jsm
@@ +457,5 @@
>        this._hosts.set(cookie.host, new Map());
>      }
>  
> +    let originAttributesMap = this._hosts.get(cookie.host);
> +    let originAttributes = ChromeUtils.originAttributesToSuffix(

Can you put a comment here that documents what the default return value will be be when `cookie.originAttributes` is null?

@@ +458,5 @@
>      }
>  
> +    let originAttributesMap = this._hosts.get(cookie.host);
> +    let originAttributes = ChromeUtils.originAttributesToSuffix(
> +        cookie.originAttributes);

Please keep this on 1 line, 80ch is not a hard limit in my book.

Comment 10

[Jonathan Hao (inactive) [:jhao]]

Attachment: Test if cookies are restored correctly to container tabs.

Thank you, Mike. I moved the test to session restore folder, and fixed the nits. Please check again.

Comment 11

[Jonathan Hao (inactive) [:jhao]]

Attachment: Map SessionCookies by origin attributes.

Added a comment. I'll push a try run soon.

Comment 12

[Jonathan Hao (inactive) [:jhao]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2466772dc7a

Comment 13

[Mike de Boer [:mikedeboer]]

Comment on attachment 8768320 [details] [diff] [review]
Test if cookies are restored correctly to container tabs.

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

Nice! Looks very clean to me.

Comment 14

[Jonathan Hao (inactive) [:jhao]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7df4e14a35ea30cf39fc2372fcfd62447b9a28ae
Bug 1283709 - Map SessionCookies by origin attributes. r=mikedeboer

https://hg.mozilla.org/integration/mozilla-inbound/rev/51cc2f61dad67a7abae005a494b6c1502481c0ed
Bug 1283709 - Test if cookies are restored correctly to container tabs. r=mikedeboer

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7df4e14a35ea
https://hg.mozilla.org/mozilla-central/rev/51cc2f61dad6

Comment 16

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Comment on attachment 8768320 [details] [diff] [review]
Test if cookies are restored correctly to container tabs.

Approval Request Comment
[Feature/regressing bug #]: Testpilot for containers
[User impact if declined]: Session restore will be broken for testpilot users.
[Describe test coverage new/current, TreeHerder]:  Some Origin Attributes testing is covered in CAPS unit tests, more is in progress.
[Risks and why]: This involves platform changes that cannot be reprecated by an addon.  In order to do a Test Pilot experiment for Containers in September / Firefox 49 release, we need this code in release.  Without it, testing is delayed until November and getting the capabilities of the feature to users is delayed even further.
[String/UUID change made/needed]: None

Comment 17

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Comment on attachment 8768323 [details] [diff] [review]
Map SessionCookies by origin attributes.

Approval Request Comment
As above - this is the second part of a fix for the same issue.

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8768320 [details] [diff] [review]
Test if cookies are restored correctly to container tabs.

Session restore fix for Test Pilot work. Please uplift.

Comment 19

[Carsten Book [:Tomcat]]

the second part don;t apply cleanly to aurora
grafting 353061:51cc2f61dad6 "Bug 1283709 - Test if cookies are restored correctly to container tabs. r=mikedeboer"
merging browser/components/sessionstore/test/browser_sessionStoreContainer.js
warning: conflicts while merging browser/components/sessionstore/test/browser_sessionStoreContainer.js!

Comment 20

[Jonathan Hao (inactive) [:jhao]]

Attachment: Test if cookies are restored correctly to container tabs.

Rebased for aurora uplift.

Comment 21

[Jonathan Hao (inactive) [:jhao]]

Please try again. Thank you.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ad99b4dccc17
https://hg.mozilla.org/releases/mozilla-aurora/rev/ff681ca2951f

Comment 23

[Kamil Jozwiak [:kjozwiak]]

Reproduced the original issue following the STR from comment#0 using the following build:
* https://archive.mozilla.org/pub/firefox/nightly/2016/06/2016-06-30-03-02-07-mozilla-central/
** fx50.0a1, buildId: 20160630030207, changeset: d700dc054751

Went through verification using the following builds:
* https://archive.mozilla.org/pub/firefox/nightly/2016/08/2016-08-02-03-04-37-mozilla-central/
** fx51.0a1, buildId: 20160802030437, changeset: ffac2798999c
* https://archive.mozilla.org/pub/firefox/nightly/2016/07/2016-07-31-00-40-03-mozilla-aurora/
** fx49.0a2, buildId: 20160731004003, changeset: 77ab90179681

OS's Used:

* macOS 10.11.6 x64 - PASSED
* Windows 10 x64 VM - PASSED
* Ubuntu 14.04.5 LTS - PASSED

* went through the original test case from comment#0 using e10s and non-e10s window/tabs without any issues
* ensured that "Show my windows and tabs from last time" under about:preferences#general worked without any issues (using the STR from comment#0)