1248865 - Implement Xrays for ArrayBuffer

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P3)

Description

[janekptacijarabaci]

User Agent: Opera/9.80 (Windows NT 6.1; WOW64) Presto/2.12.388 Version/12.17

Steps to reproduce:

Reported to Greasemonkey here: https://github.com/greasemonkey/greasemonkey/issues/2364

1) Open Scratchpad

2) Menu: "Environment-Browser"

3) Insert the code:

var code = "" +
"'use strict';" +
"var worker_source_code = \"" +
    "'use strict';" +
    "self.onmessage = function(e)" +
    "{" +
      "console.error('worker receive and send', e.data, e.data.byteLength, typeof e.data.slice);" +
      "postMessage(e.data, [e.data]);" +

      "var ab = new ArrayBuffer(10);" +
      "console.error('worker send', ab, ab.byteLength, typeof ab.slice);" +
      "postMessage(ab, [ab]);" +

      "var obj = {byteLength: 100500, slice: ''};" +
      "console.error('worker send', obj, obj.byteLength, typeof obj.slice);" +
      "postMessage(obj);" +
    "};" +
"\";" +
"var worker = new Worker(URL.createObjectURL(new Blob([worker_source_code], {type: 'application/javascript;charset=UTF-8'})));" +
"worker.onmessage = function(e)" +
"{" +
  "console.error('main receive', e.data, e.data.byteLength, typeof e.data.slice);" +
"};" +
"var ab = new ArrayBuffer(10);" +
"console.error('main send', ab, ab.byteLength, typeof ab.slice);" +
"worker.postMessage(ab, [ab]);";

var sandbox = new Components.utils.Sandbox(
    content, {wantXrays: true, sandboxPrototype: content});
Components.utils.evalInSandbox(code, sandbox);

4) Run

5) The result:

Firefox 32.0.3-:
main send ArrayBuffer { byteLength: 0 } 10 function
worker receive and send ArrayBuffer { byteLength: 10 } 10 function
worker send ArrayBuffer { byteLength: 10 } 10 function
worker send Object { byteLength: 100500, slice: "" } 100500 string
main receive ArrayBuffer { byteLength: 10 } 10 function
main receive ArrayBuffer { byteLength: 10 } 10 function
main receive Object { byteLength: 100500, slice: "" } 100500 string

Firefox 33.0+ [e10s off]:
main send ArrayBuffer { byteLength: 0 } 10 function
worker receive and send ArrayBuffer { byteLength: 10 } 10 function
worker send ArrayBuffer { byteLength: 10 } 10 function
worker send Object { byteLength: 100500, slice: "" } 100500 string
XrayWrapper denied access to property "byteLength" (reason: object is not safely Xrayable). See https://developer.mozilla.org/en-US/docs/Xray_vision for more information. Note that only the first denied property access from a given global object will be reported.
main receive ArrayBuffer { byteLength: 10 } undefined undefined
main receive ArrayBuffer { byteLength: 10 } undefined undefined
main receive Object { byteLength: 100500, slice: "" } 100500 string

I suppose it's a bug in Firefox.

See:
https://bugzilla.mozilla.org/show_bug.cgi?id=987163
https://bugzilla.mozilla.org/show_bug.cgi?id=987669
https://bugzilla.mozilla.org/show_bug.cgi?id=1020609

See also:
https://bugzilla.mozilla.org/show_bug.cgi?id=734215
https://bugzilla.mozilla.org/show_bug.cgi?id=1150771

Thank you in advance for solving the problem.

Comment 1

[Till Schneidereit [:till]]

The reason for this behavior is that ArrayBuffers aren't xrayed, as opposed to typed arrays. If GreaseMonkey can switch to exposing a Uint8Array with the ArrayBuffer as its buffer instead, that should work.

Bobby, is there a fundamental reason for not xraying ArrayBuffers, or did it just not seem important enough?

Comment 2

[Bobby Holley (:bholley)]

