1290901 - Fix devtools APIs schema incompatibilities

Status: RESOLVED FIXED

(WebExtensions :: Developer Tools, defect, P1)

Description

[Luca Greco [:rpl] [:luca] [:lgreco]]

This goal of this issue is to describe and implement the changes to the WebExtensions internals that handle the schema loading and the wrapping of the injected api needed to be able to load and use the devtools API's JSON schema files mostly unmodified (besides marking as `unsupported` what it is not going to be part of the initial subset of DevTools API, and as `async` any method which takes a callback).

Follows a brief list of the incompatibilities between the current "WebExtensions API JSON schema internals" and the syntaxes used in the DevTools API schemas [1]:

- the DevTools schema is not contained in a single schema file, on the contrary each of the namespaces nested in the `devtools` namespace has its own schema file which uses a nested namespace (e.g. `devtools.inspectedWindow`, `devtools.panels`, `devtools.network`)

- the custom "types" defined in the DevTools schemas have a set of "events" and "properties" (and currently we are support only set of "functions", which works correctly, but we are currently raising exceptions if "events" or "properties" are defined in a custom type)   

[1] The original schema file from the chromium repo: https://cs.chromium.org/chromium/src/chrome/common/extensions/api/devtools/

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1290901 - [webext] Handle nested namespaced API schema.

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

Comment 2

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1290901 - [webext] Allow "events" and "properties" in SubModuleType defined in JSON schema.

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

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1290901 - [webext] Allow APIs implementation to return null when the API should not be injected in a context.

needed to be able to return null when the registered schema API should not be available
based on the context type instead of WebExtensions permissions.

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

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1290901 - [webext] Add xpcshell test cases on nested namespaces and null/empty api objects.

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

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

The set of 4 patches attached are composed by:
 
- 3 small patches that introduces the needed changes (splitted into smaller patches to make it easier to figure out what is the goal of each of the patches), 

- the last patch introduces new xpcshell tests to check that the changes are working as expected (in particular that the nested namespaces becomes nested objects and if a registered API implementation returns a null or an empty object, the API is not injected in that context)

Push to try of the above set of 4 patches:

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=785c9c984e355d34865de6dd8fff6a379cbc8a94

Comment 6

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/68956/#review66086

