577048 - Notifications about add-on installs and error aren't shown when loading from a tab with no host (any about: page)

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Given by bug 407315 and bug 292854 we fail in showing up a message when an extension cannot be installed. Right now I have catched the following two situations:

* Invalid XML file (install.rdf)
* Invalid extension type, i.e. 245

Comment 1

[Dave Townsend [:mossop]]

I cannot reproduce this. In both the mentioned cases I see error messages. Can you attach an extension that does not produce an error message.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: test extension

(In reply to comment #0)
> * Invalid XML file (install.rdf)

That case throws an error in the error console. Would it be possible to expose that as a direct error message to the user?

Error: unclosed token
Source File: jar:file:///Volumes/data/acr.xpi!/install.rdf
Line: 1, Column: 1
Source Code:
<?xml version="1.0" encoding="UTF-8">

> * Invalid extension type, i.e. 245

See this attached extension, where I have simply changed the id to an unsupported value. We fail silently or skip the installation completely.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The same happens for add-ons which are not supported via the targetPlatform entry.

Comment 4

[Dave Townsend [:mossop]]

(In reply to comment #2)
> Created attachment 458183 [details]
> test extension
> 
> (In reply to comment #0)
> > * Invalid XML file (install.rdf)
> 
> That case throws an error in the error console. Would it be possible to expose
> that as a direct error message to the user?

It shows an error in the notification box for me.

> > * Invalid extension type, i.e. 245
> 
> See this attached extension, where I have simply changed the id to an
> unsupported value. We fail silently or skip the installation completely.

When I click on the attached extension I see the message "The add-on downloaded from bugzilla.mozilla.org could not be installed because it appears to be corrupt." Is this not what you see?

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Oh, I haven't tried to install the XPI from a website. For testing purposes I have downloaded all of my testing add-ons to disk. Eventually that's the reason. But even installing the XPI via this attachment fails on trunk, because of a "Error Downloading", whatever that means. It works in 3.6.

Comment 6

[Dave Townsend [:mossop]]

So what are the steps to reproduce here?

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Save it to the local disk and use "File | Open File" to install the extension.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I can confirm that the error is shown when installing via the web but not from local disk. Updating summary.

Comment 9

[Dave Townsend [:mossop]]

Using the method in comment 7 still gives me the error notification.

Do you see anything in the error console when this happens? What OS are you testing on?

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I'm running on OS X and well, there is another bug here:

Once you have tried to install an extension via bugzilla attachment and then trying again via local disk, it will always use the first extension. Even after a restart it mentions "download from Bugzilla". But that's probably another issue I should file as a new bug? For now just rename the xpi on your disk so we really use the downloaded file and not the one from Bugzilla.

Comment 11

[Dave Townsend [:mossop]]

(In reply to comment #10)
> I'm running on OS X and well, there is another bug here:
> 
> Once you have tried to install an extension via bugzilla attachment and then
> trying again via local disk, it will always use the first extension. Even after
> a restart it mentions "download from Bugzilla". But that's probably another
> issue I should file as a new bug? For now just rename the xpi on your disk so
> we really use the downloaded file and not the one from Bugzilla.

That is a different issue, but I'm confused how you are seeing it when you aren't seeing a notification for local installs.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ok, I tested again in a fresh profile and there I get this error notification. Looks like something in the profile suppresses the notification bar to appear. I will investigate tomorrow.

Comment 13

[Tony Chung [:tchung]]

I can reproduce the missing notification error with an incompatible extension.  It only happens if you open a new blank tab.  It wont reproduce if you replace the path in an existing URL tab

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b3pre) Gecko/20100721 Minefield/4.0b3pre

Repro:
1) open a new tab
2) enter in a path to an incompatible extension outside of AMO (eg. ftp://ftp.mozilla.org/pub/labs/weave/development/1.4.x/weave-1.4.2b1-dev.xpi)
3) Verify no notification occurs.  
4) now do the same, but replace the URL in an existing tab that already has a site open
5) Verify notification pops down saying extension is not compatibible with 4.0b3
6) Also verify the extension sits in the addons manager awaiting to be downloaded.

Expected:
- notification still occurs in an about:blank page when trying to download the incompatible extension

Comment 14

[Dave Townsend [:mossop]]

Thanks Tony, I've got a rough idea of what might be causing this now.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

We also don't show notifications for about:config and eventual other about: pages (not tested yet)

Comment 16

[Dave Townsend [:mossop]]

Attachment: patch rev 1

