Move add-on installation code to a separate JSM

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: kmag, Assigned: aswan)

Tracking

(Blocks 1 bug)

unspecified
mozilla61
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox61 fixed)

Details

(Whiteboard: [triaged][fxperf:p3])

Attachments

(14 attachments)

59 bytes, text/x-review-board-request
kmag
: review+
Details
59 bytes, text/x-review-board-request
mccr8
: review+
Details
59 bytes, text/x-review-board-request
aswan
: review+
Details
59 bytes, text/x-review-board-request
aswan
: review+
Details
59 bytes, text/x-review-board-request
aswan
: review+
Details
59 bytes, text/x-review-board-request
aswan
: review+
Details
59 bytes, text/x-review-board-request
zombie
: review+
Details
59 bytes, text/x-review-board-request
zombie
: review+
Details
59 bytes, text/x-review-board-request
zombie
: review+
Details
59 bytes, text/x-review-board-request
aswan
: review+
Details
59 bytes, text/x-review-board-request
aswan
: review+
Details
59 bytes, text/x-review-board-request
aswan
: review+
Details
59 bytes, text/x-review-board-request
aswan
: review+
Details
59 bytes, text/x-review-board-request
aswan
: review+
Details
We currently load a lot of code in XPIProvider.jsm that isn't needed at all in the vast majority of sessions, which adds a lot of unnecessary overhead to startup. This in particular applies to the code only needed during add-on installation, including code for parsing install and update manifests.

There's quite a lot of code in XPIProviderUtils which is likewise only needed when installing new add-ons, but not when simply loading the database to read metadata.

