I have constructed a Sandbox into which I have imported the WebSocket constructor. When I make a connection from this WebSocket to a remote server using binaryType "arraybuffer" and read a message from the socket, I am unable to read the buffer. Specifically, socket.onmessage = function(msg) { console.log("msg.data.byteLength is "+ msg.data.byteLength); var array = new Uint8Array(msg.data); console.log("array.buffer.byteLength is " + array.buffer.byteLength } shows that msg.data.byteLength is (e.g.) 4. array.buffer is defined, but array.buffer.byteLength (and array.length) are zero. A testcase is attached; it constructs the worker and attempts to connect to Facebook's chat server (with a bogus username and password, so the message it receives is an error). The testcase should not be used for automation; we would need to use a test websocket server. Any I/O should be sufficient for the test as long as the length was >0. I note that using binaryType="blob" and a FileReader in the same setup works fine.

An extension that demonstrates the issue. The connection is run immediately on bootstrap startup.

Additional notes from email: bholley: Well, sure - that's the "spidermonkey doesn't handle proxies/wrappers properly" case. In any case though, you may be right. When constructing a typed array with the security-wrapped array buffer, we'll get here: http://mxr.mozilla.org/mozilla-central/source/js/src/jstypedarray.cpp#1693 This check will fail (since the object is a proxy), and we'll end up in the bailout case below. However, I would think that the bailout case should work (albeit slowly), since, if the object is a proxy, all of the property-getting should work transparently. sicking: That's not a "slow path". That's a "We were handed an Array, not an ArrayBuffer" path. The first thing it does is get the .length property but ArrayBuffers have no such property, only a .byteLength property. And even if it did, ArrayBuffers don't have [0], [1], etc accessors to grab the individual bytes (you have to wrap in a view to get at the actual data). In other words, the two paths behave very differently. Intentionally. bholley: Ah! I see. Then yeah, that's probably exactly what's happening. So the fix is probably either to make this code understand proxies (questionably worth the effort), or to just try to unwrap if we hit a wrapper. The latter involves making a security decision, which my patches for bug 667388 let us do. So if Blake signs off on them, fixing this should be pretty straightforward.

Marking this bug as dependent on the structured clone bug, since presumably we'd want to use the same infrastructure here.

This is probably a dupe of bug 737245 - which is where all the cool kids are hanging out and working on a fix.

this is fixed with the patches in bug 737245

This problem has either come back, or I was mistaken that bug 737245 fixed it (I'm still trying to confirm that). In the current code, the problem is that in the case of a proxy, the call: new Uint8Array(array_buffer_proxy); end up calling js_GetLengthProperty() on array_buffer_proxy - however, array buffer's don't have a .length, but a .byteLength. Hence the length of the new array is zero. If I change the call to js_GetLengthProperty to code that gets .byteLength things work as expected, but I'm not sure this is the correct thing to do or if there is something else I'm missing. I'll attach the patch anyway to see what people think.

Attaching a JS shell test-case which is much simpler to run and debug. It creates an object, then tests an expression executed both in the same compartment as the object and then in another - the same expression should be true in both compartments. I've tried to make it the start of something that can end up checked into the tests/ directory. Unfortunately, it seems to demonstrate the patch I attached isn't complete - of the 3 tests here, 2 fail without the patch - but 1 still fails with the patch. It's late, so I'll attach it here as I may not get back to it for a couple of days... Before the patch: OK: 'let a= new ArrayBuffer(10);' -> 'a.byteLength==10' worked. FAILED: 'let a = new ArrayBuffer(10);' -> 'new Uint8Array(a).length==10' failed FAILED: 'let a = new ArrayBuffer(10); new Uint8Array(a)[0] = 123;' -> 'new Uint8Array(a)[0]==123' failed done - 1 worked, 2 failed After the patch: OK: 'let a= new ArrayBuffer(10);' -> 'a.byteLength==10' worked. OK: 'let a = new ArrayBuffer(10);' -> 'new Uint8Array(a).length==10' worked. FAILED: 'let a = new ArrayBuffer(10); new Uint8Array(a)[0] = 123;' -> 'new Uint8Array(a)[0]==123' failed done - 2 worked, 1 failed

The other patch was wrong - the branch it changed is handling the creation of a TypedArray from a regular javascript array so it broke that. A better fix seems to be to simply unwrap the object before checking it is already an array. The existing typedarray.js tests all pass and there is a new test file, typedarray_cc.js, which also passes with the patch.

markh: sorry, should've talked to you earlier. I'll look at your patch tomorrow, but I'm pretty sure I re-broke this one with bug 711843, because it fixed bug 743000. I'll look tomorrow, but you could check whether your patch is going to re-break 743000. I have a fix in my patch queue, but it's kind of messy. It might help if you could explain the scenario where you're wanting to create a view on an ArrayBuffer in another compartment. (I also have a test that looks very similar to yours!)

I'll try and look at that other bug, but a simplified usecase is, from a sandbox: """ let buffer = new ArrayBuffer(10); let reader = new FileReader(); reader.onload = function(event) { let a = new Uint8Array(reader.result); } let blob = new Blob([buffer], {type: "binary"}); reader.readAsArrayBuffer(blob); """ The actual use-case is using WebSockets, where the initial "buffer" came from the websocket's onmessage. The "real" usecase is in the initial attachment and that minimal case is in the bootstrap.js attachment (both now obsoleted but available)

*sigh* - yes, the first js shell test case from bug 743000 fails with this in place. Moving the unwrap up the stack fixes this and none of the other js shell tests fail in a debug build - but even then I suspect you to say the UnwrapObject() is still wrong - but that's OK, it's all a learning exercise for me :) Updating the patch which includes 3 new tests, one of which failed with the previous version of the patch. All JS tests pass in a debug build with this change (well - except for one strange test that times out, but that happens without the patch applied for me...)

So the basic problem is that if you just unwrap to allow new TypedArray(foreignArrayBuffer) to work, then you've created a bit of a monstrosity: a typed array in one compartment with a slot directly holding an array buffer from another compartment. I think cross-compartment wrappers are about the only thing that does things like that, and they need special handling by the garbage collector. One way to fix this would be to detect this case and copy the ArrayBuffer from its origin compartment into the created typed array's compartment, but then changes made through a different view in the origin compartment would not be visible. If you knew that nothing would ever touch that ArrayBuffer in the origin compartment, you could create a new ArrayBuffer but steal the actual data from the original ArrayBuffer (and neuter it so nothing can reach the data through it). I have a patch for that in bug 720949, and I think that's probably what you'd actually want in the use case you gave in comment 11. The general fix is to do the opposite -- detect when you're crossing compartments, and create the typed array in the *origin* compartment. Then you'd wrap it for the calling compartment and return the wrapper. I have a slightly outdated patch for that in bug 741041, but it's a little messy and Luke was wondering whether this case would actually show up in practice. I'm still not entirely sure if it does, given that it seems like transferable ArrayBuffers are probably better for the example you gave. I'll upload the latest patches there, anyway. And the 3rd possibility is that I'm missing something obvious and there's an easy fix. :)

I agree with the option of "fix it on the JS engine side": this is a bug that is made visible to plain web content with compartment-per-global, so we should just fix it.

Requesting tracking for web-visible regression from cpg here.

I have a fix for this in bug 741041, which already has r+ but I just need to rebase it on top of the RootedVar/Handle changes. Should be landing shortly.

According to bug 741040 comment 9, this was in fact *not* fixed by bug 741041. That is bad. Lemme see how badly I can mess up the tracking flags...

(In reply to Steve Fink [:sfink] from comment #17) > According to bug 741040 comment 9, this was in fact *not* fixed by bug > 741041. That is bad. To let you know, I got a "permission denied to access object" exception when doing "new Uint8Array()"

mark was able to get a test together for this last night, I tweeked it a bit this morning. this tests our use case in both content iframes and xul iframes, as well is differentiating the hidden window to make sure that is not an issue. The content test currently passes, the xul tests fail.

Comment on attachment 631464 [details] new test for error This test relies on some test infrastructure from the social project. It might be better if the test can be changed back to not relying on this so the test could be run by the platform/content guys, and even dropped directly next to other FileReader tests in the tree.

Assigning to dmandelin temporarily so that we can find an assignee for this regression.

Luke: turns out I left a big gap in my test cases using the cross-compartment array buffers and their views -- I only tested function calls, not property accesses. --- When constructing a view on an ArrayBuffer in a different compartment, a proxy is returned for a typed array in that other compartment, and that typed array's prototype is itself a proxy for the prototype in the original compartment. When accessing properties on the resulting object, the correct routine will be used, but the proxy will be passed as the target object. This proxy must be unwrapped to get the properties' values.

The patch I attached will fix one of the problems I saw in a test case; I'm not sure if it fixes everything. In particular, I would expect an error message referring to TypedArray.prototype.something, not "permission denied". Oh, and this is mine unless I can track it down to XPConnect or something. Maybe the wrapping solution I explained badly above (should've said "*target*" instead of "*origin*" compartment; see below) is having trouble when one of the compartments is not accessible from the other. The test case I came up with to check that did not reproduce it, though.

Comment on attachment 634293 [details] [diff] [review] Typed array property access should allow proxies The "don't sprinkle random UnwrapObjects around b/c it makes it really easy to have cross-compartment violations" principle indicates that this patch isn't the way to go and, lo and behold, > JS_SET_RVAL(cx, vp, ObjectValue(*TypedArray::getBuffer(tarray))); a cross-compartment violation (getBuffer(tarray)->compartment() != cx->compartment)! Waldo points out that these properties are now defined to be accessors so we should make them accessors which means turning them into JSNatives which would allow us to use NonGenericMethodGuard which will, I believe, solve these problems cleanly.

(In reply to Luke Wagner [:luke] from comment #24) > Comment on attachment 634293 [details] [diff] [review] > Typed array property access should allow proxies > > Waldo points out that these properties are now defined to be accessors so we > should make them accessors which means turning them into JSNatives which > would allow us to use NonGenericMethodGuard which will, I believe, solve > these problems cleanly. That made sense to me, so I implemented it. How can I do it without reopening bug 565604? The problem is that if I have an object o whose prototype is a Float32Array, then presumably o.length should retrieve the length of the array. But o is passed as |this|, so although the correct getter will be invoked, it will be invoked with an Object |this|, so NonGenericMethodGuard will yell about it being called with the wrong clasp. I could search up the proto chain for the typed array, but then I would have to reconstruct a CallArgs with the |this| value swapped in order to use NonGenericMethodGuard. On the other hand, my reading of section 4.4.6 of the WebIDL spec seems to indicate that this shouldn't work in the first place.

Perhaps Waldo can comment on this?

It shouldn't work in the first place. I tend to think typed arrays, array buffers, and all that should have actual data properties for this stuff, not accessor properties. This is easily fixed if typed arrays were specified in ECMAScript spec style. It's not something you can fix using WebIDL. Typed arrays really push the boundaries of WebIDL, and I think we'd be better off if they didn't use it. But since typed arrays do use WebIDL now, throwing on accessing length via prototype chain is correct. Note that implementing these properties is hard in the cross-compartment case now because of how special typed arrays are (really, exactly the same way dense arrays are special now). But it'd be easy in the new object/property representation.

(In reply to Jeff Walden [:Waldo] (busy, try to prefer other reviewers if possible) from comment #27) > Note that implementing these properties is hard in the cross-compartment > case now because of how special typed arrays are (really, exactly the same > way dense arrays are special now). ...if they're to be visible as data properties, I should say.

> It shouldn't work in the first place. That's correct according to my reading of WebIDL. > I tend to think typed arrays, array buffers, and all that should have actual > data properties for this stuff, not accessor properties. When you say "this stuff" do you just mean for the immutable .length property? Or also for the indexed properties? Anyway, I'm not sure I agree. First, a typed array is a view that aliases the data of a buffer, so its values need to be in sync with the buffer. And I've failed to convince Kenneth Russell to give up on the awful .length-zeroing semantics for transferring buffers, so even that is a property whose value can change and must be kept in sync with the buffer. > This is easily > fixed if typed arrays were specified in ECMAScript spec style. It's not > something you can fix using WebIDL. Typed arrays really push the boundaries > of WebIDL, and I think we'd be better off if they didn't use it. I do agree with this, and ES6 will specify typed arrays in the usual ES spec style. There's really no point in doing it via WebIDL. It's an unnecessary layer of spec indirection when the only point to typed arrays is to be an ECMAScript feature. Dave

(In reply to Dave Herman [:dherman] from comment #29) > > I tend to think typed arrays, array buffers, and all that should have actual > > data properties for this stuff, not accessor properties. > > When you say "this stuff" do you just mean for the immutable .length > property? Or also for the indexed properties? Since ECMAScript only gives you [[GetOwnProperty]] and [[DefineOwnProperty]] rope, I think you'd have to handle every property algorithmically, indexed properties included. I think it's a bad idea to have elements exposed with odd getter/setter functions (especially since there'd have to be a different one for every element), so they'd probably have to be exposed as data properties. (Although they wouldn't be implemented as normal data properties by any engine that cares about performance at all. And possibly not even by ones that don't, for the aliasing concern.) They'd be non-configurable fromt he start so they can't be deleted, which would be shenanigans. [[DefineOwnProperty]] would prevent things like marking them non-writable. > Anyway, I'm not sure I agree. First, a typed array is a view that aliases > the data of a buffer, so its values need to be in sync with the buffer. And > I've failed to convince Kenneth Russell to give up on the awful > .length-zeroing semantics for transferring buffers, so even that is a > property whose value can change and must be kept in sync with the buffer. If you have length-zeroing, and that presumably nulls out buffer for consistency and sets byteOffset to zero as well, then yeah, the named properties have to be accessors. I'd forgotten about that additional idea. > I do agree with this, and ES6 will specify typed arrays in the usual ES spec > style. There's really no point in doing it via WebIDL. It's an unnecessary > layer of spec indirection when the only point to typed arrays is to be an > ECMAScript feature. Good stuff!

My apologies for having 3 completely different ways of defining the accessors. I couldn't come up with a template scheme that covered all of them. Note that I also avoided failing a test by nuking it entirely. Waldo said on IRC that it's ok.

Comment on attachment 636558 [details] [diff] [review] Typed array property access should allow proxies Review of attachment 636558 [details] [diff] [review]: ----------------------------------------------------------------- Excellent, thanks! ::: js/src/jstypedarray.cpp @@ +1385,5 @@ > obj->setLastPropertyInfallible(empty); > > DebugOnly<uint32_t> bufferByteLength = buffer.byteLength(); > + DebugOnly<uint32_t> arrayByteLength = static_cast<uint32_t>(byteLengthValue(obj).toInt32()); > + DebugOnly<uint32_t> arrayByteOffset = static_cast<uint32_t>(byteOffsetValue(obj).toInt32()); Could you use the non-Value-returning accessors instead of the *Value(obj).toInt32()? (Here and maybe 5 other times below.) @@ +1392,2 @@ > JS_ASSERT(buffer.dataPointer() <= getDataOffset(obj)); > JS_ASSERT(getDataOffset(obj) <= offsetData(obj, bufferByteLength)); I think it would look nicer just wrap the whole thing in #ifdef DEBUG. It would also avoid the possibility that the compiler doesn't throw everything away. @@ +1516,5 @@ > + { > + RootedId id(cx, NameToId(name)); > + unsigned flags = JSPROP_PERMANENT | JSPROP_READONLY | JSPROP_SHARED | JSPROP_GETTER; > + > + JSObject *getter = js_NewFunction(cx, NULL, Getter<ValueGetter>, 0, 0, global, NULL); You can now use cx->compartment->global() instead of passing a handle to the global. ::: js/src/jstypedarrayinlines.h @@ +67,3 @@ > inline uint32_t > TypedArray::getLength(JSObject *obj) { > + return lengthValue(obj).toInt32(); Since you are tweaking these anyways, could you rename getX() to x() for X = {Length, ByteOffset, ByteLength, ...}, since these are all infallible getters?

(In reply to Luke Wagner [:luke] from comment #32) > ::: js/src/jstypedarray.cpp > @@ +1392,2 @@ > > JS_ASSERT(buffer.dataPointer() <= getDataOffset(obj)); > > JS_ASSERT(getDataOffset(obj) <= offsetData(obj, bufferByteLength)); > > I think it would look nicer just wrap the whole thing in #ifdef DEBUG. It > would also avoid the possibility that the compiler doesn't throw everything > away. Fixed. > @@ +1516,5 @@ > > + { > > + RootedId id(cx, NameToId(name)); > > + unsigned flags = JSPROP_PERMANENT | JSPROP_READONLY | JSPROP_SHARED | JSPROP_GETTER; > > + > > + JSObject *getter = js_NewFunction(cx, NULL, Getter<ValueGetter>, 0, 0, global, NULL); > > You can now use cx->compartment->global() instead of passing a handle to the > global. Ok, though I may have done it just in time for njn to make it fallible. I guess he gets to switch it to getGlobal() for me. > ::: js/src/jstypedarrayinlines.h > @@ +67,3 @@ > > inline uint32_t > > TypedArray::getLength(JSObject *obj) { > > + return lengthValue(obj).toInt32(); > > Since you are tweaking these anyways, could you rename getX() to x() for X = > {Length, ByteOffset, ByteLength, ...}, since these are all infallible > getters? Done, though this propagated to a number of different files. Also note that I had to modify some regression tests that depended on eg Object.create(new Uint32Array(1)).byteLength being accessible through the proto chain. https://hg.mozilla.org/integration/mozilla-inbound/rev/d861452b261c

Backed out on inbound because of build errors: https://hg.mozilla.org/integration/mozilla-inbound/rev/2963d4018347 https://tbpl.mozilla.org/php/getParsedLog.php?id=13052793&tree=Mozilla-Inbound

Should've built after rebasing. My tree was a day old. https://hg.mozilla.org/integration/mozilla-inbound/rev/c408dd243e7a

Should this web regression fix be considered for uplift to FF15? What's the risk profile? Please nominate for approval if low risk and/or high reward.

(In reply to Alex Keybl [:akeybl] from comment #37) > Should this web regression fix be considered for uplift to FF15? What's the > risk profile? Please nominate for approval if low risk and/or high reward. The bug's kind of old by now, so if it affected the web I'd expect we'd have heard by now. I think this was about something in a sandbox for social API or other chrome code not working. Somebody correct me if I'm wrong.

(In reply to David Mandelin [:dmandelin] from comment #38) > (In reply to Alex Keybl [:akeybl] from comment #37) > > Should this web regression fix be considered for uplift to FF15? What's the > > risk profile? Please nominate for approval if low risk and/or high reward. > > The bug's kind of old by now, so if it affected the web I'd expect we'd have > heard by now. I think this was about something in a sandbox for social API > or other chrome code not working. Somebody correct me if I'm wrong. That is correct, and the feature this is being used for (socialapi) is being targeted at FF16 so this bug isn't blocking that.

[Triage comment] Given comment 39 I'm going to untrack this for 15 then and mark as wontfix. It can ride the trains.