1156826 - Provide api to set uninstall url

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P2)

Description

[Erik Vold [:erikvold] (please needinfo? me)]

It should be possible to set a url to be opened when an add-on is uninstalled as google chrome does.  I would like there to be a user pref created which would allow users to disable this however.

https://developer.chrome.com/extensions/runtime#method-setUninstallURL

Comment 3

[Andy McKay]

Attachment: chrome.runtime.unInstallURL.patch

This seems a pretty simple way to do this, but I note there's `onShutdown` on the object. But it wasn't clear to me when shutdown gets called, I think that's get called on more occasions than `uninstall`.

Secondly is there a better way to throw an error, the error in the browser console isn't great when you trigger one of the error conditions.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8702645 [details] [diff] [review]
chrome.runtime.unInstallURL.patch

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

This looks pretty good. Besides the review comments, a few more things:
* If the URL is empty, we're should set extension.uninstallURL to null.
* We need to call the callback they provide in case of success.
* I wonder if Chrome saves the uninstall URL so that the extension only needs to set it when it's installed. Probably not worth worrying about for now.

::: toolkit/components/extensions/Extension.jsm
@@ +1032,5 @@
>      }
>  
> +    // Call an uninstall URL if set by chrome.runtime.setUninstallURL.
> +    if (this.uninstallURL) {
> +      let wm = Cc["@mozilla.org/appshell/window-mediator;1"].getService(Components.interfaces.nsIWindowMediator);

You can use Services.wm.getMostRecentWindow.

::: toolkit/components/extensions/ext-runtime.js
@@ +100,5 @@
>          let info = {os, arch};
>          runSafe(context, callback, info);
>        },
> +
> +      setUninstallURL: function(url) {

You'll need to remove "unsupported": true in runtime.json for setUninstallURL.

@@ +101,5 @@
>          runSafe(context, callback, info);
>        },
> +
> +      setUninstallURL: function(url) {
> +        if (url.length > 255) {

No need to check this. The schema code will do it for you.

@@ +104,5 @@
> +      setUninstallURL: function(url) {
> +        if (url.length > 255) {
> +          throw win.error("setUninstallURL maximum length is 255 chars.");
> +        }
> +        let uri = NetUtil.newURI(url).QueryInterface(Ci.nsIURL);

Don't need to QI to nsIURL. You'll get an nsIURI, which has .scheme, and that's all you need.

@@ +106,5 @@
> +          throw win.error("setUninstallURL maximum length is 255 chars.");
> +        }
> +        let uri = NetUtil.newURI(url).QueryInterface(Ci.nsIURL);
> +        if (uri.scheme != "http" && uri.scheme != "https") {
> +          throw win.error("setUninstallURL must have the scheme http or https.");

I don't think you have a |win| here. Maybe just add a note to fix this when bug 1190680 is fixed.

Comment 5

[Andrew Swan [:aswan]]

Attachment: Implement browser.runtime.setUninstallURL()

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 8717213 [details] [diff] [review]
Implement browser.runtime.setUninstallURL()

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

This looks pretty good. Just some minor changes and I think it's good to land.

::: browser/components/extensions/ext-runtime-uninstall.js
@@ +1,5 @@
> +"use strict";
> +
> +/* eslint-disable mozilla/balanced-listeners */
> +extensions.on("uninstall", (msg, url) => {
> +  Services.wm.getMostRecentWindow("navigator:browser").gBrowser.addTab(url);

This is a bit nitpicky, but if the add-on manager is loaded in the current tab, we should really probably pass `{ relatedToCurrent: true }` as a second argument to addTab, so the tab opens just after the about:addons tab.

::: browser/components/extensions/test/browser/browser_ext_runtime_setUninstallURL.js
@@ +17,5 @@
> +let xpi = Extension.generateXPI("[e-mail]", {
> +  background: "(" + backgroundScript.toString() + ")()",
> +});
> +
> +let fileURI = Services.io.newFileURI(xpi);

Most of the above should be inside the task. Anything with side-effects (like the generateXPI call) definitely needs to be inside the task.

@@ +58,5 @@
> +  let uninstalledTab = yield uninstallPromise;
> +  isnot(uninstalledTab, null, "opened tab with uninstall url");
> +  BrowserTestUtils.removeTab(uninstalledTab);
> +
> +  cleanupXPI();

It would be better to register a cleanup function as soon as you create the XPI:

SimpleTest.registerCleanupFunction(cleanupXPI);

::: browser/components/extensions/test/browser/file_uninstall_load.html
@@ +1,1 @@
> +<!DOCTYPE HTML>

I don't think we need to create files for these. Opening something like 'http://example.com/addon_loaded' and 'http://example.com/addon_uninstalled' should do.

::: browser/components/nsBrowserGlue.js
@@ +550,5 @@
>      ExtensionManagement.registerScript("chrome://browser/content/ext-utils.js");
>      ExtensionManagement.registerScript("chrome://browser/content/ext-browserAction.js");
>      ExtensionManagement.registerScript("chrome://browser/content/ext-pageAction.js");
>      ExtensionManagement.registerScript("chrome://browser/content/ext-contextMenus.js");
> +    ExtensionManagement.registerScript("chrome://browser/content/ext-runtime-uninstall.js");

I don't think we need a separate script just for handling uninstalls. Maybe `ext-desktop-runtime.js`, so it can be used for other desktop-specific runtime code?

::: toolkit/components/extensions/Extension.jsm
@@ +98,5 @@
>  const LOGGER_ID_BASE = "addons.webextension.";
>  
>  var scriptScope = this;
>  
> +var ExtensionPage, UninstallObserver, GlobalManager;

There's no need to predeclare UninstallObserver here. The others are only up here because they're referenced earlier than their definition.

@@ +320,5 @@
> +
> +  onUninstalling: function(addon) {
> +    let extension = GlobalManager.extensionMap.get(addon.id);
> +    if (extension && extension.uninstallURL) {
> +      Management.emit("uninstall", extension.uninstallURL);

I think we should just pass the extension as the argument here, and let the listener check for the uninstallURL.

::: toolkit/components/extensions/ext-runtime.js
@@ +86,5 @@
>          return Promise.resolve(info);
>        },
> +
> +      setUninstallURL: function(url) {
> +        let uri = NetUtil.newURI(url).QueryInterface(Ci.nsIURL);

No need for the `.QueryInterface(Ci.nsIURL)`. `.scheme` is part of nsIURI.

Note that newURI might throw if the URL is invalid, so we should probably catch that error and do something sensible with it.

Comment 7

[Andrew Swan [:aswan]]

(In reply to Kris Maglione [:kmag] from comment #6)
> ::: browser/components/extensions/ext-runtime-uninstall.js
> @@ +1,5 @@
> > +"use strict";
> > +
> > +/* eslint-disable mozilla/balanced-listeners */
> > +extensions.on("uninstall", (msg, url) => {
> > +  Services.wm.getMostRecentWindow("navigator:browser").gBrowser.addTab(url);
> 
> This is a bit nitpicky, but if the add-on manager is loaded in the current
> tab, we should really probably pass `{ relatedToCurrent: true }` as a second
> argument to addTab, so the tab opens just after the about:addons tab.

Alright, I think the only way to get here is by taking an action in the add-on manager, so I'll just pass that option unconditionally..

> ::: browser/components/extensions/test/browser/file_uninstall_load.html
> @@ +1,1 @@
> > +<!DOCTYPE HTML>
> 
> I don't think we need to create files for these. Opening something like
> 'http://example.com/addon_loaded' and 'http://example.com/addon_uninstalled'
> should do.

Okay, I was shooting to make the test standalone so it can run without external connectivity, sounds like that's overdoing it and this pattern (using example.com) is used elsewhere so I should just stick with it?

Thanks for the comments, I'll fix everything else up and add more unit tests for bad parameters to setUninstallURL()

Comment 8

[Andy McKay]

(In reply to Andrew Swan [:aswan] from comment #7)
> Okay, I was shooting to make the test standalone so it can run without
> external connectivity, sounds like that's overdoing it and this pattern
> (using example.com) is used elsewhere so I should just stick with it?

I'm not keen that we are using example.com, I just put it in my tests because I saw it in other tests too (and attempts to use about:* pages failed). I'd like to see all our tests run with external connectivity.

Maybe file a bug for removing all that and setting a new standard for ourselves?

Comment 9

[Kris Maglione [:kmag]]

(In reply to Andrew Swan [:aswan] from comment #7)
> Alright, I think the only way to get here is by taking an action in the
> add-on manager, so I'll just pass that option unconditionally..

You could do it with something like:

  let {gBrowser} = Services.wm.getMostRecentWindow("navigator:browser");
  if (gBrowser.selectedBrowser.currentURI.spec == "about:addons") {
    ...
  }

But passing it unconditionally sounds fine too.

> Okay, I was shooting to make the test standalone so it can run without
> external connectivity, sounds like that's overdoing it and this pattern
> (using example.com) is used elsewhere so I should just stick with it?

As it happens, mochitests aren't allowed any external connectivity.
example.com is one of the hosts that we redirect[1] to the mochitest server.
The URLs will return 404, but that doesn't matter for your use case.

[1] https://dxr.mozilla.org/mozilla-central/source/build/pgo/server-locations.txt

Comment 10

[Andrew Swan [:aswan]]

Attachment: Implement browser.runtime.setUninstallURL()

Comment 11

[Andrew Swan [:aswan]]

Attachment: Implement browser.runtime.setUninstallURL()

Comment 12

[Andrew Swan [:aswan]]

Alright, comments addressed and more tests added, I think this should be good to go...

Comment 13

[Kris Maglione [:kmag]]

Comment on attachment 8717555 [details] [diff] [review]
Implement browser.runtime.setUninstallURL()

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

This is great. Just a few minor nits to fix. Feel free to set r+ yourself on the updated patch when you fix them.

Thanks!

::: browser/components/extensions/test/browser/browser_ext_runtime_setUninstallURL.js
@@ +33,5 @@
> +
> +  // A WebExtension is started asynchronously, we have our test extension
> +  // open a new tab to signal that the background script has executed.
> +  let loadTab = yield loadPromise;
> +  BrowserTestUtils.removeTab(loadTab);

This should be `yield BrowserTestUtils.removeTab(loadTab);`

In practice, removeTab is always sync, but that might not always be true, so we should wait for it to notify us that it's done.

@@ +47,5 @@
> +        .then(() => {
> +          browser.test.notifyFail("setUninstallURL should have failed with bad url");
> +        })
> +        .catch(error => {
> +          browser.test.assertTrue(error.message.match(/MALFORMED_URI/), "error message indicates malformed url");

I generally prefer /MALFORMED_URI/.test(error.message), since it doesn't rely on the type of the property (and doesn't create an unnecessary match object).

@@ +50,5 @@
> +        .catch(error => {
> +          browser.test.assertTrue(error.message.match(/MALFORMED_URI/), "error message indicates malformed url");
> +        }),
> +
> +      browser.runtime.setUninstallURL("file:/etc/passwd")

This isn't technically a valid file url. "file:///etc/passwd" would be fine.

@@ +88,5 @@
> +  addon.uninstall(true);
> +  info("uninstalled");
> +
> +  // no need to explicitly check for the absence of a new tab,
> +  // BrowserTestUtils will complain if one is opened.

It only complains if a tab is left open at the end of the test file, so this could technically affect the next task if it wound up opening a tab.

Probably not worth worrying about, but might be good to note that in the comment.

::: toolkit/components/extensions/ext-runtime.js
@@ +94,5 @@
> +        let uri, errmsg;
> +        try {
> +          uri = NetUtil.newURI(url);
> +        } catch (e) {
> +          errmsg = e.message;

Let's just return the rejected promise here, and likewise for the check below. It's technically possible `e.message` could be empty, so that makes it a bit easier to avoid corner cases.

In this case, it might be better to just use a custom error message rather than the one newURI throws (since those can be cryptic, and often contain source locations).

return Promise.reject({ message: `Invalid URL: ${JSON.stringify(url)}` })

@@ +102,5 @@
> +          errmsg = "setUninstallURL: url must have the scheme http or https";
> +        }
> +
> +        if (errmsg) {
> +          return Promise.reject(new context.cloneScope.Error(errmsg));

You can just use `Promise.reject({ message; errmsg })` and it'll automatically be transformed into an error object for the content scope.

Comment 14

[Andrew Swan [:aswan]]

(In reply to Kris Maglione [:kmag] from comment #13)
> :::
> browser/components/extensions/test/browser/
> browser_ext_runtime_setUninstallURL.js
> @@ +50,5 @@
> > +        .catch(error => {
> > +          browser.test.assertTrue(error.message.match(/MALFORMED_URI/), "error message indicates malformed url");
> > +        }),
> > +
> > +      browser.runtime.setUninstallURL("file:/etc/passwd")
> 
> This isn't technically a valid file url. "file:///etc/passwd" would be fine.

Hm, but NetUtil.newURI() accepted it?

Thanks for all the other comments (especially the one about Promise.reject() automatically building appropriate error objects, that's really helpful to know.

Comment 15

[Kris Maglione [:kmag]]

(In reply to Andrew Swan [:aswan] from comment #14)
> > This isn't technically a valid file url. "file:///etc/passwd" would be fine.
> 
> Hm, but NetUtil.newURI() accepted it?

It normalizes it to "file:///etc/passwd", but we shouldn't rely on that.

Comment 16

[Andrew Swan [:aswan]]

Attachment: Implement browser.runtime.setUninstallURL()

Comment 17

[:shell escalante]

moved to P2:major as part of unblocking ghostery - 15M users for e10s compat.  :)

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/cb08d55f4e8b

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cb08d55f4e8b

Comment 20

[Will Bamberg [:wbamberg]]

This has a docs page already, so I think all that's needed is to update the browser compat table, which is done: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/runtime/setUninstallURL.

I'm marking this as dev-doc-complete, but let me know if anything else is needed.