1191460 - Set userContextId on the docShell for new non-remote (non-e10s) tabs

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Steven Englehardt [:englehardt]]

When a user creates a New Tab with a non-default value for the userContextId (Bug 1179557), this value should be set during the docShell's creation. In this bug, we'll add the plumbing necessary for value to propagate from the UI to the originAttibutes dictionary in the docShell, in both e10s and non-e10s conditions.

See Bug 1191418 for more information.

Comment 1

[Steven Englehardt [:englehardt]]

Attachment: 1191460.patch

WIP Patch. Sets the userContextId on the docShell for non-e10s browser windows.

* Need to de-couple from the WIP Patch in Bug 1191442
* e10s windows don't open about:newtab as remote (and this is the current starting page for containers). Will need some additional changes to TabParent.cpp / TabChild.cpp to to support e10s.
* Update/Remove the changes to BasePrincipal.h/.cpp. Should file another bug to remove of the constructor which takes AppId/InBrowser as explicit arguments.
* In nsDocShell::GetUserContextId() I think we'll have to traverse up the docShell's parents as is done in nsDocShell::GetAppId()(https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#13739). Though the inheritance work in Bug 1165466 might make this unnecessary.

Comment 2

[Steven Englehardt [:englehardt]]

Attachment: 1191460-pt1-ui-support.patch

Moving UI support for userContextId over from 1191442.

Comment 3

[Steven Englehardt [:englehardt]]

Attachment: 1191460-pt2-non-e10s.patch

Works only for non-remote tabs. It seems that about:newtab (the current landing page for container tabs created using Bug 1191442) is never remote, so this works in both e10s and non-e10s windows.

* Support for remote tabs coming in pt3 patch.
* In an e10s window, when about:newtab is loaded it the tab is not remote. When other content is loaded in the tab it becomes remote. Similarly tabs dragged between windows may change remoteness. Need to make sure the id persists in these cases. (See: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#1505)

Comment 4

[Steven Englehardt [:englehardt]]

Bobby: Am I correct in thinking that once the changes you discuss in Bug 1165466 are implemented (https://bugzilla.mozilla.org/show_bug.cgi?id=1165466#c8), child docShells will inherit the attributes from their parents as they are created? This will remove the need to traverse up the parent docShells in an attribute getter until a non-unknown attribute is found (as is done for appId and as I do here: https://bugzilla.mozilla.org/attachment.cgi?id=8647223&action=diff#a/docshell/base/nsDocShell.cpp_sec4)?

Comment 5

[Bobby Holley (:bholley)]

(In reply to Steven Englehardt [:senglehardt] from comment #4)
> Bobby: Am I correct in thinking that once the changes you discuss in Bug
> 1165466 are implemented
> (https://bugzilla.mozilla.org/show_bug.cgi?id=1165466#c8), child docShells
> will inherit the attributes from their parents as they are created? This
> will remove the need to traverse up the parent docShells in an attribute
> getter until a non-unknown attribute is found (as is done for appId and as I
> do here:
> https://bugzilla.mozilla.org/attachment.cgi?id=8647223&action=diff#a/
> docshell/base/nsDocShell.cpp_sec4)?

That is correct, so long as you make the property appropriately inheritable.

Comment 6

[Tanvi Vyas[:tanvi]]

Comment on attachment 8647223 [details] [diff] [review]
1191460-pt2-non-e10s.patch

Once originAttributes are inherited by child docshells, we can remove the UNKNOWN_USER_CONTEXT_ID.  And simplify GetUserContextId to simply:

+nsDocShell::GetUserContextId(uint32_t* aUserContextId)
+{
+    *aUserContextId = mUserContextId;
+    return NS_OK;
+}

Comment 7

[Steven Englehardt [:englehardt]]

Attachment: 1191460-pt3-e10s.patch

WIP patch.

Comment 8

[Steven Englehardt [:englehardt]]

Attachment: 1191460-pt1-ui-support.patch

Comment 9

[Steven Englehardt [:englehardt]]

Attachment: 1191460-pt2-non-e10s.patch

Johnny: Since this touches dom and docshell you seem like the best person to review. Please let me know otherwise.

Comment 10

[Steven Englehardt [:englehardt]]

Attachment: 1191460-pt3-e10s.patch

Comment 11

[Steven Englehardt [:englehardt]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1176060e2958

Comment 12

[Steven Englehardt [:englehardt]]

Attachment: 1191460-pt1-ui-support.patch

Should fix a majority of issues from last try push. New push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9da8e2926132

Comment 13

[:Paolo Amadini]

Comment on attachment 8648258 [details] [diff] [review]
1191460-pt1-ui-support.patch

Heh, I'm not familiar with this area of the code but the changes look straightforward. I can probably take a closer look later this week.

Comment 14

[Bobby Holley (:bholley)]

Comment on attachment 8647841 [details] [diff] [review]
1191460-pt2-non-e10s.patch

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

I can review the docshell pieces.

We shouldn't manually track this attribute on the docshell. We should track it inside of the OriginAttributes added in bug 1165466 (that takes care of the inheritance part too).

There's also no reason to add an UNKNOWN_* id. That was a hack that was added when appId was introduced, and we've been working very hard to get rid of it.

Comment 15

[Bobby Holley (:bholley)]

Comment on attachment 8647843 [details] [diff] [review]
1191460-pt3-e10s.patch

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

Let's wait until the other patch sorts itself out, but I'm pretty sure that the solution here is going to be making the existing code more generic (such that it operates in terms of OriginAttributes) rather than adding more special handling for this additional attribute.

The basic idea here is that we want to be minimize the work required for future projects like this where we want to add a new OriginAttribute, and centralize all the logic to deal with them. This patch doesn't look like it's taking us in that direction.

Comment 16

[:Paolo Amadini]

Comment on attachment 8648258 [details] [diff] [review]
1191460-pt1-ui-support.patch

Clearing the review flag while we figure out how to handle this at the platform level.

Comment 17

[Steven Englehardt [:englehardt]]

After speaking with Bobby yesterday, the plan for this bug is as follows:

(Part 1) Move the UI component back to Bug 1191442 to avoid blocking Bug 1191494. Even if the attribute isn't consumed anywhere.
(Part 2) Wait on Bug 1165466 for the non-e10s case. Update the getter and setter in that patch to use originAttributes. Land this part of the patch independent of e10s tabs.
(Part 3) Move the e10s case to Bug 1195881 blocking on Bug 1191740. This will largely require a rewrite of that patch.

Comment 18

[Kate McKinley [:kmckinley, :Kate]]

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

Comment 19

[Kate McKinley [:kmckinley, :Kate]]

Treeherder failures are in unrelated code. Windows mochitests don't appear to run, even after a second attempt. (https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b5cd4732b2a)

Comment 20

[Kate McKinley [:kmckinley, :Kate]]

(In reply to Kate McKinley [:kmckinley] from comment #19)
> Treeherder failures are in unrelated code. Windows mochitests don't appear
> to run, even after a second attempt.
> (https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b5cd4732b2a)

Disregard

Comment 21

[Kate McKinley [:kmckinley, :Kate]]

Attachment: WIP - Set and use the user context ID based on the tab.

This WIP patch updates Steve's patch to mozilla-current and fixes a mismatch where BasePrincipal is expecting "userContextId", while tabbrowser.xml uses "userContextId". It seems as though there is something else in the stack that has this dependency, though I haven't found it yet. The only other thing I have is that dom/bindings/ChromeUtilsBinding.cpp, which is auto-generated, uses camel-case "userContextId".

Comment 22

[Bobby Holley (:bholley)]

Comment on attachment 8683495 [details] [diff] [review]
WIP - Set and use the user context ID based on the tab.

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

I'm busy with other things right now, so review feedback is going to be very laggy going forward. It might be better to flag sicking.

::: browser/base/content/tabbrowser.xml
@@ +1778,1 @@
>                t.setAttribute("usercontextid", aUserContextId);

Why do we want both?

::: caps/nsIScriptSecurityManager.idl
@@ +266,5 @@
>      const unsigned long UNKNOWN_APP_ID = 4294967295; // UINT32_MAX
>      const unsigned long SAFEBROWSING_APP_ID = 4294967294; // UINT32_MAX - 1
>  
>      const unsigned long DEFAULT_USER_CONTEXT_ID = 0;
> +    const unsigned long UNKNOWN_USER_CONTEXT_ID = 4294967295; // UINT32_MAX

I think I complained about this before, but I don't think we want UNKNOWN_USER_CONTEXT_ID - dealing with 'unknown' was a huge problem with appid.

I think we should get rid of it, and switch the userContextId on the docshell to Maybe<long> mOwnUserContextId.

::: docshell/base/nsIDocShell.idl
@@ +841,5 @@
> +
> +  /**
> +   * Returns the id of the userContext associated with this docshell.
> +   */
> +  [infallible] readonly attribute unsigned long userContextId;

I don't think this needs to be settable from script, or gettable outside the OriginAttributes getter.

Instead, just add a C++-only setter on nsDocShell and use nsDocShell::Cast from nsFrameLoader to emplace the Maybe<>.

::: dom/base/nsFrameLoader.cpp
@@ +1831,5 @@
> +    docShell->GetUserContextId(&userContextId);
> +    mDocShell->SetUserContextId(userContextId);
> +    didSetUserContextId = true;
> +  }
> +

I don't think we want this to be settable on arbitrary iframes by content. I think we should put this somewhere whereby it only gets set on a top-level content docshell.

Comment 23

[Kate McKinley [:kmckinley, :Kate]]

Attachment: set and use the userContextId

Simplified the patch as much as possible and addressed Bobby's comments.

Comment 24

[Tanvi Vyas[:tanvi]]

Comment on attachment 8684039 [details] [diff] [review]
set and use the userContextId

>+/* [infallible] */ NS_IMETHODIMP
>+nsDocShell::GetUserContextId(uint32_t* aUserContextId)
>+{
>+  if (mUserContextId) {
>+    *aUserContextId = mUserContextId.value();
>+    return NS_OK;
>+  }
>+
>+  nsCOMPtr<nsIDocShell> parent;
>+  GetSameTypeParentIgnoreBrowserAndAppBoundaries(getter_AddRefs(parent));
>+
>+  if (!parent) {
>+    *aUserContextId = nsIScriptSecurityManager::DEFAULT_USER_CONTEXT_ID;

Should we ever hit this code path?  mUserContextId is supposed to be set when the tab is created, so shouldn't we always have a context id at this point.  Or do we not set mUserContextId when we use the default context of 0?  Can you add a breakpoint here and see what time of cases we hit this?  If we don't hit it, we may consider adding an assert.  I just want to make sure we never end up using the default context when the user has asked to use a different context.

>+    return NS_OK;
>+  }
>+
>+  nsDocShell *parentDocShell = nsDocShell::Cast(parent);
>+  return parentDocShell->GetUserContextId(aUserContextId);

Also, shouldn't this inheritance happen when the child docshell is created?  When parent creates child, create it with the inherited mUserContextId.

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8684039 [details] [diff] [review]
set and use the userContextId

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

::: docshell/base/nsDocShell.cpp
@@ +13793,5 @@
> +    return NS_OK;
> +  }
> +
> +  nsCOMPtr<nsIDocShell> parent;
> +  GetSameTypeParentIgnoreBrowserAndAppBoundaries(getter_AddRefs(parent));

I don't think you need to make it this complicated. Or indeed that you should.

Let each docshell simply have its own userContextId. When we create child docshells we should inherit that contextid down to the child docshell.

So no need to walk up the parent chain of docshells like this.

That also means that you can make mUserContextId be a simple integer which defaults to the default value.

The inheritance stuff should happen in bug 1209162 or a followup bug to it.

Comment 26

[Kate McKinley [:kmckinley, :Kate]]

Attachment: contextual identity test

This patch adds a basic test to determine if different user context ids see different cookies.

TODO: localStorage, sessionStorage

Comment 27

[Kate McKinley [:kmckinley, :Kate]]

Attachment: set and use the userContextId

Removed dead code.

Comment 28

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8686992 [details] [diff] [review]
contextual identity test

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

I don't think i know enough about browser-chrome tests to be a good person to review this.

That said, you should also check that APIs that make network requests end up sending the correct cookies to the server. Unfortunately we have a lot of APIs these days so it's hard to test them all. But add a grab-bag of the most common ones, and a few weird ones, like

<img>
<iframe>
<link rel=stylesheet>
<script>
XHR
fetch()
importScripts in a worker
XHR in a worker
navigator.sendBeacon
imported-stylesheet-in-<link rel=stylesheet>
font-in-imported-stylesheet-in-<link rel=stylesheet>

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8686993 [details] [diff] [review]
set and use the userContextId

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

::: docshell/base/nsDocShell.h
@@ +994,5 @@
>    // find it by walking up the docshell hierarchy.)
>    uint32_t mOwnOrContainingAppId;
>  
> +  // userContextId signifying which container we are in
> +  mozilla::Maybe<uint32_t> mUserContextId;

Why not use a plain uint32_t?

Comment 30

[Kate McKinley [:kmckinley, :Kate]]

Attachment: set and use the userContextId

use uint32_t instead of maybe

Comment 31

[Kate McKinley [:kmckinley, :Kate]]

Attachment: contextual identity test

Update test to add failing localStorage test as "todo"

Comment 32

[Kate McKinley [:kmckinley, :Kate]]

Attachment: contextual identity test

Rebased patch

Comment 33

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Contextual Identity implementation

Comment 34

[Kate McKinley [:kmckinley, :Kate]]

Attachment: contextual identity test

Comment 35

[Tanvi Vyas[:tanvi]]

Comment on attachment 8688257 [details] [diff] [review]
Contextual Identity implementation

>@@ -14036,16 +14052,17 @@ nsDocShell::ShouldPrepareForIntercept(ns
>       if (isThirdPartyURI) {
>         return NS_OK;
>       }
>     }
>   }
> 
>   if (aIsNonSubresourceRequest) {
>     OriginAttributes attrs(GetAppId(), GetIsInBrowserElement());
>+    attrs.mUserContextId = mUserContextId;
Can we just do "OriginAttributes attrs = GetOriginAttributes();" instead

>     *aShouldIntercept = swm->IsAvailable(attrs, aURI);
>     return NS_OK;
>   }
> 
>   nsCOMPtr<nsIDocument> doc = GetDocument();
>   if (!doc) {
>     return NS_ERROR_NOT_AVAILABLE;
>   }
>@@ -14127,16 +14144,17 @@ nsDocShell::ChannelIntercepted(nsIInterc
>     if (!doc) {
>       return NS_ERROR_NOT_AVAILABLE;
>     }
>   }
> 
>   bool isReload = mLoadType & LOAD_CMD_RELOAD;
> 
>   OriginAttributes attrs(GetAppId(), GetIsInBrowserElement());
>+  attrs.mUserContextId = mUserContextId;
Same here: OriginAttributes attrs = GetOriginAttributes();

> 
>   ErrorResult error;
>   nsCOMPtr<nsIRunnable> runnable =
>     swm->PrepareFetchEvent(attrs, doc, aChannel, isReload, isSubresourceLoad, error);
>   if (NS_WARN_IF(error.Failed())) {
>     return error.StealNSResult();
>   }
> 
>diff --git a/docshell/base/nsDocShell.h b/docshell/base/nsDocShell.h
>--- a/docshell/base/nsDocShell.h
>+++ b/docshell/base/nsDocShell.h
>@@ -14,16 +14,17 @@
> #include "nsIBaseWindow.h"
> #include "nsINetworkInterceptController.h"
> #include "nsIScrollable.h"
> #include "nsITextScroll.h"
> #include "nsIContentViewerContainer.h"
> #include "nsIDOMStorageManager.h"
> #include "nsDocLoader.h"
> #include "mozilla/BasePrincipal.h"
>+#include "mozilla/Maybe.h"
I don't think you need this anymore.

Comment 36

[Tanvi Vyas[:tanvi]]

Comment on attachment 8688288 [details] [diff] [review]
contextual identity test

Hi Kate,
Thanks for adding tests!  They are however quite difficult to follow.  Can you add more comments and more descriptive variable names for expected, expectation, and idx.  r- for now and I'd like to see another patch.  Also probably needs review by a peer.


>diff --git a/browser/components/contextualidentity/test/browser/browser_usercontext.js b/browser/components/contextualidentity/test/browser/browser_usercontext.js
>+  let tabs = {};
>+
>+  for (var exp in expectations) {
>+    // load the page in 3 different contexts and set a cookie
>+    // which should only be visibile in that context
>+    var userContextId = expectations[exp]['userContextId'];
>+    let tab = openTabInUserContext(uri+'&cookie=' + expectations[exp]['cookie'],
>+        expectations[exp]['userContextId']);
>+    tabs[expectations[exp]['cookie']] = userContextId;
What is this for?  This sets tabs[default]=0.  We cycle through tabs below, but don't actually use its value.

>+
>+    //let tab = getTabFromUserContextId(userContextId);
Remove this

>diff --git a/browser/components/contextualidentity/test/browser/browser_usercontext.sjs b/browser/components/contextualidentity/test/browser/browser_usercontext.sjs
>new file mode 100644
>--- /dev/null
>+++ b/browser/components/contextualidentity/test/browser/browser_usercontext.sjs
>@@ -0,0 +1,64 @@
>+// SJS file for contextual identity mochitests
>+
>+function writeResponse(response, file) {
>+  if (file) {
>+    response.write(loadResponseFromFile(file));
>+  } else {
>+    response.write('...');
>+  }
>+}
>+
Is there a way you could incorporate what file_reflect_cookie_into_title.html does directly into writeResponse, so you don't need a separate file and don't need loadResponseFromFile().


...

>+
>+  // avoid confusing cache behaviors
>+  response.setHeader("Cache-Control", "no-cache", false);
>+
>+  // Deliver the CSP policy encoded in the URI
Why does this mention a CSP policy?

>diff --git a/browser/components/contextualidentity/test/browser/file_reflect_cookie_into_title.html b/browser/components/contextualidentity/test/browser/file_reflect_cookie_into_title.html
>new file mode 100644
>--- /dev/null
>+++ b/browser/components/contextualidentity/test/browser/file_reflect_cookie_into_title.html
>@@ -0,0 +1,29 @@
>+<html>
>+  <head>
>+    <meta charset="UTF-8">
>+    <title>title not set</title>
>+    <script>
>+      var query = window.location.search.substr(1).split('&');
>+      var userContext = '';
>+      for (var idx in query) {
What does idx stand for?  Isn't there just one & in the location for "&cookie="?

>+        [ name, value ] = query[idx].split('=');
>+        console.log('setting userContext to '+value);
Where do we actually set userContext to value?  Isn't userContext = '' above?

>+        if (name == 'cookie') {
>+          localStorage.clear();
>+          localStorage.setItem('userContext', value);
>+          sessionStorage.clear();
>+          sessionStorage.setItem('userContext', value);
>+        }
cookie is the only idx, so you may not need this if.  Why do you clear localStorage and sessionStorage first?

>+      }
>+
>+      var [name, val] = document.cookie.split('=');
>+
>+      document.title = 'cookie=' + val + '|'
>+                       + 'local=' + localStorage.getItem('userContext') + '|'
>+                       + 'session=' + sessionStorage.getItem('userContext')
>+      ;

Comment 37

[Kate McKinley [:kmckinley, :Kate]]

Attachment: contextual identity test - cookie, localStorage

Cleaned up and significantly shortened the test patch.

For now, per our conversation, this patch only tests cookies and localStorage. Currently localStorage is not separated by user context, so those tests are marked `todo'. (See bug 1225363)

The reason we load the file is to be able to load different files in subsequent test, similar to how CSP tests work. Most of that is stripped out for now.

Comment 38

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Contextual Identity implementation

Comment 39

[Kate McKinley [:kmckinley, :Kate]]

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

Comment 40

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8688782 [details] [diff] [review]
contextual identity test - cookie, localStorage

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

(The moz.build files/changes will need review from a build peer.)

The approach looks great in general, but I think we can simplify things a little bit.

::: browser/components/contextualidentity/test/browser/browser_usercontext.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Tests should have public domain headers nowadays.

@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +add_task(function test() {

add_task(function* test() {

@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +add_task(function test() {
> +  // a simple mapping of
> +  const userContexts = [

Please move out of the task and to the top, name it USER_CONTEXTS.

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

Define this outside of the task to keep it more readable?

@@ +15,5 @@
> +  function openTabInUserContext(uri, userContextId) {
> +    // open the tab in the correct userContextId
> +    let tab = gBrowser.addTab(uri, {
> +      userContextId: userContextId
> +    });

Nit: could be

let tab = gBrowser.addTab(uri, {userContextId});

@@ +20,5 @@
> +
> +    // select tab and make sure its browser is focused
> +    let browser = gBrowser.getBrowserForTab(tab);
> +    browser.selectedTab = tab;
> +    browser.ownerDocument.defaultView.focus();

gBrowser.selectedTab = tab;
tab.ownerDocument.defaultView.focus();

@@ +31,5 @@
> +  // is enabled.
> +  SpecialPowers.pushPrefEnv({"set": [
> +    ["browser.tabs.remote.autostart", false],
> +    ["privacy.userContext.enabled", true]
> +  ]});

I think this would be nice to move into an initial and separate "setup" task, together with the cleanup function.

@@ +40,5 @@
> +  });
> +
> +  const reflectFile = 'browser/browser/components/contextualidentity/'
> +                      + 'test/browser/file_reflect_cookie_into_title.html';
> +  const uri = 'http://mochi.test:8888/browser/browser/components/'

Please move those consts to the top of the file (outside the task) and use upper case letters.

@@ +45,5 @@
> +    + 'contextualidentity/test/browser/'
> +    + 'browser_usercontext_testserver.sjs?file='
> +    + escape(reflectFile);
> +
> +  for (var userContextId in userContexts) {

for (let userContextId of Object.keys(USER_CONTEXTS)) {

@@ +47,5 @@
> +    + escape(reflectFile);
> +
> +  for (var userContextId in userContexts) {
> +    // load the page in 3 different contexts and set a cookie
> +    // which should only be visibile in that context

Nit: visible

@@ +54,5 @@
> +    // open our tab in the given user context
> +    let tab = openTabInUserContext(uri+'&cookie='+cookie, userContextId);
> +
> +    // wait for tab load
> +    yield BrowserTestUtils.browserLoaded(gBrowser.getBrowserForTab(tab), true);

Why is includeSubFrames=true? I think we don't need that.

@@ +69,5 @@
> +    yield BrowserTestUtils.browserLoaded(gBrowser.getBrowserForTab(tab), true);
> +    gBrowser.removeTab(tab);
> +  }
> +
> +  for (var userContextId in userContexts) {

for (let userContextId of Object.keys(USER_CONTEXTS)) {

@@ +77,5 @@
> +    let tab = openTabInUserContext(uri, userContextId);
> +
> +    // wait for load
> +    let browser = gBrowser.getBrowserForTab(tab);
> +    yield BrowserTestUtils.browserLoaded(browser, true);

yield BrowserTestUtils.browserLoaded(browser);

@@ +82,5 @@
> +
> +    // get the title
> +    let title = browser.contentDocument.title.trim().split('|');
> +    let storageMethodName = '';
> +    let value = '';

Nit: please use double quotes everywhere

@@ +85,5 @@
> +    let storageMethodName = '';
> +    let value = '';
> +
> +    // check each item in the title and validate it meets expectatations
> +    for (var item in title) {

for (let item of title) {

@@ +86,5 @@
> +    let value = '';
> +
> +    // check each item in the title and validate it meets expectatations
> +    for (var item in title) {
> +      [storageMethodName, value] = title[item].split('=');

let [storageMethodName, value] = title[item].split('=');

(So you could remove the declarations above.)

@@ +96,5 @@
> +      if (storageMethodName == 'local') {
> +        todo_is(value, expectedContext,
> +            "the title reflects the expected contextual identity of "
> +             + expectedContext + " for method " + storageMethodName + ": " + value);
> +      }

Maybe do:

let is_f = storageMethodName == "cookie" ? is : todo_is;
is_f(value, expectedContext,
            "the title reflects the expected contextual identity of " +
            expectedContext + " for method " + storageMethodName + ": " + value);

::: browser/components/contextualidentity/test/browser/browser_usercontext_testserver.sjs
@@ +4,5 @@
> +
> +function loadResponseFromFile(path) {
> +  // Load the Content to return in the response from file.
> +  // Since it's relative to the cwd of the test runner, we start there and
> +  // append to get to the actual path of the file.

Alternatively, we could add separate "browser_usercontext_*^headers^.html" files for every context that simply contain:

(browser_usercontext_default^headers^.html)
HTTP 302 Moved Temporarily
Set-Cookie: userContextId=some-value
Location: file_reflect_cookie_into_title.html

If we then do gBrowser.addTab("browser_usercontext_default.html") we set a cookie and redirect. And then to check we would simply load file_reflect_cookie_into_title.html.

But OTOH, we never bother to check the titles of tabs when setting cookies anyway. So why not simply have the .sjs as you have right now and send an empty or nonsense response body? By loading file_reflect_cookie_into_title.html we'd check that the right cookie is there.

That way we could also get rid of some complexity and simply load "browser_usercontext_testserver.sjs?cookie_value" and can directly use request.queryString as the cookie value, without splitting strings.

@@ +32,5 @@
> +  }
> +}
> +
> +function handleRequest(request, response)
> +{

function handleRequest(request, response) {

@@ +51,5 @@
> +
> +  // Deliver the cookie encoded in the URI, if found
> +  if(cookieValue){
> +    response.setHeader("Set-Cookie", "userContextId="+cookieValue, false);
> +  }

Couldn't we achieve the same by setting a cookie via |document.cookie|? Do we need that .sjs file?

::: browser/components/contextualidentity/test/browser/file_reflect_cookie_into_title.html
@@ +4,5 @@
> +    <title>title not set</title>
> +    <script>
> +      var query = window.location.search.substr(1).split('&');
> +      for (var item in query) {
> +        [ name, value ] = query[item].split('=');

for (let item of query) {
  let [name, value] = item.split("=");

@@ +13,5 @@
> +        }
> +      }
> +
> +      // get the cookie
> +      var [name, val] = document.cookie.split('=');

let

@@ +17,5 @@
> +      var [name, val] = document.cookie.split('=');
> +
> +      // set the title to reflect the cookie and local storage values we find
> +      document.title = 'cookie=' + val + '|'
> +                       + 'local=' + localStorage.getItem('userContext');

Please use double quotes in this file too.

Comment 41

[Kate McKinley [:kmckinley, :Kate]]

Attachment: contextual identity test - cookie, localStorage

Simplified substantially from :ttaubert's comments. Notably:

* Removed browser_usercontext_testserver.sjs as it was no longer needed
* file_reflect_cookie_into_title.html simplified
* cleanup and simplification of browser_usercontext.js
* moved build files into own patch for separate review

Comment 42

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Contextual identity tests

Moved build back in after conversation with Gregory Szorc. He said generally build changes like this do not require separate review. Since they follow the same pattern as private browsing, and have run successfully on Treeherder, I think the risk is low.

Comment 43

[Tanvi Vyas[:tanvi]]

Comment on attachment 8689306 [details] [diff] [review]
Contextual identity tests

+  // open the tab in the correct userContextId
+  let tab = gBrowser.addTab(uri, {userContextId});
How does this work? Don't we need to update "addTab" to handle a userContextId parameter?  Was that already done?

+add_task(function* setup() {
+  // even though we ask to be skipped under e10s for now,
+  // explicitly force it off, and make sure userContext
+  // is enabled.
+  SpecialPowers.pushPrefEnv({"set": [
+    ["browser.tabs.remote.autostart", false],
+    ["privacy.userContext.enabled", true]
+  ]});

Why do you add this e10s bit?  Then when we go to enable the test, we will also have to remember to remove this pref.

+    // Set a cookie in a different context so we can detect if that affects
+    // cross-context properly. If we don't do that, we get an UNEXPECTED-PASS
+    // for the localStorage case for the last tab we set.
How do you get an unexpected pass?

Comment 44

[Kate McKinley [:kmckinley, :Kate]]

Attachment: contextual identity test

Removed set e10s off in test.

Carried over r+ from Tanvi

Comment 45

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tanvi Vyas [:tanvi] from comment #43)
> +  // open the tab in the correct userContextId
> +  let tab = gBrowser.addTab(uri, {userContextId});
> How does this work? Don't we need to update "addTab" to handle a
> userContextId parameter?  Was that already done?

Yeah, that was done in bug 1191442.

Comment 46

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Kate McKinley [:kmckinley] from comment #42)
> Moved build back in after conversation with Gregory Szorc. He said generally
> build changes like this do not require separate review.

Ah okay. Sorry for the confusion, didn't know that.

Comment 47

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8690702 [details] [diff] [review]
contextual identity test

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

LGTM, thanks!

::: browser/components/contextualidentity/test/browser/browser_usercontext.js
@@ +78,5 @@
> +    // get the title
> +    let title = browser.contentDocument.title.trim().split("|");
> +
> +    // check each item in the title and validate it meets expectatations
> +    for (var item in title) {

for (let part of title) {
  let [storageMethodName, value] = part.split("=");
  ...

Comment 48

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8688784 [details] [diff] [review]
Contextual Identity implementation

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

r=me with the below fixed.

::: docshell/base/nsDocShell.cpp
@@ +13818,5 @@
>    return parent->GetAppId(aAppId);
>  }
>  
> +/* [infallible] */ NS_IMETHODIMP
> +nsDocShell::GetUserContextId(uint32_t* aUserContextId)

Do we need this function? I'd prefer if callers called

GetOriginAttributes and then grabbed the userContextId from that.

@@ +14060,1 @@
>      *aShouldIntercept = swm->IsAvailable(attrs, aURI);

Can you remove the temporary here and just do

...IsAvailable(GetOriginAttributes(), aURI);

?

@@ +14151,4 @@
>  
>    ErrorResult error;
>    nsCOMPtr<nsIRunnable> runnable =
>      swm->PrepareFetchEvent(attrs, doc, aChannel, isReload, isSubresourceLoad, error);

Same here.

::: dom/base/nsFrameLoader.cpp
@@ +3085,5 @@
> +  // set the userContextId on the attrs before we pass them into
> +  // the tab context
> +  if (mOwnerContent) {
> +    nsAutoString userContextIdStr;
> +    mOwnerContent->GetAttribute(NS_LITERAL_STRING("usercontextid"), userContextIdStr);

Call GetAttr here too. Rather than GetAttribute.

::: dom/base/nsGkAtomList.h
@@ +2401,5 @@
>  
>  GK_ATOM(vr_state, "vr-state")
> +
> +// Contextual Identity / Containers
> +GK_ATOM(userContextId, "usercontextid")

Use all lowercase on this to make it clear what string it represents.

Comment 49

[Kate McKinley [:kmckinley, :Kate]]

Attachment: contextual identity tests

Carry over r+ from tanvi,ttaubert

Comment 50

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Contextual Identity implementation

Implement requested changes
Carry over r+ from tanvi,sicking

Comment 51

[Kate McKinley [:kmckinley, :Kate]]

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

Comment 52

[Tanvi Vyas[:tanvi]]

(In reply to Kate McKinley [:kmckinley] from comment #51)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=e44f925c1677

Try looks good!  Can we land this?

Comment 53

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dfbfbeb2abde
https://hg.mozilla.org/integration/mozilla-inbound/rev/82c89f573e53

Comment 54

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b986da70ee7
https://hg.mozilla.org/integration/mozilla-inbound/rev/3320ed6efa63

Comment 55

[Carsten Book [:Tomcat]]

(In reply to Pulsebot from comment #54)
> Backout:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/2b986da70ee7
> https://hg.mozilla.org/integration/mozilla-inbound/rev/3320ed6efa63

sorry had to back this out, since this caused merge conflicts like:warning: conflicts while merging docshell/base/nsDocShell.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging dom/base/nsFrameLoader.cpp! (edit, then use 'hg resolve --mark')

Comment 56

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Contextual Identity implementation

Rebased patch to current, carried over r+

Comment 57

[Kate McKinley [:kmckinley, :Kate]]

(In reply to Carsten Book [:Tomcat] from comment #55)
> (In reply to Pulsebot from comment #54)
> > Backout:
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/2b986da70ee7
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/3320ed6efa63
> 
> sorry had to back this out, since this caused merge conflicts like:warning:
> conflicts while merging docshell/base/nsDocShell.cpp! (edit, then use 'hg
> resolve --mark')
> warning: conflicts while merging dom/base/nsFrameLoader.cpp! (edit, then use
> 'hg resolve --mark')

Rebased the patch to current tip

Comment 58

[Carsten Book [:Tomcat]]

Hi, this failed to apply:

patching file browser/components/moz.build
Hunk #1 FAILED at 0
1 out of 1 hunks FAILED -- saving rejects to file browser/components/moz.build.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh file_1191460.txt

could you take a look, thanks!

Comment 59

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Contextual identity tests

Rebased patch

Comment 60

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5d2c5607178
https://hg.mozilla.org/integration/mozilla-inbound/rev/81c1c91f18c5

Comment 61

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f5d2c5607178
https://hg.mozilla.org/mozilla-central/rev/81c1c91f18c5