1192921 - Create a new install location for system add-ons

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

Unlike other install locations add-ons will be installed in here as <id>_<version>.xpi. It also maintains a "known-good set" somewhere, likely prefs. On startup read the known good set and check all those add-ons exist in the install location, if not copy over the known-good set from the application directory and update the known-good set to use those.

Comment 1

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1192921: Load XPIProviderUtils in a sandbox to simulate the way it is loaded in B2G.

Bug 1192921: Refactor add-on manager startup loop to better support validating install locations after scanning.

The add-ons manager recognises the notion of "install locations". Each location
can contain add-ons that are installed in the application. There are two main
types, directory locations which exist as a directory somewhere in the
filesystem and registry locations which exist in the Windows registry. The
profile location is the one where add-ons installed through the UI exist, the
other locations are for add-ons that are bundled with the application,
installed by the OS or by third-party applications.

Install locations have priorities. The profile location has the highest priority
then the others gradually lower priorities. When an add-on exists in more than
one install location the version in the highest priority location is the one
that is visible and can be active in the application. We still retain details
about the other versions in the database.

On every startup the add-ons manager scans over these install locations to see
if the set of installed add-ons has changed at all. A very quick check is done
to see if the more thorough check in processFileChanges (which synchronously
loads the add-ons database and install manifests for the add-ons) is needed.

The job of processFileChanges is to load information about all the add-ons and
update the add-ons database to match. It has to decide which add-ons to make
visible, track what changes were made to the visible set of add-ons and call
restartless add-ons install and uninstall scripts.

The original version of processFileChanges attempted to optimise this by doing
all of the work in a single loop over the add-ons in the locations. This mostly
worked but made certain situations difficult to handle (see bug 607818 f.e.).
There isn't much need for this level of optimisation. We're already in a slow
pass and once all the data is loaded off the disk looping over it is fast.

This changeset moves processFileChanges into the XPIProviderUtils file which is
lazy loaded when necessary. While most of the code is the same it instead does
one loop to update the database and gather information, then a second loop to
update add-on visibility, record changes and call bootstrap scripts.

Comment 2

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1192921: Refactor add-on manager startup loop to better support validating install locations after scanning. r=rhelmer

Bug 1192921: Remove most assumptions that add-on IDs match filenames.

Normal directory install locations expect add-ons to exist on disk with the
naming convention "<id>.xpi". Originally system add-ons were going to do
something different so I started working on this. In the end it is unnecessary
but this work did reveal some cases where _sourceBundle wasn't being updated
for add-ons and removing most of these assumptions is still valuable.

Comment 3

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1192921: Remove most assumptions that add-on IDs match filenames. r=rhelmer

Bug 1192921: Split DirectoryInstallLocation into an immutable and mutable version.

Most directory install locations are immutable at runtime. Only the profile
location can be installed into and uninstalled from. The system add-on locations
will be immutable as well but also be extended with some extra functionality so
it is useful to split the immutable parts out into a shared class that both
the mutable location and eventually system add-on locations can inherit from.

Comment 4

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1192921: Split DirectoryInstallLocation into an immutable and mutable version. r=rhelmer

Bug 1192921: Add an install location for system add-ons.

This adds two new directory install locations. One contains the default system
add-ons that ship with the application, the other contains system add-on that
will eventually be updatable at runtime.

The updatable location tracks the expected list of add-ons in a pref. and only
returns add-ons from that list when asked for its list of add-ons.

After processFileChanges has scanned all add-ons and updated the database it
checks if the updated system add-ons match the expected set. If not we ignore
those add-ons when working out which add-ons should be visible. If they do match
then we ignore the app-shipped system add-ons when working out which are
visible.

Comment 5

[Dave Townsend [:mossop]]

Sorry for the size of the first changeset, unfortunately code moves of that size aren't easy to review so let me know if I can help in any way. Also note that we have a lot of tests verifying that the startup code does what it is expected to do so I have a very high confidence that these changes aren't regressing anything,

Comment 6

[Robert Helmer [:rhelmer]]

