Closed Bug 1323845 Opened 7 years ago Closed 6 years ago

Allow bundled experiments, and support for content-side APIs

Categories

(WebExtensions :: Experiments, defect, P1)

defect

Tracking

(firefox59 fixed)

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: andy+bugzilla, Assigned: kmag)

References

Details

(Whiteboard: [experiments] triaged)

Attachments

(11 files)

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
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
59 bytes, text/x-review-board-request
aswan
: review+
Details
Kris to enter some more details here.
Flags: needinfo?(kmaglione+bmo)
Priority: -- → P1
Whiteboard: [experiments] triaged
Assignee: nobody → kmaglione+bmo
Priority: P1 → P2
Blocks: 1364976
No longer blocks: webext-oop
Flags: needinfo?(kmaglione+bmo)
Summary: Allow experiments to work with out of process → Allow experiments to include content-side APIs
Priority: P2 → P1
Comment on attachment 8941289 [details]
Bug 1323845: Part 0 - Cleanup some code formatting.

https://reviewboard.mozilla.org/r/211568/#review217640
Attachment #8941289 - Flags: review?(aswan) → review+
Comment on attachment 8941290 [details]
Bug 1323845: Part 1 - Support multiple schema root namespaces.

https://reviewboard.mozilla.org/r/211570/#review217688

This looks good, but some documentation would be nice (just an overview of how a SchemaRoot can be overlayed on an existing SchemaRoot and the intended use is one SchemaRoot for all the built-in APIs with additional roots for extensions that use different experiments).  If that documentation is actually coming in a subsequent patch that I haven't seen yet, ignore this remark.

