Closed Bug 577048 Opened 9 years ago Closed 9 years ago

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

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla2.0b3
Tracking Status
blocking2.0 --- final+

People

(Reporter: whimboo, Assigned: mossop)

References

Details

(Whiteboard: [AddonsRewrite])

Attachments

(2 files, 1 obsolete file)

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
blocking2.0: ? → beta2+
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.
blocking2.0: beta2+ → final+
Attached file 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.
The same happens for add-ons which are not supported via the targetPlatform entry.
(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?
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.
So what are the steps to reproduce here?
Save it to the local disk and use "File | Open File" to install the extension.
I can confirm that the error is shown when installing via the web but not from local disk. Updating summary.
Summary: In certain situations we don't show a message when extensions can't be installed → In certain situations we don't show a message when extensions from the local disk can't be installed
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?
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.
(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.
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.
Assignee: nobody → dtownsend
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
Thanks Tony, I've got a rough idea of what might be causing this now.
Summary: In certain situations we don't show a message when extensions from the local disk can't be installed → Notifications about add-on installs and error aren't shown when loading from a blank tab
We also don't show notifications for about:config and eventual other about: pages (not tested yet)
Summary: Notifications about add-on installs and error aren't shown when loading from a blank tab → Notifications about add-on installs and error aren't shown when loading from a tab with no host (any about: page)
Attached patch patch rev 1 (obsolete) — Splinter Review
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.
Attachment #460742 - Flags: review?(gavin.sharp)
Status: NEW → ASSIGNED
(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?
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...
(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.
(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.
"because of a filesystem error" was what threw me off, but I can get over that.
Attached patch patch rev 2Splinter Review
Updated patch that doesn't try to use the host for the disabled case since that string doesn't actually require any formatting.
Attachment #460742 - Attachment is obsolete: true
Attachment #461255 - Flags: review?(gavin.sharp)
Attachment #460742 - Flags: review?(gavin.sharp)
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?
Attachment #461255 - Flags: review?(gavin.sharp) → review+
Fixed: http://hg.mozilla.org/mozilla-central/rev/2f1f78c2adf9

I separated out the items in the locale to make it clearer.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b3
Blocks: 581974
Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b5pre) Gecko/20100825 Minefield/4.0b5pre
Status: RESOLVED → VERIFIED
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.
You need to log in before you can comment on or make changes to this bug.