Closed Bug 1363886 Opened 7 years ago Closed 7 years ago

Check/normalize API return values, callback/listener arguments

Categories

(WebExtensions :: General, enhancement)

Product:
WebExtensions
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox56 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: zombie, Assigned: zombie)

References

Details

(Whiteboard: triaged)

Attachments

(3 files, 2 obsolete files)

Bug 1363886 - Part 1: Check API function results against schema
59 bytes, text/x-review-board-request
kmag
: review+
Details
Bug 1363886 - Part 2: Fix a number of async callback parameters schemas
59 bytes, text/x-review-board-request
kmag
: review+
Details
Bug 1363886 - Part 3: Check async callback arguments against schema
59 bytes, text/x-review-board-request
kmag
: review+
Details 
at least in debug builds.
Attachment #8869807 - Attachment is obsolete: true
Attachment #8869808 - Attachment is obsolete: true
Attachment #8867469 - Flags: review?(aswan)
Attachment #8869822 - Flags: review?(aswan)
Attachment #8869823 - Flags: review?(aswan)
Trying this uncovered several yaks of different colours, so I'll file a followup, but I still want to land something useful.
Assignee: nobody → tomica
Attachment #8867469 - Flags: review?(aswan) → review?(kmaglione+bmo)
Attachment #8869822 - Flags: review?(aswan) → review?(kmaglione+bmo)
Attachment #8869823 - Flags: review?(aswan) → review?(kmaglione+bmo)
Whiteboard: triaged
Comment on attachment 8869822 [details]
Bug 1363886 - Part 2: Fix a number of async callback parameters schemas

Hey Luca, can you please check out the devtools.inspectWindow schema, and recommend the best way we should change it here?
Attachment #8869822 - Flags: feedback?(lgreco)
Comment on attachment 8869822 [details]
Bug 1363886 - Part 2: Fix a number of async callback parameters schemas

https://reviewboard.mozilla.org/r/141370/#review151286

