1357205 - move built-in add-ons into omni.ja

Status: NEW

(Toolkit :: Add-ons Manager, enhancement, P3)

Description

[Robert Helmer [:rhelmer]]

System add-ons ship as separate XPIs in the app dir right now.

Shipping these inside the omni.ja instead would make it less trivial for malware to drop in unwanted replacements, and should also be a decent startup perf improvement (fewer files to open overall at startup, and benefit from omni.ja-specific optimizations like readahead).

We'd need to have a list of build-in add-on IDs (bug 1348981) first, since we don't want to cause unnecessary reads on omni.ja

Comment 1

[Robert Helmer [:rhelmer]]

Attachment: Bug 1357205 - provide a list of valid system add-ons for packaging

Review commit: https://reviewboard.mozilla.org/r/179916/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/179916/

Comment 2

[Robert Helmer [:rhelmer]]

Attachment: Bug 1357205 - create a base class for install locations in addons manager

Review commit: https://reviewboard.mozilla.org/r/179918/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/179918/

Comment 3

[Robert Helmer [:rhelmer]]

Attachment: Bug 1357205 - only load system add-ons from a built-in list

Review commit: https://reviewboard.mozilla.org/r/179920/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/179920/

Comment 5

[Masatoshi Kimura [:emk]]

How can we update system add-ons after this change? If we use the app dir for updated addons, it will have no security benefit (malware authors can still drop in malware add-ons). If we don't support updating system add-ons, why do we have to use addons in the first place?

Comment 6

[Florian Quèze [:florian]]

