A nice safe place to store my patches.

This patch: - abstracts xpi fe dialog urls away to prefs so that other apps (firebird, thunderbird) can supply their own XPInstall UI. This removes the dependency on chrome://communicator/ - some changes to support non-modal downloading UI. - provides support for InstallTrigger.install({"Foo 1.0":{URL:"http://www.site.com/foo.xpi",IconURL:"http://www.site.com/foo.gif"}}); while maintaining support for the 1.0 API usage.

target.

Benjamin, I'm going to be sending mail about some of the changes I'm working on here shortly.

I have extended the XPInstall Install Trigger exposed to web pages. I want to be able to have the page provide a small icon that is displayed in the confirmation dialog and in the install progress dialog. The method I have exposed to web developers is: InstallTrigger.install({"Extension Name": { URL: "http://www.site.com/foo.xpi", IconURL: "http://www.site.com/foo.gif"}); Currently, the install trigger use looks like this: InstallTrigger.install({"Extension Name": "http://www.site.com/foo.xpi"}); I have modified the code in various places of xpinstall/src to support the new IconURL property, and JSObjects as property values for InstallTrigger install parameters. I have retained support for string property values for 1.0-1.5 compatibility so all existing web usages of InstallTrigger.install will continue to work as before. I have done this in a patch, which I will be attaching shortly (cvs is diffing as I type), combined with some prefs which allow an arbitrary XUL app (such as Firebird or Thunderbird) to provide its own XPInstall User Experience. The side effect is that the dependency on xpfe/communicator is removed from this code. I have added prefs and some slight changes to the Seamonkey front end here to ensure that it operates correctly. Footnote: Future Benefits of using an Object parameter: Using a JS Object as an install parameter block is a much more extensible solution. Using an object lets us expose a large number of additional install parameters in the future, which will allow extension authors to remove some of those horrible alert() dialogs that they use in their install scripts to get configuration info from the user before the install commences. For example: After download, many extension XPIs present an alert() dialog that clumsily asks the user whether they want to install into the profile or the install directory. Assuming hyatt's Chrome reg/extension changes didn't call for the removal of the ability to install into the install directory, this decision is best made before the XPInstall procedure is invoked, i.e. when the user is still in the context of the web page and thus good UI options are available. alert()s are used later on because the InstallTrigger.install() object only supports limited parameters and the install scripts are executed in a custom js environment without the benefit of proper chrome. Thus extending this concept further we could have InstallTrigger.install({"Extension Name": { URL: "http://www.site.com/foo.xpi", IconURL: "http://www.site.com/foo.gif", InstallToProfile: somePageUICheckbox.checked});

I need this for 1.6f, since it will be a part of Firebird 0.8. Benjamin, Dan, Brendan, can you review my patch? (Once it's attached).

Adding Dan's correct bugzilla addr.

Comment on attachment 137219 [details] [diff] [review] Patch. I'm a little bit worried about this for the following reason: while the object-oriented syntax provides good forward-compatibility, there isn't a way for browser JS to detect whether this new syntax is supported. In addition, this is a bit radical for a 1.6f landing... can we land it on a mini-branch for fb0.8 and give it time to bake on the trunk during 1.7a?

Comment on attachment 137219 [details] [diff] [review] Patch. Do we really need an NS_REINTERPRET_CAST between PRUnichar and jschar? If they're always compatible types why won't they automatically cast, and if they're not, why is it safe to reinterpret_cast them?

Comment on attachment 137219 [details] [diff] [review] Patch. bsmedberg: yeah, C++ does type equivalence by name, not by "structure", so two different unsigned short typedefs are different types -- so we need reinterpret_cast (the DOM has this all over). Drivers could consider this for 1.6, but it would be an easy sell all around to put it on a fb0.8 branch, land it in the 1.7a trunk, and avoid the approval song and dance. Separate issue from code review, though -- back to that. /be

I am open to a branch, I have some other changes that I want for .8 that might not make 1.6

As for versioning... it's quite simple. Assuming this makes 1.6: var params; var url = "http://www.site.com/foo.xpi" if (geckoVersion >= 1.6) params = { URL: url, IconURL: "http://www.site.com/foo.gif" }; else params = url; InstallTrigger.install({"Foo" : params }); Assuming a 0.8 firebird branch: var params; var url = "http://www.site.com/foo.xpi" if (geckoVersion >= 1.7 || (geckoVersion >= 1.6 && userAgent contains "Firebird")) { params = { URL: url, IconURL: "http://www.site.com/foo.gif" }; else params = url; InstallTrigger.install({"Foo" : params }); This sort of checking would be required, even if other, less scalable solutions were implemented.

This one makes the XPInstallManager register itself as an observer, so that places in the code that do not have a XPInstallManager pointer can nonetheless send the XPInstallManager messages via the correct nsIObserverService APIs.

Ben/brendan: if you implement a .toString method on the object, would this JS work without the hacky version check? var url = "http://www.site.com/foo.xpi"; var params = { URL: url, IconURL: "http://www.site.com/foo.gif", toString: function() { return this.URL; } }; InstallTrigger.install({"Foo" : params });

Comment on attachment 137226 [details] [diff] [review] Third Patch ok, I've tested my suggestion in comment 14, and it works with current mozilla versions. When we make this public, that's the code snippet we should use; pure JS is better than version-check hackery. Pursuant to bug 224578, the changes to all.js should end up in browser-prefs.js because they are seamonkey-specific. I hope to land that bug immediately when 1.7a opens (perhaps in a carpool). Finally, do we need to security-check the iconURL parameter? I can't really imagine a security hazard caused by an image, but would SVG images (with script) embedded in a chrome extension-management be granted chrome privs?

I like the flexibility of changing InstallTrigger.install() to take objects, but would it be better to change that form to a simple array of objects rather than keeping the associative array syntax? It's probably only an aesthetic difference in the usual single-package case ...install({"my package": {URL: "http://blah", ...}}, cbfn); vs ...install([{Name:"my Package", URL:"http://blah", ...}], cbfn); If the user created a package object the second form becomes a little more readable: ...install({"my package": new package("http://blah",...)}, cbfn); vs. ...install([new package("my package","http://blah", ...)], cbfn); They're nearly identical in the single-install case, logically I like having the name part of the same object as the rest of the information. What I'd ultimately like is to see "recommended extension" or "tune up" pages where people can check off multiple items and install at once. Which syntax would lead to easier coding of such a page? Assuming the page is generated from objects already looping through doing installme[list[i].Name] = list[i]; is pretty similar to isntallme.push([list[i]]); (Assuming of course that the objects in your list have conveniently-named members) So, OK, I guess I talked myself into either way, and a working patch trumps my hot air. I'll be back after lunch to do the super-review.

dveditz: I prever BenG's way because author's can write cross-compatible JS.

ah, I didn't get as far as your comment 14 before responding. Yeah, that's definitely a swing vote for Ben's syntax. I really should know better by now than to respond before getting to the end of the thread. One bad thing about Ben's patch is that it breaks the nsIXPIDialogService embedding interface since there are now 4 strings per package instead of 3. In the original (pre-signing) version there were 2 per package so maybe this is just a bad interface that we need to replace anyway. But we do have to deal with it. If we *must* break it (never got beyond "under review", and I don't know if anyone ever ended up using it) let's do so in a way that we don't have to break it again the next time someone invents another interesting parameter. If the embedding interfaces were sufficient could the firebird customizations be done that way? The "built-in" UI ought to be done that way, really. I doubt Ben was looking to take on that much work.

Hey Dan... good point on that interface... seems like it'd be good to have something better there. I'm a bit skittish about making changes of that magnitude before 1.6/0.8 however... and if the interface is not frozen yet I figure this is something we could comprehensively review in 1.7/0.9 when we'll be looking in some detail at how extensions and other xpi packages are installed. Sound fair?

Hey cool trick Benjamin... I'll definitely add that to the note I send out with 0.8 I'm retargeting this bug to 1.7a and checking in this patch on the Fb 0.8 branch.

On review diffs the -p option is very helpful, and please, please, PLEASE use more than the default 3 lines of context unless it's a really trivial patch (e.g. the all.js change is fine). I'm uncomfortable having the install chrome pointed at by prefs, meaning that if anyone ever found a way to modify prefs they could also supply their own (silent) install UI. At least validate that the urls are "chrome:" so they're something the user installed and don't come from the WEB -- although a tricky hacker might be able to get the victim to install a skin file that had extra .xul files in it so event that's not totally safe. Since this is for Firebird, why doesn't Firebird simply override the xpinstall chrome? Ah, because it's in the communicator package. I'd much rather move this to a top level chrome://xpinstall package. It's logically not part of communicator anyway, it could be used by Firebird, Thunderbird, whatever Gecko app. Then, if you still want different UI from the Mozilla app-suite the Firebird app could simply provide their own package, rather than the riskier pref-pointing. What do you want to accomplish from the observer service? nsSoftwareUpdate.cpp is more central to the process and might be a better place, although it's not called until after the download is done (do you really care than an install is downloading--but might get interrupted--or do you want to know about actual installs?). It knows things like whether the install succeeded or not (it also supports a fairly detailed listener object, but that would be hard to use in general since you wouldn't want to load the xpinstall dll to instal the listener unless you knew someone wanted to install things). In both cases where you use the pref service you check that you got it, but if you don't you fall through and try to open dialogs using an uninitialized char* pointer. You removed the resizability from the confirmation prompt, which possibly allows malicious folks to supply really long package names to push the signing and URL information off the dialog without allowing a way for users to expand the dialog to see it.

Dan - 1- Just a "FWIW", all XUL app default front ends are pointed to by a pref ("browser.chromeURL") so assuming an exploit as you mention, it's theoretically possible to replace the entire browser with a rogue replacement. ;-) However, neither system (hard coded chrome URL or pref abstracted chrome URL) is IMO best... If the XPInstall system is to be used by embedding apps (such as Camino) for installing updates, shouldn't its front end be fully replaceable via a system that is applicable to that embedding app? I.e. a Cocoa FE for Camino. This would seem to argue for a XPCOM component approach as used by the download progress front end ... there's a progress dialog interface that the helper app service uses to invoke a front end (which is determined by whoever implements the progress dialog interface... in Firebird it's a js component that opens a XUL window, in Camino it's a C++ object that opens native Cocoa windows). This abstracts things away nicely and doesn't assume a XUL FE, or place XUL FE urls perilously in preferences. 2- I made the XPInstallManager register itself as an observer with the system-wide observer service because it *must* be a system-wide observer of the "xpinstall-progress" message... this is sort of the point of making the XPInstallManager implement nsIObserver I thought rather than simply adding another method to its public API. My particular reason for needing this to work this way is that in the Firebird Download Manager, which also handles XPInstall progress notifications, when the app shuts down the DLMgr needs to be able to cancel all active xpinstall operations. However the code doesn't have a reference to the xpinstallmgr at the time it is notified that a shutdown is happening, so the only means of telling the xpinstall manager to stop what it's doing is by sending a message using the global observer service. 3- would be addressed if we tried to use something that I describe in (1) 4- I removed the resizability because it's not really warranted on a dialog like this. The security problem you mention is a valid one, but it's not a valid argument for window resizability. The window as it exists in Seamonkey has no resizer widget in the bottom right corner, so for all intents and purposes to the average user it isn't resizable at all (and even there were a resizability marker, it's likely the user would not notice it anyway). The real solution to this problem is to ensure that dialog content cannot expand so far that it pushes other items off the edge of the Window. A real solution to this would be to add crop attributes to non-essential text fields like the name field, and make the URL field a selectable, unstyled textfield that looks like static text but can be scrolled, copied from, etc. This is what Firebird is doing with its new FE. What do you think about these things? If we can agree on 1 & 4 I can begin on a new trunk patch for 1.7a.

I agree totally with 1), and in fact XPInstall has such an embedded UI API (these are the interfaces which your changes break because you change the number of strings per package). The XPInstall chrome is used only if an external UI component cannot be loaded. Eventually the default UI should be implemented this way as well but I hadn't gotten that far before the people who were pushing for this API decided other things were more important. As to 4) I have no great love for the current UI, so if you come up with something better I'd love to steal it. You mention installs popping their own alerts asking for profile or global installation. If the script author were clever they could pass the install choice in via the arguments and then only pop the dialog if the arguments were missing. At the lowest level (nsISoftwareUpdate) arguments are just a string passed to the InstallJar() method, but as a direct link or InstallTrigger api you just tack them on to the URL they way you would in a CGI script URL. http://example.com/foopy.xpi?profile=true The query string shows up in the script as Install.arguments and the parsing/interpretation is up to the script author. Since most people cut-n-paste in their scripts if you set up a standard extensions page you could set up some defacto settings that everyone would copy in order to be compatible with your page.

Ben (Bucksch): This bug may have some relation to the changes you were also trying to make in this area.

Ben: I followed the code down and it does feed into nsWindowWatcher where we correctly check that only chrome: urls can be given chrome privileges. I'm OK with using prefs for this. My preference would be to "robustify" the embedding API's in whatever ways necessary and then implement the default UI using those interfaces, and then Firebird or whoever could replace them easily using that. But that's possibly a lot of work compared to your rather simple patch so you don't need to go there right now.

Well, since I have my changes using chrome urls in prefs are on the 0.8 branch only it might be worth investigating the better, embedding-savvy fix for 1.7a. I can come up with a rough sketch of how this might look... I've filed bug 229477 to handle this task. As for the new FE... we could relocate the "confirmation" dialog section of my new FE to mozilla/xpinstall where it could be shared by all... and the resizability issues should vanish. (The download status parts would probably have to remain distinct... since the FB section integrates with its Download Manager and there's a huge amount of custom code there to deal with that).

OK, it looks like my schedule is far more packed for this milestone than I figured, so I won't be able to get to bug 229477 until ~1.8... Dan, if you're not opposed to the pref approach, can you review the patch as an interim solution?

I think this patch broke extension installation for thunderbird

This patch is causing shutdown crashes on OS/2 when installing XPIs. The problem is that in this code: + nsCOMPtr<nsIObserverService> os(do_GetService("@mozilla.org/observer-service;1")); + os->RemoveObserver(this, XPI_PROGRESS_TOPIC); os is coming back null and we crash dereferencing the COMPtr.

Closing as FIXED since it looks like this was all done and dusted for Firefox 1.0