307770 - Can't install XPI with a click on the link with target="_blank"

Status: RESOLVED FIXED

(Firefox :: Security, defect, P2)

Description

[Vincent Lefevre]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4

I wanted to install <http://tmp.gary.elixant.com/beta/tabmix.xpi> from the above
URL. But each time I click on the link, a blank tab just appears (with no errors).

When I pasted the URL into the URL bar, the XPI could be installed without any
problem.

Reproducible: Always

Steps to Reproduce:
1. Open the above URL (the forum one).
2. Click on the link that is shown near the top.

Actual Results:  
A blank tab appears.

Expected Results:  
The XPI install dialog box should have appeared.

Comment 1

[Phil Ringnalda (:philor)]

JS console says:

Error: [Exception... "Component returned failure code: 0x80004005
(NS_ERROR_FAILURE) [nsIURI.host]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)" 
location: "JS frame :: chrome://browser/content/browser.js :: anonymous :: line
457"  data: no]
Source File: chrome://browser/content/browser.js
Line: 457

where 457 is

var host =
browser.docShell.QueryInterface(Components.interfaces.nsIWebNavigation).currentURI.host;

Comment 2

[Phil Ringnalda (:philor)]

Meant to resummarize: on a site that's not in the XPInstall whitelist, a link to
an XPI that opens a new window with target="_blank" correctly doesn't throw up
the install prompt, but also silently fails to throw up the blocked install
infobar, because it tries to get the host for the linking site from the current
docShell, which doesn't even have the incorrect host for the XPI, much less the
linking site.

Comment 3

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

Seems to me like the front end code needs to be able told which host triggered
the install without having to rely on the currentURI, which as shown is not
reliable (about:blank in the new window/tab case), since
nsInstallTrigger::HandleContent cancels the load and handles it itself.
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpinstall/src/nsInstallTrigger.cpp&rev=1.74&mark=180#178
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/browser.js&rev=1.500&mark=515#511

I thought that using the data param
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpinstall/src/nsInstallTrigger.cpp&rev=1.74&mark=262#249
might be a good way of doing that, since that data is currently ignored by the
front end (it appears to have been reserved for differentiating between themes
and extensions, but never fully implemented)
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/browser.js&rev=1.500&mark=519-521#511

I really don't know whether that's a sane idea or not. I'd imagine it passing
the host of either |uri| or |referringURI|, depending on which was used.
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpinstall/src/nsInstallTrigger.cpp&rev=1.74&mark=209,245#199

Comment 4

[Dave Townsend [:mossop]]

*** Bug 317931 has been marked as a duplicate of this bug. ***

Comment 5

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

Attachment: possible patch

This patch:
1) Makes nsInstallTrigger::HandleContent pass the host that was checked to the "xpinstall-install-blocked" observer.

2) Makes the observer use that passed in host instead of trying to get it from currentURI

3) Introduces an aData param to <browsermessage> to allow passing arbitrary data (in this case the host) to it's observer

This kind of leads to confusing UI in the case of <a href="xpi" target="_blank">. A new blank window appears with the popup bar saying "this site ...", which could be misleading, and while it then allows you to add the site, you have to switch back to the other window and click again for it to install. I think this is better than the current behavior, but it's not optimal. I can't think of any optimizations, though, since installTrigger can only kick in once the load is started and the content-type is determined.

Thoughts?

Comment 6

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

Oh, and as I mentioned in comment 3, the aData param was previously meant to distinguish themes from extensions, apparently, but that was never implemented and the observers basically ignored it (see the removed XXXben comment). I figured it could just be removed.

Comment 7

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

(this patch fixes the two test cases in bug 317931 comment 8)

Comment 8

[Asaf Romano (gone)]

Note there are three more notifiers of "xpinstall-install-blocked".

Comment 9

[Ronny Perinke]

This bug still exists in current Firefox 2.0 builds.

Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.1) Gecko/20060915 BonEcho/2.0


Fehler: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIURI.host]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://browser/content/browser.js :: anonymous :: line 528"  data: no]
Quelldatei: chrome://browser/content/browser.js
Zeile: 528

Comment 10

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

*** Bug 354359 has been marked as a duplicate of this bug. ***

Comment 11

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

Attachment: updated patch

(In reply to comment #8)
> Note there are three more notifiers of "xpinstall-install-blocked".

I don't think those are relevant - they're the "native" xpinstall functions used from install.js, I think.

Comment 12

[Asaf Romano (gone)]

Comment on attachment 274467 [details] [diff] [review]
updated patch

r=mano

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 274467 [details] [diff] [review]
updated patch

sr=dveditz

Comment 14

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

mozilla/browser/base/content/browser.js 	1.823
mozilla/xpinstall/src/nsInstallTrigger.cpp 	1.79

Comment 15

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

(In reply to comment #11)
> (In reply to comment #8)
> > Note there are three more notifiers of "xpinstall-install-blocked".
> 
> I don't think those are relevant - they're the "native" xpinstall functions
> used from install.js, I think.

Bah, turns out they are :( See bug 392885.