Closed Bug 1395172 Opened 7 years ago Closed 7 years ago

Drop "binary" type from Schemas.jsm

Categories

(WebExtensions :: General, enhancement)

57 Branch
enhancement
Not set
normal

Tracking

(firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: robwu, Assigned: robwu)

Details

Attachments

(1 file)

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
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
(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 on attachment 8902881 [details]
Bug 1395172 - Remove "binary" type from Schemas.jsm

https://reviewboard.mozilla.org/r/174578/#review180128
Attachment #8902881 - Flags: review?(tomica)
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)
(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.
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)
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 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+
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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
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)
Not needed, this is just a small refactor of a broken internal API that was not used anywhere.
Flags: needinfo?(rob) → qe-verify-
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.