Closed Bug 401624 Opened 12 years ago Closed 12 years ago

Remove install.js support when MOZ_XUL_APP is defined

Categories

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

x86
Windows Vista
defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.9beta2

People

(Reporter: tchung, Assigned: rstrong)

References

()

Details

(Keywords: dev-doc-complete, relnote)

Attachments

(3 files)

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.
I think I know what's going on here
Assignee: nobody → dtownsend
Flags: in-litmus?
Flags: blocking1.9?
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?
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.
Assignee: dtownsend → nobody
Blocks: 252830
Flags: blocking1.9? → blocking1.9+
Priority: -- → P1
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).
Assignee: nobody → bugspam
OK, moving this down after discussion with tchung. The browser still works, Java just doesn't install.
Priority: P1 → P2
Priority: P2 → P3
Assignee: bugspam → robert.bugzilla
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?
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.
note: they already have published an exe installer and would only need to remove the xpi.
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.
Attachment #287750 - Flags: review?(benjamin) → review+
Attachment #287750 - Flags: approval1.9?
Attachment #287750 - Flags: approval1.9? → approval1.9+
(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.
(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.
Depends on: 406028
Any update on this? b2 freeze is tomorrow...
Whiteboard: [needs landing]
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.
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.
(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.
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.
Whiteboard: [needs landing]
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
Keywords: relnote
Target Milestone: --- → mozilla1.9 M10
Status: NEW → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Litmus added: https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=5014
Flags: in-litmus? → in-litmus+
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.
Version: unspecified → Trunk
(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
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.
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.
Status: RESOLVED → VERIFIED
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.
Notes:
1. There is no follow-up bug to remove the old style completely
2. Some more code is redundant, such as: SetupInstallContext
The old style install.js is now unusable. Removing the now unnecessary code will take place in bug 406807
Changing summary to what this bug is really about.
Summary: Crash @ntdll.dll@0x42e7b on Java XPInstaller for Win Vista → Remove install.js support when MOZ_XUL_APP is defined
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.