Fix devtools APIs schema incompatibilities

RESOLVED FIXED in Firefox 51

Status

()

Toolkit
WebExtensions: Developer Tools
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: rpl, Assigned: rpl)

Tracking

(Blocks: 1 bug)

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [devtools.inspectedWindow][devtools.network][devtools.panels] triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Assignee)

Description

2 years ago
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/
(Assignee)

Updated

2 years ago
Assignee: nobody → lgreco
Status: NEW → ASSIGNED
Iteration: --- → 51.1 - Aug 15
(Assignee)

Updated

2 years ago
Priority: -- → P1
(Assignee)

Updated

2 years ago
Whiteboard: [devtools] triaged
(Assignee)

Updated

2 years ago
Whiteboard: [devtools] triaged → [devtools.inspectedWindow][devtools.network][devtools.panels] triaged
(Assignee)

Updated

2 years ago
Blocks: 1211859
(Assignee)

Comment 1

2 years ago
Created attachment 8777380 [details]
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/
(Assignee)

Comment 2

2 years ago
Created attachment 8777381 [details]
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/
(Assignee)

Comment 3

2 years ago
Created attachment 8777382 [details]
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/
(Assignee)

Comment 4

2 years ago
Created attachment 8777383 [details]
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/
(Assignee)

Comment 5

2 years ago
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
(Assignee)

Updated

2 years ago
Attachment #8777380 - Flags: feedback?(aswan)
(Assignee)

Updated

2 years ago
Attachment #8777381 - Flags: feedback?(aswan)
(Assignee)

Updated

2 years ago
Attachment #8777382 - Flags: feedback?(aswan)
(Assignee)

Updated

2 years ago
Attachment #8777383 - Flags: feedback?(aswan)

Comment 6

2 years ago
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?

Updated

2 years ago
Attachment #8777380 - Flags: feedback?(aswan) → feedback+
(Assignee)

Comment 7

2 years ago
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

2 years ago
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?

Updated

2 years ago
Attachment #8777381 - Flags: feedback?(aswan) → feedback+

Comment 9

2 years ago
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?
(Assignee)

Comment 10

2 years ago
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: {
      ...
    }
  };
});
      
```
(Assignee)

Comment 11

2 years ago
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.
(Assignee)

Updated

2 years ago
Blocks: 1293301
(Assignee)

Updated

2 years ago
Blocks: 1293298

Updated

2 years ago
Attachment #8777382 - Flags: feedback?(aswan) → feedback+

Updated

2 years ago
Attachment #8777383 - Flags: feedback?(aswan) → feedback+
(Assignee)

Comment 12

2 years ago
mozreview-review-reply
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.
(Assignee)

Comment 13

2 years ago
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.
(Assignee)

Updated

2 years ago
Blocks: 1291737
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 18

2 years ago
mozreview-review
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.
Attachment #8777380 - Flags: review?(aswan) → review+

Comment 19

2 years ago
mozreview-review
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
Attachment #8777381 - Flags: review?(aswan) → review+

Comment 20

2 years ago
mozreview-review
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
Attachment #8777382 - Flags: review?(aswan)

Comment 21

2 years ago
mozreview-review
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
Attachment #8777382 - Flags: review+

Comment 22

2 years ago
mozreview-review
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.
Attachment #8777383 - Flags: review?(aswan) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 27

2 years ago
mozreview-review-reply
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.
(Assignee)

Comment 28

2 years ago
mozreview-review-reply
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.
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 29

2 years ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed

Comment 30

2 years ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.