1338409 - Lazily parse schema data

Status: RESOLVED FIXED

(WebExtensions :: General, defect)

Description

[Kris Maglione [:kmag]]

This is currently one of the most expensive parts of startup. We don't need to process the vast majority of our schemas for the vast majority of our extensions, particularly immediately at startup.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1338409: Part 1 - Lazily parse the schema data for each namespace as it is needed.

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

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1338409: Part 2 - Lazily parse the schema data for each namespace property, as it is needed.

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

Comment 3

[Andrew Swan [:aswan]]

Comment on attachment 8835845 [details]
Bug 1338409: Part 1 - Lazily parse the schema data for each namespace as it is needed.

https://reviewboard.mozilla.org/r/111420/#review112894

I think the "don't do error checking unless DEBUG is true" gets a little over-zealous in some places, and things will get really confusing if something goes wrong in a non-debug build.
Which leads to another issue, I hope we get to the point of enabling experiments in release builds at some point, I think we would want a way to enable the debug checks when loading the schema for an experiment in such a case.  We can cross that bridge when we reach it though...

::: toolkit/components/extensions/Schemas.jsm:640
(Diff revision 1)
>     * `context` is used to call the actual implementation
>     * of a given function or event.
>     *
>     * @param {Array<string>} path The API path, e.g. `["storage", "local"]`.
> -   * @param {string} name The method name, e.g. "get".
>     * @param {object} dest The object where `path`.`name` should be stored.

This comment no longer makes sense as written

::: toolkit/components/extensions/Schemas.jsm:704
(Diff revision 1)
>     * @throws {Error}
>     *        An error describing the first invalid property found in the
>     *        schema object.
>     */
>    static checkSchemaProperties(schema, path, extra = []) {
> +    if (DEBUG) {

just return early if `!DEBUG` to avoid an unneeded level of indentation?

::: toolkit/components/extensions/Schemas.jsm:917
(Diff revision 1)
>                      schema.maxLength || Infinity,
>                      pattern,
>                      format);
>    }
>  
> -  constructor(schema, enumeration, minLength, maxLength, pattern, format) {
> +  constructor(schema, name, enumeration, minLength, maxLength, pattern, format) {

I got tripped up over what `name` means when a string is used for something other than a top-level property, perhaps a brief comment to explain

::: toolkit/components/extensions/Schemas.jsm:1009
(Diff revision 1)
>      this.checkSchemaProperties(schema, path, extraProperties);
>  
>      let parseProperty = (schema, extraProps = []) => {
>        return {
>          type: Schemas.parseSchema(schema, path,
> -                                  ["unsupported", "onError", "permissions", ...extraProps]),
> +                                  DEBUG && ["unsupported", "onError", "permissions", ...extraProps]),

is this really needed?  checkSchemaProperties already turns into a no-op if !DEBUG

::: toolkit/components/extensions/Schemas.jsm:1958
(Diff revision 1)
> -    let targetType = ns && ns.get(type.$extend);
>  
>      // Only allow extending object and choices types for now.
>      if (targetType instanceof ObjectType) {
>        type.type = "object";
> -    } else if (!targetType) {
> +    } else if (DEBUG) {

why the DEBUG check here?  it doesn't look like we're doing anything very expensive below and things will go off the rails if we don't throw here and proceed with a non-extensible type

Comment 4

[Andrew Swan [:aswan]]

Comment on attachment 8835846 [details]
Bug 1338409: Part 2 - Lazily parse the schema data for each namespace property, as it is needed.

https://reviewboard.mozilla.org/r/111422/#review112924

::: mobile/android/components/extensions/test/mochitest/test_ext_tabs_captureVisibleTab.html:150
(Diff revision 1)
>        "permissions": ["tabs"],
>      },
>  
>      background() {
> -      browser.test.assertFalse("captureVisibleTab" in browser.tabs,
> +      browser.test.assertEq(undefined, browser.tabs.captureVisibleTab,
> -                               'Extension without "<all_tabs>" permission should not have access to captureVisibleTab');
> +                            'Extension without "<all_tabs>" permission should not have access to captureVisibleTab');

I know you didn't change this but I don't think there's any such permissions as <all_tabs>

::: toolkit/components/extensions/Schemas.jsm:1563
(Diff revision 1)
>        this.checkDeprecated(context);
>        return apiImpl.getProperty();
>      };
>  
>      let desc = {
> -      configurable: false,
> +      get: Cu.exportFunction(getStub, context.cloneScope),

I didn't realize we were doing this before...

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8835845 [details]
Bug 1338409: Part 1 - Lazily parse the schema data for each namespace as it is needed.

https://reviewboard.mozilla.org/r/111420/#review112894

I don't think it gets overzealous. There's no need to do error checking after it's been done once, and all of the little checks add up when we do them hundreds of times.

The issue with experiments had occurred to me, yes. My medium term plan, which needs these changes to build on, is to allow multiple global schema namespaces, and then check the debug flag on the root namespace. That way extensions with experiments get loaded into their own, private schema group, and we can set the debug flag the first time they're loaded.

Doing that sanely requires further changes, which depend on these changes, though, so I'm willing to leave that hole open for now.

> This comment no longer makes sense as written

I suppose. s/should/will/

> just return early if `!DEBUG` to avoid an unneeded level of indentation?

I guess. I tried it both ways and slightly preferred this one. But I don't really have a strong opinion.

> is this really needed?  checkSchemaProperties already turns into a no-op if !DEBUG

Creating the new array and destructuring the `extraProps` array into it is more expensive than you might expect.

> why the DEBUG check here?  it doesn't look like we're doing anything very expensive below and things will go off the rails if we don't throw here and proceed with a non-extensible type

When we do this a hundred times, it starts to add up, and these checks don't serve any purpose after the first time we load the schema.

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 8835846 [details]
Bug 1338409: Part 2 - Lazily parse the schema data for each namespace property, as it is needed.

https://reviewboard.mozilla.org/r/111422/#review112924

> I know you didn't change this but I don't think there's any such permissions as <all_tabs>

Heh. Good catch.

> I didn't realize we were doing this before...

Oh, I actually didn't even mean to change that behavior. I just removed those properties since I added defaults to `defineLazyProperty`. But it is a necessary change.

Comment 7

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a075cc9b1325ea1f7af1a306a407fcd997e4f383
Bug 1338409: Part 1 - Lazily parse the schema data for each namespace as it is needed. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/537d3ea987f3dbbfa56fc73208b32f515afe32ac
Bug 1338409: Part 2 - Lazily parse the schema data for each namespace property, as it is needed. r=aswan

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a075cc9b1325
https://hg.mozilla.org/mozilla-central/rev/537d3ea987f3