Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Provide api to set uninstall url

RESOLVED FIXED in Firefox 47

Status

()

Toolkit
WebExtensions: Untriaged
P2
major
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: erikvold, Assigned: aswan, Mentored)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

34 Branch
mozilla47
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
blocking-webextensions +

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: [runtime]triaged)

Attachments

(2 attachments, 3 obsolete attachments)

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
Priority: -- → P2
Blocks: 1161828
Duplicate of this bug: 1190684
Component: General → WebExtensions
Product: Add-on SDK → Toolkit
Version: unspecified → 34 Branch
Mentor: wmccloskey@mozilla.com

Updated

2 years ago
Assignee: nobody → amckay
Keywords: dev-doc-needed

Updated

2 years ago
Iteration: --- → 44.2 - Oct 19

Updated

2 years ago
Whiteboard: [runtime]

Updated

2 years ago
Blocks: 1214433

Updated

2 years ago
Iteration: 44.2 - Oct 19 → 44.3 - Nov 2

Updated

2 years ago
Flags: blocking-webextensions+
Duplicate of this bug: 1217893

Updated

2 years ago
Iteration: 44.3 - Nov 2 → 45.1 - Nov 16

Updated

2 years ago
Iteration: 45.1 - Nov 16 → 45.2 - Nov 30

Updated

2 years ago
Iteration: 45.2 - Nov 30 → 45.3 - Dec 14

Comment 3

2 years ago
Created attachment 8702645 [details] [diff] [review]
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.
Attachment #8702645 - Flags: feedback?

Updated

2 years ago
Attachment #8702645 - Flags: feedback? → feedback?(wmccloskey)
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.
Attachment #8702645 - Flags: feedback?(wmccloskey) → feedback+

Updated

2 years ago
Assignee: amckay → aswan

Updated

2 years ago
Iteration: 45.3 - Dec 14 → 47.1 - Feb 8
Blocks: 1235639
(Assignee)

Updated

2 years ago
Depends on: 612168

Updated

2 years ago
Whiteboard: [runtime] → [runtime]triaged
(Assignee)

Comment 5

2 years ago
Created attachment 8717213 [details] [diff] [review]
Implement browser.runtime.setUninstallURL()
Attachment #8717213 - Flags: review?(kmaglione+bmo)
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("test_uinstallurl@tests.mozilla.org", {
> +  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.
Attachment #8717213 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 7

2 years ago
(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

2 years ago
(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?
(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
(Assignee)

Comment 10

2 years ago
Created attachment 8717552 [details] [diff] [review]
Implement browser.runtime.setUninstallURL()
Attachment #8717552 - Flags: review?(kmaglione+bmo)
(Assignee)

Updated

2 years ago
Attachment #8717213 - Attachment is obsolete: true
(Assignee)

Comment 11

2 years ago
Created attachment 8717555 [details] [diff] [review]
Implement browser.runtime.setUninstallURL()
Attachment #8717555 - Flags: review?(kmaglione+bmo)
(Assignee)

Updated

2 years ago
Attachment #8717552 - Attachment is obsolete: true
Attachment #8717552 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 12

2 years ago
Alright, comments addressed and more tests added, I think this should be good to go...
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.
Attachment #8717555 - Flags: review?(kmaglione+bmo) → review+
(Assignee)

Comment 14

2 years ago
(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.
(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.
(Assignee)

Comment 16

2 years ago
Created attachment 8717597 [details] [diff] [review]
Implement browser.runtime.setUninstallURL()
(Assignee)

Updated

2 years ago
Attachment #8717555 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8717597 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 17

2 years ago
moved to P2:major as part of unblocking ghostery - 15M users for e10s compat.  :)
Severity: normal → major

Comment 18

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/cb08d55f4e8b
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cb08d55f4e8b
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.