Chrome docs: https://developer.chrome.com/extensions/management#method-getSelf This is needed to support some popular Chrome extensions as per bug 1280062

This is an innocuous API that only provides information about the extension itself. In Chrome it does not even need `management` permissions to be called [1], so I suppose we should do the same with our implementation. [1] https://developer.chrome.com/extensions/management#method-getSelf

Review commit: https://reviewboard.mozilla.org/r/64078/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/64078/

Comment on attachment 8770681 [details] Bug 1283116 - Implement chrome.management.getSelf, Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64078/diff/1-2/

https://reviewboard.mozilla.org/r/64078/#review61356 ::: toolkit/components/extensions/ext-management.js:8 (Diff revision 2) > "use strict"; > > +XPCOMUtils.defineLazyModuleGetter(this, "AddonManager", > + "resource://gre/modules/AddonManager.jsm"); > + > +function installType(addon) { I was thinking that maybe I could create an xpcshell test for this to test at least system add-ons, if not also sideloaded add-ons. ::: toolkit/components/extensions/ext-management.js:32 (Diff revision 2) > + id: extension.id, > + name: addon.name || "", > + shortName: m.short_name || "", > + description: addon.description || "", > + version: m.version || "", > + mayDisable: !addon.isSystem, There is no actual test for this yet, and I'm not sure if we can create one. ::: toolkit/components/extensions/ext-management.js:33 (Diff revision 2) > + name: addon.name || "", > + shortName: m.short_name || "", > + description: addon.description || "", > + version: m.version || "", > + mayDisable: !addon.isSystem, > + enabled: addon.isActive, There is also no test for this, and thinking about trying to test it, I wonder if it can ever be false. How can you call `getSelf` from an extension that is itself disabled? That seems impossible.

Comment on attachment 8770681 [details] Bug 1283116 - Implement chrome.management.getSelf, Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64078/diff/2-3/

After doing some work on management.get, I realized that there was other data that I can take from the addon, as opposed to the manifest, so I've updated the commit. It is ready to be reviewed, in case you were waiting for something. I'm also interested to hear what you think about the tests I mentioned. I think this is probably okay to land without any additional tests, but if you can think of a way to test the things that aren't tested I'd be glad to add some.