::: toolkit/components/extensions/Schemas.jsm:1137
(Diff revision 1)
> -  static parseSchema(schema, path, extraProperties = []) {
> +  static parseSchema(root, schema, path, extraProperties = []) {
>      this.checkSchemaProperties(schema, path, extraProperties);
>  
>      return new this(schema);
>    }

is this function ever actually called?  it looks like we only call it on derived types

::: toolkit/components/extensions/Schemas.jsm:2685
(Diff revision 1)
>      return super.has(key);
>    }
>  }
>  
> -this.Schemas = {
> -  initialized: false,
> +class SchemaRoot extends Namespace {
> +  constructor(base, schemaJSON) {

can we have a documentation comment here?  (one for the whole class would be good too)

::: toolkit/components/extensions/test/xpcshell/test_ext_schemas_roots.js:104
(Diff revision 1)
> +
> +let talliedErrors = [];
> +
> +let permissions = new Set();
> +
> +class TallyingAPIImplementation extends SchemaAPIInterface {

all this stuff is duplicated with other tests, can it move to a head.js file?

::: toolkit/components/extensions/test/xpcshell/test_ext_schemas_roots.js:200
(Diff revision 1)
> +  baseSchema.inject(base, wrapper);
> +  schema.inject(root, wrapper);
> +
> +  equal(typeof base.base, "object", "base.base exists");
> +  equal(typeof root.base, "object", "root.base exists");
> +  equal(typeof base.experiments, "undefined", "base.experiments exists not");

o.O

::: toolkit/components/extensions/test/xpcshell/test_ext_schemas_roots.js:237
(Diff revision 1)
> +  Assert.throws(() => root.experiments.foo.foo("Meh."),
> +                /Incorrect argument types/,
> +                "root.experiments.foo.foo()");
> +
> +  Assert.throws(() => root.experiments.bar.bar("Meh."),
> +                /Incorrect argument types/,

shouldn't this be "wrong number of arguments" or something?
Attachment #8941290 - Flags: review?(aswan) → review+
Comment on attachment 8941291 [details]
Bug 1323845: Part 2a - Support separate API managers for each extension.

https://reviewboard.mozilla.org/r/211572/#review217936

::: toolkit/components/extensions/ExtensionCommon.jsm:963
(Diff revision 1)
>      this.apis = new DefaultWeakMap(() => new Map());
>  
>      this._scriptScopes = [];
>    }
>  
> +  onStartup(extension) {

I don't see this getting called outside of the main process.  I'm assuming this is laying groundwork for something in a future patch, leaving this note here to myself while I read the other patches.
Comment on attachment 8941292 [details]
Bug 1323845: Part 2b - Support separate schema roots for each API manager.

https://reviewboard.mozilla.org/r/211574/#review217942

::: toolkit/components/extensions/ExtensionContent.jsm:76
(Diff revision 1)
>  const CATEGORY_EXTENSION_SCRIPTS_CONTENT = "webextension-scripts-content";
>  const CONTENT_SCRIPT_INJECTION_HISTOGRAM = "WEBEXT_CONTENT_SCRIPT_INJECTION_MS";
>  
>  var apiManager = new class extends SchemaAPIManager {
>    constructor() {
> -    super("content");
> +    super("content", Schemas);

I expected this (and other occurences below) to be Schemas.rootSchema, but I guess that would defeat lazy initilization?
Comment on attachment 8941293 [details]
Bug 1323845: Part 2c - Lazy init API global only when APIs are first loaded.

https://reviewboard.mozilla.org/r/211576/#review217944

::: toolkit/components/extensions/ExtensionCommon.jsm
(Diff revision 1)
>     * @param {SchemaRoot} schema
>     */
>    constructor(processType, schema) {
>      super();
>      this.processType = processType;
> -    this.global = this._createExtGlobal();

we should leave the initializer, just set to null
Attachment #8941293 - Flags: review?(aswan) → review+
Comment on attachment 8941292 [details]
Bug 1323845: Part 2b - Support separate schema roots for each API manager.

https://reviewboard.mozilla.org/r/211574/#review217942

> I expected this (and other occurences below) to be Schemas.rootSchema, but I guess that would defeat lazy initilization?

Yeah, this winds up being called before the root schema is fully initialized at the moment. My plan for the near future is to get rid all of the dynamic schema adding and removal that we need for old-style experiments, and then just use the root schema directly.
Comment on attachment 8941291 [details]
Bug 1323845: Part 2a - Support separate API managers for each extension.

https://reviewboard.mozilla.org/r/211572/#review217936

> I don't see this getting called outside of the main process.  I'm assuming this is laying groundwork for something in a future patch, leaving this note here to myself while I read the other patches.

It's not used in the content process. I mainly added this to make it easier to broadcast the startup event to all of the API managers for an extension, rather than just the main one.
Comment on attachment 8941295 [details]
Bug 1323845: Part 4 - Add Extension.isPrivileged property.

https://reviewboard.mozilla.org/r/211580/#review217958

::: toolkit/components/extensions/Extension.jsm:572
(Diff revision 1)
>        if (this.manifest.devtools_page) {
>          permissions.add("devtools");
>        }
>  
>        for (let perm of manifest.permissions) {
> -        if (perm === "geckoProfiler") {
> +        if (perm === "geckoProfiler" && !this.isPrivileged) {

why do this here?  we should change gecko profiler to be signed as privileged, but at that point we should just add "mozillaAddons" as a permission in the schema and remove this whole fragment.

::: toolkit/components/extensions/Extension.jsm:1221
(Diff revision 1)
>    get manifestCacheKey() {
>      return [this.id, this.version, Services.locale.getAppLocaleAsLangTag()];
>    }
>  
> +  get isPrivileged() {
> +    return (this.addonData.signedState === AddonManager.SIGNEDSTATE_PRIVILEGED ||

I think we'll probably want this to be true for system addons as well, but you're preserving the existing behavior here so that can happen in a separate bug.

::: toolkit/components/extensions/Extension.jsm:1234
(Diff revision 1)
> +  }
> +
>    async _parseManifest() {
>      let manifest = await super.parseManifest();
>      if (manifest && manifest.permissions.has("mozillaAddons") &&
> -        this.addonData.signedState !== AddonManager.SIGNEDSTATE_PRIVILEGED) {
> +        !this.isPrivileged) {

There's a behavior change here that this grants access to mozillaAddons APIs to anything installed temporarily.  I don't really have an opinion on whether we should or shouldn't do that, but was it deliberate?
Attachment #8941295 - Flags: review?(aswan) → review+
Comment on attachment 8941294 [details]
Bug 1323845: Part 3 - Auto-create intermediate API namespace objects when necessary.

https://reviewboard.mozilla.org/r/211578/#review217960
Attachment #8941294 - Flags: review?(aswan) → review+
Comment on attachment 8941291 [details]
Bug 1323845: Part 2a - Support separate API managers for each extension.

https://reviewboard.mozilla.org/r/211572/#review217964
Attachment #8941291 - Flags: review?(aswan) → review+
Comment on attachment 8941292 [details]
Bug 1323845: Part 2b - Support separate schema roots for each API manager.

https://reviewboard.mozilla.org/r/211574/#review217968

::: toolkit/components/extensions/ProxyScriptContext.jsm:391
(Diff revision 1)
>    let can = new CanOfAPIs(this, proxyScriptAPIManager, localAPIs);
>    proxyScriptAPIManager.lazyInit();
>  
>    let browserObj = Cu.createObjectIn(this.sandbox);
>    let injectionContext = new ProxyScriptInjectionContext(this, can);
> -  Schemas.inject(browserObj, injectionContext);
> +  proxyScriptAPIManager.schema.inject(browserObj, injectionContext);

I guess this should really be in the previous patch but don't we need a separate ProxyScriptAPIManager per extension?
Attachment #8941292 - Flags: review?(aswan) → review+
Comment on attachment 8941292 [details]
Bug 1323845: Part 2b - Support separate schema roots for each API manager.

https://reviewboard.mozilla.org/r/211574/#review217968

> I guess this should really be in the previous patch but don't we need a separate ProxyScriptAPIManager per extension?

If we want to support experiments in proxy script contexts, yes. But I'd really rather just get rid of proxyScriptAPIManager.
Blocks: 1419884
Comment on attachment 8941296 [details]
Bug 1323845: Part 5a - Allow extensions to bundle experiment API modules.

https://reviewboard.mozilla.org/r/211582/#review217994

I feel sorry for anybody who ends up trying to make sense out this patch in isolation in the future since in this patch, MultiAPIManager is a big old no-op wrapped around LazyAPIManager.  I realize it is laying the foundation for letting other extensions use an experimental API, but as we talked about on IRC I would really prefer just not supporting that at all which would cut down on the amount and complexity of the code here.

::: toolkit/components/extensions/Extension.jsm:646
(Diff revision 1)
> +          if (!schemaPromises.has(schema)) {
> +            schemaPromises.set(schema, this.readJSON(data.schema).then(json => Schemas.processSchema(json)));
> +          }

can you just use a DefaultMap here?

::: toolkit/components/extensions/ExtensionCommon.jsm:969
(Diff revision 1)
> +    if (schema) {
> -    this.schema = schema;
> +      this.schema = schema;
> +    }

This is mildly gross, taking this out of this class and adding it to a thin subclass would be nice.  Up to you...

::: toolkit/components/extensions/ExtensionCommon.jsm:1403
(Diff revision 1)
> +  get schema() {
> +    if (!this._schema) {
> +      this._schema = new SchemaRoot(Schemas.rootSchema, this.schemaURLs);
> +      this._schema.parseSchemas();
> +    }
> +    return this._schema;

any reason not to just use defineLazyGetter()

::: toolkit/components/extensions/ExtensionCommon.jsm:1428
(Diff revision 1)
> +        if (child.lazyInit) {
> +          let res = child.lazyInit();
> +          if (res && typeof res.then === "function") {
> +            await res;
> +          }
> +        }

what's this for?  from my reading, children is only ever SchemaAPIManaer or LazyAPIManager, neither of which has a lazyInit method

::: toolkit/components/extensions/ExtensionCommon.jsm:1459
(Diff revision 1)
> +    let promises = [];
> +    for (let child of this.children) {
> +      promises.push(child.onStartup(extension));
> +    }
> +    return Promise.all(promises);

why not just `return Promise.all(this.children.map(child => child.onStartup(extension)));`

::: toolkit/components/extensions/schemas/experiments.json:118
(Diff revision 1)
> +        "type": "string",
> +        "enum": [
> +          "addon_parent",
> +          "content_parent",
> +          "devtools_parent",
> +          "proxy_script"

I think we don't actually support these?
Attachment #8941296 - Flags: review?(aswan) → review+
Comment on attachment 8941296 [details]
Bug 1323845: Part 5a - Allow extensions to bundle experiment API modules.

https://reviewboard.mozilla.org/r/211582/#review217994

`MultiAPIManager` isn't a no-op in this patch. It allows us to use both the bundled experiment API manager and the base API manager in the same extension.

> can you just use a DefaultMap here?

I guess, but I think it would be harder to follow, since we'd just wind up doing no-op `.get(schema)`s and throwing away the result.

> any reason not to just use defineLazyGetter()

Probably not.

> what's this for?  from my reading, children is only ever SchemaAPIManaer or LazyAPIManager, neither of which has a lazyInit method

For the base API manager, it's a SchemaAPIManager sub-class that has a lazyInit method that needs to be called.
Comment on attachment 8941297 [details]
Bug 1323845: Part 5b - Add tests for bundled experiments APIs.

https://reviewboard.mozilla.org/r/211584/#review218358

Testing an experiment that defines multiple APIs would be good, but the test could get arbitrarily complicated, I'm not sure where to draw the line.
Attachment #8941297 - Flags: review?(aswan) → review+
Comment on attachment 8941296 [details]
Bug 1323845: Part 5a - Allow extensions to bundle experiment API modules.

https://reviewboard.mozilla.org/r/211582/#review218360

::: toolkit/components/extensions/schemas/experiments.json:104
(Diff revision 1)
> +      {
> +        "id": "APIEvent",
> +        "type": "string",
> +        "enum": [
> +          "startup"
> +        ]
> +      },

Is there some reason we can't support manifest entries, updates, uninstalls?  If not can you at least file a bug to add them later?
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #26)
> `MultiAPIManager` isn't a no-op in this patch. It allows us to use both the
> bundled experiment API manager and the base API manager in the same
> extension.

Oops, let me revise that, the fact that for the common case of a single xpi with an experiment and the extension that uses it, the base and experiment schemas are merged from LazyAPIManager while the actual API implementations are merged from MultiAPIManager is making this harder to follow than it would be if this didn't need to be written to handle the more general case.
Comment on attachment 8941298 [details]
Bug 1323845: Part 6a - Support WebExtension-style experiment API provider extensions.

https://reviewboard.mozilla.org/r/211586/#review218362

::: toolkit/components/extensions/Schemas.jsm:2709
(Diff revision 1)
> +    }
> +  }
> +}
> +
> +class SchemaRoots extends Namespaces {
> +  constructor(root, roots) {

These names could be clearer.  `targetRoot` and `sourceRoots` maybe?  or src/dst or something?

::: toolkit/components/extensions/Schemas.jsm:2756
(Diff revision 1)
> +      yield* root.getNamespaces(name);
> +    }
> +  }
> +}
> +
> +let alreadyInjected = new DefaultWeakMap(() => new Set());

why can't this just be a property of the context?
Attachment #8941298 - Flags: review?(aswan) → review+
Comment on attachment 8941298 [details]
Bug 1323845: Part 6a - Support WebExtension-style experiment API provider extensions.

https://reviewboard.mozilla.org/r/211586/#review218364

Arg hit publish too early.  Please add comments explaining the `Namespaces` and `SchemaRoots` classes.  Explaining when and how they are used is more useful than documenting individual methods...
Comment on attachment 8941299 [details]
Bug 1323845: Part 6b - Test WebExtension-style experiments API providers.

https://reviewboard.mozilla.org/r/211588/#review218366
Attachment #8941299 - Flags: review?(aswan) → review+
Comment on attachment 8941296 [details]
Bug 1323845: Part 5a - Allow extensions to bundle experiment API modules.

https://reviewboard.mozilla.org/r/211582/#review218360

> Is there some reason we can't support manifest entries, updates, uninstalls?  If not can you at least file a bug to add them later?

We need to parse and normalize the manifest without relying on experiment schemas, and I'm hesitant to try to hack in support for experiment manifest entries until I have a good use case.

I'd like to support updates and uninstalls, but those are currently implemented in a weird way that have access to an extension instance, and without an extension instance, we have no API managers. I was planning to fix that in a follow-up, though.
Comment on attachment 8941290 [details]
Bug 1323845: Part 1 - Support multiple schema root namespaces.

https://reviewboard.mozilla.org/r/211570/#review217688

> all this stuff is duplicated with other tests, can it move to a head.js file?

Ideally, yes. But the mocking is all pretty kludgy, at this point, and I'd like to do something more sensible before I try to generalize it enough to be shared between tests.

> shouldn't this be "wrong number of arguments" or something?

Ideally, yes. But the the overloading rules mean that the number of expected arguments depends on their types, and the argument checking logic isn't smart enough to figure that out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a0764b417e993c0fee98396d1965fdae55ad52a
Bug 1323845: Part 0 - Cleanup some code formatting. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/a72e4624c800733ee6b6f557352a884fd6e32d3a
Bug 1323845: Part 1 - Support multiple schema root namespaces. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/099c60efb4f1da18636efd8e2abb23492fc57f5b
Bug 1323845: Part 2a - Support separate API managers for each extension. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/346bd1b4769ae35b065a7c3ce27d8fb102413c02
Bug 1323845: Part 2b - Support separate schema roots for each API manager. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/43f9533906cfada71309e73d977326b69ef9c7de
Bug 1323845: Part 2c - Lazy init API global only when APIs are first loaded. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/77442f18cc027a5cd4bef9a3ac5cdc3d615e3805
Bug 1323845: Part 3 - Auto-create intermediate API namespace objects when necessary. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8b83e2001740feec71bf29ec88a00825f3c4c79
Bug 1323845: Part 4 - Add Extension.isPrivileged property. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/179d025d51cdf2c5d0e7eea3357d8f5fd1dc9b6c
Bug 1323845: Part 5a - Allow extensions to bundle experiment API modules. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/bbf3129f4a56db20b1806247bc8d4bab0e8a430f
Bug 1323845: Part 5b - Add tests for bundled experiments APIs. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/17ee1f919ec1d4a98c9a6ecaab695cf2501992c7
Bug 1323845: Part 6a - Support WebExtension-style experiment API provider extensions. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8cb79d5fb06904ad04000042fb23360f145c4b6
Bug 1323845: Part 6b - Test WebExtension-style experiments API providers. r=aswan
https://hg.mozilla.org/integration/mozilla-inbound/rev/c513b6a6c7d7681bf213563e79e3c0520c8ee8fe
Bug 1323845: Follow-up: Fix more debug bustage in browserSettings normalization. r=bustage
Pushed by maglione.k@gmail.com
https://hg.mozilla.org/integration/mozilla-inbound/rev/c513b6a6c7d7681bf213563e79e3c0520c8ee8fe
Follow-up: Fix more debug bustage in browserSettings normalization. r=bustage
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(kmaglione+bmo)
Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.
Flags: needinfo?(kmaglione+bmo)
I'm really wondering why the devs here have not provided an example of an combined experiment API. According my findings this bug is not yet a replacement for the legacy experiments. I have migrated the toolbar API from mixedpuppy https://github.com/mixedpuppy/web-ext-toolbar to a combined experiment.

As first there was some adjustments needed to get the API working as legacy version for Nightly 59, but this was no problem.
Then i migrated this to combined experiment API with help of the tests in this bug. So far so good.

But there was always the addon disabled with only remove button in addon manager. I checked the extensions.jsm in profile and found still "dependencies":["toolbar@experiments.addons.mozilla.org"]. I removed the dependency and set it as enabled in the jsm. After next start the addon was able disable and enable again. But from time time the dependency reappears again and it is needed to remove this again and again. I deleted the entire profile to get sure that there is a profile from scratch without old addon installations. But no success, the dependencies comes back.

Then i changed the permission to toolbar2 in manifest:
"permissions": ["experiments.toolbar2"],

and changed the schema.json also to toolbar2

    "namespace": "manifest",
    "types": [
      {
        "$extend": "Permission",
        "choices": [{
          "type": "string",
          "enum": [ "toolbar2" ]
        }]
      },
....

    "namespace": "toolbarAction",
    "description": "Use toolbar actions to add a toolbar to Firefox.",
    "permissions": ["experiments.toolbar2"],
    "functions": [

Result in extensions.jsm:
,"dependencies":["toolbar2@experiments.addons.mozilla.org"],"hasEmbeddedWebExtension":false,"userPermissions":{"origins":["moz-extension://a05a4b60-704c-4688-869e-e645a29efa27/*"],"apis":["toolbar2"],"permissions":[]}

At the moment i have following results:
- a constantly disabled not enable-able addon with a permission property
- permission property seems to have a relation to dependencies is extensions.jsm
- the registered API browser.toolbarAction seems accessable
- at browser start a WARN Loading extension 'null': Reading manifest: Error processing toolbar_action: An unexpected property was found in the WebExtension manifest -> later toolbar_action is available -> Bug 1394140 is very annoying and got only P5. No comments.

The new toolbar is also not yet visible if i remove the permissions. No failure message on enable. Need more checks for initialization now.
latest results:
- it looks like the complete script api.js environment will not be executed at startup
- if i change the name of the script in manifest to a not exist file, there appears no "file not found" message
- if i try to call the API functions in bg i get the expected "file not found" message
- if i change back file name and call the API function i get fun is not a function

I will stop here now.
Found still some points in toolbar API experiment.
- script file was not executed because there was some commas and semicolons to much which was no problem with the legacy experiments API
- fun is not a function was caused by manifest property "paths": [["experiments", "toolbarAction", "setPanel", "getPanel"], was split to 2 arrays now
- for execution of the script file i need still to call a API function from bg
- on call of API functions i get still module is not a constructor  ExtensionCommon.jsm:1203

Toolbar is visible now.
On mailing list i found a important comment from Kris. The ExtensionAPI constructor must now match the name of the API in the "experiment_apis" object. This fixes the module is not a constructor error. The API script will called from startup now.

So for me remains only two problems. The permission problem in comment 44 and I need to disable/enable the addon to get the toolbar visible, which was not the case with the legacy API.
Summary: Allow experiments to include content-side APIs → Allow bundled experiments, and support for content-side APIs
Flags: needinfo?(kmaglione+bmo) → qe-verify-
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: