Mindaugas <LA-MJ> Reporter
	Description • 9 years ago

https://developer.chrome.com/extensions/permissions

API is listed on https://wiki.mozilla.org/WebExtensions#List_of_APIs_we_will_likely_support_in_the_future

	Dan Callahan [:callahad]
	Comment 1 • 9 years ago

Tweaking the bug title to make it more findable in searches. The permissions API is intrinsically tied to the optional_permissions manifest property.

	Markus Jaritz [:designakt] (UX)
	Comment 2 • 8 years ago

Yesterday you mentioned this having 2 steps, where the first one would not require UX. Is that correct?
What is the timeline for UX on this?
How do we surface required permissions? (https://developer.chrome.com/extensions/permission_warnings)
What is the advantage for the user?
What is the incentive for the Dev to use optional permissions?
What is an example on Chrome that uses optional permissions?

	Andy McKay
	Comment 3 • 8 years ago

(In reply to Markus Jaritz [:maritz] (UX) from comment #2)
> Yesterday you mentioned this having 2 steps, where the first one would not
> require UX. Is that correct?

After review, I don't think so. There was talk about using some built in API for this, I don't think they'll work.

Based on the fact that we don't have UX for this and its a big bug, I'm going to take this off tracking 48 and we'll see when UX has had opportunity to review it.

> What is the timeline for UX on this?

I think we should do this sooner rather than later. The ability of WebExtensions to explicitly ask for permissions is a good things for users and something we've never had with add-ons before.

> How do we surface required permissions?
> (https://developer.chrome.com/extensions/permission_warnings)
> What is the advantage for the user?
> What is the incentive for the Dev to use optional permissions?
> What is an example on Chrome that uses optional permissions?

	Kris Maglione [:kmag]
	Comment 4 • 8 years ago

(In reply to Andy McKay [:andym] from comment #3)
> (In reply to Markus Jaritz [:maritz] (UX) from comment #2)
> > Yesterday you mentioned this having 2 steps, where the first one would not
> > require UX. Is that correct?
> 
> After review, I don't think so. There was talk about using some built in API
> for this, I don't think they'll work.
> 
> Based on the fact that we don't have UX for this and its a big bug, I'm
> going to take this off tracking 48 and we'll see when UX has had opportunity
> to review it.

We have some UX for permission prompts that are designed for web 
pages. I think we'd most likely be able to reuse most of that, 
but we'd need some changes to make it clear that the message is 
related to an extension rather than a web page.

At some point we'll also need a UI to manage and revoke options 
permissions. I think that's going to have to depend on bug 
1212685.

	Thomas Andersen
	Comment 5 • 8 years ago

> What is the incentive for the Dev to use optional permissions?

I'll try with a simple scenario:
Create a simple feed reader which displays a feed list in a popup/action. The user should be able to customize what feed/url to fetch.

As far as I know there is no way to request a custom url given by the user as the permission does not allow it.

	Markus Jaritz [:designakt] (UX)
	Comment 6 • 8 years ago

Work on this bug is happening on GitHub:
https://github.com/mozilla/addons/issues/51

	Kris Maglione [:kmag]
	Comment 7 • 8 years ago

Un-assigning since this has morphed from a UX bug to a tracking bug.

	Andrew Swan [:aswan] Assignee
	Comment 10 • 7 years ago

Kris, this needs tests of course and I haven't yet touched content scripts but can you take a look and let me know how you want to connect what's here with bug 1333477?  I can put in message manager dispatching/handling here or would you rather handle that in 1333477?  Note that we also need to update whiteListedHosts in various contexts, as the bugs are worded that would make more sense here but I dont' really care where we do it.

	Kris Maglione [:kmag]
	Comment 15 • 7 years ago
mozreview-review

Comment on attachment 8841600 [details]
Bug 1197420 Part 1 Schema groundwork for optional permissions

https://reviewboard.mozilla.org/r/115752/#review123618

::: toolkit/components/extensions/schemas/manifest.json:238
(Diff revision 2)
> +          { "$ref": "OptionalPermission" },
> +          {
> +            "type": "string",
> +            "enum": [
> +              "alarms",
> +              "geolocation",

Hm. Seems like at least geolocation should work as an optional permission...

	Kris Maglione [:kmag]
	Comment 16 • 7 years ago
mozreview-review

Comment on attachment 8841601 [details]
Bug 1197420 Part 3 Initial browser.permissions api support

https://reviewboard.mozilla.org/r/115754/#review123620

Looks good, but I think we need a better solution for the user input handling.

::: browser/modules/ExtensionsUI.jsm:214
(Diff revision 2)
> +      let strings = this._buildStrings({
> +        type: "optional",
> +        addon: {name},
> +        permissions,
> +      });
> +      this.showPermissionsPrompt(browser, strings, icon).then(resolve);

Nit: `resolve(this.showPermissionsPrompt(...))`

That way we pass along any rejections, too.

::: toolkit/components/extensions/ExtensionPermissions.jsm:24
(Diff revision 2)
> +async function lazyInit() {
> +  if (prefs) {
> +    return;
> +  }
> +
> +  let profileDir = Services.dirsvc.get("ProfD", Ci.nsIFile).path;

`OS.Constants.Path.profileDir`

::: toolkit/components/extensions/ExtensionPermissions.jsm:28
(Diff revision 2)
> +
> +  let profileDir = Services.dirsvc.get("ProfD", Ci.nsIFile).path;
> +  prefs = new JSONFile({path: OS.Path.join(profileDir, FILE_NAME)});
> +
> +  await prefs.load();
> +  prefs.ensureDataReady();

Not necessary. Awaiting on `load()` guarantees this.

::: toolkit/components/extensions/ExtensionPermissions.jsm:38
(Diff revision 2)
> +}
> +
> +this.ExtensionPermissions = {
> +  async get(extension) {
> +    await lazyInit();
> +    return prefs.data[extension.id] || emptyPermissions();

We should probably clone or freeze this, so we don't wind up with changes that don't get saved right away.

::: toolkit/components/extensions/ExtensionPermissions.jsm:53
(Diff revision 2)
> +      if (!prefs.data[extension.id].permissions.includes(perm)) {
> +        added.permissions.push(perm);
> +        prefs.data[extension.id].permissions.push(perm);
> +      }

Please do something like `let perms = prefs.data[extension.id].permissions` outside of the loop. Same for origins and `.remove()`

::: toolkit/components/extensions/ExtensionPermissions.jsm:84
(Diff revision 2)
> +
> +    let removed = emptyPermissions();
> +
> +    for (let perm of permissions.permissions) {
> +      let i = prefs.data[extension.id].permissions.indexOf(perm);
> +      if (i != -1) {

`if (i > 0)` please... Checking explicitly against -1 always makes me nervous.

::: toolkit/components/extensions/ext-c-permissions.js:10
(Diff revision 2)
> +        let winUtils = context.contentWindow.getInterface(Ci.nsIDOMWindowUtils);
> +        if (!winUtils.isHandlingUserInput) {
> +          throw new ExtensionError("May only request permissions from a user input handler");
> +        }

I'd rather we send this information to the parent at the schema level. One of mixedpuppy's patches also requires this information in the parent in several places, so it would simplify a lot of things.

::: toolkit/components/extensions/ext-permissions.js:22
(Diff revision 2)
> +const {
> +  classifyPermission,
> +  ExtensionError,
> +} = ExtensionUtils;
> +
> +XPCOMUtils.defineLazyPreferenceGetter(this, "PROMPTS", "extensions.webextOptionalPermissionPrompts");

Please no ALL_CAPS for non-constant values.

::: toolkit/components/extensions/ext-permissions.js:39
(Diff revision 2)
> +            let type = classifyPermission(perm);
> +            if (type.origin) {
> +              origins.push(perm);
> +            }
> +          }
> +          optionalOrigins = new MatchPattern(origins);

Can we just make this a lazy getter on the Extension instead?

::: toolkit/components/extensions/ext-permissions.js:42
(Diff revision 2)
> +        let {permissions, origins} = perms;
> +        permissions = permissions || [];
> +        origins = origins || [];

Please just set `"default": []` for these in the schema. Or `let {permissions = [], origins = []}` if that's not an option for some reason.

::: toolkit/components/extensions/ext-permissions.js:81
(Diff revision 2)
> +        await ExtensionPermissions.add(context.extension, perms);
> +        return true;
> +      },
> +
> +      async getAll() {
> +        let perms = context.extension.userPermissions();

Please just make `userPermissions` a getter rather than a method.

::: toolkit/components/extensions/ext-permissions.js:84
(Diff revision 2)
> +
> +      async getAll() {
> +        let perms = context.extension.userPermissions();
> +        return {
> +          permissions: perms.permissions,
> +          origins: perms.hosts,

Why hosts in one place and origins in another?

::: toolkit/components/extensions/ext-permissions.js:89
(Diff revision 2)
> +        for (let perm of permissions.permissions) {
> +          if (!context.extension.hasPermission(perm)) {
> +            return false;
> +          }
> +        }
> +
> +        for (let origin of permissions.origins) {

What happens if either of these is null?

::: toolkit/components/extensions/schemas/permissions.json:53
(Diff revision 2)
> +        "description": "Get a list of all the extension's permissions.",
> +        "parameters": [
> +          {
> +            "name": "callback",
> +            "type": "function",
> +            "optional": true,

No `"optional": true`, please.

	Kris Maglione [:kmag]
	Comment 17 • 7 years ago
mozreview-review

Comment on attachment 8841601 [details]
Bug 1197420 Part 3 Initial browser.permissions api support

https://reviewboard.mozilla.org/r/115754/#review123624

::: toolkit/components/extensions/ExtensionPermissions.jsm:20
(Diff revision 2)
> +  if (prefs) {
> +    return;
> +  }

Oh, also, we need to store a promise the first time this is called, and return that if we're not fully initialized yet. Otherwise, when there are concurrent callers, some will wind up with non-fully-initialized pref stores.

	Kris Maglione [:kmag]
	Comment 18 • 7 years ago
mozreview-review

Comment on attachment 8848272 [details]
Bug 1197420 Part 4 Apply dynamic permission changes

https://reviewboard.mozilla.org/r/121204/#review123622

::: toolkit/components/extensions/Extension.jsm:690
(Diff revision 1)
> +    this.on("remove-permissions", (ignoreEvent, permissions) => {
> +      for (let perm of permissions.permissions) {
> +        this.permissions.delete(perm);
> +      }
> +
> +      if (permissions.origins.length > 0) {

No need for the `if`.

::: toolkit/components/extensions/Extension.jsm:927
(Diff revision 1)
>          return;
>        }
>  
>        GlobalManager.init(this);
>  
> +      return ExtensionPermissions.get(this);

We should do this earlier, as part of a `Promies.all`, or at least start the load earlier, and then wait on the promise here.

::: toolkit/components/extensions/ExtensionChild.jsm:786
(Diff revision 1)
>    hasPermission(permission) {
>      return this.context.extension.hasPermission(permission);
>    }
> +
> +  isPermissionRevokable(permission) {
> +    return ChildAPIManager.OPTIONAL_PERMISSIONS.includes(permission);

I think we should check against the manifest's set of optional permissions here, instead. If it's not an optional permission for this particular extension, it'll never be lazily instantiated or revoked.

::: toolkit/components/extensions/ExtensionChild.jsm:790
(Diff revision 1)
> +    // XXX when does this get cleaned up
> +    this.context.extension.permissionChangedCallbacks.add(callback);

I think we probably want a separate set of these per context, and then the context can do `extension.on("add-permissions", ...)` and such, and remove the listeners on unload. Otherwise we need to store the set of listeners specific to this context, and remove them from the extension instance on unload.

::: toolkit/components/extensions/ExtensionChild.jsm:796
(Diff revision 1)
> +  let type = Schemas.rootNamespace
> +                    .get("manifest")
> +                    .get("OptionalPermission");
> +  let permissions = [];
> +  for (let choice of type.choices) {
> +    if (choice.enumeration) {
> +      permissions = permissions.concat(...choice.enumeration);
> +    }
> +  }
> +  return permissions;

No need for this if we check against the extension's own set of optional permissions. They're already validated against this.

::: toolkit/components/extensions/ExtensionContent.jsm:831
(Diff revision 1)
> +      if (permissions.permissions.length > 0) {
> +        for (let perm of permissions.permissions) {
> +          this.permissions.add(perm);
> +        }
> +        for (let callback of this.permissionChangedCallbacks) {
> +          callback();

Please catch errors here. Not catching errors from callbacks always makes me nervous... Same below.

	Kris Maglione [:kmag]
	Comment 19 • 7 years ago
mozreview-review

Comment on attachment 8848273 [details]
Bug 1197420 Part 5 Tests for optional permissions

https://reviewboard.mozilla.org/r/121206/#review123626

::: toolkit/components/extensions/test/mochitest/chrome.ini:21
(Diff revision 1)
> +[test_chrome_ext_downloads_saveAs.html]
> +[test_chrome_ext_eventpage_warning.html]
>  [test_chrome_ext_hybrid_addons.html]
> +[test_chrome_ext_idle.html]
> +[test_chrome_ext_identity.html]
> +skip-if = os == 'android' # unsupported.
> +[test_chrome_ext_permissions.html]
> +[test_chrome_ext_storage_cleanup.html]
> +[test_chrome_ext_trackingprotection.html]
>  [test_chrome_ext_trustworthy_origin.html]
>  [test_chrome_ext_webnavigation_resolved_urls.html]
> +[test_chrome_ext_webrequest_background_events.html]
> +[test_chrome_ext_webrequest_host_permissions.html]

Thank you.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_permissions.html:15
(Diff revision 1)
> +<body>
> +
> +<script type="text/javascript">
> +"use strict";
> +
> +add_task(async function test_permissions() {

+1

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_permissions.html:18
(Diff revision 1)
> +      permissions: ["cookies"],
> +      origins: ["*://example.com/"],

Please also add tests for only "permissions", only "origins", and probably also with neither, for all of the relevant methods.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_permissions.html:35
(Diff revision 1)
> +      if (msg == "set-cookie") {
> +        try {
> +          browser.cookies.set({
> +            url: "http://example.com/",
> +            name: "COOKIE",
> +            value: "NOM NOM",

Hm...

::: toolkit/components/extensions/test/xpcshell/test_ext_permissions.js:160
(Diff revision 1)
> +  // Restart, verify permissions are still present
> +  await AddonTestUtils.promiseRestartManager();
> +  await extension.awaitStartup();

Would be good to test initial startup (with the settings store uninitialized) with multiple current extensions.

	Andrew Swan [:aswan] Assignee
	Comment 20 • 7 years ago
mozreview-review-reply

Comment on attachment 8841601 [details]
Bug 1197420 Part 3 Initial browser.permissions api support

https://reviewboard.mozilla.org/r/115754/#review123620

> Why hosts in one place and origins in another?

`hosts` is never externally visible and that code was written before any of the optional permissions code.  In the optional permissions API, `origins` is part of the extension-facing api and so it needs to be this way to be compatible with Chrome (and of course it it more sensible this way).  I was wary of changing to much at once but I think I should switch everything over to origins, will do that in a revision.

	Andrew Swan [:aswan] Assignee
	Comment 21 • 7 years ago
mozreview-review-reply

Comment on attachment 8841600 [details]
Bug 1197420 Part 1 Schema groundwork for optional permissions

https://reviewboard.mozilla.org/r/115752/#review123618

> Hm. Seems like at least geolocation should work as an optional permission...

I just copied the existing list from Chrome: https://developer.chrome.com/extensions/permissions

	Andrew Swan [:aswan] Assignee
	Comment 22 • 7 years ago
mozreview-review-reply

Comment on attachment 8841601 [details]
Bug 1197420 Part 3 Initial browser.permissions api support

https://reviewboard.mozilla.org/r/115754/#review123620

> We should probably clone or freeze this, so we don't wind up with changes that don't get saved right away.

I'm not sure I follow, are you concerned about the caller writing to the returned object?  This is only called from one place (extension startup) where I don't think there's much danger of somebody adding code that changes the object.  I can add a guard if you feel strongly though...

> I'd rather we send this information to the parent at the schema level. One of mixedpuppy's patches also requires this information in the parent in several places, so it would simplify a lot of things.

Do you mind landing this as-is and doing that work in a follow-up?  I'm happy to do it and will do it during the 55 cycle.

> Please just set `"default": []` for these in the schema. Or `let {permissions = [], origins = []}` if that's not an option for some reason.

Whoops these do have `default: []` in the schema, this was just old code

> What happens if either of these is null?

They have a default value of `[]` so they can't be null

	Andrew Swan [:aswan] Assignee
	Comment 23 • 7 years ago
mozreview-review-reply

Comment on attachment 8841601 [details]
Bug 1197420 Part 3 Initial browser.permissions api support

https://reviewboard.mozilla.org/r/115754/#review123624

> Oh, also, we need to store a promise the first time this is called, and return that if we're not fully initialized yet. Otherwise, when there are concurrent callers, some will wind up with non-fully-initialized pref stores.

whoops thanks for catching that.  it seems very likely to happen at startup to anybody with more than one webextension installed.

	Andrew Swan [:aswan] Assignee
	Comment 24 • 7 years ago
mozreview-review-reply

Comment on attachment 8848272 [details]
Bug 1197420 Part 4 Apply dynamic permission changes

https://reviewboard.mozilla.org/r/121204/#review123622

> I think we probably want a separate set of these per context, and then the context can do `extension.on("add-permissions", ...)` and such, and remove the listeners on unload. Otherwise we need to store the set of listeners specific to this context, and remove them from the extension instance on unload.

Okay, but the callbacks are going to call `hasPermission()` which consults the Extension.  So we need to make sure that the extension is updated before we call any of the callbacks.  We can do that by relying on EventEmitter handlers being called in the order they were originally added, but that makes me nervous.  Maybe it shouldn't though...

	Kris Maglione [:kmag]
	Comment 31 • 7 years ago
mozreview-review

Comment on attachment 8849285 [details]
Bug 1197420 Part 2 Extension cleanups for optional permissions

https://reviewboard.mozilla.org/r/122098/#review125638

	Kris Maglione [:kmag]
	Comment 32 • 7 years ago
mozreview-review

Comment on attachment 8841601 [details]
Bug 1197420 Part 3 Initial browser.permissions api support

https://reviewboard.mozilla.org/r/115754/#review125640

::: toolkit/components/extensions/ExtensionPermissions.jsm:24
(Diff revision 3)
> +    return;
> +  }
> +
> +  prefs = new JSONFile({path: OS.Path.join(OS.Constants.Path.profileDir, FILE_NAME)});
> +
> +  await prefs.load();

I think we need to store this load promise, and await on it the next time someone calls `lazyInit()`. Otherwise they may wind up with a not-fully-initialized copy.

	Kris Maglione [:kmag]
	Comment 33 • 7 years ago
mozreview-review

Comment on attachment 8841601 [details]
Bug 1197420 Part 3 Initial browser.permissions api support

https://reviewboard.mozilla.org/r/115754/#review125646

	Kris Maglione [:kmag]
	Comment 34 • 7 years ago
mozreview-review

Comment on attachment 8848272 [details]
Bug 1197420 Part 4 Apply dynamic permission changes

https://reviewboard.mozilla.org/r/121204/#review125648

::: toolkit/components/extensions/Extension.jsm:912
(Diff revision 3)
> -  startup() {
> +  async startup() {
>      let started = false;
> -    return this.loadManifest().then(() => {
> +
> +    try {
> +      let perms;
> +      [, perms] = await Promise.all([this.loadManifest(), ExtensionPermissions.get(this)]);

Nit: `let [, perms] = ...`

::: toolkit/components/extensions/Extension.jsm:1039
(Diff revision 3)
>  
> -    return this.permissions.has(perm);
> +    if (this.permissions.has(perm)) {
> +      return true;
> +    }
> +
> +    if (includeOptional && (this.manifest.optional_permissions || []).includes(perm)) {

Please just use `"default": []` for `optional_permissions` in the manifest schema, and drop the `|| []`

	Andrew Swan [:aswan] Assignee
	Comment 35 • 7 years ago
mozreview-review-reply

Comment on attachment 8841601 [details]
Bug 1197420 Part 3 Initial browser.permissions api support

https://reviewboard.mozilla.org/r/115754/#review123620

> Do you mind landing this as-is and doing that work in a follow-up?  I'm happy to do it and will do it during the 55 cycle.

Filed bug 1350151 for this

	Andrew Swan [:aswan] Assignee
	Comment 39 • 7 years ago

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

	Mozilla Autoland
	Comment 45 • 7 years ago

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 88db4c72a9d8 -d 8528543cc258: rebasing 383932:88db4c72a9d8 "Bug 1197420 Part 1 Schema groundwork for optional permissions r=kmag"
rebasing 383933:6bf3abe0db8b "Bug 1197420 Part 2 Extension cleanups for optional permissions r=kmag"
merging browser/base/content/test/webextensions/browser_extension_sideloading.js
merging toolkit/components/extensions/Extension.jsm
merging toolkit/mozapps/extensions/content/extensions.js
merging toolkit/mozapps/extensions/internal/XPIProvider.jsm
rebasing 383934:46451e501104 "Bug 1197420 Part 3 Initial browser.permissions api support r=kmag"
merging browser/locales/en-US/chrome/browser/browser.properties
merging toolkit/components/extensions/Extension.jsm
merging toolkit/components/extensions/ExtensionUtils.jsm
rebasing 383935:5fe8342b1042 "Bug 1197420 Part 4 Apply dynamic permission changes r=kmag"
merging toolkit/components/extensions/Extension.jsm
merging toolkit/components/extensions/ExtensionChild.jsm
merging toolkit/components/extensions/ExtensionCommon.jsm
merging toolkit/components/extensions/ExtensionContent.jsm
rebasing 383936:4fb68e7d2f3d "Bug 1197420 Part 5 Tests for optional permissions r=kmag" (tip)
merging toolkit/components/extensions/test/mochitest/chrome.ini
merging toolkit/components/extensions/test/xpcshell/xpcshell.ini
warning: conflicts while merging toolkit/components/extensions/test/mochitest/chrome.ini! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

	Pulsebot
	Comment 51 • 7 years ago

Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6c4c29c0b05a
Part 1 Schema groundwork for optional permissions r=kmag
https://hg.mozilla.org/integration/autoland/rev/3de2de388ac9
Part 2 Extension cleanups for optional permissions r=kmag
https://hg.mozilla.org/integration/autoland/rev/7df6cc66a2eb
Part 3 Initial browser.permissions api support r=kmag
https://hg.mozilla.org/integration/autoland/rev/cb352ddee812
Part 4 Apply dynamic permission changes r=kmag
https://hg.mozilla.org/integration/autoland/rev/5750ae148c78
Part 5 Tests for optional permissions r=kmag

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 52 • 7 years ago

Backed out for failing test_ext_all_apis.html:

https://hg.mozilla.org/integration/autoland/rev/32b07901a07087717a53ab3e7fd15e8baca7925e
https://hg.mozilla.org/integration/autoland/rev/d16b5157f406d77b592857b5c1d70ab74a3cd535
https://hg.mozilla.org/integration/autoland/rev/7f34c9359eb8beb3c945031b0eefba82c6d13954
https://hg.mozilla.org/integration/autoland/rev/1e0885d9302764b0e8952ddf723d891f35e57b64
https://hg.mozilla.org/integration/autoland/rev/83cea5c428a741b01564bad066c4721139983b9d

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=5750ae148c78047de794a6cb242b1c9a6069c2f6&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=86294637&repo=autoland

12:11:36     INFO - TEST-PASS | browser/components/extensions/test/mochitest/test_ext_all_apis.html | content script APIs 
12:11:36     INFO - SpawnTask.js | Leaving test test_enumerate_content_script_apis
12:11:36     INFO - SpawnTask.js | Entering test test_enumerate_background_script_apis
12:11:36     INFO - Extension loaded
12:11:36     INFO - Buffered messages finished
12:11:36     INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/mochitest/test_ext_all_apis.html | background script APIs -     Structures begin differing at:
12:11:36     INFO - got[20] = "browser.permissions.contains"
12:11:36     INFO - expected[20] = "browser.runtime.OnInstalledReason"
12:11:36     INFO - 
12:11:36     INFO -     SimpleTest.isDeeply@SimpleTest/SimpleTest.js:1595:9
12:11:36     INFO -     test_enumerate_background_script_apis@browser/components/extensions/test/mochitest/test_ext_all_apis.js:160:3
12:11:36     INFO -     onFulfilled@SimpleTest/SpawnTask.js:69:15
12:11:36     INFO -     promise callback*next@SimpleTest/SpawnTask.js:104:45
12:11:36     INFO -     onFulfilled@SimpleTest/SpawnTask.js:73:7
12:11:36     INFO -     promise callback*next@SimpleTest/SpawnTask.js:104:45
12:11:36     INFO -     onFulfilled@SimpleTest/SpawnTask.js:73:7
12:11:36     INFO -     co/<@SimpleTest/SpawnTask.js:58:5
12:11:36     INFO -     co@SimpleTest/SpawnTask.js:54:10
12:11:36     INFO -     toPromise@SimpleTest/SpawnTask.js:122:60
12:11:36     INFO -     next@SimpleTest/SpawnTask.js:103:19
12:11:36     INFO -     onFulfilled@SimpleTest/SpawnTask.js:73:7
12:11:36     INFO -     promise callback*next@SimpleTest/SpawnTask.js:104:45
12:11:36     INFO -     onFulfilled@SimpleTest/SpawnTask.js:73:7
12:11:36     INFO -     co/<@SimpleTest/SpawnTask.js:58:5
12:11:36     INFO -     co@SimpleTest/SpawnTask.js:54:10
12:11:36     INFO -     add_task/<@SimpleTest/SpawnTask.js:270:9
12:11:36     INFO -     setTimeout handler*SimpleTest_setTimeoutShim@SimpleTest/SimpleTest.js:672:12
12:11:36     INFO -     add_task@SimpleTest/SpawnTask.js:269:7
12:11:36     INFO -     @browser/components/extensions/test/mochitest/test_ext_all_apis.js:127:1

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 53 • 7 years ago

Also failing xpcshell test test_ext_permissions.js: https://treeherder.mozilla.org/logviewer.html#?job_id=86296671&repo=autoland

	Mozilla Autoland
	Comment 59 • 7 years ago

hg error in cmd: hg identify upstream -r tip:

	Pulsebot
	Comment 60 • 7 years ago

Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3e690bbe8b5a
Part 1 Schema groundwork for optional permissions r=kmag
https://hg.mozilla.org/integration/autoland/rev/440bab141509
Part 2 Extension cleanups for optional permissions r=kmag
https://hg.mozilla.org/integration/autoland/rev/46e135035f10
Part 3 Initial browser.permissions api support r=kmag
https://hg.mozilla.org/integration/autoland/rev/925e3a9499ee
Part 4 Apply dynamic permission changes r=kmag
https://hg.mozilla.org/integration/autoland/rev/8a0125e00903
Part 5 Tests for optional permissions r=kmag

	Phil Ringnalda (:philor)
	Comment 61 • 7 years ago

Backed out in https://hg.mozilla.org/integration/autoland/rev/0fa8a7ec4b42 for timing out on Android, https://treeherder.mozilla.org/logviewer.html#?job_id=86373358&repo=autoland and https://treeherder.mozilla.org/logviewer.html#?job_id=86373354&repo=autoland

	Pulsebot
	Comment 63 • 7 years ago

Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b6af7d779efc
Part 1 Schema groundwork for optional permissions r=kmag
https://hg.mozilla.org/integration/autoland/rev/680dd7916a23
Part 2 Extension cleanups for optional permissions r=kmag
https://hg.mozilla.org/integration/autoland/rev/d1628b66e5f8
Part 3 Initial browser.permissions api support r=kmag
https://hg.mozilla.org/integration/autoland/rev/f4fbd8e60288
Part 4 Apply dynamic permission changes r=kmag
https://hg.mozilla.org/integration/autoland/rev/b56c89bfeb0e
Part 5 Tests for optional permissions r=kmag

	Pulsebot
	Comment 64 • 7 years ago

Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/2b54d882c024
Backed out changeset b56c89bfeb0e 
https://hg.mozilla.org/integration/autoland/rev/94e7f0558069
Backed out changeset f4fbd8e60288 
https://hg.mozilla.org/integration/autoland/rev/0b0d2e8ada83
Backed out changeset d1628b66e5f8 
https://hg.mozilla.org/integration/autoland/rev/a1667683da70
Backed out changeset 680dd7916a23 
https://hg.mozilla.org/integration/autoland/rev/66fe37afe8f1
Backed out changeset b6af7d779efc on request of developer. r=backout

	Andrew Swan [:aswan] Assignee
	Comment 65 • 7 years ago

New try run, good on Android:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=40f4bed27c01baa24815a37ef1328ff4e135c528

	Pulsebot
	Comment 67 • 7 years ago

Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0a1daa74303c
Part 1 Schema groundwork for optional permissions r=kmag
https://hg.mozilla.org/integration/autoland/rev/52c2b953c811
Part 2 Extension cleanups for optional permissions r=kmag
https://hg.mozilla.org/integration/autoland/rev/a9a6dad3b0b7
Part 3 Initial browser.permissions api support r=kmag
https://hg.mozilla.org/integration/autoland/rev/2ba59dc585a3
Part 4 Apply dynamic permission changes r=kmag
https://hg.mozilla.org/integration/autoland/rev/4e319a074b49
Part 5 Tests for optional permissions r=kmag

	Phil Ringnalda (:philor)
	Comment 68 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/0a1daa74303c
https://hg.mozilla.org/mozilla-central/rev/52c2b953c811
https://hg.mozilla.org/mozilla-central/rev/a9a6dad3b0b7
https://hg.mozilla.org/mozilla-central/rev/2ba59dc585a3
https://hg.mozilla.org/mozilla-central/rev/4e319a074b49

	Will Bamberg [:wbamberg]
	Comment 69 • 7 years ago

Ive added:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/permissions
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/optional_permissions

(I forgot to add compat data for permissions.Permissions, but that's on its way.)

I filed bugs for a few issues I've seen:
https://bugzilla.mozilla.org/show_bug.cgi?id=1369577
https://bugzilla.mozilla.org/show_bug.cgi?id=1369776
https://bugzilla.mozilla.org/show_bug.cgi?id=1369581

For those issues, I've added compat notes where it seems likely that the current behavior is what we want (https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/permissions/request#Browser_compatibility). Where the behavior seems like a bug that we want to fix (e.g. bug 1369577, and not being able to grant only activeTab) I've not documented the current behavior, in the expectation that the current behavior will change soonish.

Let me know if this works for you!

	Andrew Swan [:aswan] Assignee
	Comment 70 • 7 years ago

Looks good, thank you.  Just a couple of nits:
- I don't think we need a browser compatibility note about silently granting some permissions, I believe Chrome does the same
- In the main page for the permissions api I think there are a few other reasons one might use optional permissions.  One is if an extension needs host permissions but what hosts it will access is not known until runtime (ie in response to a user action or configuration), so instead of asking for <all_urls> the extension can ask at runtime for a specific host or domain.

	Will Bamberg [:wbamberg]
	Comment 71 • 7 years ago

Thanks for checking Andy. I've made those changes, and also included the point Christiane made in bug 1369776, comment 5.