Comment on attachment 8770681 [details] Bug 1283116 - Implement chrome.management.getSelf, https://reviewboard.mozilla.org/r/64078/#review62696 ::: toolkit/components/extensions/ext-management.js:14 (Diff revision 3) > + if (addon.temporarilyInstalled) { > + return "development"; > + } else if (addon.foreignInstall) { > + return "sideload"; > + } else if (addon.isSystem) { > + return "other"; Do we even want to include system add-ons, or any other hidden add-ons here? ::: toolkit/components/extensions/ext-management.js:31 (Diff revision 3) > + let extInfo = { > + id: extension.id, > + name: addon.name || "", > + shortName: m.short_name || "", > + description: addon.description || "", > + version: addon.version || "", No need for the `|| ""` for this or `addon.name`. Neither of them is optional. ::: toolkit/components/extensions/ext-management.js:32 (Diff revision 3) > + id: extension.id, > + name: addon.name || "", > + shortName: m.short_name || "", > + description: addon.description || "", > + version: addon.version || "", > + mayDisable: !addon.isSystem, `!!(addon.permissions & AddonManager.PERM_CAN_DISABLE)` ::: toolkit/components/extensions/ext-management.js:35 (Diff revision 3) > + description: addon.description || "", > + version: addon.version || "", > + mayDisable: !addon.isSystem, > + enabled: addon.isActive, > + homepageUrl: addon.homepageURL, > + updateUrl: addon.updateURL, `homepageUrl` and `updateUrl` should be omitted if they're missing. ::: toolkit/components/extensions/ext-management.js:39 (Diff revision 3) > + homepageUrl: addon.homepageURL, > + updateUrl: addon.updateURL, > + optionsUrl: addon.optionsURL || "", > + icons: m.icons && Array.from(Object.keys(m.icons)).map(key => { > + return {size: Number(key), url: m.icons[key]}; > + }) || undefined, No need for the `|| undefined`. Except that the property should only be added if there are icons. ::: toolkit/components/extensions/ext-management.js:42 (Diff revision 3) > + icons: m.icons && Array.from(Object.keys(m.icons)).map(key => { > + return {size: Number(key), url: m.icons[key]}; > + }) || undefined, > + permissions: m.permissions && m.permissions.filter(perm => { > + return !extension.whiteListedHosts.pat.includes(perm); > + }) || [], We should pull these from the `Extension` object instead. There's always a `permissions` Set there, and the filtering is already done. ::: toolkit/components/extensions/test/mochitest/test_ext_management.html:49 (Diff revision 3) > + } > + > + let manifest = { > + applications: { > + gecko: { > + id: id, No need for the `id` here. The test harness will add it. ::: toolkit/components/extensions/test/mochitest/test_ext_management.html:85 (Diff revision 3) > + is(extInfo.shortName, manifest.short_name, "getSelf returned the expected shortName"); > + is(extInfo.mayDisable, true, "getSelf returned the expected value for mayDisable"); > + is(extInfo.enabled, true, "getSelf returned the expected value for enabled"); > + is(extInfo.homepageUrl, manifest.homepage_url, "getSelf returned the expected homepageUrl"); > + is(extInfo.updateUrl, manifest.applications.gecko.update_url, "getSelf returned the expected updateUrl"); > + ok(extInfo.optionsUrl.includes(manifest.options_ui.page), "getSelf returned the expected optionsUrl"); `.endsWith()` would be better than `.includes()` ::: toolkit/components/extensions/test/mochitest/test_ext_management.html:86 (Diff revision 3) > + is(extInfo.mayDisable, true, "getSelf returned the expected value for mayDisable"); > + is(extInfo.enabled, true, "getSelf returned the expected value for enabled"); > + is(extInfo.homepageUrl, manifest.homepage_url, "getSelf returned the expected homepageUrl"); > + is(extInfo.updateUrl, manifest.applications.gecko.update_url, "getSelf returned the expected updateUrl"); > + ok(extInfo.optionsUrl.includes(manifest.options_ui.page), "getSelf returned the expected optionsUrl"); > + let icons = Array.from(Object.keys(manifest.icons)); `Object.keys` returns an array. No need for `Array.from`. ::: toolkit/components/extensions/test/mochitest/test_ext_management.html:87 (Diff revision 3) > + is(extInfo.enabled, true, "getSelf returned the expected value for enabled"); > + is(extInfo.homepageUrl, manifest.homepage_url, "getSelf returned the expected homepageUrl"); > + is(extInfo.updateUrl, manifest.applications.gecko.update_url, "getSelf returned the expected updateUrl"); > + ok(extInfo.optionsUrl.includes(manifest.options_ui.page), "getSelf returned the expected optionsUrl"); > + let icons = Array.from(Object.keys(manifest.icons)); > + for (let index of [0, 1]) { The ordering of object keys is technically non-deterministic. Something like `for (let [index, size] of Object.keys(manifest.icons).sort().entries())` would be safer. ::: toolkit/components/extensions/test/mochitest/test_ext_management.html:88 (Diff revision 3) > + is(extInfo.homepageUrl, manifest.homepage_url, "getSelf returned the expected homepageUrl"); > + is(extInfo.updateUrl, manifest.applications.gecko.update_url, "getSelf returned the expected updateUrl"); > + ok(extInfo.optionsUrl.includes(manifest.options_ui.page), "getSelf returned the expected optionsUrl"); > + let icons = Array.from(Object.keys(manifest.icons)); > + for (let index of [0, 1]) { > + is(extInfo.icons[index].size, Number(icons[index]), "getSelf returned the expected icon size"); `+icons[index]` ::: toolkit/components/extensions/test/mochitest/test_ext_management.html:91 (Diff revision 3) > + let icons = Array.from(Object.keys(manifest.icons)); > + for (let index of [0, 1]) { > + is(extInfo.icons[index].size, Number(icons[index]), "getSelf returned the expected icon size"); > + is(extInfo.icons[index].url, manifest.icons[icons[index]], "getSelf returned the expected icon url"); > + } > + is(String(extInfo.permissions), String(permissions), "getSelf returned the expected permissions"); The ordering here should not be deterministic. Better to do `isDeeply(extInfo.permissions.sort(), permissions.sort(), ...)` Same for the one below. ::: toolkit/components/extensions/test/mochitest/test_ext_management.html:93 (Diff revision 3) > + is(extInfo.icons[index].size, Number(icons[index]), "getSelf returned the expected icon size"); > + is(extInfo.icons[index].url, manifest.icons[icons[index]], "getSelf returned the expected icon url"); > + } > + is(String(extInfo.permissions), String(permissions), "getSelf returned the expected permissions"); > + is(String(extInfo.hostPermissions), String(hostPermissions), "getSelf returned the expected hostPermissions"); > + is(extInfo.installType, "development", "getSelf returned the expected installType"); Should also test some non-"development" add-ons. ::: toolkit/components/extensions/test/mochitest/test_ext_management.html:100 (Diff revision 3) > +}); > + > +add_task(function* test_management_get_self_minimal() { > + const id = "get_self_test_minimal@tests.mozilla.com"; > + > + function background() { This can be shared between the two tasks. ::: toolkit/components/extensions/test/mochitest/test_ext_management.html:129 (Diff revision 3) > + } > + for (let prop of ["shortName", "description", "optionsUrl"]) { > + is(extInfo[prop], "", `getSelf returned the expected ${prop}`); > + } > + for (let prop of ["homepageUrl", " updateUrl", "icons"]) { > + is(extInfo[prop], undefined, `getSelf returned nothing for the ${prop}`); We should test that these keys are not present, rather than undefined. In any case, missing optional keys are normalized to `null` rather than `undefined` in the cases where the properties are present. ::: toolkit/components/extensions/test/mochitest/test_ext_management.html:132 (Diff revision 3) > + } > + for (let prop of ["homepageUrl", " updateUrl", "icons"]) { > + is(extInfo[prop], undefined, `getSelf returned nothing for the ${prop}`); > + } > + for (let prop of ["permissions", "hostPermissions"]) { > + is(extInfo[prop].length, 0, `getSelf returned the expected ${prop}`); `isDeeply(extInfo[prop], [], ...)` will give better diagnostics.

