401624 - Remove install.js support when MOZ_XUL_APP is defined

Status: VERIFIED FIXED

(Core Graveyard :: Installer: XPInstall Engine, defect, P3)

Description

[Tony Chung [:tchung]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a9pre) Gecko/2007102904 Minefield/3.0a9pre
Build Identifier: 

Crash occurs when attempting to install Java plugin on Vista for XPInstaller.  Occured on vista.   this is on trunk.

Crash Stack Trace: http://crash-stats.mozilla.com/report/index/003d9738-8648-11dc-8f21-001a4bd43ed6?date=2007-10-29-17

Reproducible: Always

Steps to Reproduce:
1. install clean build of latest trunk minefield
2. Check to see java plugin is not installed 
3. Launch AMO plugin page, or use PFS to detect missing Java Plugin
4. Goto Java download page and install the Java xpi.  http://www.java.com/en/download/windows_xpi.jsp?locale=en&host=www.java.com:80
5. Allow the site to be added to Allowed sites list. 
6. Click download, but notice the errors appear:

"The installation of package http://java.sun.com/update/1.6.0/jre-6u3-windows-i586-jc.xpi failed with -203"

7. Click 'Restart minefield' in the Extension Manager window.
8. Verify on restart, a crash occurs.  See stack trace above.
Actual Results:  

no Crash occurs, and Java plugin installs successfully

Expected Results:  
Crash occurs and java plugin cant be installed.

Comment 1

[Dave Townsend [:mossop]]

I think I know what's going on here

Comment 2

[Tony Chung [:tchung]]

Attachment: Java Plugin Error Screenshot

Comment 3

[Dave Townsend [:mossop]]

Can you expand a little on step 4. When I go to that url clicking the begin download button displays the install notification briefly, then it goes away and the page reloads. Then clicking the button displays the notification properly and I can install. Is that what you see?

Comment 4

[Dave Townsend [:mossop]]

Ok I can't reproduce this, but I don't think this is what I thought. I think what is happening is there is an unused attempt to install (see previous comment) that is getting collected at shutdown. The installinfo keeps a reference to the page JS context because a callback method was passed to InstallTrigger.install. Because of this on cleaning up we attempt to do something to the JS runtime that has at that point gone away.

This is a new problem from bug 252830 because we are now holding a reference to the installinfo in the browser window JS scope, in a closure no less. It might be possible to drop this reference earlier so it can clean up while the JS runtime is still alive.

I won't be able to look further at this for at least two weeks so clearing assignee for now.

Comment 5

[Robert Sayre]

Assigning to Rob Strong. Rob, if you don't have time for this, please delegate to someone else (you will make the best decision on that, I think).

Comment 6

[Robert Sayre]

OK, moving this down after discussion with tchung. The browser still works, Java just doesn't install.

Comment 7

[Robert Strong (they/them - no direct email)]

Quick summary:
a) I can confirm comment #6 in that the install of Java fails and the browser still works.
b) Installing a plugin via a plugin's installer works.
c) Installing a plugin via the Plugin Finder service with the fix from bug 352762 works.
d) Installing via a plugin's xpi directly doesn't work.

Per bug 352762 Comment #39
> 2) Remove support for install.js-style xpinstall. This will be trunk-only
> and if it doesn't make it for FF3 that's ok, but old-style xpinstall is
> permanently broken on Windows Vista and we should stop supporting it.

We might be able to teach XPInstall to ask for elevation whenever it launches an executable to try to get this working on Vista.
or
We could deprecate install.js style XPInstall installations per bug 352762 Comment #39.

I personally believe we should deprecate install.js style XPInstall installations and that we should notify vendors to instead just provide links to their installers. In every instance I know of when a vendor provides a link to an xpi to install their plugin they also provide a link to their installer to install the plugin. This would remove maintenance of a redundant installation method for the vendor and will likely lessen the complexity of our code.

Benjamin, could you chime in with your thoughts on this?

Comment 8

[Benjamin Smedberg]

We should deprecate install.js and I've said as much in several bugs. I'd love a simple patch which disables that codepath altogether, without ripping xpinstall apart.

Morgamic may have contacts at Sun so we can get them to publish an .exe or .msi installer for Firefox users instead of an .xpi.

Comment 9

[Robert Strong (they/them - no direct email)]

note: they already have published an exe installer and would only need to remove the xpi.

Comment 10

[Robert Strong (they/them - no direct email)]

