1322856 - Expose ContextualIdentities (aka containers) to WebExtensions

Status: RESOLVED FIXED

(WebExtensions :: General, defect)

Description

[Andrea Marchesini [:baku]]

I didn't find a way to put this interface behind pref but probably, because a special permission is required, this should be enough.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: container.patch

Comment 2

[Kris Maglione [:kmag]]

Comment on attachment 8817827 [details] [diff] [review]
container.patch

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

Looks good, aside from some minor issues. Thanks!

::: toolkit/components/extensions/ext-contextualIdentities.js
@@ +31,5 @@
> +
> +      getAll(details) {
> +        let identities = [];
> +        ContextualIdentityService.getIdentities().forEach(identity => {
> +          if ("name" in details && details.name &&

No need for the `"name" in details` check. We only emit warnings for undefined properties if they're accessed in a non-boolean context. And optional properties are always normalized to exist, in any case.

@@ +60,5 @@
> +        if (!identity) {
> +          return Promise.resolve(null);
> +        }
> +
> +        if ("name" in details) {

The binding code automatically defines omitted optional properties as `null`, so please check `details.name !== null` and so forth.

::: toolkit/components/extensions/ext-cookies.js
@@ +79,5 @@
>      result.expirationDate = cookie.expiry;
>    }
>  
>    if (cookie.originAttributes.userContextId) {
> +    result.storeId = global.getCookieStoreIdForContainer(cookie.originAttributes.userContextId);

Nit: No need for the `global.` Just add the function to the list of globals in .eslintrc

::: toolkit/components/extensions/schemas/contextual_identities.json
@@ +10,5 @@
> +        "$extend": "Permission",
> +        "choices": [{
> +          "type": "string",
> +          "enum": [
> +            "contextualIdentities"

We should probably filter out this permission, and warn, if contextual identities aren't enabled, in `ExtensionData.readManifest` in Extensions.jsm.

@@ +46,5 @@
> +            "name": "cookieStoreId",
> +            "description": "The ID of the contextual identity cookie store. "
> +          },
> +          {
> +            "type": "function",

Callback APIs are only used for compatibility with Chrome. Since this doesn't exist in Chrome, please just use `"async": true`, and omit the callback argument. Same for the other methods.

@@ +65,5 @@
> +        "parameters": [
> +          {
> +            "type": "object",
> +            "name": "details",
> +            "description": "Information to filter the contextual identities being retrieved.",

For consistency with other APIs, if this function is going to accept a filter, it should probably be called "query", rather than "getAll".

::: toolkit/components/extensions/test/mochitest/chrome.ini
@@ +27,5 @@
>  [test_ext_cookies_expiry.html]
>  [test_ext_cookies_permissions_bad.html]
>  [test_ext_cookies_permissions_good.html]
>  [test_ext_cookies_containers.html]
> +[test_ext_contextual_identities.html]

Is there any reason not to make this a plain mochitest? We currently don't run chrome mochitests out-of-process, so we lose some test coverage.

::: toolkit/components/extensions/test/mochitest/test_ext_contextual_identities.html
@@ +63,5 @@
> +    ci = await browser.contextualIdentities.update(ci.cookieStoreId, {name: "barfoo", color: "blue", icon: "icon icon"});
> +    browser.test.assertTrue(!!ci, "We have an identity");
> +    browser.test.assertEq("barfoo", ci.name, "identity.name is correct");
> +    browser.test.assertEq("blue", ci.color, "identity.color is correct");
> +    browser.test.assertEq("icon icon", ci.icon, "identity.icon is correct");

We should test that update() with only a subset of properties doesn't change the ones that were omitted.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: container.patch

Comment 5

[Andrea Marchesini [:baku]]

Attachment: container2.patch

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a145b21bd64
Expose ContextualIdentities (aka containers) to WebExtensions - part 1, r=kmaglione
https://hg.mozilla.org/integration/mozilla-inbound/rev/5db9fcefa874
Expose ContextualIdentities (aka containers) to WebExtensions - part 2, r=kmaglione

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/48994775ce8f
Eslint failures in Extension.jsm fixed - part 3, r=me

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing own test test_ext_contextual_identities.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a199014fd2b5fe100e10b9b4557558f6341c16cb
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcb0721c0c50771d89daeefdddf23329d6e35e50
https://hg.mozilla.org/integration/mozilla-inbound/rev/90b91d69b5760c4bb454ae6d6ad4e04ae3782f20

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=5db9fcefa874c25c01983bc09b733d43f8db278f
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=40711440&repo=mozilla-inbound

[task 2016-12-16T22:44:47.401619Z] 22:44:47     INFO -  TEST-PASS | toolkit/components/extensions/test/xpcshell/test_ext_contextual_identities.js | test_contextualIdentities_without_permissions - [test_contextualIdentities_without_permissions : 166] test result correct - "contextualIdentities_permission" == "contextualIdentities_permission"
[task 2016-12-16T22:44:47.403295Z] 22:44:47     INFO -  (xpcshell/head.js) | test run_next_test 1 pending (2)
[task 2016-12-16T22:44:47.404663Z] 22:44:47     INFO -  (xpcshell/head.js) | test test_contextualIdentities_without_permissions finished (2)
[task 2016-12-16T22:44:47.406297Z] 22:44:47     INFO -  toolkit/components/extensions/test/xpcshell/test_ext_contextual_identities.js | Starting test_contextualIdentity_no_containers
[task 2016-12-16T22:44:47.407791Z] 22:44:47     INFO -  (xpcshell/head.js) | test test_contextualIdentity_no_containers pending (2)
[task 2016-12-16T22:44:47.409174Z] 22:44:47     INFO -  "Extension loaded"
[task 2016-12-16T22:44:47.410722Z] 22:44:47     INFO -  (xpcshell/head.js) | test run_next_test 1 finished (2)
[task 2016-12-16T22:44:47.412351Z] 22:44:47     INFO -  PROCESS | 9351 | ++DOCSHELL 0x7f76d76c2800 == 3 [pid = 9351] [id = {9ed1c7ec-065e-4a63-a9d1-346d7edad87f}]
[task 2016-12-16T22:44:47.414125Z] 22:44:47     INFO -  PROCESS | 9351 | ++DOMWINDOW == 8 (0x7f76d76cb000) [pid = 9351] [serial = 8] [outer = (nil)]
[task 2016-12-16T22:44:47.421388Z] 22:44:47     INFO -  PROCESS | 9351 | ++DOMWINDOW == 9 (0x7f76db6ef800) [pid = 9351] [serial = 9] [outer = 0x7f76d76cb000]
[task 2016-12-16T22:44:47.423124Z] 22:44:47     INFO -  PROCESS | 9351 | ++DOMWINDOW == 10 (0x7f76dd98f000) [pid = 9351] [serial = 10] [outer = 0x7f76d76cb000]
[task 2016-12-16T22:44:47.424808Z] 22:44:47     INFO -  PROCESS | 9351 | ++DOMWINDOW == 11 (0x7f76d3c05800) [pid = 9351] [serial = 11] [outer = 0x7f76d76cb000]
[task 2016-12-16T22:44:47.429807Z] 22:44:47     INFO -  PROCESS | 9351 | ++DOCSHELL 0x7f76d3c0b000 == 4 [pid = 9351] [id = {482a3d50-d183-4f2f-b510-3a4f76edcdab}]
[task 2016-12-16T22:44:47.431516Z] 22:44:47     INFO -  PROCESS | 9351 | ++DOMWINDOW == 12 (0x7f76d3c0c000) [pid = 9351] [serial = 12] [outer = (nil)]
[task 2016-12-16T22:44:47.433123Z] 22:44:47     INFO -  PROCESS | 9351 | ++DOMWINDOW == 13 (0x7f76d3c12000) [pid = 9351] [serial = 13] [outer = 0x7f76d3c0c000]
[task 2016-12-16T22:44:47.434900Z] 22:44:47     INFO -  PROCESS | 9351 | JavaScript strict warning: chrome://global/content/bindings/browser.xml, line 409: ReferenceError: reference to undefined property "localName"
[task 2016-12-16T22:44:47.436513Z] 22:44:47     INFO -  PROCESS | 9351 | ++DOMWINDOW == 14 (0x7f76d3c16800) [pid = 9351] [serial = 14] [outer = 0x7f76d3c0c000]
[task 2016-12-16T22:44:47.438175Z] 22:44:47     INFO -  "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property "localName"" {file: "chrome://global/content/bindings/browser.xml" line: 409}]"
[task 2016-12-16T22:44:47.439885Z] 22:44:47     INFO -  PROCESS | 9351 | [9351] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057: file /home/worker/workspace/build/src/dom/base/nsContentUtils.cpp, line 6175
[task 2016-12-16T22:44:47.441740Z] 22:44:47  WARNING -  TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/xpcshell/test_ext_contextual_identities.js | test_contextualIdentity_no_containers - [test_contextualIdentity_no_containers : 92] contextualIdentities API is not available when the containers are disabled - false == true
[task 2016-12-16T22:44:47.443407Z] 22:44:47     INFO -  resource://testing-common/ExtensionXPCShellUtils.jsm:attachListeners/<:92
[task 2016-12-16T22:44:47.444847Z] 22:44:47     INFO -  resource://gre/modules/ExtensionUtils.jsm:runSafeSyncWithoutClone:71
[task 2016-12-16T22:44:47.446381Z] 22:44:47     INFO -  resource://gre/modules/ExtensionUtils.jsm:emit/promises<:384
[task 2016-12-16T22:44:47.448654Z] 22:44:47     INFO -  resource://gre/modules/ExtensionUtils.jsm:emit:383
[task 2016-12-16T22:44:47.450571Z] 22:44:47     INFO -  resource://gre/modules/Extension.jsm:receiveMessage:695
[task 2016-12-16T22:44:47.452496Z] 22:44:47     INFO -  /home/worker/workspace/build/tests/xpcshell/head.js:_do_main:210
[task 2016-12-16T22:44:47.454426Z] 22:44:47     INFO -  /home/worker/workspace/build/tests/xpcshell/head.js:_execute_test:545
[task 2016-12-16T22:44:47.459931Z] 22:44:47     INFO -  -e:null:1

Android shows more failures: https://treeherder.mozilla.org/logviewer.html#?job_id=40710593&repo=mozilla-inbound
 TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/xpcshell/test_ext_contextual_identities.js | test_contextualIdentity_with_permissions - [test_contextualIdentity_with_permissions : 221] A promise chain failed to handle a rejection: An unexpected error occurred - rejection date: Fri Dec 16 2016 22:20:31 GMT+0000 (GMT) - stack: null - false == true
TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/xpcshell/test_ext_contextual_identities.js | test_contextualIdentity_with_permissions - [test_contextualIdentity_with_permissions : 73] Extension left running at test shutdown - "running" == "unloaded"

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

The Android failures persisted with part 3 applied.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/077c30e250d6
Expose ContextualIdentities (aka containers) to WebExtensions - part 1, r=kmaglione
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f7b9683818a
Expose ContextualIdentities (aka containers) to WebExtensions - part 2, r=kmaglione

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/077c30e250d6
https://hg.mozilla.org/mozilla-central/rev/6f7b9683818a

Comment 12

[Will Bamberg [:wbamberg]]

I've written some docs for this API: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contextualIdentities

Please let me know if this looks OK.

Comment 13

[Andrea Marchesini [:baku]]

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contextualIdentities/remove
the title should be: ContextualIdentities.remove()

looks good to me.

Comment 14

[Jonathan Kingston [:jkt] he/him]

There should likely be a warning that the API works in Android however it doesn't have any UX related to it.

Comment 15

[Will Bamberg [:wbamberg]]

Thanks, I've made those two updates.