https://reviewboard.mozilla.org/r/64078/#review62696 > Do we even want to include system add-ons, or any other hidden add-ons here? I'm not sure. As this is just a function that evaluates the installType, I figured it made sense to cover all possibilities. I suppose someone writing a system addon could want to use `getSelf` in some way, and in that case installType should probably return a correct value.

Comment on attachment 8770681 [details] Bug 1283116 - Implement chrome.management.getSelf, Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64078/diff/3-4/

https://reviewboard.mozilla.org/r/64078/#review62696 > Should also test some non-"development" add-ons. How do I install an extension in a test that does not end up being a temporary add-on? It looks like that's what happens when you set `useAddonManager` to true. Do we need to interact with the AddonManager directly in the test? If so, I don't see anything obvious in AddonManager to install a non-temporary extension. Also, if we are going to do that, should we make it a utility function?

https://reviewboard.mozilla.org/r/64078/#review62696 > How do I install an extension in a test that does not end up being a temporary add-on? It looks like that's what happens when you set `useAddonManager` to true. Do we need to interact with the AddonManager directly in the test? If so, I don't see anything obvious in AddonManager to install a non-temporary extension. Also, if we are going to do that, should we make it a utility function? You'd need to either use an xpcshell test, or bundle a signed XPI. There's currently no utility for it, though. Either way, this seems like something we should test, since I imagine that developers will want to use it to trigger different behavior in development environments.

Comment on attachment 8770681 [details] Bug 1283116 - Implement chrome.management.getSelf, clearing the r? for now, please reset it if this was waiting for review and i made an error