(In reply to Till Schneidereit [:till] from comment #1)
> The reason for this behavior is that ArrayBuffers aren't xrayed, as opposed
> to typed arrays. If GreaseMonkey can switch to exposing a Uint8Array with
> the ArrayBuffer as its buffer instead, that should work.
> 
> Bobby, is there a fundamental reason for not xraying ArrayBuffers, or did it
> just not seem important enough?

No good reason. I'd certainly take a patch. Bug should block bug 914970.

Comment 3

[The 8472]

Maybe it would be good to ensure that any future structured-clonable or transferable object can also be xrayed?

Comment 4

[Bobby Holley (:bholley)]

(In reply to The 8472 from comment #3)
> Maybe it would be good to ensure that any future structured-clonable or
> transferable object can also be xrayed?

I agree that it would be good, it just involves writing code and tests which nobody has yet signed up to do. The old world (before JS Xrays) was insecure by default. The new world (where things are either either JS-Xrayed or Opaque) is secure by default, but inconvenient in some corner cases. That inconvenience is worth fixing, but wasn't important enough to block making the platform secure by default.

See http://bholley.net/blog/2016/the-right-fix.html for more context on this.

Comment 5

[The 8472]

I'm not complaining, I actually appreciate xrays, they're better than chrome's total isolation from content that requires those eval hacks.

I just wanted to point out that this story may potentially repeat itself when new types are added to the structured clone algorithm. If automated testing for all those cases is too complex that's fine too, someone will file a bug eventually if something doesn't work :P

Comment 6

[Bobby Holley (:bholley)]

Yeah, we do have some test that should catch new constructors, which requires them to get review from me, and then I can hassle them for adding Xrays. I can't guarantee that will always work, but it worked for Promises at least (when we moved them into the JS engine). See bug 911216 :-)

Comment 7

[janekptacijarabaci]

Btw, this bug is also very old: bug 1021289

Comment 8

[Tom S [:evilpie]]

I wanted to do this ever since running into that problem with Promises and ArrayBuffers from a different compartment. I hope I can get to this soon.

Comment 9

[Lars T Hansen [:lth]]

Many bonus points for you if you also do SharedArrayBuffer, so that I don't have to.  That may be separately filed as bug 1130988.

Comment 10

[Tom S [:evilpie]]

Attachment: WIP

Implements ClassSpec for ArrayBuffer and some Xray bits to get everything working. Still needs more cleanups and some tests. Assuming SharedArrayBuffer works the same I am going to do that as well.

Comment 11

[Tom S [:evilpie]]

Attachment: Change ArrayBuffer to use ClassSpec

Comment 12

[Tom S [:evilpie]]

Attachment: Some changes to JSXray

We have to allow Xrays to ArrayBuffer, we can remove some special case that was added for ArrayBuffers, but never extended. And a test!

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8803887 [details] [diff] [review]
Change ArrayBuffer to use ClassSpec

>+    JS_SELF_HOSTED_FN("slice", "ArrayBufferStaticSlice", 3, 0),

Does this behave as expected over Xrays?  I can't tell what it _should_ behave like in cross-global cases, because afaict this doesn't exist in the spec? 

>+    JS_SELF_HOSTED_FN("slice", "ArrayBufferSlice", 2, 0),

This one is a bit more interesting.  Per spec, it should use the constructor gotten via "this.constructor", right?  What will this.constructor be in the Xray case?  Will it be the target compartment's ctor or the Xray compartment's ctor?  Which behavior is actually desired here, and what happens in the non-Xray cross-compartment case?

Please make sure the tests test this scenario.

r=me with that addressed

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8803888 [details] [diff] [review]
Some changes to JSXray

>+    is(t.slice(4).byteLength, 8, "ArrayBuffer byteLength is correct after slicing");

Should test what the proto of t.slice(4) is.

>+    var i32Array = new Int32Array(t);

Should test what the proto of i32Array is, and that i32Array.buffer == t;

Also, please test that new win.Int32Array(t) does the right thing in terms of its .buffer and what its proto is.

r=me with those tests added.

Comment 15

[Tom S [:evilpie]]

See this interesting discussion on IRC how typed array buffers and array buffers across compartments interact: http://logs.glob.uno/?c=mozilla%23jsapi&s=24+Oct+2016&e=24+Oct+2016#c804293

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc837a7093fe
Change ArrayBuffer to use ClassSpec. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/28191c1f1a9a
Some small changes to JSXray and tests. r=bz

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/dc837a7093fe
https://hg.mozilla.org/mozilla-central/rev/28191c1f1a9a