1302697 - Containers and WebExtensions

Status: RESOLVED FIXED

(WebExtensions :: Experiments, defect, P3)

Description

[Andrea Marchesini [:baku]]

I would like to expose Containers to addon developers. My idea is to change Tab and Window in this way:

a. Tab.create() should receive an additional parameter: userContextId. By default it's 0 (default container).
b. Same thing for Window.create().
c. I also want to have Tab.userContextId as attribute in the type Tab.

This should be enough for having Containers for addons. Feedback?

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

I would be personally very excited to see this happening!

Added to the [design-decision-needed] whiteboard, so that the proposal can be discussed and evaluated.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 1 - Tab API

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 1 - Tab API

Forgot to add the test.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 2 - cookies API

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 2 - cookies API

Better userContextId check.

Comment 6

[Andrea Marchesini [:baku]]

If these 2 patches are OK, I'll propose a separate one for exposing ContextualIdentityService.

Comment 7

[Kris Maglione [:kmag]]

Comment on attachment 8794895 [details] [diff] [review]
part 1 - Tab API

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

Mostly looks good, but I think we need to figure out the overlap between cookie store IDs and user context IDs.

::: browser/components/extensions/ext-tabs.js
@@ +523,5 @@
>            }
>  
> +          let options = {};
> +          if (createProperties.userContextId &&
> +              Services.prefs.getBoolPref("privacy.userContext.enabled")) {

We should cache this (`XPCOMUtils.defineLazyPreferenceGetter`).

Also, shouldn't we reject if a `userContextId` is provided and user contexts aren't enabled?

::: browser/components/extensions/test/browser/browser_ext_tabs_userContextId.js
@@ +4,5 @@
> +
> +function* testTabsCreateUserContextId(data) {
> +  let extension = ExtensionTestUtils.loadExtension({
> +    manifest: {
> +      "permissions": ["tabs"],

This permission shouldn't be necessary.

@@ +10,5 @@
> +
> +    background: function() {
> +      browser.test.sendMessage("ready");
> +      browser.test.onMessage.addListener((msg, data) => {
> +        browser.tabs.create({userContextId: data.userContextId}, (tab) => {

Please use the promise-based versions of APIs rather than passing callbacks.

@@ +16,5 @@
> +            browser.test.assertTrue(!!tab, "we have a tab");
> +            browser.test.assertEq(data.expectedUserContextId, tab.userContextId, "tab should have the correct userContextId");
> +
> +            // Remove the opened tab is any.
> +            browser.tabs.remove(tab.id);

We should wait for this to resolve before going to the next test.

@@ +42,5 @@
> +}
> +
> +add_task(function* setup() {
> +  // make sure userContext is enabled.
> +  yield new Promise(resolve => {

`pushPrefEnv` already returns a promise.

@@ +52,5 @@
> +
> +add_task(function* () {
> +  info("Start testing tabs.create with userContextId");
> +
> +  let dataURLPage = `data:text/html,

This isn't used.

@@ +70,5 @@
> +    {userContextId: 2, success: true, expectedUserContextId: 2},
> +    {userContextId: 42, success: false},
> +  ];
> +
> +  while (testCases.length) {

`for (let testCase of testCases) ...`

Comment 8

[Kris Maglione [:kmag]]

Comment on attachment 8794939 [details] [diff] [review]
part 2 - cookies API

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

I think that each user context needs its own cookie store ID, and as far as the extension is concerned, they should probably the same thing. At the very least, we shouldn't have multiple user contexts with the same cookie store ID. Which means that we need to figure out how they overlap with private browsing sessions, and how to allocate IDs between them.

We also need to enumerate the existing user contexts and the tabs that belong to them from `getAllCookieStores`.

Aside from that, the code looks good.

::: toolkit/components/extensions/ext-cookies.js
@@ +301,5 @@
>          }
>  
> +        let userContextId = 0;
> +        if (details.userContextId &&
> +            Services.prefs.getBoolPref("privacy.userContext.enabled")) {

This should be cached, the same as for the tabs module.

::: toolkit/components/extensions/test/mochitest/test_ext_cookies_userContextId.html
@@ +16,5 @@
> +
> +add_task(function* setup() {
> +  // make sure userContext is enabled.
> +  yield new Promise(resolve => {
> +    SpecialPowers.pushPrefEnv({"set": [

`pushPrefEnv` already returns a promise.

@@ +17,5 @@
> +add_task(function* setup() {
> +  // make sure userContext is enabled.
> +  yield new Promise(resolve => {
> +    SpecialPowers.pushPrefEnv({"set": [
> +      ["privacy.userContext.enabled", true]

Trailing comma, please.

@@ +90,5 @@
> +  yield extension.startup();
> +  info("extension loaded");
> +  yield extension.awaitFinish("cookies");
> +  yield extension.unload();
> +  info("extension unloaded");

We've been trying to get rid of these kinds of `info` messages.

Comment 9

[Andrea Marchesini [:baku]]

> Mostly looks good, but I think we need to figure out the overlap between
> cookie store IDs and user context IDs.

This is super interesting. It seems to me that we have just 1 ocookieStore at the moment (default), right?
Is this cookieStore exposed to tab anyhow?

Comment 10

[Andrea Marchesini [:baku]]

What about if:

1. instead calling userContextId, I call it 'storeID' (as in Cookie API). And the first patch is kept as it is (+ your comments, + the renaming).

2. we use storeID in cookie API as userContextId... that would be awesome because we use a concept that is already part of the API.

Comment 11

[Andrea Marchesini [:baku]]

Attachment: part 1 - Tab API

Comment 12

[Andrea Marchesini [:baku]]

Attachment: part 2 - cookies API

Comment 13

[Kris Maglione [:kmag]]

Comment on attachment 8797208 [details] [diff] [review]
part 1 - Tab API

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

This is definitely on the right track. Most of my comments are nits. My only real concern is how we deal with attempting to set a user context ID in a private window.

::: browser/components/extensions/ext-tabs.js
@@ +542,5 @@
>  
> +          let options = {};
> +          if (extension.hasPermission("cookies")) {
> +            if (isPrivateCookieStoreId(createProperties.cookieStoreId)) {
> +              return Promise.reject({message: `Use incognito instead of cookieStoreId for private store`});

We don't allow setting `incognito` when creating tabs, since they always inherit the private browsing context of their parent window.

So we should probably also disallow this property if the tab is being created in a private window, unless it matches the cookie store ID of that window?

@@ +550,5 @@
> +              if (!isContainerCookieStoreId(createProperties.cookieStoreId)) {
> +                return Promise.reject({message: `Illegal cookieStoreId: ${createProperties.cookieStoreId}`});
> +              }
> +
> +              if (Services.prefs.getBoolPref("privacy.userContext.enabled")) {

Again, we should really cache this pref rather than checking it every time.

@@ +552,5 @@
> +              }
> +
> +              if (Services.prefs.getBoolPref("privacy.userContext.enabled")) {
> +                let containerId = getContainerForCookieStoreId(createProperties.cookieStoreId);
> +                if (containerId === false) {

It seems like `null` would be more appropriate than `false` here.

@@ +553,5 @@
> +
> +              if (Services.prefs.getBoolPref("privacy.userContext.enabled")) {
> +                let containerId = getContainerForCookieStoreId(createProperties.cookieStoreId);
> +                if (containerId === false) {
> +                  return Promise.reject({message: `Illegal cookieStoreId: ${createProperties.cookieStoreId}`});

Maybe something like "No cookie store exists with ID..." rather than "Illegal"?

::: browser/components/extensions/test/browser/browser_ext_tabs_cookieStoreId.js
@@ +10,5 @@
> +
> +    background: function() {
> +      browser.test.sendMessage("ready");
> +      browser.test.onMessage.addListener((msg, data) => {
> +        browser.tabs.create({cookieStoreId: data.cookieStoreId}, (tab) => {

Please use promises rather than callbacks in tests.

@@ +23,5 @@
> +                                    "runtime.lastError should report the expected error message");
> +          } else if (data.failure == "private") {
> +            browser.test.assertTrue(/Use incognito/.test(browser.runtime.lastError.message),
> +                                    "runtime.lastError should report the expected error message");
> +          } else {

We should also test what happens when we try to pass a container ID for a tab in a private browsing window. I'm not entirely sure what should be expected in that case...

@@ +24,5 @@
> +          } else if (data.failure == "private") {
> +            browser.test.assertTrue(/Use incognito/.test(browser.runtime.lastError.message),
> +                                    "runtime.lastError should report the expected error message");
> +          } else {
> +            browser.test.assertTrue(false, "The test is broken");

`browser.test.fail`

@@ +35,5 @@
> +  });
> +
> +  yield extension.startup();
> +
> +  yield extension.awaitMessage("ready");

This isn't necessary in new tests. `startup()` only resolves after the background script has run, now.

::: toolkit/components/extensions/ext-cookies.js
@@ +36,5 @@
> +  return storeId == DEFAULT_STORE || storeId == null;
> +}
> +
> +global.isContainerCookieStoreId = function(storeId) {
> +  return storeId !== null && storeId.indexOf(CONTAINER_STORE) == 0;

`storeId.startsWith(CONTAINER_STORE)`

Is the null check really necessary?

@@ +41,5 @@
> +}
> +
> +global.getContainerForCookieStoreId = function(storeId) {
> +  if (!isContainerCookieStoreId(storeId)) {
> +    return false;

Per above, this should probably be `null` rather than `false`. Also, a JSDoc comment would be helpful.

@@ +69,5 @@
>    if (!cookie.isSession) {
>      result.expirationDate = cookie.expiry;
>    }
>  
> +  if (cookie.originAttributes.userContextId) {

We should probably break out the ternary in the object literal above and just fold it into this if-else, so it's a bit easier to follow.

Comment 14

[Kris Maglione [:kmag]]

Comment on attachment 8797209 [details] [diff] [review]
part 2 - cookies API

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

This looks pretty good, aside from a few quirks with how we handle private mode.

I do think that we probably need to handle the additional cookie stores in `getAllCookieStores` before it can land, though. I was considering asking for it as a follow-up, but the results are basically a lie now, and don't match up with tabs' `cookieStoreId` properties.

::: toolkit/components/extensions/ext-cookies.js
@@ +189,2 @@
>    let isPrivate = context.incognito;
> +  if (isDefaultCookieStoreId(details.storeId)) {

I think we need a special case to use the value of `context.incognito` when `details.storeId` is null, since `isDefaultCookieStoreId` will be true there. We should also add a test for it, I guess.

@@ +194,5 @@
> +  } else if (isContainerCookieStoreId(details.storeId)) {
> +    userContextId = getContainerForCookieStoreId(details.storeId);
> +    isPrivate = false;
> +    if (userContextId === false) {
> +      return;

Should we throw in this case?

@@ +204,5 @@
> +    storeId = details.storeId;
> +  }
> +
> +  if (isPrivate) {
> +    storeId = PRIVATE_STORE;

I wonder if we should just get rid of `isPrivate` now. We should also be able to get rid of the `usePrivateMode` call and just set `privateProwsingId` in the origin attributes pattern based on the store ID.

Either way, I don't like the idea of getting in a situation where `isPrivate` doesn't match the `storeId` and we don't throw.

@@ +227,4 @@
>      });
>    } else {
>      Services.cookies.usePrivateMode(isPrivate, () => {
>        enumerator = Services.cookies.enumerator;

Would be nice to also use an origin-attributes-aware enumerator here, so it's not such a special case... Maybe we should add a version of `getCookiesWithOriginAttributes` that takes a dictionary rather than a string?

@@ +359,1 @@
>            isPrivate = false;

Same issues as above. We sill need to default to using `context.incognito` when no `storeId` is passed, and I'm not sure we really need `isPrivate` anymore.

@@ +394,5 @@
>            // Todo: could there be multiple per subdomain?
>            return Promise.resolve({
>              url: details.url,
>              name: details.name,
> +            storeId: storeId,

storeId,

::: toolkit/components/extensions/test/mochitest/chrome.ini
@@ +20,5 @@
>  skip-if = os != "mac" && os != "linux"
>  [test_ext_cookies_expiry.html]
>  [test_ext_cookies_permissions.html]
> +[test_ext_cookies_containers.html]
> +skip-if = buildapp == 'b2g'

No need for the `skip-if` anymore.

::: toolkit/components/extensions/test/mochitest/test_ext_cookies_containers.html
@@ +70,5 @@
> +      return browser.cookies.remove({url: TEST_URL, name: "name1", storeId: "firefox-container-1"});
> +    }).then(details => {
> +      assertExpected({url: TEST_URL, name: "name1", storeId: "firefox-container-1"}, details);
> +      return browser.cookies.get({url: TEST_URL, name: "name1", storeId: "firefox-container-1"});
> +    }).then(cookie => {

We should also test that the correct onChanged listeners fire for these changes.

Comment 15

[Kris Maglione [:kmag]]

Comment on attachment 8797208 [details] [diff] [review]
part 1 - Tab API

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

::: browser/components/extensions/ext-utils.js
@@ +706,5 @@
>        audible: tab.soundPlaying,
>        mutedInfo,
>      };
>      if (this.extension.hasPermission("cookies")) {
> +      result.cookieStoreId = getCookieStoreIdForTab(result, tab);

Oh, right, and we should also handle this in `tabs.query`

Comment 16

[Andrea Marchesini [:baku]]

Attachment: part 1 - Tab API

Comment 17

[Andrea Marchesini [:baku]]

Attachment: part 2 - cookies API

Comment 18

[Andrea Marchesini [:baku]]

Attachment: part 2 - cookies API

Comment 19

[Andrea Marchesini [:baku]]

Attachment: part 3 - getAllCookieStores

Comment 20

[Andrea Marchesini [:baku]]

Any news for the review of these patches?

Comment 21

[Kris Maglione [:kmag]]

(In reply to Andrea Marchesini [:baku] from comment #20)
> Any news for the review of these patches?

Sorry, I've been majorly side-tracked most of these week. I'll finish reviewing them today.

Comment 22

[Andrea Marchesini [:baku]]

Just because it block the removing of AppID, I NI you again.

Comment 23

[Kris Maglione [:kmag]]

Comment on attachment 8797385 [details] [diff] [review]
part 1 - Tab API

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

Sorry this took so long to review. The past month has been a constant stream of critical uplifts and distractions.

r=me with the private cookie store IDs changed to include the private browsing ID, the same way the user context IDs do.

Thanks!

::: browser/components/extensions/ext-tabs.js
@@ +542,5 @@
>              }
>            }
>  
> +          let options = {};
> +          if (createProperties.cookieStoreId && extension.hasPermission("cookies")) {

If we're not going to respect this parameter when the extension doesn't have `cookies` permissions, we should probably make it an error if it's passed.

@@ +549,5 @@
> +            }
> +
> +            let privateWindow = PrivateBrowsingUtils.isBrowserPrivate(window.gBrowser);
> +            if (privateWindow && !isPrivateCookieStoreId(createProperties.cookieStoreId)) {
> +              return Promise.reject({message: `Illegal to set non private cookieStorageId in a private window`});

Nit: "non-private"

@@ +553,5 @@
> +              return Promise.reject({message: `Illegal to set non private cookieStorageId in a private window`});
> +            }
> +
> +            if (!privateWindow && isPrivateCookieStoreId(createProperties.cookieStoreId)) {
> +              return Promise.reject({message: `Illegal to set private cookieStorageId in a non private window`});

Nit: "non-private"

::: browser/components/extensions/test/browser/browser_ext_tabs_cookieStoreId.js
@@ +7,5 @@
> +  yield new Promise(resolve => {
> +    SpecialPowers.pushPrefEnv({"set": [
> +      ["privacy.userContext.enabled", true],
> +    ]}, resolve);
> +  });

`pushPrefEnv` returns a promise, so just `return SpecialPowers.pushPrefEnv(...)` rather than `new Promise(...)`

@@ +45,5 @@
> +      }
> +
> +      function runTest(data) {
> +        // Tab Creation
> +        browser.tabs.create({ windowId: data.privateTab ? this.privateWindowId : this.defaultWindowId,

Nit: No space after `{`. ESLint will reject it.

@@ +50,5 @@
> +                              cookieStoreId: data.cookieStoreId})
> +
> +        // Tests for tab creation
> +        .then(
> +        (tab) => {

Nit: Weird indentation.

@@ +80,5 @@
> +
> +        // Tests for tab querying
> +        .then((tab) => {
> +          if (tab) {
> +            return browser.tabs.query({ windowId: data.privateTab ?  this.privateWindowId : this.defaultWindowId,

Nit: No space inside braces.

@@ +92,5 @@
> +         })
> +
> +        .then(() => {
> +          browser.test.sendMessage("test-done");
> +        });

Nit: Please add a `.catch(e => { ... })` and fail the test with details about the error before this last .then() clause. These tests get a bit hard to debug when they error out and we have to wait for timeouts and uncaught rejection reporting to find out what went wrong.

@@ +140,5 @@
> +  extension.sendMessage("be-ready");
> +  yield extension.awaitMessage("ready");
> +  info("Tests are ready to run!");
> +
> +  while (testCases.length) {

`for (let test of testCases) ...` please.

@@ +153,5 @@
> +  yield extension.awaitMessage("gone");
> +
> +  yield extension.unload();
> +
> +  info("done");

Nit: The test harness will report when the task ends, so not really necessary.

::: toolkit/components/extensions/ext-cookies.js
@@ +12,5 @@
>    EventManager,
>  } = ExtensionUtils;
>  
>  var DEFAULT_STORE = "firefox-default";
>  var PRIVATE_STORE = "firefox-private";

This should be "firefox-private-"

@@ +18,4 @@
>  
> +global.getCookieStoreIdForTab = function(data, tab) {
> +  if (data.incognito) {
> +    return PRIVATE_STORE;

We should probably include the private browsing ID in this.

@@ +49,5 @@
> +  if (!ContextualIdentityService.getIdentityFromId(containerId)) {
> +    return null;
> +  }
> +
> +  return parseInt(containerId);

Should be `parseInt(containerId, 10)`, just to be safe.

And, I think this might be slightly easier to follow as:

  if (ContextualIdentityService.getIdentityFromId(containerId)) {
    return parseInt(containerId);
  }

  return null;

Also, I'm slightly surprised that this works passing a string to
getIdentityFromId. I guess that's because of JS non-strict equality. But we
should probably `parseInt` before we pass it, just to be safe. And also
probably change the value test to something like:

  /^firefox-container-(0|[1-9]\d+)$/

@@ +77,5 @@
>  
> +  if (cookie.originAttributes.userContextId) {
> +    result.storeId = CONTAINER_STORE + cookie.originAttributes.userContextId;
> +  } else if (cookie.originAttributes.privateBrowsingId || isPrivate) {
> +    result.storeId = PRIVATE_STORE;

This should probably be `PRIVATE_STORE + privateBrowsingId`

Comment 24

[Kris Maglione [:kmag]]

Comment on attachment 8797390 [details] [diff] [review]
part 2 - cookies API

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

::: toolkit/components/extensions/ext-cookies.js
@@ +220,1 @@
>    }

How about:

  if (isPrivate) {
    storeId = PRIVATE_STORE;
  } else if ("storeId" in details) {
    storeId = details.storeId;
  }

@@ +372,4 @@
>            isPrivate = true;
> +        } else if (isContainerCookieStoreId(details.storeId)) {
> +          let containerId = getContainerForCookieStoreId(details.storeId);
> +          if (!containerId) {

Can we check `containerId == null` instead?

@@ +405,5 @@
>            // Todo: could there be multiple per subdomain?
>            return Promise.resolve({
>              url: details.url,
>              name: details.name,
> +            storeId: storeId,

Nit: `storeId,`

::: toolkit/components/extensions/test/mochitest/chrome.ini
@@ +20,5 @@
>  skip-if = os != "mac" && os != "linux"
>  [test_ext_cookies_expiry.html]
>  [test_ext_cookies_permissions.html]
> +[test_ext_cookies_containers.html]
> +skip-if = buildapp == 'b2g'

Nit: Skipping tests on b2g isn't necessary anymore.

::: toolkit/components/extensions/test/mochitest/test_ext_cookies_containers.html
@@ +16,5 @@
> +
> +add_task(function* setup() {
> +  // make sure userContext is enabled.
> +  yield new Promise(resolve => {
> +    SpecialPowers.pushPrefEnv({"set": [

No `new Promise`, just `yield SpecialPowers.pushPrefEnv(...)`

@@ +33,5 @@
> +      browser.test.assertEq(Object.keys(expected).length, Object.keys(cookie).length, "all expected properties found");
> +    }
> +
> +    const TEST_URL = "http://example.org/";
> +    const THE_FUTURE = Date.now() + 5 * 60;

`Date.now() / 1000 + ...`

@@ +95,5 @@
> +  yield extension.startup();
> +  info("extension loaded");
> +  yield extension.awaitFinish("cookies");
> +  yield extension.unload();
> +  info("extension unloaded");

Please remove `info("extension loaded/unloaded")`

Comment 25

[Kris Maglione [:kmag]]

Comment on attachment 8797398 [details] [diff] [review]
part 3 - getAllCookieStores

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

::: browser/components/extensions/test/browser/browser_ext_tabs_cookieStoreId.js
@@ +94,5 @@
> +        .then((tab) => {
> +           if (tab) {
> +             return browser.cookies.getAllCookieStores()
> +                    .then(stores => {
> +                      let store = stores.find(store => store.id == tab.cookieStoreId);

Nit: s/==/===/

::: toolkit/components/extensions/ext-cookies.js
@@ +428,3 @@
>          let result = [];
> +        for (let key in data) {
> +          result.push({id: key, tabIds: data[key]});

I know this isn't technically necessary, but can we also add an `incognito` property that's true for private browsing stores?

Comment 26

[Andrea Marchesini [:baku]]

All the comments applied except for the firefox-private-<id> because that requires several changes.
I prefer to do it in a follow up.

Comment 27

[Kris Maglione [:kmag]]

OK. That's fine with me

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca0b48d85434
Containers and WebExtensions - part 1 - Tab API, r=kmag
https://hg.mozilla.org/integration/mozilla-inbound/rev/691162eba717
Containers and WebExtensions - part 2 - Cookie API, r=kmag
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c05c0ff09ba
Containers and WebExtensions - part 3 - getAllCookieStores, r=kmag

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ca0b48d85434
https://hg.mozilla.org/mozilla-central/rev/691162eba717
https://hg.mozilla.org/mozilla-central/rev/4c05c0ff09ba

Comment 30

[Andy McKay]

Just to check :baku, this API will currently ride all the way to release. Are the other parts of the UX that interact with containers right now in Firefox (e.g. New Container Tab in Nightly) riding all the way to release too?

Comment 31

[Kris Maglione [:kmag]]

Also, if possible, I'd like for the private browsing store IDs to remain as consistent as possible. Will you be able to post a follow-up patch to add the private browsing ID before the merge deadline? If not, it might need to be uplifted after the merge...

Thanks!

Comment 32

[Andrea Marchesini [:baku]]

Currently, containers are behind pref. I implemented the same pattern for WebExtensions. It's not possible to set a container ID (or cookieStoreId) if the pref is set to false.

About private-browserID, We can set it to '1', directly without changing too much code. At the moment we are not supporting multiple private browsing sessions.

Comment 33

[Andy McKay]

General ping to dveditz to let him know we landed a new API in WebExtensions. Didn't ask for a formal security though. As Andrea said, its currently behind the same pref as containers.

Comment 34

[The 8472]

I see see tabs.create being covered in the tests. Does the same logic also apply to tabs.update()? The use-case would be changing containers when the top level domain of a tab gets navigated.

FF's default behavior of assigning a container once per tab seems too inflexible and I would like to automate that with an addon, but for that I need to be able to change the container on navigation.

Comment 35

[Kris Maglione [:kmag]]

(In reply to The 8472 from comment #34)
> I see see tabs.create being covered in the tests. Does the same logic also
> apply to tabs.update()? The use-case would be changing containers when the
> top level domain of a tab gets navigated.

No. Changing the userContextId of a tab or browser after it's been created is not supported.

Comment 36

[The 8472]

Well, could it be added then?

I already outlined my use-case in bug 1191418 comment 10 months ago and nobody said it won't be possible... so I'm surprised that I'm getting negative response this late in the process.

Comment 37

[Kris Maglione [:kmag]]

It's not that it isn't supported by the API, it's that changing the userContextId of a browser after it's created is illegal and enforced at several levels. It was an intentional design decision.

Comment 38

[The 8472]

That design decision turned out to be wrong. privacy.firstparty.isolate clearly demonstrates that there is a need to change the effective cookie context along with the current tab's state.

But neither containers nor first-party isolation allow the fine-grained control necessary to do something similar to µMatrix (but for containerization).

I hoped webextensions would finally enable that customization, but that does not seem to be the case.

I'll file a new bug in the hope that the design can be revisited.

Comment 39

[The 8472]

Filed bug 1323873.

Now, a related issue: since it also isn't possible to change the context for tabs spawned through user actions, e.g. middle click from within an already containerized tab, how many unintended side-effects could the following workaround for that particular case have?

1. detect that a new tab is spawned from a container
2. check the destination top level URL before it loads the content
3. close new tab (if we want to change the container)
4. create another tab in its place with the desired container

Comment 40

[Kris Maglione [:kmag]]

(In reply to The 8472 from comment #38)
> That design decision turned out to be wrong.

Just because you don't like something, that doesn't make it wrong.

There are a lot of reasons this restriction is in place. Leaving aside the
subjective and user experience reasons, it's necessary for technical and
security reasons that the origin attributes of a docshell never change. The
only way to safely implement this would be to destroy the docshell and replace
it with a new one with different origin attributes.

The amount of complexity that that would add means that there would have to be
a *very* good reason to do it, and I'm not convinced that there is.

Comment 41

[Will Bamberg [:wbamberg]]

Docs updated: "cookieStoreId" is added to "tabs.Tab", and the compat data is updated:

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/Tab

Please let me know if we need anything else here.

Comment 42

[Andrea Marchesini [:baku]]

cookieStoreId has been add to: create and query methods of Tabs. Same for Cookies.query

Comment 43

[Will Bamberg [:wbamberg]]

(In reply to Andrea Marchesini [:baku] from comment #42)
> cookieStoreId has been add to: create and query methods of Tabs.

Thanks! I added it to those pages, too.

> Same for Cookies.query

There isn't a Cookies.query. Do you mean cookies.get (etc)? I thought that was just storeId.

Comment 44

[Andrea Marchesini [:baku]]

Right. cookies.get() is what I meant.