Closed Bug 1248865 Opened 4 years ago Closed 3 years ago

Implement Xrays for ArrayBuffer

Categories

(Core :: XPConnect, defect, P3)

47 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: janekptacijarabaci, Assigned: evilpie)

References

(Blocks 1 open bug)

Details

(Keywords: addon-compat, Whiteboard: dom-triaged btpp-backlog)

Attachments

(2 files, 1 obsolete file)

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.
Keywords: addon-compat
OS: Unspecified → All
Hardware: Unspecified → All
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?
Component: JavaScript Engine → XPConnect
Flags: needinfo?(bobbyholley)
(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.
Flags: needinfo?(bobbyholley)
Blocks: XrayToJS
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
Whiteboard: dom-triaged btpp-backlog
Maybe it would be good to ensure that any future structured-clonable or transferable object can also be xrayed?
(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.
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
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 :-)
Btw, this bug is also very old: bug 1021289
Summary: ArrayBuffer (etc.), received from Worker, denied access to properties -> undefined (Components.utils.evalInSandbox) → ArrayBuffer (etc.), received from Worker, denied access to properties -> undefined (Components.utils.evalInSandbox) (need xrays for arraybuffer)
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.
Assignee: nobody → evilpies
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.
Attached patch WIP (obsolete) — Splinter Review
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.
We have to allow Xrays to ArrayBuffer, we can remove some special case that was added for ArrayBuffers, but never extended. And a test!
Attachment #8803662 - Attachment is obsolete: true
Summary: ArrayBuffer (etc.), received from Worker, denied access to properties -> undefined (Components.utils.evalInSandbox) (need xrays for arraybuffer) → Implement Xrays for ArrayBuffer
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
Attachment #8803887 - Flags: review+
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.
Attachment #8803888 - Flags: review+
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
Pushed by evilpies@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/dc837a7093fe
https://hg.mozilla.org/mozilla-central/rev/28191c1f1a9a
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.