252830 - Blocked XPI install should have allow once feature

Status: VERIFIED FIXED

(Core Graveyard :: Installer: XPInstall Engine, enhancement, P2)

Description

[zenervation]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040722 Firefox/0.9.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040722 Firefox/0.9.0

When a popup is blocked, a bar appears. One of the options is to display the
popup that was blocked. Similiar functionality should be implemented into the
blocked xpi install bar - there should be an option to allow the install once.

Reproducible: Always
Steps to Reproduce:
1. Visit a site which is not on the whitelist, and attempt to install an xpi
file from it (such as http://extensionroom.mozdev.org/more-info/webdeveloper)
2. The information bar will appear which tells you that the software install has
been blocked

Actual Results:  
The only option you have is to permanently allow that site to install software.

Expected Results:  
You should be able to give permission for software to be installed from that
site once.

Comment 1

[logan]

I understand what you are saying, but wouldn't that kindof undermine the purpose
of the white-list? Be pretty much back to where we were before, prompting for
the installation of an xpi.

Comment 2

[zenervation]

What happens now:
-Try to install software from a non-whitelisted site
-Bar pops up
-You have to go into the menu, whitelist it, and then reclick the xpi install link
-You then (should) remove that site from the whitelist.

It just seems counter-intuitive, especially to people who aren't familiar with
it.   

Comment 3

[logan]

I don't know what anyone's thoughts are on this matter, but surely there's some
validity to this... It's not much fun to jump through all these hoops, just to
install one extension or theme.

Hardware & OS -> All, marking NEW

Comment 4

[Michael Lefevre]

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

Comment 5

[Noah (NoahSUMO/SolidSnake) [:Noah]]

WORKAROUND: Just go to About:config, type "whitelist", you should see
"xpinstall.whitelist.required" > Set to "False".

NEVER BUGGED AGAIN.

Should be marked fixed. Maybe consequently there should be an option [check-box]
to turn on and off a "Require whitelist" setting somewhere in the browser
options. Maybe underneath Web Features > near "Allow software to install".

I would consider renaming this bug to that and maybe you'll see this option.

Comment 6

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

The point of this bug is that you do want to be warned about untrusted
extensions, but with the possibility of making a one time judgment call for
certain ones if necessary.

Comment 7

[Andy Strain]

(In reply to comment #6)
> The point of this bug is that you do want to be warned about untrusted
> extensions, but with the possibility of making a one time judgment call for
> certain ones if necessary.

I agree, disabling the whitelist isn't a valid option. For example, my whitelist
has www.mozilla.org and update.mozilla.org in it. However, there are times when
I wish to install xpi's from another site.

As for "back to where we were before, prompting for the installation of an xpi",
it's already like that. When I go to install an xpi not on www.mozilla.org or
update.mozilla.org, I get a bar up at the top explaining that "To protect your
computer, Firefox prevented this site (jedbrown.net) from installing software on
your computer." The only option I'm given is to "Edit Options..." and the only
thing that does is allow me to add jedbrown.net to my whitelist, which isn't
what I want to do. I just want to install the bbcode.xpi.

To install the bbcode.xpi I have to add jedbrown.net to my whitelist, install
the bbcode.xpi, and them remove jedbrown.net from my whitelist. Also, going into
"about:config", turning off the whitelist feature, installing bbcode.xpi, and
then going back into "about:config" and turning the whitelist feature back on is
just as bad.

Please don't read into this that I don't like the whitelist, I love it. I like
the fact that Firefox is preventing sites from not installing stuff on my
machine. Non-whitelist xpi's should continue to be blocked, but I should be able
to override that, on a case by case basis, without having to turn the whitelist
feature off and without having to add the site to my whitelist. I don't mind
being bugged and am perfectly okay with the way it is now, save that fact that I
have to go through such hoops to get the xpi installed.

Comment 8

[Justin Kerk]

As a workaround, you can save the xpi to disk and open it from there.

Comment 9

[Seth Kinast]

There's really no workaround needed as such; this is not a loss of function but
rather a RFE.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Attachment: Mockup of new install warning bar

(In reply to comment #1)
> I understand what you are saying, but wouldn't that kindof undermine the
purpose
> of the white-list? Be pretty much back to where we were before, prompting for

> the installation of an xpi.

It's similar, but there's a significant difference. Previously, clicking on an
XPI installation link would result in a dialog packed with a lot of
information:

1)Software Installation

  1)A web site is requesting permission to install the following item:

  [2)ICON]   3)xpiname.xpi		      4)Signed/Unsigned
  [  ICON]   5)from: http://somewhere.someurl.someplace
  ..................................................................

  6)Malicious software can damage your computer or violate your 
  privacy.

  7)You should only install software from sources that 
  you trust.

					[ Install Now ] [ Cancel ]