Comment on attachment 8770681 [details] Bug 1283116 - Implement chrome.management.getSelf, https://reviewboard.mozilla.org/r/64078/#review61356 > I was thinking that maybe I could create an xpcshell test for this to test at least system add-ons, if not also sideloaded add-ons. Because this is getSelf, I don't think we can test a system add-on, unless we can create and install one ourself. I'm still not sure about how to test a sideloaded add-on, or if we even need to. > There is no actual test for this yet, and I'm not sure if we can create one. Again, unless we can test with a system add-on, I don't think we can test this. > There is also no test for this, and thinking about trying to test it, I wonder if it can ever be false. How can you call `getSelf` from an extension that is itself disabled? That seems impossible. Based on what I said above, I'm going to drop this issue.

I switched the r? to kmag as he was the most recent one to do a detailed review of the code. I'm not sure why the reviewer changed, but feel free to switch it between yourselves if that makes sense.

Comment on attachment 8770681 [details] Bug 1283116 - Implement chrome.management.getSelf, https://reviewboard.mozilla.org/r/64078/#review68848 Some nits in the tests, but otherwise looks great. Thanks! ::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js:56 (Diff revision 6) > "resource://testing-common/MockRegistrar.jsm"); > XPCOMUtils.defineLazyModuleGetter(this, "MockRegistry", > "resource://testing-common/MockRegistry.jsm"); > > +// WebExtension wrapper for ease of testing > +ExtensionTestUtils.init(this); We should do this in the test files that need it, rather than in head.js, since the vast majority of tests in this directory don't need it. ::: toolkit/mozapps/extensions/test/xpcshell/test_ext_management.js:1 (Diff revision 6) > +"use strict"; Please always use `hg mv` when moving files. ::: toolkit/mozapps/extensions/test/xpcshell/test_ext_management.js:4 (Diff revision 6) > +"use strict"; > + > +add_task(function* setup() { > + yield ExtensionTestUtils.startAddonManager(); We should use the `head_addons.js` helpers to do this in `mozapps/extensions` tests. ::: toolkit/mozapps/extensions/test/xpcshell/test_ext_management.js:22 (Diff revision 6) > + background: `(${background})()`, > + }); > + yield extension.startup(); > + yield extension.awaitFinish("management-schema"); > + yield extension.unload(); > +}); I think I'd rather keep this test in `components/extensions` ::: toolkit/mozapps/extensions/test/xpcshell/test_ext_management.js:29 (Diff revision 6) > +// Shared background function for getSelf tests > +function backgroundGetSelf() { > + browser.management.getSelf().then(extInfo => { > + browser.test.sendMessage("management-getSelf", extInfo); > + }, error => { > + browser.test.notifyFail(`getSelf rejected with error: ${String(error)}`); No need for `String(...)`. String coercion is implied. ::: toolkit/mozapps/extensions/test/xpcshell/test_ext_management.js:57 (Diff revision 6) > + }, > + icons: { > + "16": "icons/icon-16.png", > + "48": "icons/icon-48.png", > + }, > + permissions: permissions.concat(hostPermissions), The more usual style these days is `[...permissions, ...hostPermissions]` ::: toolkit/mozapps/extensions/test/xpcshell/test_ext_management.js:62 (Diff revision 6) > + permissions: permissions.concat(hostPermissions), > + }; > + > + let extension = ExtensionTestUtils.loadExtension({ > + manifest, > + background: `(${backgroundGetSelf})()`, No need for the stringification. `background: backgroundGetSelf` will do. ::: toolkit/mozapps/extensions/test/xpcshell/test_ext_management.js:119 (Diff revision 6) > + } > + for (let prop of ["shortName", "description", "optionsUrl"]) { > + equal(extInfo[prop], "", `getSelf returned the expected ${prop}`); > + } > + for (let prop of ["homepageUrl", " updateUrl", "icons"]) { > + equal(extInfo.hasOwnProperty(prop), false, `getSelf did not return a ${prop} property`); `hasOwnProperty` always makes me itch... but I guess it's OK. I'd prefer something like `equal(Object.getOwnPropertyDescriptor(extInfo, prop), undefined, ...)` though.