There are two problems here, first is falling back to the active tab's currentURI when there was no referring URI (for urls typed into the address bar, loaded from file - open, dragged into the window etc.), the second is assuming that all URLs have a host.

In the case of the blocked notification we know we always have a referer that has a host (otherwise it would not have been blocked).

In the other cases we should use the referring host if possible and if not fall back to the host of the URI that the add-on is being downloaded from. If we have no host at all then the add-on must be being installed locally in which case we need some better messages.

I've added tests for these cases as well as updating the existing cases with checks that the displayed message is correct.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #16)
> In the case of the blocked notification we know we always have a referer that
> has a host (otherwise it would not have been blocked).

Can you elaborate here? Couldn't you e.g. be installing from a file with xpinstall disabled?

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Assuming that URIs with no hosts represent local files is somewhat bothersome to me as well, since it's possible for other URI types to not have hosts (granted, they are edge cases). Not sure there's a better solution for that, though. I suppose you could add an explicit check for scheme=="file" and then have an even more generic message for the other cases...

Comment 19

[Dave Townsend [:mossop]]

(In reply to comment #17)
> (In reply to comment #16)
> > In the case of the blocked notification we know we always have a referer that
> > has a host (otherwise it would not have been blocked).
> 
> Can you elaborate here? Couldn't you e.g. be installing from a file with
> xpinstall disabled?

That is a good point, I think the old code was broken in this case too. Luckily there is a simple way out, the message for xpinstall.disabled doesn't actually require a hostname. For cases where the url isn't in the whitelist we are safe because any url without a host is automatically allowed.

Comment 20

[Dave Townsend [:mossop]]

(In reply to comment #18)
> Assuming that URIs with no hosts represent local files is somewhat bothersome
> to me as well, since it's possible for other URI types to not have hosts
> (granted, they are edge cases). Not sure there's a better solution for that,
> though. I suppose you could add an explicit check for scheme=="file" and then
> have an even more generic message for the other cases...

I think the existing messages are probably generic enough, but I could rename the keys if you think that is worthwhile.

Comment 21

[:Gavin Sharp [email: [e-mail]]]

"because of a filesystem error" was what threw me off, but I can get over that.

Comment 22

[Dave Townsend [:mossop]]

Attachment: patch rev 2

Updated patch that doesn't try to use the host for the disabled case since that string doesn't actually require any formatting.

Comment 23

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 461255 [details] [diff] [review]
patch rev 2

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>     case "addon-install-failed":
>       // TODO This isn't terribly ideal for the multiple failure case
>       installInfo.installs.forEach(function(aInstall) {
>-        var error = "addonError";
>+        // Some URIs (file:, about:) have no host and trying to get one will throw
>+        var host = null;
>+        if (installInfo.originatingURI) {
>+          try {
>+            host = installInfo.originatingURI.host;
>+          }
>+          catch (e) {
>+          }
>+        }
>+
>+        // Fallback to the host the add-on was downloaded from
>+        if (!host && aInstall.sourceURI) {
>+          try {
>+            host = aInstall.sourceURI.host;
>+          }
>+          catch (e) {
>+          }
>+        }

This could all just be:

var host = (installInfo.originatingURI instanceof Ci.nsIStandardURL) && installInfo.originatingURI.host;
if (!host)
  host = (aInstall.sourceURI instanceof Ci.nsIStandardURL) && aInstall.sourceURI.host;

>+        if (host)
>+          messageString = messageString.replace("#2", host);

>diff --git a/browser/locales/en-US/chrome/browser/browser.properties b/browser/locales/en-US/chrome/browser/browser.properties

> # LOCALIZATION NOTE (addonError-1, addonError-2, addonError-3, addonError-4, addonErrorIncompatible, addonErrorBlocklisted):
> # #1 is the add-on name, #2 is the host name, #3 is the application name
> # #4 is the application version

Perhaps worth noting here that localizers shouldn't add #2 to strings that don't have it, since it isn't replaced consistently?

Comment 24

[Dave Townsend [:mossop]]

Fixed: http://hg.mozilla.org/mozilla-central/rev/2f1f78c2adf9

I separated out the items in the locale to make it clearer.

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b5pre) Gecko/20100825 Minefield/4.0b5pre

Comment 26

[Alex Faaborg [:faaborg] (Firefox UX)]

The installation of an add-on from disk shouldn't be triggering a site level notification (doorhanger).  I've filed follow up bug 594564 to convert this to a traditional dialog box.