(In reply to Masatoshi Kimura [:emk] from comment #5)
> How can we update system add-ons after this change?

I think the profile dir is currently used for updated system add-ons.

These add-ons should be signed, so I don't see a risk of introducing malware here.

I care about this bug mostly to avoid the disk I/O during startup that opening these individual .xpi files currently causes.

Comment 7

[Andrew Swan [:aswan]]

(In reply to Florian Quèze [:florian] [:flo] from comment #6)
> (In reply to Masatoshi Kimura [:emk] from comment #5)
> > How can we update system add-ons after this change?
> 
> I think the profile dir is currently used for updated system add-ons.
> 
> These add-ons should be signed, so I don't see a risk of introducing malware
> here.

Yes, confirming that updates to system addons are read from the profile and they must be signed.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Hey florian, what priority would you set on this given its potential impact on start-up?

Comment 9

[Kris Maglione [:kmag]]

For what it's worth, there are probably other changes we can make to get us similar startup perf wins, which would still help when loading updated system add-ons from the profile. Adding support for non-Omnijar archives to the URLPreloader, and opening them in a background thread, would probably be the most helpful. But that's a bit tricky, and requires some careful locking in the jar cache code.

Either way, I think we should do this, but it probably shouldn't be the first priority for startup perf.

Comment 10

[(No longer employed by Mozilla) Aaron Klotz]

I should point out that moving that stuff into the omnijar does give you the benefit of the various optimizations that we do during PGO to make omnijar disk access as fast as possible.

(1) We reorder the archive members depending on which ones are accessed earliest in startup;
(2) We readahead those parts of the archive that will be used during startup, thus getting as much data into the OS filesystem cache as possible to reduce hard faults.

Comment 11

[Kris Maglione [:kmag]]

(In reply to Aaron Klotz [:aklotz] from comment #10)
> I should point out that moving that stuff into the omnijar does give you the
> benefit of the various optimizations that we do during PGO to make omnijar
> disk access as fast as possible.
> 
> (1) We reorder the archive members depending on which ones are accessed
> earliest in startup;
> (2) We readahead those parts of the archive that will be used during
> startup, thus getting as much data into the OS filesystem cache as possible
> to reduce hard faults.

That's true, but it's unfortunately only useful here when we're loading system add-ons from omni.ja. Once they update, the extra space their files take up in the pre-fetch header is actually a significant downside.

Also, the last I checked, we (somewhat unfortunately) seem to enable aggressive read-ahead on any zip archive we open from the jar cache on Windows.

Comment 12

[Florian Quèze [:florian]]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #11)

> That's true, but it's unfortunately only useful here when we're loading
> system add-ons from omni.ja. Once they update, the extra space their files
> take up in the pre-fetch header is actually a significant downside.

Are we really updating system add-ons often enough for that case to outweigh the normal case? Until now my assumption has been that most system add-ons aren't updated often (if at all).

Comment 13

[Robert Helmer [:rhelmer]]

Making the summary a little more generic - the real work here is making the add-ons manager able to load add-ons from a location in the omni jar, rather than assuming that each add-on maps to a XPI.

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #11)
> That's true, but it's unfortunately only useful here when we're loading
> system add-ons from omni.ja. Once they update, the extra space their files
> take up in the pre-fetch header is actually a significant downside.

In practice we tend to not update features shipped as add-ons very frequently. This might change in the future, but so far the primary use case has been supporting built-in features such as Screenshots which is actually a WebExtension but presented as a built-in feature.

Updates have been almost exclusively used for hotfixes and feature roll-outs, both of which generally don't override a built-in add-on. e10srollout was an exception here but we're not planning to use that same approach in the future.

Comment 14

[Robert Helmer [:rhelmer]]

A while back I managed to hack up a working prototype for loading add-ons from the omni jar, one bit I wasn't 100% sure about is that there is an AddonPathService added in bug 1017302 that maps add-on IDs to paths to the XPI on the filesystem.

It looks like it just treats paths as strings, so presumably we could use a jar URL. I'm not really sure what all this service is used for though and if they'll be able to handle this.

Comment 15

[Florian Quèze [:florian]]

(In reply to Mike Conley (:mconley) (:⚙️) (Totally backlogged on reviews and needinfos) from comment #8)
> Hey florian, what priority would you set on this given its potential impact
> on start-up?

P2, significant impact on cold startup (which unfortunately we currently don't measure).

Comment 16

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #14)
> A while back I managed to hack up a working prototype for loading add-ons
> from the omni jar, one bit I wasn't 100% sure about is that there is an
> AddonPathService added in bug 1017302 that maps add-on IDs to paths to the
> XPI on the filesystem.
> 
> It looks like it just treats paths as strings, so presumably we could use a
> jar URL. I'm not really sure what all this service is used for though and if
> they'll be able to handle this.

FWIW I looked into this and I think it is fine, that service seems to just be used for determining where on the filesystem an extension is so its memory use can be accounted for. We could either teach AddonPathService to resolve resource URLs, or just give it the filesystem path to the omni.ja

Comment 17

[Kris Maglione [:kmag]]

(In reply to Robert Helmer [:rhelmer] from comment #16)
> FWIW I looked into this and I think it is fine, that service seems to just
> be used for determining where on the filesystem an extension is so its
> memory use can be accounted for. We could either teach AddonPathService to
> resolve resource URLs, or just give it the filesystem path to the omni.ja

I deleted the AddonPathService ages ago.

Comment 18

[Robert Helmer [:rhelmer]]

(In reply to Kris Maglione [:kmag] from comment #17)
> (In reply to Robert Helmer [:rhelmer] from comment #16)
> > FWIW I looked into this and I think it is fine, that service seems to just
> > be used for determining where on the filesystem an extension is so its
> > memory use can be accounted for. We could either teach AddonPathService to
> > resolve resource URLs, or just give it the filesystem path to the omni.ja
> 
> I deleted the AddonPathService ages ago.

Hah, even better. Thanks.

Comment 19

[:Gijs (he/him)]

Rob, are you still actively working on this?

Comment 20

[Robert Helmer [:rhelmer]]

(In reply to :Gijs (he/him) from comment #19)

Rob, are you still actively working on this?

I am not, updated bug to reflect this. Happy to help anyone who wants to take it if needed.

Comment 21

[Andrew Swan [:aswan]]

I'd like to transform this bug into "deprecate system add-ons".
The immediate motivation is that we spend a lot of time (nearly 2 seconds in a recent profile on the 2018 reference device: https://perfht.ml/2KfLqkX) "installing" system add-ons (this is just initializing the addon manager state for them, the actual xpis are just read from the application directory) when creating a new profile. I suspect we spend a similar amount of time starting up after an application update, though I haven't measured it.

We currently ship a lot of code in system addons that we never update out-of-cycle. In some cases (screenshots, webcompat-reporter) this is since the code is written as an extension and system addons are currently the only way to deploy such an extension. I'm going to explore using the new built-in extension capability for screenshots in bug 1568270. In other cases (formautofill), the non-changing code is simply bundled with code that we want to update periodically (ie, site heuristics for finding form fields to autofill). Moving the non-changing code into built-in components will mitigate some of the startup cost of system addons but it will also yield other benefits. For instance, system addons can't use fluent and use various hacks to do localization. Moving code that uses localized strings to built-in components will let us use the same localization tools as other browser code, which will be much smoother. It will also make it easier to make changes to when these components are loaded and initialized during startup.

For the situations where we want to be able to ship changes out-of-cycle (eg webcompat interventions, formautofill heuristics), we have Normandy, which isn't able to deliver extensions in this way yet but that work is planned in bug 1436111. At least formautofill and perhaps webcompat will need a bit of work to separate the unchanging bits that we want to just ship with the browser from the parts that we want to be able to update on the fly, but that should be a modest amount of work. I'll file bugs for that work shortly. But once Normandy is ready, there's so reason for us to maintain two different methods to ship updates.

Comment 22

[:Gijs (he/him)]

(In reply to Andrew Swan [:aswan] from comment #21)

For the situations where we want to be able to ship changes out-of-cycle (eg webcompat interventions, formautofill heuristics), we have Normandy, which isn't able to deliver extensions in this way yet but that work is planned in bug 1436111.

We could also use remote settings here, for these two examples, I think? That may have some benefits over using normandy (mostly not needing to wrap code in extension wrappers at all anymore, which would likely keep some of the extant overhead). That said, for webcompat interventions we may need to do some thinking, given that we may in some cases need to block content loads until we've loaded interventions, in case they e.g. modify UA headers for the content requests, just like for blocking webrequest listeners in the add-on world...

At least formautofill and perhaps webcompat will need a bit of work to separate the unchanging bits that we want to just ship with the browser from the parts that we want to be able to update on the fly, but that should be a modest amount of work. I'll file bugs for that work shortly. But once Normandy is ready, there's so reason for us to maintain two different methods to ship updates.

Did you intend to file bugs blocking this bug?

Comment 23

[Nick Alexander :nalexander [he/him]]

(In reply to Andrew Swan [:aswan] from comment #21)

I'd like to transform this bug into "deprecate system add-ons".

I read this as "system add-ons => built-in add-ons, system add-on upgrades => remote settings". That sounds great: it would have avoided complicated races like that in Bug 1570690.

Comment 24

[Robert Helmer [:rhelmer]]

(In reply to Nick Alexander :nalexander [he/him] from comment #23)

(In reply to Andrew Swan [:aswan] from comment #21)

I'd like to transform this bug into "deprecate system add-ons".

I read this as "system add-ons => built-in add-ons, system add-on upgrades => remote settings". That sounds great: it would have avoided complicated races like that in Bug 1570690.

Yes I think it's fair to characterize it this way. The distinction between "Remote Settings" and "Normandy" isn't super useful for the purposes of this bug (Normandy uses Remote Settings to do its thing, for instance), but the gist is that Normandy is the thing that can do complex user targeting. System add-ons used AUS aka Balrog for this purpose (which wasn't as expressive as Normandy FWIW).

The goal of deprecating the system add-ons code in the add-ons manager should be the main thing for this bug, I think. One thing we'll need to take care about is to keep the old system going until we've phased out the last release we care about - for armag-add-on this was 2 ESRs back. This is mostly a server-side concern though I think ripping out the client code shouldn't be a problem, although I think we should have at least one release where both Normandy and SAOs are functional for delivering add-ons, just in case.

(In reply to :Gijs (he/him) from comment #22)

We could also use remote settings here, for these two examples, I think? That may have some benefits over using normandy (mostly not needing to wrap code in extension wrappers at all anymore, which would likely keep some of the extant overhead). That said, for webcompat interventions we may need to do some thinking, given that we may in some cases need to block content loads until we've loaded interventions, in case they e.g. modify UA headers for the content requests, just like for blocking webrequest listeners in the add-on world...

I agree that in general we should try to send data updates to users, and not privileged code. Sometimes extensions make sense but in many cases I think we can still update users rapidly and restartlessly without the overhead.

Comment 25

[Andrew Swan [:aswan]]

(In reply to :Gijs (he/him) from comment #22)

(In reply to Andrew Swan [:aswan] from comment #21)

For the situations where we want to be able to ship changes out-of-cycle (eg webcompat interventions, formautofill heuristics), we have Normandy, which isn't able to deliver extensions in this way yet but that work is planned in bug 1436111.

We could also use remote settings here, for these two examples, I think? That may have some benefits over using normandy (mostly not needing to wrap code in extension wrappers at all anymore, which would likely keep some of the extant overhead).

Maybe? This may be dated now but in the case of formautofill, :MattN told me at one point that form detection heuristics need to do such a variety of things that they are shipped as javascript rather than some sort of declarative pattern-matching rules. So, I think using remote settings would mean shipping a string that we take care to evaluate in a sandbox of some sort? I had originally thought that would be undesirable but perhaps it wouldn't be so bad.

That said, for webcompat interventions we may need to do some thinking, given that we may in some cases need to block content loads until we've loaded interventions, in case they e.g. modify UA headers for the content requests, just like for blocking webrequest listeners in the add-on world...

Yes, I think Tom has been thinking through this exact scenario and discussing it with :kmag. I'm not sure where that discussion left off or if there's an open bug for planned work on webcompat (ni to Tom for this question)

At least formautofill and perhaps webcompat will need a bit of work to separate the unchanging bits that we want to just ship with the browser from the parts that we want to be able to update on the fly, but that should be a modest amount of work. I'll file bugs for that work shortly. But once Normandy is ready, there's so reason for us to maintain two different methods to ship updates.

Did you intend to file bugs blocking this bug?

I did intend to, yes :/ As mentioned above, I think Tom and the webcompat team are already making plans. Tom, can you make any relevant bugs a dependency of this one? I can file bugs for webcompat-reporter and formautofill, leaving the ni to myself to remember to do that.

Comment 26

[Thomas Wisniewski [:twisniewski]]

There are no relevant bugs to mark as dependencies yet, as my proposal to kmag may not be worthwhile. Do we know how long it might take to block content loads until the webcompat addon starts up? If it won't take long, then my proposal is moot.

I was just concerned that it may take a long time before we can improve the webcompat module, so I proposed moving much of it into Core as its own module, so it could start up independently of the addon framework, and do so before content loads occur. But I wouldn't want to go this route if we can just fix the addons code relatively soon.

Of course we need webRequest and its related filter capabilities (modifying headers or incoming script content, blocking requests). We also need to be able to run content scripts before page scripts run.

But ideally the webcompat module needs to be able to detect more conditions on all content frames that we can currently detect with existing addon APIs. For instance, we cannot reliably tell if a broken JS library is loading based on its URL, because there are so many webpacked scripts out there. We need to be able to detect when (say) jQuery is defined on a window, and run some JS to check its version and fix it.

One way to handle that would be to allow the compat addon to run a simple content script on all content frames so it can detect the variables we're interested in, and potentially let it load further content scripts to polyfill things before the rest of the page scripts may run. Of course we could presumably also add a new addon filter which does the same for us.

(Note also that our addon registers the about:compat page using an experiment. It would be nice if that could happens before session restore tries to load it, resulting in an "address isn't valid" error page).

Comment 27

[Andrew Swan [:aswan]]

It looks like Shane is taking this over, I'm not sure if he wants to handle these extensions one-by-one or all at once.

It appears to me that that most straightforward path here is to use Normandy to move these to the built-in location and use Normandy to ship updates to them. I think this depends on bug 1613647, but there was some confusion about this on Matrix. Perhaps Shane could fill us in here on his plans...

Comment 28

[Shane Caraveo (:mixedpuppy)]

I don't have definitive plans. I took a look and it seemed simple to load these into omni.jar. My initial goal is to see if we still have issues or not per bug 1631932 comment 7.

to use Normandy to move these to the built-in location

I don't understand that, why would normandy need to move these?

use Normandy to ship updates to them

That's the plan, to transition from Balrog to Normandy, then shut Balrog down.

Comment 29

[Andrew Swan [:aswan]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #28)

to use Normandy to move these to the built-in location

I don't understand that, why would normandy need to move these?

Sorry, that was an abbreviated way of describing the approach of distributing the default addons in the builtin location and any out-of-cycle updates via Normandy. The current system add-on implementation is all-or-nothing, moving the defaults to a different location is going to cause it to think it has to download updated versions of everything. You could try changing the default system addon location to read from resource: urls but that would be throwaway work if we're just going to get rid of the old system addon implementation.

Comment 30

[Nick Alexander :nalexander [he/him]]

use Normandy to ship updates to them

That's the plan, to transition from Balrog to Normandy, then shut Balrog down.

Shane -- does "shut Balrog down" mean "stop using Balrog to serve add-on updates"? Or is it more general?

Comment 31

[Shane Caraveo (:mixedpuppy)]

(In reply to Nick Alexander :nalexander [he/him] from comment #30)

use Normandy to ship updates to them

That's the plan, to transition from Balrog to Normandy, then shut Balrog down.

Shane -- does "shut Balrog down" mean "stop using Balrog to serve add-on updates"? Or is it more general?

I suppose that balrog will be necessary for some period of time after normandy is delivering updates. I don't know what that period of time is, but yes, ultimately there would be some point in time where it could be shut down. Unfortunately, it probably will have to live long enough to serve ESR 78 (if we do that). I think the normandy team will have a better idea of a timeline in which they are handling the functionality that balrog provides.

Comment 32

[Robert Helmer [:rhelmer]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #31)

(In reply to Nick Alexander :nalexander [he/him] from comment #30)

use Normandy to ship updates to them

That's the plan, to transition from Balrog to Normandy, then shut Balrog down.

Shane -- does "shut Balrog down" mean "stop using Balrog to serve add-on updates"? Or is it more general?

I suppose that balrog will be necessary for some period of time after normandy is delivering updates. I don't know what that period of time is, but yes, ultimately there would be some point in time where it could be shut down. Unfortunately, it probably will have to live long enough to serve ESR 78 (if we do that). I think the normandy team will have a better idea of a timeline in which they are handling the functionality that balrog provides.

I suspect Nick's concern is around Balrog-the-server being shut down, vs. us not using it to serve system add-ons anymore. Nick is that correct?

If I have that right, then the answer is that Balrog will still be around for App Update and Media Plugins, just not System Add-ons.

Comment 33

[Nick Alexander :nalexander [he/him]]

(In reply to Robert Helmer [:rhelmer] from comment #32)

(In reply to Shane Caraveo (:mixedpuppy) from comment #31)

(In reply to Nick Alexander :nalexander [he/him] from comment #30)

use Normandy to ship updates to them

That's the plan, to transition from Balrog to Normandy, then shut Balrog down.

Shane -- does "shut Balrog down" mean "stop using Balrog to serve add-on updates"? Or is it more general?

I suppose that balrog will be necessary for some period of time after normandy is delivering updates. I don't know what that period of time is, but yes, ultimately there would be some point in time where it could be shut down. Unfortunately, it probably will have to live long enough to serve ESR 78 (if we do that). I think the normandy team will have a better idea of a timeline in which they are handling the functionality that balrog provides.

I suspect Nick's concern is around Balrog-the-server being shut down, vs. us not using it to serve system add-ons anymore. Nick is that correct?

Correct.

If I have that right, then the answer is that Balrog will still be around for App Update and Media Plugins, just not System Add-ons.

Thanks!