Comment on attachment 8770681 [details] Bug 1283116 - Implement chrome.management.getSelf, https://reviewboard.mozilla.org/r/64078/#review68848 > Please always use `hg mv` when moving files. Is there something I need to do to this patch to address this, or is this just information for the future? > We should use the `head_addons.js` helpers to do this in `mozapps/extensions` tests. I tried to do this, using `startupManager()` as you'll see in the latest commit. This has caused a couple of problems, however, which I'm not sure how to address: 1. For some reason, the value of `addon.permissions` for the addon returned by `AddonManager.getAddonByID` is now `9`, whereas with the previous version of the code it was `13`. The latter allowed `getSelf` to return `true` for `mayDisable`, whereas the current code causes `getSelf` to return `false` for `mayDisable`, which seems incorrect. 2. The test I added which installs a permanent extension now simply times out seemingly before the extension has finished installing. I looked over the console messages but nothing pops out at me to explain what the problem may be. A complete console log from running the test can be found at https://gist.github.com/bobsilverberg/dcafa81ea0513f9e9a4bcde935e7e32a. > `hasOwnProperty` always makes me itch... but I guess it's OK. I'd prefer something like `equal(Object.getOwnPropertyDescriptor(extInfo, prop), undefined, ...)` though. Why do you prefer `Object.getOwnPropertyDescriptor` over `obj.hasOwnProperty`? Don't they do essentially the same thing, i.e., check for the existence of a property on the object?

Comment on attachment 8770681 [details] Bug 1283116 - Implement chrome.management.getSelf, https://reviewboard.mozilla.org/r/64078/#review68848 > Is there something I need to do to this patch to address this, or is this just information for the future? Unless you're going to move the existing parts of that test back to toolkit/components/extensions like I asked, then yes, the patch needs to be amended to record the move. You can use `hg mv -A` for that. > I tried to do this, using `startupManager()` as you'll see in the latest commit. This has caused a couple of problems, however, which I'm not sure how to address: > > 1. For some reason, the value of `addon.permissions` for the addon returned by `AddonManager.getAddonByID` is now `9`, whereas with the previous version of the code it was `13`. The latter allowed `getSelf` to return `true` for `mayDisable`, whereas the current code causes `getSelf` to return `false` for `mayDisable`, which seems incorrect. > 2. The test I added which installs a permanent extension now simply times out seemingly before the extension has finished installing. I looked over the console messages but nothing pops out at me to explain what the problem may be. A complete console log from running the test can be found at https://gist.github.com/bobsilverberg/dcafa81ea0513f9e9a4bcde935e7e32a. I think the problem is that the add-on is being disabled because `head_addons.js` starts by setting the app version to "1.0", while `ExtensionTestUtils.startAddonManager` sets it to "48.0". You can fix that by calling `yield promiseRestartManager("48.0")` instead of `startupManager`. > Why do you prefer `Object.getOwnPropertyDescriptor` over `obj.hasOwnProperty`? Don't they do essentially the same thing, i.e., check for the existence of a property on the object? Because `obj.hasOwnProperty` can be overwritten, and it tends to only be called on objects that you don't know a lot of details about the exact properties of. Although, I guess we're technically supposed to use `Reflect.getOwnPropertyDescriptor` rather than `Object.getOwnPropertyDescriptor` these days...

Comment on attachment 8770681 [details] Bug 1283116 - Implement chrome.management.getSelf, https://reviewboard.mozilla.org/r/64078/#review68848 > I think the problem is that the add-on is being disabled because `head_addons.js` starts by setting the app version to "1.0", while `ExtensionTestUtils.startAddonManager` sets it to "48.0". You can fix that by calling `yield promiseRestartManager("48.0")` instead of `startupManager`. (incidentally, mozreview linkification apparently sucks, so try to remember not to type a '.' right after URLs in the future...)

