1009332 - Allow using type="jetpack" for native SDK add-ons internally but present through the API as type="extension"

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Erik Vold [:erikvold] (please needinfo? me)]

Since the plan for native jetpacks is to use Addon.type = 'extension' as it is for bootstrapped extensions, I was hoping to add a `jetpack` property to the Addon object, as there is a `bootstrap` property now, used to distinguish restartless add-ons from old school add-ons.

Comment 1

[Erik Vold [:erikvold] (please needinfo? me)]

Are you guys ok with this change?

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

What's the rationale behind needing this?

Comment 3

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Blair McBride [:Unfocused] from comment #2)
> What's the rationale behind needing this?

To distinguish jetpacks quickly, the alternative would be to provide a `isJetpack` function which would have to check for a `package.json` file on each call, which would slow startup times I think.

As I understand things, some Addon metadata is stored in some db (in the XPIProviderUtils.js file), which is used to cache addon metadata to speed up startup times.  If this is correct then this new `jetpack` property would be added to this db which XPIProviderUtils.js uses add speed up startup time because we don't check for an install.rdf first for one thing.

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Hmm, I'm generally wary of adding properties that are specific to only one add-on type. The Addon interface is meant to be generically applicable to any type of add-on (XPI extensions, lightweight themes, NPAPI plugins, user scripts, etc etc).

So what I'm really asking is:
(1) Why does any code outside of XPIProvider/the bootstrap script need to know if an extension uses the Jetpack SDK or not?
(2) Is there an alternate way to expose this, as a more generic API? 

For (2), I've sometimes wondered if we should add something like an API that exposes a (read-only?) Set of strings as flags, which would let providers surface these types of things more generically.

Comment 5

[:Irving Reid (No longer working on Firefox)]

Also, if this is information needed at every start up, please *don't* put it in XPIProviderUtils / extensions.json; we don't load XPIProviderUtils.js or extensions.json on the fast path. We currently use a mix of 'extensions.ini' (for non-bootstrapped XPIs) and a preference (for bootstrap).

The existing extensions.ini/pref mix desperately needs to be cleaned up and made properly async...

Comment 6

[Dave Townsend [:mossop]]

Same questions as Blair...

Comment 7

[Cody Crews]

Maybe what Erik is looking for is something pushing addons to be a bit more standardized and even listed as jetpack(no restart) BUT I think this is already the case and since the addons are mainly applicable to mozilla product I myself am confused here too.  Have you tried to implement this locally and if so have you done some benchmark on start up times to see if there actually is an improvement?

Comment 8

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to :Irving Reid from comment #5)
> Also, if this is information needed at every start up, please *don't* put it
> in XPIProviderUtils / extensions.json; we don't load XPIProviderUtils.js or
> extensions.json on the fast path. We currently use a mix of 'extensions.ini'
> (for non-bootstrapped XPIs) and a preference (for bootstrap).
> 
> The existing extensions.ini/pref mix desperately needs to be cleaned up and
> made properly async...

Alright, in light of this I think it's probably best to just duck type restartless add-ons on the fly to start.

Comment 9

[Dave Townsend [:mossop]]

We might be missing something here. Are you talking about the internal addon objects (DBAddonInternal etc.) which XPIProvider uses or the AddonWrapper objects exposed through the API to the rest of the application? If the former then I think it would be fine to include such a property. We're just nervous of exposing such a property to the rest of the application without a good use case.

Comment 10

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Dave Townsend [:mossop] from comment #9)
> We might be missing something here. Are you talking about the internal addon
> objects (DBAddonInternal etc.) which XPIProvider uses or the AddonWrapper
> objects exposed through the API to the rest of the application? If the
> former then I think it would be fine to include such a property. We're just
> nervous of exposing such a property to the rest of the application without a
> good use case.

My current patch add the `jetpack` property to the AddonWrapper and the data stored in the pref that Irving mentioned in comment 5.

Comment 11

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Dave Townsend [:mossop] from comment #9)
> We're just
> nervous of exposing such a property to the rest of the application without a
> good use case.

I understand, and I don't think I have a good enough reason to add the property atm, because adding a `isJetpack()` helper function which uses the same code as `Addon.hasResource` to check for a package.json file is good enough for now.

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #11)
> I understand, and I don't think I have a good enough reason to add the
> property atm, because adding a `isJetpack()` helper function which uses the
> same code as `Addon.hasResource` to check for a package.json file is good
> enough for now.