Attachment: patch -w (remove support for install.js type packages when MOZ_XUL_APP is defined

I'll run the string by Madhava or Beltzner

Comment 11

[Benjamin Smedberg]

Comment on attachment 287750 [details] [diff] [review]
patch -w (remove support for install.js type packages when MOZ_XUL_APP is defined

Please file a followup on removing the deprecated code completely.

Comment 12

[Daniel Veditz [:dveditz]]

(In reply to comment #4)
> This is a new problem from bug 252830 because we are now holding a
> reference to the installinfo in the browser window JS scope, in amight
>  closure no less. It may be possible to drop this reference earlier

How about if the site is not whitelisted we immediately return user-cancelled to the callback (I think we already do) and then kill the callback before sending the install info off to the infobar.

If that's the problem, though, it could happen on any non-AMO served extension page that uses a call-back function. It wouldn't be just Java, and the patch here wouldn't help.

Comment 13

[Dave Townsend [:mossop]]

(In reply to comment #12)
> How about if the site is not whitelisted we immediately return user-cancelled
> to the callback (I think we already do) and then kill the callback before
> sending the install info off to the infobar.

I've been thinking more that we could have a "cancel" method on the installinfo that could be called when the notification bar goes away (because of window close or user close) that calls the callback. This would keep the callback working right but make it get called before the runtime goes away

> If that's the problem, though, it could happen on any non-AMO served extension
> page that uses a call-back function. It wouldn't be just Java, and the patch
> here wouldn't help.

Yeah I would expect it to be a problem elsewhere.

Comment 14

[Reed Loden [:reed]]

Any update on this? b2 freeze is tomorrow...

Comment 15

[Dave Townsend [:mossop]]

From the add-ons security review we might want to change the flag used to disable install.js support. I suspect there is also a bunch of other code savings we can get from removing support as well.

Comment 16

[Benjamin Smedberg]

Dave, I don't understand your last comment... what flag? The patch here removes install.js support unconditionally. It's ready to land AFAICT, except Rob's away for a couple days.

Comment 17

[Dave Townsend [:mossop]]

(In reply to comment #16)
> Dave, I don't understand your last comment... what flag? The patch here removes
> install.js support unconditionally. It's ready to land AFAICT, except Rob's
> away for a couple days.

dveditz had suggested that it might be a good idea to use something other than MOZ_XUL_APP to determine whether to include install.js support. Anyway if we want to land this for b2 and Rob doesn't have time then I can land it.

Comment 18

[Benjamin Smedberg]

Camino is the only non-MOZ_XUL-APP left... I'd prefer to remove install.js support entirely, but this patch is a lot easier to write.

Comment 19

[Dave Townsend [:mossop]]

Checked in.

Checking in toolkit/locales/en-US/chrome/global/xpinstall/xpinstall.properties;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/global/xpinstall/xpinstall.properties,v  <--  xpinstall.properties
new revision: 1.10; previous revision: 1.9
done
Checking in xpinstall/src/nsInstall.h;
/cvsroot/mozilla/xpinstall/src/nsInstall.h,v  <--  nsInstall.h
new revision: 1.106; previous revision: 1.105
done
Checking in xpinstall/src/nsSoftwareUpdateRun.cpp;
/cvsroot/mozilla/xpinstall/src/nsSoftwareUpdateRun.cpp,v  <--  nsSoftwareUpdateRun.cpp
new revision: 1.111; previous revision: 1.110
done

Comment 20

[Tony Chung [:tchung]]

Litmus added: https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=5014

Comment 21

[Eric Shepherd [:sheppy]]

Updated various documents to note that install.js is no longer supported in Firefox 3 effective with beta 2, and added information to the Firefox 3 for developers and extension updating articles.

Comment 22

[Atsushi Sakai]

(In reply to comment #8)
> Morgamic may have contacts at Sun so we can get them to publish an .exe or .msi
> installer for Firefox users instead of an .xpi.

How about Adobe Flash?
Flash is also provided with install.js-style xpi via plugin-finder.

https://pfs.mozilla.org/plugins/PluginFinderService.php?mimetype=application%2Fx-shockwave-flash&appID={ec8030f7-c20a-464f-9b0e-13a3a9e97384}&appVersion=2007120405&clientOS=Windows%20NT%205.0&chromeLocale=en-US

Comment 23

[Marcia Knous [:marcia]]

Attachment: Screenshot of error message post checkin

I don't see the crash, but the I get a blank dialog for the error message as you can see in the attached screenshot. I can file a spinoff bug for that.

Comment 24

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b2pre) Gecko/2007121008 Minefield/3.0b2pre.  crash is gone.

marcia, can you open a separate bug for the error screenshot you see?  i can reproduce it also on this build.

Comment 25

[Tony Chung [:tchung]]

copied the wrong build ID in my last comment.   should be Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b2pre) Gecko/2007121009 Minefield/3.0b2pre.

Comment 26

[Alfred Kayser]

Notes:
1. There is no follow-up bug to remove the old style completely
2. Some more code is redundant, such as: SetupInstallContext

Comment 27

[Dave Townsend [:mossop]]

The old style install.js is now unusable. Removing the now unnecessary code will take place in bug 406807

Comment 28

[Frank Wein [:mcsmurf]]

Changing summary to what this bug is really about.