406807 - Remove unneccesary parts of xpinstall

Status: RESOLVED FIXED

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

Description

[Dave Townsend [:mossop]]

Bug 401624 dropped support for install.js based xpi bundles but only ifdefed out the final call to the install code. We can probably get good codesize savings by being a bit more aggressive with ifdefing out other areas of code. As an example the OSX box appears to have cleverly noticed that we no longer used half the code and made a saving of about 100k.

Maybe we could even drop install.js support entirely depending on the Camino guys?

Comment 1

[Dave Townsend [:mossop]]

Might as well take this, did some experimenting today and have a patch that takes the xpinstall library from 400k down to 140k.

Comment 2

[Dave Townsend [:mossop]]

From testing, inspection of a Camino compreg.dat and bug 332384 comment 11, it looks like Camino do not use xpinstall. Will double check with one of their dev team though.

Comment 3

[Dave Townsend [:mossop]]

What I have drops the xpinstall down to below 100k on my machine, but it's a big patch and has some consequences, it might even go a bit too far in what it removes. I'll try to go through everything.

It drops the xpinstall stub usage and since we no longer use xpinstall based installers removes the code in nsAppRunner to do with that.

For web content the methods InstallTrigger.compareVersion and InstallTrigger.getVersion both just return -5 (NOT_FOUND) regardless of how you call them. The prototype InstallVersion doesn't exist at all, that might need to still exist r some websites would be throwing where they weren't before. Not that it would be of any use to them now.

The public interfaces nsIXPINotifier.idl, nsPIXPIProxy.idl and nsPIXPIStubHook.idl and nsIDOMInstallVersion.h are gone.

In nsIDOMInstallTriggerGlobal.h I've dropped the unused methods Install, InstallChrome, StartSoftwareUpdate, CompareVersion and GetVersion.

I've unthreaded the installation process. Since we can't call the extension manager on a non-main thread anyway the only thing that is happening on the install thread at the moment is the certificate verification for the zip file. I think things are a lot simpler and safer without the threading and I don't believe it impacts performance much, though I haven't as yet gathered any exact numbers on that.

Unthreading saves us getting proxies to everything and should fix bug 404495 and bug 374470.

I've unified the install process depending on whether the package is marked as NOT_CHROME or not, so now we always download to an xpi file in the temp dir and just pass off to the extension manager.

xpinstall no longer tries to create an install log about package installation. I think if we actually want an install log then the EM ought to be creating one.

Much of the rest of the changes are removals of the install.js based routines for running the actual installer script.

Comment 4

[Dave Townsend [:mossop]]

Attachment: file removals

This patch just covers the files that can be deleted.

Comment 5

[Dave Townsend [:mossop]]

Attachment: code cleanups

This covers the code changes

Comment 6

[Dave Townsend [:mossop]]

Attachment: file removals (plain list)

Just in case it's easier for you to read over, this is just a list of the files to be deleted.

Comment 7

[Benjamin Smedberg]

Comment on attachment 293046 [details] [diff] [review]
code cleanups

>+static nsresult
>+VerifySigning(nsIZipReader* hZip, nsIPrincipal* aPrincipal)

I'd like a comment explaining what aPrincipal is for, and why we don't go poking for a principal if didn't find one the first time.

I think we ought to remove InstallTrigger.compareVersion and .getVersion completely, rather than having a stub impl that always fails.

Otherwise I think this looks fine, but I'd like to see the fixed patch.

Comment 8

[Dave Townsend [:mossop]]

Attachment: code cleanups rev 2

Adds the commenting and drops the getVersion and compareVersion methods.

On the subject of checking signing the conclusion is that anyone going to the effort of signing the xpi should be testing that it shows up as signed and in the event that it is signed incorrectly there is no reason to fail to install when we have told users it is unsigned.

Also there are just a few bits dropped out of the REQUIRES section of the xpinstall/src/Makefile.in that are no longer needed.

Comment 9

[Dave Townsend [:mossop]]

Attachment: code cleanups rev 3

Updated for trunk and added a few license changes.

Comment 10

[Daniel Veditz [:dveditz]]

sr status update: This removal is not just conditional build flags and will affect non-Firefox consumers of xpinstall. I've been checking with some of them (SeaMonkey, enterprise) to make sure they can live it since there's nothing in the bug to indicate that's been done.

Comment 11

[Benjamin Smedberg]

Dan, the only consumer of xpinstall for whom this already isn't disabled is Camino: all other apps are already MOZ_XUL_APP and have had xpinstall disabled for a month or more. We already checked with Camino and they are fine with the removal.

Comment 12

[Dave Townsend [:mossop]]

Yeah sorry I apparently failed to include this comment from Stuart Morgan:

> We aren't using it deliberately, and if we are including it my  
> understanding is that we shouldn't be (per the bug you referenced from  
> 406807). No need to hold back on our account!

Comment 13

[Daniel Veditz [:dveditz]]

> all other apps are already MOZ_XUL_APP and have had xpinstall disabled
> for a month or more.

I know, but a month is not a long time for getting feedback on this type of change. There doesn't seem to have been much effort made to announce this change to people who might not notice until we ship with it (I'm worried mostly about enterprise deployments).

But there doesn't seem to be any specific known effort using this so I'll go ahead.

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 295026 [details] [diff] [review]
code cleanups rev 3