I'm still curious (/nervous) as to *why* you need to detect this.

Comment 13

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Blair McBride [:Unfocused] from comment #12)
> (In reply to Erik Vold [:erikvold] [:ztatic] from comment #11)
> > I understand, and I don't think I have a good enough reason to add the
> > property atm, because adding a `isJetpack()` helper function which uses the
> > same code as `Addon.hasResource` to check for a package.json file is good
> > enough for now.
> 
> I'm still curious (/nervous) as to *why* you need to detect this.

`loadBootstrapScope` (which is only called by `callBootstrapMethod`) is where the `bootstrap.js` lookup is performed, here:

https://github.com/mozilla/gecko-dev/blob/master/toolkit/mozapps/extensions/internal/XPIProvider.jsm#L4083-L4084

At that point I only have an id, version, and type info, where type == 'extension'.  So with that I can't detect the extension is a jetpack, I'd have check for a package.json file, unless there is more information/input when callBootstrapMethod is called.

Comment 14

[Dave Townsend [:mossop]]

So in some cases where we call that there is an internal addon object available and as I said exposing a jetpack property on that should be safe to do, just not on the AddonWrapper object. However during startup we call loadBootstrapScope where there is no addon object available because it would cost us time to load the database of add-ons to do so so I don't know that it helps you there.

Comment 15

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Dave Townsend [:mossop] from comment #14)
> So in some cases where we call that there is an internal addon object
> available and as I said exposing a jetpack property on that should be safe
> to do, just not on the AddonWrapper object. However during startup we call
> loadBootstrapScope where there is no addon object available because it would
> cost us time to load the database of add-ons to do so so I don't know that
> it helps you there.

Right, this what I was trying to say in comment 3.

So afaict start up time will be slowed unless I add this data to the pref referred to in comment 5.

Perhaps I can avoid adding a `jetpack` property to AddonWrapper and add it to AddonInternal instead?  I'll take a look if you think this is worth trying.

Comment 16

[Erik Vold [:erikvold] (please needinfo? me)]

I spoke with Irving yesterday, and he suggested two ways that we can detect whether or not an add-on is a jetpack during startup.

1) Separate jetpacks from bootstraped add-ons by using a separate pref to store the metadata used at startup.
2) change the type to be "jetpack" instead of "extension" and either treat "jetpack" types the same as type="extension" or convert it and remember which add-ons are jetpacks by some other means, like a cache, or boolean property on AddonInternal perhaps.

Comment 17

[Dave Townsend [:mossop]]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #16)

> 2) change the type to be "jetpack" instead of "extension" and either treat
> "jetpack" types the same as type="extension" or convert it and remember
> which add-ons are jetpacks by some other means, like a cache, or boolean
> property on AddonInternal perhaps.

I like this choice. Internally we can use type="jetpack" and then make all the API methods treat them the same. Shouldn't be too difficult.

Comment 18

[Erik Vold [:erikvold] (please needinfo? me)]

Hey Dave,

Can I pass this one on to you?

Comment 19

[Dave Townsend [:mossop]]

Yep

Comment 20

[Dave Townsend [:mossop]]

Erik, where is the bug that tracks reading package.json instead of install.rdf for native jetpack add-ons?

Comment 21

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Dave Townsend [:mossop] from comment #20)
> Erik, where is the bug that tracks reading package.json instead of
> install.rdf for native jetpack add-ons?

I don't think I've made a bug specifically for that yet, the closest I have made is bug 915376

Comment 22

[Dave Townsend [:mossop]]

Attachment: patch

This looks about right but until we have code to actually create an add-on of type "jetpack" I can't write any tests for this. In the database and on AddonInternal objects we can have one type used ("jetpack") but any calls to the API will see them as "extension". The aliasing technique is general so we could do this for other stuff in the future.

Comment 23

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8449015 [details] [diff] [review]
patch

Review of attachment 8449015 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +3706,5 @@
> +
> +        if (typeset.has(TYPE_ALIASES[alias]))
> +          typeset.add(alias);
> +      }
> +      typesToGet = [...typeset.values()];

Don't need to call .values() - the spread operator works on any iterable, and a Set instance is an iterable.

@@ +5845,5 @@
>     "progress", "maxProgress", "certificate", "certName"].forEach(function(aProp) {
>      this.__defineGetter__(aProp, function AIW_propertyGetter() aInstall[aProp]);
>    }, this);
>  
> +  this.__defineGetter__("type", _ => getExternalType(aInstall.type));