::: toolkit/components/extensions/Schemas.jsm:1633
(Diff revision 1)
> +      } else if (namespace.includes(".")) {
> +        // Special handling needed on any nested namespaced API (e.g devtools.inspectedWindow)
> +        // to turn `dest["nested.namespace"]` into `dest["nested"]["namespace"]`.
> +
> +        let apiObj = dest[namespace];
> +        delete dest[namespace];
> +
> +        let nsLevels = namespace.split(".");
> +        let currentObj = dest;
> +        for (let nsLevel of nsLevels.slice(0, -1)) {
> +          if (!currentObj[nsLevel]) {
> +            // Create the namespace level if it doesn't exist yet.
> +            currentObj = Cu.createObjectIn(currentObj, {defineAs: nsLevel});
> +          } else {
> +            // Move currentObj to the nested object if it already exists.
> +            currentObj = currentObj[nsLevel];
> +          }
> +        }
> +
> +        // Copy the apiObj as the final nested level.
> +        currentObj[nsLevels.pop()] = apiObj;

On first reading, I was wondering why you don't just do all this work before injecting.  But I guess that doing it this way means you don't need any special case cleanup code to clear the whole hierarchy if you end up not injecting anything into a nested namespace?

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

https://reviewboard.mozilla.org/r/68956/#review66086

> On first reading, I was wondering why you don't just do all this work before injecting.  But I guess that doing it this way means you don't need any special case cleanup code to clear the whole hierarchy if you end up not injecting anything into a nested namespace?

Exactly, if the nested namespace is empty then it is cleared by the "cleaup code" that was already there in 1 step.

If it is not empty, then we turn the nested namespace into nested objects.

Comment 8

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/68958/#review66110

::: toolkit/components/extensions/Schemas.jsm:1348
(Diff revision 1)
> -      checkTypeProperties("functions");
> +      // NOTE: "events" and "properties" are currently ignored, because they are used
> +      // in the DevTools schema files, but they are not currently used by anything in the
> +      // initial set of supported DevTools APIs. See Bug 1290901 for rationale.

Is there a separate bug to add full schema-level support for these fields?

Comment 9

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/68960/#review66114

Sorry I don't understand this one, can you give a concrete example of when this is needed?

Comment 10

[Luca Greco [:rpl] [:luca] [:lgreco]]

https://reviewboard.mozilla.org/r/68960/#review66114

for sure! 

An example of its usage can be found in the [initial patch that implements the devtools APIs](https://reviewboard.mozilla.org/r/68966/diff/1#0).

basically, I'd like to be able to disable an API in a particular context by returning a null/empty implementation, e.g. the api can decide that the API should not be available for a particular context type (or even based on the extension):

```
extensions.registerSchemaAPI("mycontextspecificapi", (extension, context) => {
  // Filter out any contexts that should not have access to this API object
  if (!isThisTheExpectedTypeOfContextForThisAPI(context)) {
    return null;
  }
  
  return {
    mycontextspecificapi: {
      ...
    }
  };
});
      
```

Comment 11

[Luca Greco [:rpl] [:luca] [:lgreco]]

https://reviewboard.mozilla.org/r/68958/#review66110

> Is there a separate bug to add full schema-level support for these fields?

I was waiting to open the issue (just to be sure that the plan that I'm proposing has been already double-checked at least once ;-)), I intended to open it after the initial feedback on this patch.

I'm going to create the new bugzilla issue and add its bug number into this comment as a reference.

Comment 12

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8777381 [details]
Bug 1290901 - [webext] Allow "events" and "properties" in SubModuleType defined in JSON schema.

https://reviewboard.mozilla.org/r/68958/#review66110

> I was waiting to open the issue (just to be sure that the plan that I'm proposing has been already double-checked at least once ;-)), I intended to open it after the initial feedback on this patch.
> 
> I'm going to create the new bugzilla issue and add its bug number into this comment as a reference.

added the bug numbers of the related follow up issue in the new version of the above inline comment.

Comment 13

[Luca Greco [:rpl] [:luca] [:lgreco]]

side note: in the last update, the attached patches have been rebased, to adapt them to recent changes around the code where these patches are applied.

Comment 14

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8777380 [details]
Bug 1290901 - [webext] Handle nested namespaced API schema.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68956/diff/1-2/

Comment 15

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8777381 [details]
Bug 1290901 - [webext] Allow "events" and "properties" in SubModuleType defined in JSON schema.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68958/diff/1-2/

Comment 16

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8777382 [details]
Bug 1290901 - [webext] Allow APIs implementation to return null when the API should not be injected in a context.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68960/diff/1-2/

Comment 17

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8777383 [details]
Bug 1290901 - [webext] Add xpcshell test cases on nested namespaces and null/empty api objects.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68962/diff/1-2/

Comment 18

[Andrew Swan [:aswan]]

Comment on attachment 8777380 [details]
Bug 1290901 - [webext] Handle nested namespaced API schema.

https://reviewboard.mozilla.org/r/68956/#review68824

::: toolkit/components/extensions/Schemas.jsm:1656
(Diff revision 2)
>          }
>        }
>  
>        if (!Object.keys(obj).length) {
>          delete dest[namespace];
> +      } else if (namespace.includes(".")) {

Could you include a comment just noting that this needs to come after lines 1654-5 so that we don't accidentally inject empty intermediate nodes for nested namespaces?
I also think the flow would be marginally clearer if you put return after line 1655 and then a separate "if namespace.includes..." clause with the comment leading into it, but that's a matter of taste, feel free to leave it as is if you prefer it.

Comment 19

[Andrew Swan [:aswan]]

Comment on attachment 8777381 [details]
Bug 1290901 - [webext] Allow "events" and "properties" in SubModuleType defined in JSON schema.

https://reviewboard.mozilla.org/r/68958/#review68826

Comment 20

[Andrew Swan [:aswan]]

Comment on attachment 8777382 [details]
Bug 1290901 - [webext] Allow APIs implementation to return null when the API should not be injected in a context.

https://reviewboard.mozilla.org/r/68960/#review68828

Comment 21

[Andrew Swan [:aswan]]

Comment on attachment 8777382 [details]
Bug 1290901 - [webext] Allow APIs implementation to return null when the API should not be injected in a context.

https://reviewboard.mozilla.org/r/68960/#review68830

Comment 22

[Andrew Swan [:aswan]]

Comment on attachment 8777383 [details]
Bug 1290901 - [webext] Add xpcshell test cases on nested namespaces and null/empty api objects.

https://reviewboard.mozilla.org/r/68962/#review68836

::: toolkit/components/extensions/test/xpcshell/test_ext_schemas_api_injection.js:86
(Diff revision 2)
> +    return null;
> +  });
> +
> +  let extension = ExtensionTestUtils.loadExtension({
> +    background() {
> +      if (browser.testnamespace.noBackgroundAPI) {

could you put the two schemas in different intermediate namespaces and test that the intermediate name doesn't get injected when its leaf object is empty?
i.e., i think you can just invert the order of testnamespace and {backgroundAPI,noBackgroundAPI} then test that browser.noBackgroundAPI doesn't exist.

Comment 23

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8777380 [details]
Bug 1290901 - [webext] Handle nested namespaced API schema.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68956/diff/2-3/

Comment 24

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8777381 [details]
Bug 1290901 - [webext] Allow "events" and "properties" in SubModuleType defined in JSON schema.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68958/diff/2-3/

Comment 25

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8777382 [details]
Bug 1290901 - [webext] Allow APIs implementation to return null when the API should not be injected in a context.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68960/diff/2-3/

Comment 26

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8777383 [details]
Bug 1290901 - [webext] Add xpcshell test cases on nested namespaces and null/empty api objects.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68962/diff/2-3/

Comment 27

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8777380 [details]
Bug 1290901 - [webext] Handle nested namespaced API schema.

https://reviewboard.mozilla.org/r/68956/#review68824

> Could you include a comment just noting that this needs to come after lines 1654-5 so that we don't accidentally inject empty intermediate nodes for nested namespaces?
> I also think the flow would be marginally clearer if you put return after line 1655 and then a separate "if namespace.includes..." clause with the comment leading into it, but that's a matter of taste, feel free to leave it as is if you prefer it.

Agree, I've update the patch to `continue` to the next namespace if the produced api object is empty (we can't use `return` because that `if` statement is inside a `for` loop that needs to complete to process all the defined API namespaces) and added a comment to explain that any non-empty nested namespace API objects will then be turned into nested objects.

Comment 28

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8777383 [details]
Bug 1290901 - [webext] Add xpcshell test cases on nested namespaces and null/empty api objects.

https://reviewboard.mozilla.org/r/68962/#review68836

> could you put the two schemas in different intermediate namespaces and test that the intermediate name doesn't get injected when its leaf object is empty?
> i.e., i think you can just invert the order of testnamespace and {backgroundAPI,noBackgroundAPI} then test that browser.noBackgroundAPI doesn't exist.

Suggested change applied to the updated version of this patch.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f8f56101b72b
[webext] Handle nested namespaced API schema. r=aswan
https://hg.mozilla.org/integration/autoland/rev/33b059c68484
[webext] Allow "events" and "properties" in SubModuleType defined in JSON schema. r=aswan
https://hg.mozilla.org/integration/autoland/rev/d28428dae62b
[webext] Allow APIs implementation to return null when the API should not be injected in a context. r=aswan
https://hg.mozilla.org/integration/autoland/rev/90418dcd93db
[webext] Add xpcshell test cases on nested namespaces and null/empty api objects. r=aswan

Comment 30

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

https://hg.mozilla.org/mozilla-central/rev/f8f56101b72b
https://hg.mozilla.org/mozilla-central/rev/33b059c68484
https://hg.mozilla.org/mozilla-central/rev/d28428dae62b
https://hg.mozilla.org/mozilla-central/rev/90418dcd93db