The default bug view has changed. See this FAQ.

Remove unneccesary parts of xpinstall

RESOLVED FIXED in mozilla1.9beta3

Status

Core Graveyard
Installer: XPInstall Engine
RESOLVED FIXED
9 years ago
a year ago

People

(Reporter: mossop, Assigned: mossop)

Tracking

({dev-doc-complete, footprint, relnote})

Trunk
mozilla1.9beta3
dev-doc-complete, footprint, relnote
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

(Assignee)

Description

9 years ago
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?
(Assignee)

Comment 1

9 years ago
Might as well take this, did some experimenting today and have a patch that takes the xpinstall library from 400k down to 140k.
Assignee: nobody → dtownsend
(Assignee)

Updated

9 years ago
Target Milestone: --- → mozilla1.9 M11
(Assignee)

Comment 2

9 years ago
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.
(Assignee)

Comment 3

9 years ago
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.
(Assignee)

Comment 4

9 years ago
Created attachment 293045 [details] [diff] [review]
file removals

This patch just covers the files that can be deleted.
(Assignee)

Comment 5

9 years ago
Created attachment 293046 [details] [diff] [review]
code cleanups

This covers the code changes
Attachment #293046 - Flags: superreview?(dveditz)
Attachment #293046 - Flags: review?(benjamin)
Keywords: footprint
Version: unspecified → Trunk
(Assignee)

Comment 6

9 years ago
Created attachment 293049 [details]
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.
(Assignee)

Updated

9 years ago
Whiteboard: [has patch]
(Assignee)

Updated

9 years ago
Status: NEW → ASSIGNED
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.
Attachment #293046 - Flags: review?(benjamin) → review-
(Assignee)

Updated

9 years ago
Whiteboard: [has patch]
(Assignee)

Comment 8

9 years ago
Created attachment 294310 [details] [diff] [review]
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.
Attachment #293046 - Attachment is obsolete: true
Attachment #294310 - Flags: superreview?(dveditz)
Attachment #294310 - Flags: review?(benjamin)
Attachment #293046 - Flags: superreview?(dveditz)
(Assignee)

Updated

9 years ago
Whiteboard: [has patch]
(Assignee)

Comment 9

9 years ago
Created attachment 295026 [details] [diff] [review]
code cleanups rev 3

Updated for trunk and added a few license changes.
Attachment #294310 - Attachment is obsolete: true
Attachment #295026 - Flags: superreview?(dveditz)
Attachment #295026 - Flags: review?(benjamin)
Attachment #294310 - Flags: superreview?(dveditz)
Attachment #294310 - Flags: review?(benjamin)
Attachment #295026 - Flags: review?(benjamin) → review+
Blocks: 400501
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.
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.
(Assignee)

Comment 12

9 years ago
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!
> 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 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.
(Assignee)

Comment 15

9 years ago
Created attachment 298780 [details] [diff] [review]
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.
Attachment #295026 - Attachment is obsolete: true
Attachment #298780 - Flags: superreview?(dveditz)
Attachment #298780 - Flags: review+
Attachment #295026 - Flags: superreview?(dveditz)
(Assignee)

Comment 16

9 years ago
Created attachment 298781 [details] [diff] [review]
code cleanups rev 4

Full patch
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
Attachment #298780 - Flags: superreview?(dveditz) → superreview+
(Assignee)

Comment 18

9 years ago
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.
Attachment #298781 - Flags: superreview+
Attachment #298781 - Flags: review+
Attachment #298781 - Flags: approval1.9?
Comment on attachment 298781 [details] [diff] [review]
code cleanups rev 4

a1.9+=damons
Attachment #298781 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 20

9 years ago
Landed, seeing a 120K codesize win on linux, 70K on OSX
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
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?
Depends on: 415330
(Assignee)

Comment 22

9 years ago
Need docs updated on this and might want to highlight the changes for web-content in a release note.
Keywords: dev-doc-needed, relnote
Whiteboard: [has patch]
Depends on: 415912
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...
Blocks: 420496

Comment 24

9 years ago
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
Eric, I think dev-doc-needed is solved with your update on bug 420496?
Yes. :)
Keywords: dev-doc-needed → dev-doc-complete

Comment 27

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

Flash
LogMeIn
Java
etc.

Comment 28

9 years ago
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!
Depends on: 436207
Depends on: 450050
Blocks: 224602
Blocks: 352556
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.