Convention is to use () instead of _
() makes it more clear that the parameter is unused. There was a discussion about this somewhere, but I can't seem to find it.

@@ +6421,5 @@
>     "getDataDirectory"].forEach(function(aProp) {
>       this.__defineGetter__(aProp, function AddonWrapper_propertyGetter() aAddon[aProp]);
>    }, this);
>  
> +  this.__defineGetter__("type", _ => getExternalType(aAddon.type));

Ditto.

Comment 24

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8449015 [details] [diff] [review]
patch

This looks good to me!

Comment 25

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1009332: Present certain internal add-on types as other types through the API.

Bug 1009332: Present certain internal add-on types as other types through the API.

Comment 26

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1190692: Split out manifest parsing code to support web extension manifests.

Bug 1190692: Split out manifest parsing code to support web extension manifests.

Comment 27

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1190692: Parse manifest.json files into AddonInternal instances.

Bug 1190692: Parse manifest.json files into AddonInternal instances.

Comment 28

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1190692: Load web extensions.

Bug 1190692: Load web extensions.

Also corrects a race condition where if an extension was disabled before it
had finished loading its manifest it would have called GlobalManager.init but
never call GlobalManager.uninit.

Comment 29

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1009332: Present certain internal add-on types as other types through the API.

Bug 1009332: Present certain internal add-on types as other types through the API.

Comment 30

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8645232 [details]
MozReview Request: Bug 1009332: Present certain internal add-on types as other types through the API.

https://reviewboard.mozilla.org/r/15461/#review13833

Do we have to worry about getAddonsWithOperationsByTypes?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3985
(Diff revision 1)
> +        typeset.delete(alias);

I'm a little unsure why this is here. It seems like it's an error if alias is already in this set. Is that correct?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:205
(Diff revision 1)
> +  "webextension": "extension",

Do we want to add "jetpack" and "webextension" to RESTARTLESS_TYPES, COMPATIBLE_BY_DEFAULT_TYPES, and SIGNED_TYPES? The latter seems pretty necessary.

Comment 31

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8645232 [details]
MozReview Request: Bug 1009332: Present certain internal add-on types as other types through the API.

https://reviewboard.mozilla.org/r/15461/#review13839

Ship It!

Comment 32

[Dave Townsend [:mossop]]

(In reply to Bill McCloskey (:billm) from comment #30)
> Comment on attachment 8645232 [details]
> MozReview Request: Bug 1009332: Present certain internal add-on types as
> other types through the API.
> 
> https://reviewboard.mozilla.org/r/15461/#review13833
> 
> Do we have to worry about getAddonsWithOperationsByTypes?

Good catch, going to split out some of the code to share between the two.

> ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3985
> (Diff revision 1)
> > +        typeset.delete(alias);
> 
> I'm a little unsure why this is here. It seems like it's an error if alias
> is already in this set. Is that correct?

This makes sure that we aren't exposing these aliases, without it other code could to getAddonsByTypes(["webextension"]) and just get those. 

> ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:205
> (Diff revision 1)
> > +  "webextension": "extension",
> 
> Do we want to add "jetpack" and "webextension" to RESTARTLESS_TYPES,
> COMPATIBLE_BY_DEFAULT_TYPES, and SIGNED_TYPES? The latter seems pretty
> necessary.

RESTARTLESS_TYPES isn't technically necessary, it's only used in the RDF parser, but I'll put it in anyway since it probably makes sense. Definitely missed SIGNED_TYPES. I've also dropped "jetpack" from the list since we're not actually supporting it now.

For COMPATIBLE_BY_DEFAULT_TYPES I'm not sure. What is the plan for these add-ons? The manifest proposal talks about strict_min_version and strict_max_version which suggests these aren't compatible by default and I'm just defaulting the maxVersion to *.

Comment 33

[Dave Townsend [:mossop]]

Comment on attachment 8645232 [details]
MozReview Request: Bug 1009332: Present certain internal add-on types as other types through the API.

Bug 1009332: Present certain internal add-on types as other types through the API.

Comment 34

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8645232 [details]
MozReview Request: Bug 1009332: Present certain internal add-on types as other types through the API.

https://reviewboard.mozilla.org/r/15461/#review13877

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:685
(Diff revision 2)
> +  return typeset.values();

Was just trying out the patch and I noticed that you need to do [...typeset] instead. values() returns an Iterator and later on down the line we call indexOf on this thing.

Comment 35

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/1e5203891191

Comment 36

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1e5203891191