All of this code, and anything else that isn't needed during startup, should be moved to a separate JSM.
Priority: -- → P1
Whiteboard: [triaged]
I got part way through moving the install and update code into its own jsm.  There are of course dependencies between that code and the code currently in XPIProvider.jsm but also between that code and the code in XPIProviderUtils.js.  Is there a good reason to have 3 separate files here?  I'm thinking the current (intended) distinction between stuff needed at startup in XPIProvider.jsm and everything else in XPIProviderUtils.js should be sufficient and I can just move a bunch more stuff into XPIProviderUtils.js.  Am I overlooking something?
Flags: needinfo?(kmaglione+bmo)
Yes. Having a separate JSM will allow us to delay loading the installation code and update code later than the database code, since it's needed much less often, and allow it to be unloaded when idle after bug 1354820 lands.
Flags: needinfo?(kmaglione+bmo)
(In reply to Andrew Swan [:aswan] from comment #1)
> I got part way through moving the install and update code into its own jsm. 
> There are of course dependencies between that code and the code currently in
> XPIProvider.jsm but also between that code and the code in
> XPIProviderUtils.js.  Is there a good reason to have 3 separate files here? 
> I'm thinking the current (intended) distinction between stuff needed at
> startup in XPIProvider.jsm and everything else in XPIProviderUtils.js should
> be sufficient and I can just move a bunch more stuff into
> XPIProviderUtils.js.  Am I overlooking something?

It's worth considering at this point giving these more descriptive names - I don't find "Utils" to be particularly helpful in identifying how the code is split up.
Keywords: leave-open
Attachment #8870912 - Flags: review?(kmaglione+bmo)
Comment on attachment 8870912 [details]
Bug 1363925 Split some install code out of XPIProvider.jsm

https://reviewboard.mozilla.org/r/142478/#review146248

\o/

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1105
(Diff revision 1)
> - * the file when it is no longer needed.
>   *
> - * @return an nsIFile that points to a randomly named, initially empty file in
> - *         the OS temporary files directory
> + * @param  aFile
> + *         The nsIFile to remove
>   */
> -function getTemporaryFile() {
> +function recursiveRemove(aFile) {

Any reason this can't be moved to XPIInstall.jsm?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6893
(Diff revision 1)
> +Object.defineProperty(this.XPIInternal, "XPIDatabase", {
> +  get: () => gGlobalScope.XPIDatabase
> +});

Just add `get XPIDatabase() { return gGlobalScope.XPIDatabase; }` to `XPIInternal`.
Attachment #8870912 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8870912 [details]
Bug 1363925 Split some install code out of XPIProvider.jsm

https://reviewboard.mozilla.org/r/142478/#review146248

> Any reason this can't be moved to XPIInstall.jsm?

that's on my hit list for round 2
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s f3839056eabb -d f8948f4cb4ac: rebasing 398219:f3839056eabb "Bug 1363925 Split some install code out of XPIProvider.jsm r=kmag" (tip)
merging toolkit/mozapps/extensions/internal/XPIProvider.jsm and toolkit/mozapps/extensions/internal/XPIInstall.jsm to toolkit/mozapps/extensions/internal/XPIInstall.jsm
merging toolkit/mozapps/extensions/internal/XPIProvider.jsm
warning: conflicts while merging toolkit/mozapps/extensions/internal/XPIInstall.jsm! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5ae2cc64391c
Split some install code out of XPIProvider.jsm r=kmag
Dropping priority for followup work here...
Priority: P1 → P3
Duplicate of this bug: 846241
Whiteboard: [triaged] → [triaged][fxperf]
Whiteboard: [triaged][fxperf] → [triaged][fxperf:p3]
Attachment #8969963 - Flags: review?(aswan) → review?(tomica)
Attachment #8969964 - Flags: review?(aswan) → review?(tomica)
Attachment #8969965 - Flags: review?(aswan) → review?(tomica)
Comment on attachment 8969959 [details]
Bug 1363925: Part 2 - Support inferring array length from typed arrays.

https://reviewboard.mozilla.org/r/238740/#review244648

I'm not really familiar with this API, but it looks okay to me...
Attachment #8969959 - Flags: review?(continuation) → review+
Comment on attachment 8969960 [details]
Bug 1363925: Part 3 - Move more install logic from XPIProvider to XPIInstall.

https://reviewboard.mozilla.org/r/238742/#review244682

::: xpcom/io/nsIBinaryOutputStream.idl:58
(Diff revision 2)
> -    void writeBytes([size_is(aLength)] in string aString, in uint32_t aLength);
> +    void writeBytes([size_is(aLength)] in string aString,
> +                    [optional] in uint32_t aLength);

was this meant to be in the previous patch?
(In reply to Andrew Swan [:aswan] from comment #36)
> Comment on attachment 8969960 [details]
> Bug 1363925: Part 3 - Move more install logic from XPIProvider to XPIInstall.
> 
> https://reviewboard.mozilla.org/r/238742/#review244682
> 
> ::: xpcom/io/nsIBinaryOutputStream.idl:58
> (Diff revision 2)
> > -    void writeBytes([size_is(aLength)] in string aString, in uint32_t aLength);
> > +    void writeBytes([size_is(aLength)] in string aString,
> > +                    [optional] in uint32_t aLength);
> 
> was this meant to be in the previous patch?

Nope. It more or less depends on the previous patch, but it's needed for this patch, and seemed like a trivial enough change not to need its own.
Comment on attachment 8969960 [details]
Bug 1363925: Part 3 - Move more install logic from XPIProvider to XPIInstall.

https://reviewboard.mozilla.org/r/238742/#review244724
Attachment #8969960 - Flags: review?(aswan) → review+
Comment on attachment 8969961 [details]
Bug 1363925: Part 4 - Move XPIProvider install methods to XPIInstall.

https://reviewboard.mozilla.org/r/238744/#review244730
Attachment #8969961 - Flags: review?(aswan) → review+
Comment on attachment 8969962 [details]
Bug 1363925: Part 5 - Move startup update check logic to XPIInstall.

https://reviewboard.mozilla.org/r/238746/#review244732

Hm, see also bug 1433574
Comment on attachment 8969962 [details]
Bug 1363925: Part 5 - Move startup update check logic to XPIInstall.

https://reviewboard.mozilla.org/r/238746/#review244742

If we need to get an ack from product before doing bug 1433574 then this is good in the mean time
Attachment #8969962 - Flags: review?(aswan) → review+
Comment on attachment 8969963 [details]
Bug 1363925: Part 6 - Move staged add-on install logic to XPIInstall.

https://reviewboard.mozilla.org/r/238748/#review244746
Attachment #8969963 - Flags: review+
Comment on attachment 8969963 [details]
Bug 1363925: Part 6 - Move staged add-on install logic to XPIInstall.

https://reviewboard.mozilla.org/r/238748/#review244748
Comment on attachment 8970041 [details]
Bug 1363925: Part 8a - Migrate XPIProviderUtils.js to XPIDatabase.jsm.

https://reviewboard.mozilla.org/r/238794/#review244752
Attachment #8970041 - Flags: review?(aswan) → review+
Comment on attachment 8970042 [details]
Bug 1363925: Part 8b - Move AddonInternal to XPIDatabase.jsm.

https://reviewboard.mozilla.org/r/238796/#review244756
Attachment #8970042 - Flags: review?(aswan) → review+
Comment on attachment 8970043 [details]
Bug 1363925: Part 8c - Move isUsableAddon to XPIDatabase.jsm.

https://reviewboard.mozilla.org/r/238798/#review244758
Attachment #8970043 - Flags: review?(aswan) → review+
Comment on attachment 8970044 [details]
Bug 1363925: Part 8d - Move updateAddonDisabledState to XPIDatabase.

https://reviewboard.mozilla.org/r/238800/#review244764

This is a general comment, not specific to this (sub-)part.  I think it would be helpful to add comments somewhere explaining the purposes of the various XPIfoo.jsm files, explaining the goal of loading just the bare minimum amount of code for "normal case" startup, and explaining how somebody adding something new should decide in which file it should live.  Near the top of XPIProvider.jsm would be as good a place as any I guess.
Attachment #8970044 - Flags: review?(aswan) → review+
Comment on attachment 8970045 [details]
Bug 1363925: Part 8e - Convert AddonInternal classes to ES6 classes.

https://reviewboard.mozilla.org/r/238802/#review244766
Attachment #8970045 - Flags: review?(aswan) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/af2ff7d301a7b481423a8ec172f92c8bbacaf285
Bug 1363925: Part 2 - Support inferring array length from typed arrays. r=mccr8

https://hg.mozilla.org/integration/mozilla-inbound/rev/cffb4b6e971a38219fab7fa49fd8d6565e986394
Bug 1363925: Part 3 - Move more install logic from XPIProvider to XPIInstall. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/801a6d33fa88b3030a3989d602a34237fb467e5c
Bug 1363925: Part 4 - Move XPIProvider install methods to XPIInstall. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/afd6f3e6e6af154925a2c13bd363d38187cecf2a
Bug 1363925: Part 6 - Move staged add-on install logic to XPIInstall. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee1bdb001079770b20899df675e6cd0640a2d5fe
Bug 1363925: Part 7a - Turn on valid-jsdoc rule for XPIInstall.jsm. r=me,npotb

https://hg.mozilla.org/integration/mozilla-inbound/rev/2132b4cf18db3a8c0ab9ffc4977be05ec58748b1
Bug 1363925: Part 7b - Turn on valid-jsdoc rule for XPIProvider.jsm. r=me,npotb

https://hg.mozilla.org/integration/mozilla-inbound/rev/fde77a548f547094e5c06a30f927175c03ad23ae
Bug 1363925: Part 7c - Turn on valid-jsdoc rule for XPIProviderUtils.js. r=me,npotb

https://hg.mozilla.org/integration/mozilla-inbound/rev/20d1663e4a8cd7c1aa2bdef66c6b0e92bf8b81f5
Bug 1363925: Part 8a - Migrate XPIProviderUtils.js to XPIDatabase.jsm. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/8adbe703aeba227b6ec186c306a42b291b0b627f
Bug 1363925: Part 8b - Move AddonInternal to XPIDatabase.jsm. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/1646b51d323f8d98211fb48436d6204b22c6044e
Bug 1363925: Part 8c - Move isUsableAddon to XPIDatabase.jsm. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/c948486ee8e1faed6cd9f6200771ae40a9232e6a
Bug 1363925: Part 8d - Move updateAddonDisabledState to XPIDatabase. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ad2e050c66a73c748761d451d2c7a5339501773
Bug 1363925: Part 8e - Convert AddonInternal classes to ES6 classes. r=aswan
Comment on attachment 8969964 [details]
Bug 1363925: Part 7a - Turn on valid-jsdoc rule for XPIInstall.jsm.

https://reviewboard.mozilla.org/r/238750/#review244960

::: toolkit/mozapps/extensions/internal/XPIInstall.jsm:427
(Diff revision 2)
>   * switch between versions.
>   *
>   * @param {string} oldVersion The version of the existing extension instance.
>   * @param {string} newVersion The version of the extension being installed.
>   *
> - * @return {BOOSTRAP_REASONS.ADDON_UPGRADE|BOOSTRAP_REASONS.ADDON_DOWNGRADE}
> + * @returns {integer}

nit: JSDocs (and typescript, as I'm finding out these days) only know about `{number}`.

::: toolkit/mozapps/extensions/internal/XPIInstall.jsm:653
(Diff revision 2)
> -   *         The nsIRDFDatasource to read from
> -   * @param  aSource
> -   *         The nsIRDFResource to read the properties from
> -   * @param  isDefault
> -   *         True if the locale is to be read from the main install manifest
> -   *         root
> +   *         The datasource to read from.
> +   * @param {nsIRDFResource} aSource
> +   *         The resource to read the properties from.
> +   * @param {boolean} isDefault
> +   *        True if the locale is to be read from the main install manifest
> +   *        root

punctuation consistency!

::: toolkit/mozapps/extensions/internal/XPIInstall.jsm:1089
(Diff revision 2)
>  
>  /**
>   * Verifies that a bundle's contents are all correctly signed by an
>   * AMO-issued certificate
>   *
> - * @param  aBundle
> + * @param {nsIFile}aBundle

nit: space

::: toolkit/mozapps/extensions/internal/XPIInstall.jsm:1093
(Diff revision 2)
>   *
> - * @param  aBundle
> - *         the nsIFile for the bundle to check, either a directory or zip file
> - * @param  aAddon
> - *         the add-on object to verify
> - * @return a Promise that resolves to an AddonManager.SIGNEDSTATE_* constant.
> + * @param {nsIFile}aBundle
> + *        The nsIFile for the bundle to check, either a directory or zip file.
> + * @param {AddonInternal} aAddon
> + *        The add-on object to verify.
> + * @returns {Prommise<number>}

is that a British spelling?

::: toolkit/mozapps/extensions/internal/XPIInstall.jsm:1152
(Diff revision 2)
>  }
>  
>  /**
>   * Converts an iterable of addon objects into a map with the add-on's ID as key.
> + *
> + * @param {sequence<AddonInternal>} addons

Not sure what a `sequence` is exactly, but this would probably just be an `Array<>` or `ArrayLike<>` in TS (or `{{map(callback: (a: AddonInternal) => any)}}` if you want to be exact).

::: toolkit/mozapps/extensions/internal/XPIInstall.jsm:1479
(Diff revision 2)
> - * @param  aDir
> - *         Directory to look at
> - * @param  aSortEntries
> - *         True to sort entries by filename
> - * @return An array of nsIFile, or an empty array if aDir is not a readable directory
> + * @param {nsIFile} aDir
> + *        Directory to look at
> + * @param {boolean} aSortEntries
> + *        True to sort entries by filename
> + * @returns {nsIFile[]}
> + *        An array of nsIFile, or an empty array if aDir is not a readable directory

this is now restating the type instead of explaining the semantics.

::: toolkit/mozapps/extensions/internal/XPIInstall.jsm:1541
(Diff revision 2)
> -   *         The add-on this install will update if known
> -   * @param  options.name
> -   *         An optional name for the add-on
> -   * @param  options.type
> -   *         An optional type for the add-on
> -   * @param  options.icons
> +   *        The add-on this install will update if known
> +   * @param {string} [options.name]
> +   *        An optional name for the add-on
> +   * @param {string} [options.type]
> +   *        An optional type for the add-on
> +   * @param {object} [options.icons]

nit: consistency, should probably prefer `object`, like `string`, `number` or `function` (you don't actually mean `instanceof Object`).

::: toolkit/mozapps/extensions/internal/XPIInstall.jsm:3131
(Diff revision 2)
> -   *         The ID of the add-on to install
> -   * @param  source
> -   *         The source nsIFile to install from
> -   * @param  existingAddonID
> -   *         The ID of an existing add-on to uninstall at the same time
> -   * @param  action
> +   *        Installation options.
> +   * @param {string} options.id
> +   *        The ID of the add-on to install
> +   * @param {nsIFile} options.source
> +   *        The source nsIFile to install from
> +   * @param {string?} [options.existingAddonID]

Double optional?  Does that make it forbidden?
Attachment #8969964 - Flags: review?(tomica) → review+
Attachment #8969963 - Flags: review?(tomica)
Status: NEW → RESOLVED
Closed: Last year
Flags: qe-verify-
Keywords: leave-open
Resolution: --- → FIXED
Comment on attachment 8969965 [details]
Bug 1363925: Part 7b - Turn on valid-jsdoc rule for XPIProvider.jsm.

https://reviewboard.mozilla.org/r/238752/#review245110

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:708
(Diff revision 2)
> + * Yes, this description is extremely helpful. How dare you question its
> + * utility?

O_O

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:2147
(Diff revision 2)
>     * distributed add-on.
>     *
> -   * @param  aManifests
> -   *         A dictionary to add new install manifests to to save having to
> -   *         reload them later
> -   * @param  aAppChanged
> +   * @param {Object} aManifests
> +   *        A dictionary to add new install manifests to to save having to
> +   *        reload them later
> +   * @param {string} [aAppChanged]

boolean?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:2394
(Diff revision 2)
>     * mimetype.
>     *
> -   * @param  aMimetype
> -   *         The mimetype to check for
> -   * @return true if the mimetype is application/x-xpinstall
> +   * @param {string} aMimetype
> +   *        The mimetype to check for
> +   * @returns {boolean}
> +   *        True if the mimetype is application/x-xpinstall

hmm, comment managed to be longer than the code

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:2425
(Diff revision 2)
>    },
>  
>    /**
>     * Returns an Addon corresponding to an instance ID.
> -   * @param aInstanceID
> +   *
> +   * @param {Symbol} aInstanceID

huh, are we really using `Symbols` for this?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4173
(Diff revision 2)
>    /**
>     * Reads a directory linked to in a file.
>     *
> -   * @param   file
> -   *          The file containing the directory path
> -   * @return  An nsIFile object representing the linked directory.
> +   * @param {nsIFile} aFile
> +   *        The file containing the directory path
> +   * @returns {nsIFile}

can return null

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4305
(Diff revision 2)
> +   *
> +   * @param {boolean} [rescan = false]
> +   *        True if the directory should be re-scanned, even if it has
> +   *        already been initialized.
> +   *
> +   * @returns {nsIFile[]}

I guess Map is kinda like an array
Attachment #8969965 - Flags: review?(tomica) → review+
Comment on attachment 8969964 [details]
Bug 1363925: Part 7a - Turn on valid-jsdoc rule for XPIInstall.jsm.

https://reviewboard.mozilla.org/r/238750/#review244960

> nit: JSDocs (and typescript, as I'm finding out these days) only know about `{number}`.

Hm. I guess I'll make it a typedef. I'd rather keep the assertion that this should only be an integer.

> punctuation consistency!

Meh. It annoys me too, but I mostly decided not to fix the lack of punctuation in the existing docs.

> Not sure what a `sequence` is exactly, but this would probably just be an `Array<>` or `ArrayLike<>` in TS (or `{{map(callback: (a: AddonInternal) => any)}}` if you want to be exact).

It's WebIDL notation. It essentially means any iterable object.

If TS doesn't understand it, maybe I can typedef it to ArrayLike. I'd rather keep our type names closer to our WebIDL definitions where possible...

> nit: consistency, should probably prefer `object`, like `string`, `number` or `function` (you don't actually mean `instanceof Object`).

Not sure what you mean. I *did* use `object` here. That said, I tend to use `Object` in cases where I mean a plain object (like an object literal), and `object` where I mean, more or less, `typeof thing === "object"`

> Double optional?  Does that make it forbidden?

They mean different things. `string?` means the value can be null. Braces mean the property can be omitted. Optional `string?` means it can be present and null.
Comment on attachment 8970040 [details]
Bug 1363925: Part 7c - Turn on valid-jsdoc rule for XPIProviderUtils.js.

https://reviewboard.mozilla.org/r/238792/#review245138

::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:78
(Diff revision 1)
> + * @param {AddonInternal} aAddon
> + *        The add-on to annotate.
> + * @returns {AddonInternal}
> + *        The annotated add-on.
>   */
>  async function getRepositoryAddon(aAddon) {

great method name!

::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:188
(Diff revision 1)
> + *        The filter predecate. The first add-on for which it returns
> + *        true will be returned.

could be worded better

::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:490
(Diff revision 1)
>     * Reconstruct when the DB file exists but is unreadable
>     * (for example because read permission is denied)
> +   *
> +   * @param {Error} aError
> +   *        The error that triggered the rebuild.
> +   * @param {boolean} aRebuildOnError

Rebuild the db.  On error, rebuild the db?

::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:512
(Diff revision 1)
>     * necessary. If any DB load operation fails, we need to
>     * synchronously rebuild the DB from the installed extensions.
>     *
> -   * @return Promise<Map> resolves to the Map of loaded JSON data stored
> -   *         in this.addonDB; never rejects.
> +   * @returns {Promise<AddonDB>}
> +   *        Resolves to the Map of loaded JSON data stored in
> +   *        this.addonDB; never rejects.

A bold claim.

::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:748
(Diff revision 1)
>    },
>  
>    /**
>     * Synchronously gets all add-ons of a particular type(s).
>     *
> -   * @param  aType, aType2, ...
> +   * @param {Array<string>} aTypes

I think you can keep `{...string}`, at least TS understands it.

::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:1193
(Diff revision 1)
> -   *         The new state of the add-on
> -   * @param  aNewAddon
> -   *         The manifest for the new add-on if it has already been loaded
> -   * @return a boolean indicating if flushing caches is required to complete
> -   *         changing this add-on
> +   *        The new state of the add-on
> +   * @param {AddonInternal?} [aNewAddon]
> +   *        The manifest for the new add-on if it has already been loaded
> +   * @returns {boolean?}
> +   *        A boolean indicating if flushing caches is required to complete
> +   *        changing this add-on

An optional boolean return type?  It's either a mistake or needs to be documented what undefined means.
Attachment #8970040 - Flags: review?(tomica) → review+
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.