>   // -- obsolete forms, do not document. Kept for 4.x compatibility
>   {"UpdateEnabled",         InstallTriggerGlobalUpdateEnabled,         0,0,0},
>   {"StartSoftwareUpdate",   InstallTriggerGlobalStartSoftwareUpdate,   2,0,0},
>-  {"CompareVersion",        InstallTriggerGlobalCompareVersion,        5,0,0},
>-  {"GetVersion",            InstallTriggerGlobalGetVersion,            2,0,0},

You might as well kill the first two (capitalized forms) as well.

Still trying to get through the rest. Would have been tons easier if the deleted files were just a list, and the diff were -w to get rid of the spurious whitespace cleanups.

Comment 15

[Dave Townsend [:mossop]]

Attachment: code cleanups rev 4 -w

Here is an updated patch with those two files removed and just the modifications without whitespace changes, full patch to follow. The file removals are still those listed in attachment 293049 [details].

There is one additional change to this patch, the MAJOR_DIFF, MINOR_DIFF, REL_DIFF, BLD_DIFF, EQUAL and NOT_FOUND JS constants removed and the enum containing them removed from nsIDOMInstallTriggerGlobal.h. There are no longer needed with the version comparison methods being gone.

Comment 16

[Dave Townsend [:mossop]]

Attachment: code cleanups rev 4

Full patch

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 298780 [details] [diff] [review]
code cleanups rev 4 -w

Among other things this patch removes threading support from xpinstall. Unpacking the zip file and verifying signatures are both potentially time-consuming operations that can hang the UI. I guess I don't care from the xpinstall POV if that's OK with the front-end folks.

>   // -- obsolete forms, do not document. Kept for 4.x compatibility
>-  {"UpdateEnabled",         InstallTriggerGlobalUpdateEnabled,         0,0,0},
>-  {"StartSoftwareUpdate",   InstallTriggerGlobalStartSoftwareUpdate,   2,0,0},
>-  {"CompareVersion",        InstallTriggerGlobalCompareVersion,        5,0,0},
>-  {"GetVersion",            InstallTriggerGlobalGetVersion,            2,0,0},
>   {"updateEnabled",         InstallTriggerGlobalUpdateEnabled,         0,0,0},
>   // -- new forms to match JS style --

These two comment lines are now useless/obsolete, please delete them.

sr=dveditz

Comment 18

[Dave Townsend [:mossop]]

Comment on attachment 298781 [details] [diff] [review]
code cleanups rev 4

(In reply to comment #17)
> (From update of attachment 298780 [details] [diff] [review])
> Among other things this patch removes threading support from xpinstall.
> Unpacking the zip file and verifying signatures are both potentially
> time-consuming operations that can hang the UI. I guess I don't care from the
> xpinstall POV if that's OK with the front-end folks.

Verifying the signature is the potential issue. The actual extraction of the xpi to disk is performed by the EM and has always been on the main thread.

Seeking approval to land, this is a big code saving. It has a small impact on web content in that we are removing now obsolete global functions and variables.

Comment 19

[Damon Sicore (:damons)]

Comment on attachment 298781 [details] [diff] [review]
code cleanups rev 4

a1.9+=damons

Comment 20

[Dave Townsend [:mossop]]

Landed, seeing a 120K codesize win on linux, 70K on OSX

Comment 21

[Wolfgang Rosenauer [:wolfiR]]

I noticed that change by not having xpicleanup anymore so I searched MXR for xpicleanup references:
http://mxr.mozilla.org/firefox/search?string=xpicleanup

Probably there are more leftovers from xpinstall?
Should I file a new bug no those?

Comment 22

[Dave Townsend [:mossop]]

Need docs updated on this and might want to highlight the changes for web-content in a release note.

Comment 23

[Mike Kaply [:mkaply]]

I think we have more consumers of this then we realize.

Ones that come to mine are:

Rhapsody
Popcap

How many companies do we know of that use install.js to either EXE install their proprietary plugins or use install.js to put them in the plugins directory?

I think it might be useful to start assembling a list...

Comment 24

[Daniel Kirsch]

Our OnlineUpdate feature for IDA that ships individual packages depending on the user licence and the customer uses it and is now completely broken. Development of a couple of weeks...
See bug 420496

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Eric, I think dev-doc-needed is solved with your update on bug 420496?

Comment 26

[Eric Shepherd [:sheppy]]

Yes. :)

Comment 27

[Victor Bielawski]

(In reply to comment #23)
> Rhapsody
> Popcap
> I think it might be useful to start assembling a list...

Flash
LogMeIn
Java
etc.

Comment 28

[Slav Tataurov]

There is a couple of issues with installing a plugin with the new method:

1. Firefox restart is required after installing a plugin. Why? I know the technical answer, but it is conceptually wrong. Neither IE nor FF1.x/2.x require restart. FF 3 becomes degraded from this view point.

2. When I call InstallTrigger.install(...) to install our plugin, nothing happens, but the 'Install Missing Plugins...' button appears at the upper-right conner. Obviously, it doesn't install our plugin, but only confuses users. Can you fix this?

And the third point is missing InstallTrigger.getVersion() method. sure, it was not the best decision to remove it.

> I think it might be useful to start assembling a list...
Techinline (www.techinline.com)
+ lots of small/medium companies that added FF plugins support for their web services. Please do not forget us when you make such a significant changes!