::: browser/components/extensions/schemas/devtools_inspected_window.json:139
(Diff revision 1)
>              "description": "A function called when evaluation completes.",
>              "optional": true,
>              "parameters": [
>                {
>                  "name": "result",
> -                "type": "object",
> +                "type": "any",

yeah, `any` is definitely a better type for the result 

(and the original schema from Chrome is actually wrong: https://cs.chromium.org/chromium/src/chrome/common/extensions/api/devtools/inspected_window.json?q=file:inspected_window.json+package:%5Echromium$&l=129)

::: browser/components/extensions/schemas/devtools_inspected_window.json:143
(Diff revision 1)
>                  "name": "exceptionInfo",
>                  "type": "object",
> +                "optional": true,
>                  "description": "An object providing details if an exception occurred while evaluating the expression.",
>                  "properties": {

I just double-checked this object in Chrome and this object has the same behavior that we currently support in our implementation:
- if `isException` is true, then only the `value` property is defined.
- if `isError` is true, then all the other properties (besides `value` and `isException`) are defined.

Like briefly discussed over irc, the actual format of the `exceptionInfo` object in our implementation is in the remote debugging actor, eg.:
- "isError == true" => http://searchfox.org/mozilla-central/source/devtools/server/actors/webextension-inspected-window.js#430-436
- "isException == true" => http://searchfox.org/mozilla-central/rev/20963d7269b1b14d455f47bc0260d0653015bf84/devtools/server/actors/webextension-inspected-window.js#489-492

and I agree with :zombie that a "choices" here is probably helpful to make this "exceptionInfo" type more readable, e.g.:

```
{
  "name": "exceptionInfo",
  "optional": true,
  "description": "...",
  "choices": [
    {
      "type": "object",
      "properties": {
        "isException": {
          "type": "boolean",
          "description": "..."
        },
        "value": {
          "type": "string",
          "description": "..."
        }
      }
    },
    {
      "type": "object",
      "properties": {
        "isError": {
          "type": "boolean",
          "description": "..."
        },
        "code": {
          "type": "string",
          "description": "..."
        },
        "description": {
          "type": "string",
          "description": "..."
        },
        "details": {
          "type": "array",
          "items": { "type": "any" },
          "description": "..."
        }
      }
    }
  ]
}
```
Comment on attachment 8867469 [details]
Bug 1363886 - Part 1: Check API function results against schema

https://reviewboard.mozilla.org/r/139018/#review162288

::: toolkit/components/extensions/Schemas.jsm:2058
(Diff revision 8)
>  
>  // Represents a "function" defined in a schema namespace.
>  FunctionEntry = class FunctionEntry extends CallEntry {
>    static parseSchema(schema, path) {
> +    let returns = null;
> +    if ("returns" in schema) {

Please only do this in debug builds. Parsing schemas is expensive.
Attachment #8867469 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8869822 [details]
Bug 1363886 - Part 2: Fix a number of async callback parameters schemas

https://reviewboard.mozilla.org/r/141370/#review162290
Attachment #8869822 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8869823 [details]
Bug 1363886 - Part 3: Check async callback arguments against schema

https://reviewboard.mozilla.org/r/141372/#review162292
Attachment #8869823 - Flags: review?(kmaglione+bmo) → review+
Pushed by tomica@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8955980312a6
Part 1: Check API function results against schema r=kmag
https://hg.mozilla.org/integration/autoland/rev/0bb1d7f7feb0
Part 2: Fix a number of async callback parameters schemas r=kmag
https://hg.mozilla.org/integration/autoland/rev/4b3e02945e19
Part 3: Check async callback arguments against schema r=kmag
Backed out for failing mochitests test_ext_contentscript_permission.html and test_chrome_ext_contentscript_unrecognizedprop_warning.html on Android 4.3 debug:

https://hg.mozilla.org/integration/autoland/rev/07a91e554ec522bbdd4ed665aafc0a35346e8d36
https://hg.mozilla.org/integration/autoland/rev/47673d5cfce3af5fe8362e42e2b98e3cab453a41
https://hg.mozilla.org/integration/autoland/rev/ec0aed41acb04ffa0c688b048779f877bfdeaa12

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=4b3e02945e193e39623f96942172a50865c70bd4&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&selectedJob=117204424
Both tests time out, e.g.

[task 2017-07-25T08:09:45.890573Z] 08:09:45     INFO -  72 INFO None73 INFO TEST-START | toolkit/components/extensions/test/mochitest/test_chrome_ext_contentscript_unrecognizedprop_warning.html
[task 2017-07-25T08:15:06.702293Z] 08:15:06     INFO -  Buffered messages logged at 08:09:46
[task 2017-07-25T08:15:06.703058Z] 08:15:06     INFO -  74 INFO SpawnTask.js | Entering test test_contentscript
[task 2017-07-25T08:15:06.703210Z] 08:15:06     INFO -  Buffered messages logged at 08:09:47
[task 2017-07-25T08:15:06.703645Z] 08:15:06     INFO -  75 INFO Extension loaded
[task 2017-07-25T08:15:06.707755Z] 08:15:06     INFO -  Buffered messages logged at 08:09:49
[task 2017-07-25T08:15:06.707914Z] 08:15:06     INFO -  76 INFO monitorConsole | [0] matched {"message":"1500970187930\taddons.webextension.<unknown>\tWARN\tLoading extension 'null': Reading manifest: Error processing content_scripts.0.unrecognized_property: An unexpected property was found in the WebExtension manifest.","errorMessage":null,"sourceName":null,"sourceLine":null,"lineNumber":null,"columnNumber":null,"category":null,"windowID":null,"isScriptError":false,"isWarning":false,"isException":false,"isStrict":false}
[task 2017-07-25T08:15:06.708080Z] 08:15:06     INFO -  77 INFO monitorConsole | extra message | {"message":"1500970188405\taddons.webextension.{a34046cd-1296-4bf0-b9be-53b88e97dffc}\tWARN\tLoading extension '{a34046cd-1296-4bf0-b9be-53b88e97dffc}': Reading manifest: Error processing content_scripts.0.unrecognized_property: An unexpected property was found in the WebExtension manifest.","errorMessage":null,"sourceName":null,"sourceLine":null,"lineNumber":null,"columnNumber":null,"category":null,"windowID":null,"isScriptError":false,"isWarning":false,"isException":false,"isStrict":false}
[task 2017-07-25T08:15:06.708133Z] 08:15:06     INFO -  Buffered messages logged at 08:10:01
[task 2017-07-25T08:15:06.708302Z] 08:15:06     INFO -  78 INFO monitorConsole | extra message | {"message":"[JavaScript Error: \"Error: Type error for result value (Error processing 0: Property \"cookieStoreId\" is required) for tabs.query.\" {file: \"undefined\" line: 0}]","errorMessage":"Error: Type error for result value (Error processing 0: Property \"cookieStoreId\" is required) for tabs.query.","sourceName":"undefined","sourceLine":"","lineNumber":0,"columnNumber":1,"category":"content javascript","windowID":0,"isScriptError":true,"isWarning":false,"isException":false,"isStrict":false,"innerWindowID":0}
[task 2017-07-25T08:15:06.708351Z] 08:15:06     INFO -  Buffered messages finished
[task 2017-07-25T08:15:06.708424Z] 08:15:06     INFO -  79 INFO TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_chrome_ext_contentscript_unrecognizedprop_warning.html | Test timed out.
Flags: needinfo?(tomica)
Oops, I totally forgot to run android tests :(
Flags: needinfo?(tomica)
Pushed by tomica@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d2d722422f87
Part 1: Check API function results against schema r=kmag
https://hg.mozilla.org/integration/autoland/rev/c43af73943bb
Part 2: Fix a number of async callback parameters schemas r=kmag
https://hg.mozilla.org/integration/autoland/rev/4ca0f86e67cb
Part 3: Check async callback arguments against schema r=kmag
Product: Toolkit → WebExtensions
See Also: → 1515820
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: