Drop "binary" type from Schemas.jsm

RESOLVED FIXED in Firefox 57

Status

()

Toolkit
WebExtensions: General
RESOLVED FIXED
10 months ago
8 months ago

People

(Reporter: robwu, Assigned: robwu)

Tracking

57 Branch
mozilla57
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

()

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

Attachments

(1 attachment)

(Assignee)

Description

10 months ago
The "binary" type is supposed to mean "ArrayBuffer or ArrayBufferView".
I think that we should remove it:
- We do not use this type in any of our schemas.
- We have no tests for this type.
- The original meaning ("ArrayBuffer | ArrayBufferView
 is not implemented.
- "binary" has no clear meaning to API consumers.

For more context on how the "binary" type is (mis)used in Chrome, see https://crbug.com/760589
(Assignee)

Comment 1

10 months ago
I tried to declare a new API with the "binary" type, and Schemas.jsm threw "type is not defined" at the parseSchema method [1] because "binary" is not a part of the TYPES object [2].

I'm going to remove it.

[1] https://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/toolkit/components/extensions/Schemas.jsm#2702
[2] https://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/toolkit/components/extensions/Schemas.jsm#2362
Assignee: nobody → rob
Blocks: 1356543
Comment hidden (mozreview-request)
(Assignee)

Comment 3

10 months ago
(side note to accompany the patch)

By removing support for "binary", we don't have to use ChromeUtils.getClassName any more to determine whether an object is an array or not.
Incidentally, using Array.isArray is faster.

Benchmark:

var res = true;
var array = [], notarray = {};
console.time('Array.isArray(array)');
for (var i = 0; i < 100000; ++i) res = Array.isArray(array);
console.timeEnd('Array.isArray(array)');

console.time('ChromeUtils.getClassName(array, true) === "Array"');
for (var i = 0; i < 100000; ++i) res = ChromeUtils.getClassName(array, true) === 'Array';
console.timeEnd('ChromeUtils.getClassName(array, true) === "Array"');

res = false;
console.time('Array.isArray(notarray)');
for (var i = 0; i < 100000; ++i) res = Array.isArray(notarray);
console.timeEnd('Array.isArray(notarray)');

console.time('ChromeUtils.getClassName(notarray, true) === "Array"');
for (var i = 0; i < 100000; ++i) res = ChromeUtils.getClassName(notarray, true) === 'Array';
console.timeEnd('ChromeUtils.getClassName(notarray, true) === "Array"');

Results (lower is better):

Array.isArray(array): timer started
Array.isArray(array): 73.77ms
ChromeUtils.getClassName(array, true) === "Array": timer started
ChromeUtils.getClassName(array, true) === "Array": 93.59ms
Array.isArray(notarray): timer started
Array.isArray(notarray): 66.66ms
ChromeUtils.getClassName(notarray, true) === "Array": timer started
ChromeUtils.getClassName(notarray, true) === "Array": 79.63ms
There are at least two uses of the array buffer in our API, even if they are not marked as "binary" type (which I would consider to be a bug in itself):

1) http://searchfox.org/mozilla-central/source/browser/components/extensions/schemas/geckoProfiler.json#110
2) http://searchfox.org/mozilla-central/source/toolkit/components/extensions/schemas/web_request.json#181

But on a general note, I don't understand both why we should remove it, nor why this blocks bug 1356543.

Comment 5

10 months ago
mozreview-review
Comment on attachment 8902881 [details]
Bug 1395172 - Remove "binary" type from Schemas.jsm

https://reviewboard.mozilla.org/r/174578/#review180128
Attachment #8902881 - Flags: review?(tomica)
(Assignee)

Comment 6

10 months ago
Comment on attachment 8902881 [details]
Bug 1395172 - Remove "binary" type from Schemas.jsm

(In reply to Tomislav Jovanovic :zombie from comment #4)
> There are at least two uses of the array buffer in our API, even if they are
> not marked as "binary" type (which I would consider to be a bug in itself):

Because "binary" is not a sensible type (see first report and linked crbug).
In Chrome it was only introduced to have one type for ArrayBuffer, ArrayBufferView (= typed arrays + DataView). However, this has never been documented anywhere, and the type is not used in the API either for the original purpose.

> But on a general note, I don't understand both why we should remove it, nor
> why this blocks bug 1356543.

In Firefox, the "binary" type is a half-implemented alias for the ArrayBuffer type.
Half-implemented because the "binary" type is recognized, but unusable (see comment 1).
THis blocks bug 1356543 because in that bug I introduce an API that uses an ArrayBuffer (https://bugzil.la/1356543#c32).
I initially used "type": "binary" (and the resulting API is unusable because of comment 1).
Then I tried to ignore the "binary" type by using "type":"object" and "isInstanceOf":"ArrayBuffer", but that does still not work because the type is "binary", not "object".

There are no advantages for keeping "binary", hence I am removing it.


I selected you as a reviewer because you reviewed related functionality (typing in schemas) in bug 1391405.
Should I also find a WE peer for r+ even if you give r+?
Attachment #8902881 - Flags: review?(tomica)
Comment on attachment 8902881 [details]
Bug 1395172 - Remove "binary" type from Schemas.jsm

(In reply to Rob Wu [:robwu] from comment #6)
> Half-implemented because the "binary" type is recognized, but unusable

My first instinct would be to fix instead of removing it (I don't find it confusing or "not sensible").


> Should I also find a WE peer for r+ even if you give r+?

Officially, all commits need a r+ from a peer before landing.  I'm often asked to do a review before passing it off for final rs?, but for something this simple, you might as well ask Kris directly (he might be ok with removing it ;).
Attachment #8902881 - Flags: review?(tomica)
(Assignee)

Comment 8

10 months ago
(In reply to Tomislav Jovanovic :zombie from comment #7)
> (In reply to Rob Wu [:robwu] from comment #6)
> > Half-implemented because the "binary" type is recognized, but unusable
> 
> My first instinct would be to fix instead of removing it (I don't find it
> confusing or "not sensible").

A distinct type is only useful if it makes sense in the implementation or if it helps with API documentation. Neither is the case here, and here is why:

implementation ("not sensible"):
"Fixing" means that "binary" would become an alias for "ArrayBuffer". The type system will not provide special functionality to "binary"/ArrayBuffer, so that's why I don't consider it sensible to have a "binary" type. The "ArrayBuffer" type can already be expressed in the schema via isInstanceOf, and that is sufficient for validation.

documentation ("confusing"):
I view the type as "confusing", because unlike "string", "number", etc., there is no obvious definition for "binary" in JS (binary string? typed arrays? ArrayBuffer? Blob?).


The only reason for "binary" being part of Schemas.jsm is because Chrome's schema also has a "binary" type. That is hardly an argument in favor of keeping or fixing the half-implemented "binary" type, especially considering the remarks from the OP of this bug.
(Assignee)

Comment 9

10 months ago
Comment on attachment 8902881 [details]
Bug 1395172 - Remove "binary" type from Schemas.jsm

This patch removes the unused "binary" type, whose presence prevents the use of ArrayBuffer objects as parameters in extension APIs.

An alternative to removal is fixing it, but as I argued in the bug report, there is no benefit to doing so, so I choose the simpler approach of removing the "binary" type.
Attachment #8902881 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 10

10 months ago
No longer blocks bug 1356543 because the API can be declared with the schema "type": "any", "isInstanceOf": "ArrayBuffer".

My previous attempt, "type":"object", "isInstanceOf":"ArrayBuffer" works in release, but in debug build it triggered the error "InternalError: isInstanceOf can only be used with objects that are otherwise unrestricted" at https://searchfox.org/mozilla-central/rev/999385a5e8c2d360cc37286882508075fc2078bd/toolkit/components/extensions/Schemas.jsm#1628
No longer blocks: 1356543

Comment 11

10 months ago
mozreview-review
Comment on attachment 8902881 [details]
Bug 1395172 - Remove "binary" type from Schemas.jsm

https://reviewboard.mozilla.org/r/174578/#review181114
Attachment #8902881 - Flags: review?(kmaglione+bmo) → review+

Comment 12

10 months ago
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/94eb3eee0585
Remove "binary" type from Schemas.jsm r=kmag
https://hg.mozilla.org/mozilla-central/rev/94eb3eee0585
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

Comment 14

8 months ago
Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.
Flags: needinfo?(rob)
(Assignee)

Comment 15

8 months ago
Not needed, this is just a small refactor of a broken internal API that was not used anywhere.
Flags: needinfo?(rob) → qe-verify-
You need to log in before you can comment on or make changes to this bug.