Comment on attachment 8654378 [details]
MozReview Request: Bug 1192921: Split DirectoryInstallLocation into an immutable and mutable version. r=rhelmer

https://reviewboard.mozilla.org/r/17675/#review15877

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:2326
(Diff revision 1)
> +        logger.debug("Skipping unavailable install location " + aName);

Is it worth logging the exception, in case this is e.g. permissions or other problem?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:2326
(Diff revision 1)
> +        logger.debug("Skipping unavailable install location " + aName);

Is it worth logging the exception, in case this is e.g. permissions or other problem?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:7253
(Diff revision 1)
> +  this._baseDir = aDirectory;

`this._baseDir` is not used.

::: toolkit/mozapps/extensions/test/xpcshell/test_system_reset.js:60
(Diff revision 1)
> +        do_throw("Expected pref to me missing");

s/me/be/ ?

Overall this looks good to me, and seems to work - I am able to add an addon set using the pref and have it installed from the app directory, and apps are overridden correctly if I install a new version of the add-on locally (and the system add-on is re-activated if it's removed). System add-ons currently show up in the add-on manager (bug 1192926 will change this) and they can be disabled but not removed.

I only see a few minor issues, r+ from me with those fixed.

Comment 7

[Robert Helmer [:rhelmer]]

Comment on attachment 8654375 [details]
MozReview Request: Bug 1192921: Load XPIProviderUtils in a sandbox to simulate the way it is loaded in B2G.

https://reviewboard.mozilla.org/r/17669/#review16351

Comment 8

[Robert Helmer [:rhelmer]]

Comment on attachment 8654376 [details]
MozReview Request: Bug 1192921: Refactor add-on manager startup loop to better support validating install locations after scanning. r=rhelmer

https://reviewboard.mozilla.org/r/17671/#review16353

Comment 9

[Robert Helmer [:rhelmer]]

Comment on attachment 8654377 [details]
MozReview Request: Bug 1192921: Remove most assumptions that add-on IDs match filenames. r=rhelmer

https://reviewboard.mozilla.org/r/17673/#review16355

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d8233b994741
https://hg.mozilla.org/integration/fx-team/rev/c3009691dfe6
https://hg.mozilla.org/integration/fx-team/rev/f310cab1dd4e
https://hg.mozilla.org/integration/fx-team/rev/1bfa2dead61f

Comment 11

[Phil Ringnalda (:philor)]

b2g's Gu suite is going to continue to haunt you with its unexpected addon usage until you somehow cure it of its odd ways. Backed out in https://hg.mozilla.org/integration/fx-team/rev/65e92b72d584 for hanging Gu like https://treeherder.mozilla.org/logviewer.html#?job_id=4537716&repo=fx-team

Comment 12

[Dave Townsend [:mossop]]

Comment on attachment 8654375 [details]
MozReview Request: Bug 1192921: Load XPIProviderUtils in a sandbox to simulate the way it is loaded in B2G.

Bug 1192921: Load XPIProviderUtils in a sandbox to simulate the way it is loaded in B2G.

Comment 13

[Dave Townsend [:mossop]]

Comment on attachment 8654376 [details]
MozReview Request: Bug 1192921: Refactor add-on manager startup loop to better support validating install locations after scanning. r=rhelmer

Bug 1192921: Refactor add-on manager startup loop to better support validating install locations after scanning. r=rhelmer

The add-ons manager recognises the notion of "install locations". Each location
can contain add-ons that are installed in the application. There are two main
types, directory locations which exist as a directory somewhere in the
filesystem and registry locations which exist in the Windows registry. The
profile location is the one where add-ons installed through the UI exist, the
other locations are for add-ons that are bundled with the application,
installed by the OS or by third-party applications.

Install locations have priorities. The profile location has the highest priority
then the others gradually lower priorities. When an add-on exists in more than
one install location the version in the highest priority location is the one
that is visible and can be active in the application. We still retain details
about the other versions in the database.

On every startup the add-ons manager scans over these install locations to see
if the set of installed add-ons has changed at all. A very quick check is done
to see if the more thorough check in processFileChanges (which synchronously
loads the add-ons database and install manifests for the add-ons) is needed.

The job of processFileChanges is to load information about all the add-ons and
update the add-ons database to match. It has to decide which add-ons to make
visible, track what changes were made to the visible set of add-ons and call
restartless add-ons install and uninstall scripts.

The original version of processFileChanges attempted to optimise this by doing
all of the work in a single loop over the add-ons in the locations. This mostly
worked but made certain situations difficult to handle (see bug 607818 f.e.).
There isn't much need for this level of optimisation. We're already in a slow
pass and once all the data is loaded off the disk looping over it is fast.

This changeset moves processFileChanges into the XPIProviderUtils file which is
lazy loaded when necessary. While most of the code is the same it instead does
one loop to update the database and gather information, then a second loop to
update add-on visibility, record changes and call bootstrap scripts.

Comment 14

[Dave Townsend [:mossop]]

Comment on attachment 8654377 [details]
MozReview Request: Bug 1192921: Remove most assumptions that add-on IDs match filenames. r=rhelmer

Bug 1192921: Remove most assumptions that add-on IDs match filenames. r=rhelmer

Normal directory install locations expect add-ons to exist on disk with the
naming convention "<id>.xpi". Originally system add-ons were going to do
something different so I started working on this. In the end it is unnecessary
but this work did reveal some cases where _sourceBundle wasn't being updated
for add-ons and removing most of these assumptions is still valuable.

Comment 15

[Dave Townsend [:mossop]]

Comment on attachment 8654378 [details]
MozReview Request: Bug 1192921: Split DirectoryInstallLocation into an immutable and mutable version. r=rhelmer

Bug 1192921: Split DirectoryInstallLocation into an immutable and mutable version. r=rhelmer

Most directory install locations are immutable at runtime. Only the profile
location can be installed into and uninstalled from. The system add-on locations
will be immutable as well but also be extended with some extra functionality so
it is useful to split the immutable parts out into a shared class that both
the mutable location and eventually system add-on locations can inherit from.

Comment 16

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1192921: Add an install location for system add-ons. r=rhelmer

Bug 1192921: Add an install location for system add-ons. r=rhelmer

This adds two new directory install locations. One contains the default system
add-ons that ship with the application, the other contains system add-on that
will eventually be updatable at runtime.

The updatable location tracks the expected list of add-ons in a pref. and only
returns add-ons from that list when asked for its list of add-ons.

After processFileChanges has scanned all add-ons and updated the database it
checks if the updated system add-ons match the expected set. If not we ignore
those add-ons when working out which add-ons should be visible. If they do match
then we ignore the app-shipped system add-ons when working out which are
visible.

Comment 17

[Dave Townsend [:mossop]]

Attachment: patch

MozReview got terribly confused by me adding a new changeset to the start of the series so here is a patch showing what has actually changed across the whole set, it should be easier to review.

B2G does weird things with JS module globals meaning that code loaded by the subscript loader can't see a lot of the XPIProvider code unless we explicitly pass it over. This patch first switches to loading XPIProviderUtils into a sandbox, this essentially mimics what B2G does so we get the same effects on desktop and are less likely to break things like this again. It also adds the stuff that XPIProviderUtils needs for the new code.

Comment 18

[Dave Townsend [:mossop]]

https://reviewboard.mozilla.org/r/17667/#review16551

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/b5151c071c9d
https://hg.mozilla.org/integration/fx-team/rev/6a26e9891333
https://hg.mozilla.org/integration/fx-team/rev/92d8e5a2c3c2
https://hg.mozilla.org/integration/fx-team/rev/755c7faea24a
https://hg.mozilla.org/integration/fx-team/rev/5f5ffdb6dba3

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b5151c071c9d
https://hg.mozilla.org/mozilla-central/rev/6a26e9891333
https://hg.mozilla.org/mozilla-central/rev/92d8e5a2c3c2
https://hg.mozilla.org/mozilla-central/rev/755c7faea24a
https://hg.mozilla.org/mozilla-central/rev/5f5ffdb6dba3