Comment on attachment 8770681 [details] Bug 1283116 - Implement chrome.management.getSelf, https://reviewboard.mozilla.org/r/64078/#review61356 > Because this is getSelf, I don't think we can test a system add-on, unless we can create and install one ourself. I'm still not sure about how to test a sideloaded add-on, or if we even need to. Having not heard anything to the contrary, I am going to drop this issue. > Again, unless we can test with a system add-on, I don't think we can test this. Having not heard anything to the contrary, I am going to drop this issue.

Comment on attachment 8770681 [details] Bug 1283116 - Implement chrome.management.getSelf, https://reviewboard.mozilla.org/r/64078/#review62696 > I'm not sure. As this is just a function that evaluates the installType, I figured it made sense to cover all possibilities. I suppose someone writing a system addon could want to use `getSelf` in some way, and in that case installType should probably return a correct value. Having not heard anything to the contrary, I am going to drop this issue.

Comment on attachment 8770681 [details] Bug 1283116 - Implement chrome.management.getSelf, https://reviewboard.mozilla.org/r/64078/#review68848 > Unless you're going to move the existing parts of that test back to toolkit/components/extensions like I asked, then yes, the patch needs to be amended to record the move. You can use `hg mv -A` for that. I did move the test for the schema back, so I assume that fixes this. > (incidentally, mozreview linkification apparently sucks, so try to remember not to type a '.' right after URLs in the future...) I tried using that, in a number of different places, and it didn't work. Changing the call to `createAppInfo` to `createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "48", "48");` did fix the problem, however.

Thanks for the review, Kris. The try looks good, so I'm going to request checkin of this. It does have your r+, but if you want to take a look at the latest changes I made to the tests feel free to do so. If any changes are required I'll remove the checkin request and address them.

Looks like autoland hit conflicts trying to rebase and land this. Can you please fix up the patch against autoland tip and request checkin again?

Ok, I've resolved the conflicts. Please try again. Thanks Ryan.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/d0ba9997681a Implement chrome.management.getSelf, r=kmag

Backed out for android xpcshell failures like https://treeherder.mozilla.org/logviewer.html#?job_id=2110300&repo=autoland https://hg.mozilla.org/integration/autoland/rev/1c5c2183b921

Not just android, here's a linux64 one: https://treeherder.mozilla.org/logviewer.html#?job_id=2114610&repo=autoland

This is odd. The failures seem to be about the fact that the test file is not found: `test_ext_management.js`, but it's only happening on some platforms. Any idea what might be causing this, Kris?

(In reply to Bob Silverberg [:bsilverberg] from comment #35) > This is odd. The failures seem to be about the fact that the test file is > not found: `test_ext_management.js`, but it's only happening on some > platforms. Any idea what might be causing this, Kris? Yes, it's a build system bug. You need to add or remove a whitespace line from xpcshell.ini

I made the change suggested by Kris and tried to push the commit to reviewboard, but it won't let me: "abort: reviewboard error: One or more fields had errors (HTTP 400, API Error 105); identifier: Parent review request is submitted or discarded", so instead I am just attaching a patch to the bug. Please let me know if there is a different preferred workflow in this case.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c45e5c5015e3 Implement chrome.management.getSelf. r=kmag

Backed out for missing test path for added test test_ext_management.js: https://hg.mozilla.org/integration/mozilla-inbound/rev/df91c4b74c3da08815d38daa50b72fc8c5590b6d Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=34113258&repo=mozilla-inbound 07:51:41 INFO - IOError: Strict mode enabled, test paths must exist. The following test(s) are missing: [ 07:51:41 INFO - "/builds/slave/test/build/tests/xpcshell/tests/toolkit/mozapps/extensions/test/xpcshell/test_ext_management.js", 07:51:41 INFO - "/builds/slave/test/build/tests/xpcshell/tests/toolkit/mozapps/extensions/test/xpcshell/test_ext_management.js" 07:51:41 INFO - ]

According to try this patch seems to address the test not found problem.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/52dcb49307a0 Implement chrome.management.getSelf. r=kmag

I've written some docs for the management API: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/management including getSelf(): https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/management/getSelf Please let me know if this looks OK.

Thanks Will. The general docs for the management API look good, as do the docs for management.getSelf.