Closed
Bug 1363886
Opened 7 years ago
Closed 7 years ago
Check/normalize API return values, callback/listener arguments
Categories
(WebExtensions :: General, enhancement)
WebExtensions
General
Tracking
(firefox56 fixed)
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: zombie, Assigned: zombie)
References
Details
(Whiteboard: triaged)
Attachments
(3 files, 2 obsolete files)
at least in debug builds.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8869807 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8869808 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8867469 -
Flags: review?(aswan)
Attachment #8869822 -
Flags: review?(aswan)
Attachment #8869823 -
Flags: review?(aswan)
Assignee | ||
Comment 13•7 years ago
|
||
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
Assignee | ||
Updated•7 years ago
|
Attachment #8867469 -
Flags: review?(aswan) → review?(kmaglione+bmo)
Attachment #8869822 -
Flags: review?(aswan) → review?(kmaglione+bmo)
Attachment #8869823 -
Flags: review?(aswan) → review?(kmaglione+bmo)
Updated•7 years ago
|
Whiteboard: triaged
Assignee | ||
Comment 14•7 years ago
|
||
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 15•7 years ago
|
||
mozreview-review |
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 16•7 years ago
|
||
mozreview-review |
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 17•7 years ago
|
||
mozreview-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 18•7 years ago
|
||
mozreview-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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 30•7 years ago
|
||
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
Comment 31•7 years ago
|
||
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)
Comment 32•7 years ago
|
||
This caused failures in more Android tests, see https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=080aa07d32df3d0697f0a44ed72121f384fa6417&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 35•7 years ago
|
||
Oops, I totally forgot to run android tests :(
Flags: needinfo?(tomica)
Comment 36•7 years ago
|
||
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
Comment 37•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d2d722422f87 https://hg.mozilla.org/mozilla-central/rev/c43af73943bb https://hg.mozilla.org/mozilla-central/rev/4ca0f86e67cb
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•