I added the numbers to show that there are 7 seperate pieces of information
that someone must digest with this dialog. 1) That it's a software installation
request, 2) an icon, 3) the name of the bit of software, 4) whether it's signed
or not, 5) where it's coming from, 6) a warning about malicious software, and
7)  some advice. The dialog gave people information overload, and they just
clicked "Install Now" without thinking in order to get it out of their face,
enabling the potential security loophole of a page auto-loading an XPI install
and a user clicking "OK" to get the thing out of their face.

What this bug proposes is that we replace this with a single warning popup bar
across the top that reads something like:

[ICON] This site (ieview.mozdev.org) is attempting to install software on your
computer.    [ Allow ] [ Block ] [ Edit Options ...]

Two pieces of information (what's happened & who's doing it) and three very
clear options, the first two of which are non-persistent, and the third of
which allows someone to change overall settings. I've attached a mockup.

Comment 11

[zenervation]

(In reply to comment #10)

A system like that would be good, however if we went with that option, I believe
there would be a need for a 4th button: 'details'. The icon, what it's called,
signed-unsigned etc.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Mm. Good point. Although that information does get presented when you click
"Allow" and the "Software Installation" dialog pops up.

Comment 13

[zenervation]

I believe the flow should go like this:

Info bar -> This website (www.domain.com) is attempting to install
'filename.xpi'.  [Allow] [Details] [Block] [Options]

Click details -> Information window, has allow or block.

Click allow on info bar or detail window -> opens extension manager, installs
immediately. Info bar disappears.

Click block on info bar or detail window -> Info bar changes to "Installation
has been blocked. [Options] [X]

Comment 14

[Justin Kerk]

Yeah, specifying the filename is better. "trying to install software" is really
vague and doesn't give a clue that we mean XPI software.

Comment 15

[Seth Kinast]

With regards to comment #13:

There doesn't really need to be a block button; the install has already been 
blocked. The user can just do nothing if they do not wish the XPI to install.

I believe that [Allow] [Details...] [X] would be enough for the information 
bar.

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #15)

> I believe that [Allow] [Details...] [X] would be enough for the information
> bar.

You know, you're totally right. The [X] and the [Block] buttons would be pretty
much functionally equivalent. It leads to the somewhat interesting question of
what it means, from the user's perspective, to close a warning bar, but I think
its reasonable to conclude that people would expect it to mean "Warning, I have
read you and now I dismiss you!"

So to be clear, we're now saying the flow goes:

1. A XPI link is triggered (either explicit or automatic link)
2. Warning bar appears:

(ICON) Firefox has prevented URL from installing XPINAME on your system [Allow]
[Edit Options...] [X]

3. [Allow] -> installs XPI & dismisses warning
   [Edit Options ...] -> opens software installation whitelist panel
   [X]     -> dismiss warning

(I switched the string so that it makes mention of the fact that if the user
does nothing (or dismisses the warning) then the default action is to block the
installation.)

Comment 17

[zenervation]

I think allow should go to the popup we had before, which shows the
signed/unsigned information, and other information which is available.

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Agreed ... I was assuming (I'm still somewhat new and haven't walked the
codebase yet) that there was no way to install an XPI *without* hitting that
"Software Installation" dialog. So yes, I was picturing that hitting [Allow]
brings you to the dialog (reproduced in comment 10) with the additional
information and the advice, etc.

Comment 19

[Daniel Veditz [:dveditz]]

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

Comment 20

[Daniel Veditz [:dveditz]]

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

Comment 21

[Daniel Veditz [:dveditz]]

(In reply to comment #10)
> > pretty much back to where we were before, prompting for the installation
> 
> It's similar, but there's a significant difference. Previously, clicking on an
> XPI installation link would result in a dialog packed with a lot of
> information:

The more significant difference is that the original dialog was MODAL, the info
bar is not. There were, in fact, sites that used the modal-ness of the dialog to
coerce people into installing software or else shut down their browser to
escape. Not nice.

This RFE to allow a one-time passthrough of a request is how I always wanted the
infobar to work, but the 80% solution we have now was a lot easier to code. To
allow the install to go through the "xpinstall-install-blocked" notification we
send will have to contain enough information to trigger the install. In a simple
case we could just pass the URL of a single install and launch it using that.
But handling InstallTrigger.install() in general is much harder.

> [ICON] This site (ieview.mozdev.org) is attempting to install software on your
> computer.    [ Allow ] [ Block ] [ Edit Options ...]

Currently the infobar can lie: if the install is actually triggered by a frame
from another site you can end up whitelisting the wrong site and still not
getting your install to work. It's telling the truth to the extent that the
top-level site is responsible for any content it has chosen to frame, but that
doesn't necessarily help the whitelisting. The allow-once option, if we pass all
the required info, would be a workaround for this situation as well.

Comment 22

[Lincoln Ramsay]

Pardon my ignorance, but is it really a problem to just leave the whitelist
disabled (since a dialog pops up before the software is installed anyway)?

I don't mind having to click yes several times:

site foo.com is installing software bar... [Details] [Edit Options] [X]

Then details tells me about the software (what information is available) and
lets me actually install it.

This probably counts as a separate bug but the Edit Options dialog should let me
blacklist a site (so I don't even see the infobar).

Comment 23

[Shaun Braley]

(In reply to comment #22)
> Pardon my ignorance, but is it really a problem to just leave the whitelist
> disabled (since a dialog pops up before the software is installed anyway)?
> 
> I don't mind having to click yes several times:
> 
> site foo.com is installing software bar... [Details] [Edit Options] [X]
> 
> Then details tells me about the software (what information is available) and
> lets me actually install it.
> 
> This probably counts as a separate bug but the Edit Options dialog should let me
> blacklist a site (so I don't even see the infobar).

Some sites are persistent where if you decline to install it keeps trying. By
blocking it entirely this problem is eliminated.

Comment 24

[Phil Ringnalda (:philor)]

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

Comment 25

[Nickolay_Ponomarev]

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

Comment 26

[Doron Rosenberg (IBM)]

It might be worth to change how <browsermessage/> works to allow any number of
buttons - you pass in an array of labels rather than on label as is the case
now, and the callback method is told which one was clicked on.

Comment 27

[O. Atsushi (Torisugari)]

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

Comment 28

[Nickolay_Ponomarev]

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

Comment 29

[djpohly]

Wouldn't a bar such as this recreate the "race" conditions which we were trying
to avoid in XPI installation (a la bug 162020)?  That is, if I knew where the
Allow button would be, how easily could I trick a user into clicking it?

Comment 30

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #29)
> Wouldn't a bar such as this recreate the "race" conditions which we were trying
> to avoid in XPI installation (a la bug 162020)?  That is, if I knew where the

Sure, but wouldn't a similar solution to bug 162020 work?

Comment 31

[orion]

(In reply to comment #29)
> Wouldn't a bar such as this recreate the "race" conditions which we were trying
> to avoid in XPI installation (a la bug 162020)?  That is, if I knew where the
> Allow button would be, how easily could I trick a user into clicking it?

Why does it need to be a popup?  Do like *shock* IE does, allow the user to
right click on the "blocked" message and have an "allow once" option.

Comment 32

[Jeff Chien]

(In reply to comment #31)
I thought that the fact that you had to wait at least two seconds before
installing an XPI avoids the whole "race" condition problem.

> (In reply to comment #29)
> > Wouldn't a bar such as this recreate the "race" conditions which we were trying
> > to avoid in XPI installation (a la bug 162020)?  That is, if I knew where the
> > Allow button would be, how easily could I trick a user into clicking it?
> 
> Why does it need to be a popup?  Do like *shock* IE does, allow the user to
> right click on the "blocked" message and have an "allow once" option.

Comment 33

[djpohly]

Whoops... my apologies for the confusion.  I had breezed right over comments 17
and 18, which said and verified the same thing in a different way.  I had
mentally skipped the XPInstall dialog.

Speaking of which, this might fit better in the XPInstall component.

Comment 34

[Justin Kerk]

Shouldn't this be Trunk, not 1.7 Branch?

Comment 35

[djpohly]

Oh yeah... that changes along with the product/component.  -> Trunk

Comment 36

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

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

Comment 37

[Matthijs Wensveen]

For a consistent GUI, I would like to see the "blocked install" bar behave
almost exactly like the "blocked popup bar". Right click it for more options and
select either add to whitelist or allow once.

Comment 38

[Doron Rosenberg (IBM)]

(In reply to comment #37)
> For a consistent GUI, I would like to see the "blocked install" bar behave
> almost exactly like the "blocked popup bar". Right click it for more options and
> select either add to whitelist or allow once.

No, because that is 1) unaccessible and 2) illogical.  A button makes more sense
and is more usable as well.

Comment 39

[Aaron Slunt]

This is something that is very convienent.

Comment 40

[Matthijs Wensveen]

Ok, I agree with you Doron. But I still feel those gui's should be consistent.
Maybe when this is decided upon I will file a bug about the popup blocking gui,
if there isn't one already.

Comment 41

[Phil Ringnalda (:philor)]

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

Comment 42

[Phil Ringnalda (:philor)]

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

Comment 43

[Ray Booysen]

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

Comment 44

[Thomas K. (:tom)]

Attachment: Mockery of 'allow once' window for comment 29

(In reply to comment #29)
> Wouldn't a bar such as this recreate the "race" conditions which we were trying
> to avoid in XPI installation (a la bug 162020)?  That is, if I knew where the
> Allow button would be, how easily could I trick a user into clicking it?

I agree, I mean I could make an image similar to this one instructing the user to click 'yes' when the window pops up.  Heck, he might even just be typing along while a window pops up and accidentally accepts it when he hits space (how often has that happened to you?  it has happened to me a few times).  That's why I liked the timed entry idea, it'd force a user to jump through a few hoops, and prevent the entry from being permanent.  From my experience with users, the more hoops there are, the more serious something seems to be.

Comment 45

[Ryan Flint [:rflint] (ping via IRC for reviews)]

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

Comment 46

[Daniel Veditz [:dveditz]]

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

Comment 47

[Fox2]

Attachment: Alert mockup for comment #47 (Foxxen2)

Comment 48

[Fox2]

I did some thinking and I believe I have a simple solution to both this thread, and to most of the problems it addresses. It seems logical, simple, and very effective, enough that other software might choose to mirror some aspects of the approach. 

Proposed procedure:

The alert has check & install, deny, always deny options, and an "always trust" link to the whitelist under "options-->security". If the extension is approved by the user (this once) or whitelisted, it's downloaded, and then its hash is looked up on a Moz list of "known XPI's" so the user has some kind of authoritative confirmation the extension is bona fide and its status (if known), prior to completing the install.

Prerequisites:
1. Whitelist capability in FF for trusted directories.
2. Blacklist capability in FF for blocked links or directories.
2. A FF/Moz site containing a central register/database of "known" XPI's: hashes of XPIs, their title, typical URL, status and text description. Something that can be fed a hash of an XPI file, and respond with details of the extension it represents.
..... The "status" might be very simple broad categories such as: 1/ bona fide extension, 2/ considered expert only due to early release, conflict or stability issues, 3/ ad/spyware issues, 4/ unknown/unchecked. The text description can be edited.


Process:

1. User clicks an XPI link locally or on the web.
2. If the URL is a subdirectory of an item on the white list, skip the alert and go to step 4. Conversely, if it's on the blacklist pop up a small alert saying it was blocked, and if it is required it can be allowed by editing the list of blocked extensions: "link to options-->security-->locations allowed to install software".
3a. if the URL is not a subdirectory of an item on the whitelist, display the attached alert.
3b. The alert does NOT have a tab index for "check and install" or "trust"; the default is "Deny". (This takes care of the "tabbed and hit space by accident" issue, although users lacking pointing devices will not be able to install extensions - 1/ not a major problem, 2/ they can select to trust the location in "tools-->options-->security-->locations allowed to install software")
3c. The three blue hyperlinks link to Extension Manager, "Help-->extension security" and "tools-->options-->security-->locations allowed to install software".

4. Whether the location was whitelisted, or the user chose "check and install", we have got permission to proceed. We are going to try and download the extension.

5. Download the extension. No further permission is sought.
6. Hash the downloaded extension, and look it up on the database. If known we get back details, if its unknown then an entry is added by the database tagged as "status=unknown/new" for attention. This is very fast, a few bytes each way only.
7. Finally, pop up the usual "install?" box to the user. Except with this procedure, we can now tell the user for sure, the internal name, filename, description, and its status and description on FF/Moz database. The user can THEN with considerable certainty select "install" or "cancel" for safety. 
7a. Cancel should delete the file, just to be safe; install should save a copy before unpacking, in case of future need.

IMPACT ON USER:
1. The alert allows the "allow once" etc options sought, but also a more exact (directory) whitelist. Prevents vulnerability to same-domain-different-directory alert bypassing.
2. The "click install...alert...reclick install" sequence is killed.
3. The sequence is more logical -- first an alert (which can be bypassed) to trust the site enough to download the extension, and then look approval for install.
4. Users at very little cost or effort, get an accurate description of the extension checked to Moz's information, prior to install, which will probably annihilate most attempts to put malware in extensions, or distriibute malware'd versions of popular extensions, and will make inroads on helping distinguish "known and bona fide" extensions from extensions specifically known to have early release, conflict, stability or other issues, or extensions not known to the Moz team.

IMPACT ON CODING/SYSTEM:
1. Relatively simple I would have thought. 
2. A database of extension hashes and some kind of status as described is quick and easy, and code to check up a downloaded extension on it, is pretty simple.


Sample warning bar attached.

Hope this is not too forward a suggestion. Seems to solve all visible problems of extension install security and user interaction, including the two big vulnerabilities visible (cross-site alert bypass and spoofed or malware'd extensions), at very little cost. Go gently on me if it has any problems :)

Comment 49

[Fox2]

(And I hope the above isn't excessively long winded. I've tried to set it out step by step for clarity)

Comment 50

[Doron Rosenberg (IBM)]

That is probably overly complex for this bug.  Adding a install once button (named better) should be enough, the question is do we want to go to the server to check if it is evil before or after we show the infobar (before would lead to fun issues regarding timeouts, etc).

Comment 51

[Phil Ringnalda (:philor)]

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

Comment 52

[Darin Fisher]

Not going to block 1.8.1 for this, but we'll happily consider a baked-on-trunk patch.

Comment 55

[Nelson Bolyard (seldom reads bugmail)]

This issue seems to have a lot in common with the bad certificate handling
issue.  Give a user a way to override an error in the error-reporting dialog
(or bar) itself, and the user will override it every time, ESPECIALLY if the
user is in the process of being phished, and is trying with all his might to
save his bank account.  

Seems like an attacker could trivially exploit that behavioral phenomenon 
to get victims to install an evil package.  He'd send an email that claimed
to help a user recover his bank account with a link to an extension installer
(or to a page with a link to the extension installer).  

I suspect that forcing the user to have to play with preferences, altering
his whitelist, is the only way to stop phishing victims in that scenario.

Comment 56

[Nelson Bolyard (seldom reads bugmail)]

Regarding the most recent mockup, it says "you can disable or remove 
extensions at any time...".  I think most users will interpret that to mean
"... so if this extension you're about to install does any harm, you can 
recover from that harm by disabling or removing the extension after you 
install it", which, of course, isn't necessarily true, and likely isn't 
true if the extension was true malware.

Comment 57

[Dave Townsend [:mossop]]

I should point out that the work I intend to do here will be pretty much the mockup seen in comment 16.

Comment 58

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This patch is mostly backend xpinstall work to allow us to pass all the necessary information about the install to the whitelist observer and allow it to in turn pass the information back to xpinstall to start the install correctly as if there had been no block at all.

All 4 paths to start an install have essentially changed in the same way, that is the whitelist check has been moved later after the security/sanity checks. I don't think that this is really a major change, seems sensible to not check the whitelist until we know if enough parameters have been passed etc. This also means that we can gather all of the install information into a single object (nsIXPIInstallInfo) then pass that onto nsXPInstallManager or to the whitelist observer as appropriate. They all also do this without handing off parts of the information to nsInstallTrigger. nsXPInstallManager gets a new public entry point that this info object can be passed in from chrome to trigger the start install with confirmation dialog.

The nsIDOMTriggerGlobal interface has changed a little to make things easier however I left Install, InstallChrome and StartSoftwareUpdate in place for now, but none of those are used by us and could be removed assuming we are sure that no embedders etc. use them?

Finally in the UI side the notification bar gets an Allow button to trigger the blocked install. The Edit Options button has been removed since that it what the UX team advised they wanted.

This patch has a slight side effect that we may need to make web authors aware of. Currently the InstallTrigger methods of starting an install return true if the install managed to display a confirmation dialog, or false if something failed along the way (including blocking by whitelist). It's now possible that even after returning false the install may proceed. The only way to see that happening is by passing in a callback to InstallTrigger.install which will receive success/cancel reports from the confirmation dialog.

Comment 59

[Dave Townsend [:mossop]]

Forgot to mention that the indentation is some of these files is really messed up. Seems to vary between 2 and 4 spaces on a function by function basis. Could possible reformat all to one standard though that'd screw up the blame.

Comment 60

[Dave Townsend [:mossop]]

Comment on attachment 277533 [details] [diff] [review]
patch rev 1

Cancelling review for the moment. Neil had some suggestions that I think are valid and will look at them in the morning.

Comment 61

[Dave Townsend [:mossop]]

Attachment: patch rev 2

The seamonkey folks were suggesting that it would be more useful to pass the DOM window out to the blocklist observer and since we can get the docshell from that anyway it makes sense. It also turns out that nsXPInstallManager only cares about having a DOM window too so we can drop the nsIScriptGlobalObject

Comment 62

[Daniel Veditz [:dveditz]]

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

>Index: browser/base/content/browser.js

Looked OK to me, but you should get a browser peer to review/approve this bit.


@@ -240,41 +233,63 @@ nsInstallTrigger::HandleContent(const char * aContentType,
>+    nsXPITriggerInfo* trigger = new nsXPITriggerInfo();
>+    if (trigger)
>     {

In this block it looks like you'll leak "item" if new nsXPIInstallInfo() fails. You can simplify the multiple fall-through else cases by combining them:

  nsXPITriggerInfo* trigger = new nsXPITriggerInfo();
  nsXPITriggerItem* item = new nsXPITriggerItem(...args...);
  if (trigger && item) 
  { 
     trigger->Add(item);
     //etc
     //stuff that returns if all is ok
  }
  delete trigger;
  delete item;
  return NS_ERROR_OUT_OF_MEMORY;

And simplify even more by using nsAutoPtr instead of raw pointers to avoid worrying about deletes

>+                // From here trigger is owned by installInfo until passed on to nsXPInstallManager
>+                    nsCOMPtr<nsIObserverService> os(do_GetService("@mozilla.org/observer-service;1"));
>+                    if (os)
>+                        os->NotifyObservers(installInfo,
>+                                            "xpinstall-install-blocked",
>+                                            nsnull);

If you can't get "os" then you leak everything, too. maybe something like:

     if (!os)
       delete installInfo;
     os->NotifyObservers();

So once you send it off to the observers, who deletes it? I think this ends up not leaking, but only because there happens to be an observer. The observer does a QI which addrefs, and when that reference gets cleaned up it'll release itself. But if we're embedded in another front-end that doesn't have an observer it never gets cleaned up. If someone ever changed the observer service such that it happened to do something that caused an addref/release early on it could end up passing along a dead object.

Doesn't look like the path that calls StartInstall() deletes it either. I think you should use a nsRefPtr instead of a raw pointer to automatically AddRef/Release. In later code I notice you used a nsCOMPtr<nsIXPIInstallInfo> -- that works too (and in that case ignore what I said about the delete installInfo).

>@@ -479,23 +490,75 @@ InstallTriggerGlobalInstallChrome(JSContext *cx, JSObject *obj, uintN argc, jsva
>+                    else
>+                    {
>+                        delete trigger;
>+                        return JS_FALSE;
>+                    }
>+                }
>+                else
>+                {
>+                    delete trigger;
>+                    return JS_FALSE;
>+                }
>+            }
>+            else
>+            {
>+                return JS_FALSE;
>+            }
>+        }
>+        else
>+        {
>+            return JS_FALSE;
>+        }

There's got to be a way to collapse all these.

Returning JS_FALSE throws an exception and if we're doing that we should report an error to the console so people can make some sense of it. *rval is initialized to JSVAL_FALSE so the user script will know something went wrong even without the exception, so I don't think we need to throw.

Ditto in InstallTriggerGlobalStartSoftwareUpdate

>+nsXPIInstallInfo::nsXPIInstallInfo(nsIDOMWindowInternal *aOriginatingWindow,
>+                                   nsIURI *aOriginatingURI,
>+                                   nsXPITriggerInfo *aTriggerInfo,
>+                                   PRUint32 aChromeType)
>+{
>+    mOriginatingWindow = aOriginatingWindow;
>+    mOriginatingURI = aOriginatingURI;
>+    mTriggerInfo = aTriggerInfo;
>+    mChromeType = aChromeType;
>+}

You should do these constructor-style rather than as assignments
  nsXPIInstallInfo::nsXPIInstallInfo( ... )
  : mTriggerInfo(aTriggerInfo), mOriginatingWindow(aOriginatingWindow),
    etc.

>+nsXPIInstallInfo::~nsXPIInstallInfo()
>+{
>+    if (mTriggerInfo)
>+        delete mTriggerInfo;

Could just "delete mTriggerInfo;", it's OK if it's null.

>+nsXPInstallManager::InitManagerWithInstallInfo(nsIXPIInstallInfo* aInstallInfo)
 ...
>+    if ( !triggers || triggers->Size() == 0 )
>+    {
>+        // nsIXPIInstallInfo still owns triggers so no need to release
>+        NS_WARNING("XPInstallManager called with no trigger info!");
>+        NS_RELEASE_THIS();
>+        return NS_ERROR_INVALID_POINTER;
>+    }

You could skip the above test since the InitManager() call will re-check and handle it.

Seems like the rest could be made more compact with nesting. Since you've got a
local pointer to the triggers should be OK to null out the aInstallInfo()
pointer before the InitManager call, which simplifies things:

>+    nsCOMPtr<nsIDOMWindowInternal> win;
>+    rv = aInstallInfo->GetOriginatingWindow(getter_AddRefs(win));
      if (NS_SUCCEEDED(rv))
      {
          PRUint32 type;
          rv = aInstallInfo->GetChromeType(&type);
          if (NS_SUCCEEDED(rv))
          {
              // transferring ownership of triggers to InitManager
              aInstallInfo->SetTriggerInfo(nsnull);
              return InitManager(win, triggers, type);
          }
      }

      NS_RELEASE_THIS();
      return rv;
  }

r-minus mainly on the memory leaks, overall looks good.

Comment 63

[Dave Townsend [:mossop]]

Attachment: patch rev 3

(In reply to comment #62)
> (From update of attachment 277713 [details] [diff] [review])
> >Index: browser/base/content/browser.js
> 
> Looked OK to me, but you should get a browser peer to review/approve this bit.

Gavin will do a sweep over the browser bits.

> @@ -240,41 +233,63 @@ nsInstallTrigger::HandleContent(const char *
> aContentType,
> >+    nsXPITriggerInfo* trigger = new nsXPITriggerInfo();
> >+    if (trigger)
> >     {
> 
> In this block it looks like you'll leak "item" if new nsXPIInstallInfo() fails.

I don't think this is the case. Correct me if I'm wrong, but once item has been added to triggers it is owned by triggers and will get deleted in triggers destructor.

> You can simplify the multiple fall-through else cases by combining them:

Yes, this and using nsAutoPtr does simplify the code somewhat.
> If you can't get "os" then you leak everything, too. maybe something like:
> 
>      if (!os)
>        delete installInfo;
>      os->NotifyObservers();

I'm not sure where you are looking here. Everywhere I used nsIXPIInstallInfo in my patch I used it in an nsCOMPtr which should keep a reference for the duration of calls to StartInstall or the observers.

> There's got to be a way to collapse all these.

Done, it's a bit simpler now with the nsAutoPtr usage.

> >+    if ( !triggers || triggers->Size() == 0 )
> >+    {
> >+        // nsIXPIInstallInfo still owns triggers so no need to release
> >+        NS_WARNING("XPInstallManager called with no trigger info!");
> >+        NS_RELEASE_THIS();
> >+        return NS_ERROR_INVALID_POINTER;
> >+    }
> 
> You could skip the above test since the InitManager() call will re-check and
> handle it.

I added this because there was an oddity with calls to InitManager not deleting the passed triggers if that test fails. I've come to the conclusion that that is actually a bug and just added the delete call there.

I've also added some delete calls to nsInstallTrigger::InstallChrome. As I understand that function you should padd in an nsXPITriggerItem and expect InstallChrome to delete it or pass ownership on somewhere sensible. There were a couple of cases where neither were happening.

Comment 64

[Daniel Veditz [:dveditz]]

Comment on attachment 278305 [details] [diff] [review]
patch rev 3

r=dveditz for the bulk of it (xpinstall parts).
sr=dveditz

Comment 65

[Dave Townsend [:mossop]]

Attachment: Updated to trunk

Updated to trunk, carrying forward sr.

Comment 66

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

Comment on attachment 278969 [details] [diff] [review]
Updated to trunk

>Index: browser/base/content/browser.js

>-        var host = aData;
>+        var host = installInfo.originatingURI.host;

I was going to point out that this can throw for certain types of URIs (nsSimpleURIs like data:) but then Firefox 2 also throws in that case, and I'm not sure what would be better, so we can deal with this separately, I guess.

r=me on the /browser parts.

Comment 67

[Dave Townsend [:mossop]]

Filed bug 394388 for the data uri issue.
Filed bug 394388 for the data uri issue.

Comment 68

[Dave Townsend [:mossop]]

Comment on attachment 278969 [details] [diff] [review]
Updated to trunk

Seeking approval for this Firefox PRD item. I believe there is no risk to content, the only impact should be on the extension installation process.

Comment 69

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 278969 [details] [diff] [review]
Updated to trunk

This patch can't go in as-is.  iids for interfaces that got changed need changing.

Comment 70

[Dave Townsend [:mossop]]

Attachment: Change iids

Oops, my apologies for that. Changed the iids, carrying over reviews for minor change, re-requesting approval.

Comment 71

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 279084 [details] [diff] [review]
Change iids

a=bzbarsky

Comment 72

[Dave Townsend [:mossop]]

Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.833; previous revision: 1.832
done
Checking in browser/locales/en-US/chrome/browser/browser.properties;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/browser.properties,v  <--  browser.properties
new revision: 1.39; previous revision: 1.38
done
Checking in xpinstall/public/Makefile.in;
/cvsroot/mozilla/xpinstall/public/Makefile.in,v  <--  Makefile.in
new revision: 1.24; previous revision: 1.23
done
Checking in xpinstall/public/nsIDOMInstallTriggerGlobal.h;
/cvsroot/mozilla/xpinstall/public/nsIDOMInstallTriggerGlobal.h,v  <--  nsIDOMInstallTriggerGlobal.h
new revision: 1.18; previous revision: 1.17
done
RCS file: /cvsroot/mozilla/xpinstall/public/nsIXPIInstallInfo.idl,v
done
Checking in xpinstall/public/nsIXPIInstallInfo.idl;
/cvsroot/mozilla/xpinstall/public/nsIXPIInstallInfo.idl,v  <--  nsIXPIInstallInfo.idl
initial revision: 1.1
done
Checking in xpinstall/public/nsIXPInstallManager.idl;
/cvsroot/mozilla/xpinstall/public/nsIXPInstallManager.idl,v  <--  nsIXPInstallManager.idl
new revision: 1.4; previous revision: 1.3
done
Checking in xpinstall/src/Makefile.in;
/cvsroot/mozilla/xpinstall/src/Makefile.in,v  <--  Makefile.in
new revision: 1.99; previous revision: 1.98
done
Checking in xpinstall/src/nsInstallTrigger.cpp;
/cvsroot/mozilla/xpinstall/src/nsInstallTrigger.cpp,v  <--  nsInstallTrigger.cpp
new revision: 1.81; previous revision: 1.80
done
Checking in xpinstall/src/nsInstallTrigger.h;
/cvsroot/mozilla/xpinstall/src/nsInstallTrigger.h,v  <--  nsInstallTrigger.h
new revision: 1.23; previous revision: 1.22
done
Checking in xpinstall/src/nsJSInstallTriggerGlobal.cpp;
/cvsroot/mozilla/xpinstall/src/nsJSInstallTriggerGlobal.cpp,v  <--  nsJSInstallTriggerGlobal.cpp
new revision: 1.60; previous revision: 1.59
done
RCS file: /cvsroot/mozilla/xpinstall/src/nsXPIInstallInfo.cpp,v
done
Checking in xpinstall/src/nsXPIInstallInfo.cpp;
/cvsroot/mozilla/xpinstall/src/nsXPIInstallInfo.cpp,v  <--  nsXPIInstallInfo.cpp
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/xpinstall/src/nsXPIInstallInfo.h,v
done
Checking in xpinstall/src/nsXPIInstallInfo.h;
/cvsroot/mozilla/xpinstall/src/nsXPIInstallInfo.h,v  <--  nsXPIInstallInfo.h
initial revision: 1.1
done
Checking in xpinstall/src/nsXPInstallManager.cpp;
/cvsroot/mozilla/xpinstall/src/nsXPInstallManager.cpp,v  <--  nsXPInstallManager.cpp
new revision: 1.151; previous revision: 1.150
done
Checking in xpinstall/src/nsXPInstallManager.h;
/cvsroot/mozilla/xpinstall/src/nsXPInstallManager.h,v  <--  nsXPInstallManager.h
new revision: 1.43; previous revision: 1.42
done

Comment 73

[Gerd]

Unfortunately, now NOTHING happens when clicking onto an Install button.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007090205 Minefield/3.0a8pre ID:2007090205

Comment 74

[Gerd]

False Alarm: Works with empty profile.

Comment 75

[Tracy Walker [:tracy]]

verified with Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a8pre) Gecko/2007090504 Minefield/3.0a8pre

one time "Allow" in place and working