Closed
Bug 1256376
Opened 8 years ago
Closed 8 years ago
forEach on typed arrays doesn't work very well over Xrays
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox48 | --- | affected |
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(3 files)
22.18 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
22.28 KB,
patch
|
Details | Diff | Splinter Review | |
26.55 KB,
patch
|
Details | Diff | Splinter Review |
It runs in the content compartment, because TypedArrayForEach uses CallTypedArrayMethodIfWrapped. This means it doesn't get the "Error: Accessing TypedArray data over Xrays is slow" thing, but it also means that if you close over things you probably can't touch them. This matters for the same reasons bug 1256342 matters.
Assignee | ||
Comment 1•8 years ago
|
||
More precisely, for purposes of content scripts, the problem is that you get an opaque wrapper for the callback function (because the expanded principal is not system, so no COWs). Once I fix bug 1256342 and figure out how to write content script tests, I can think about this one.
Assignee | ||
Comment 2•8 years ago
|
||
I'm not a huge fan of the copy/paste, but that seemsto be the file style... Note that there is one non-Xray change here: in TypedArraySort we were not in fact wrapping the provided callback in a check for whether the arraybuffer got detached _except_ when the callback was the default TypedArrayCompare!
Attachment #8730559 -
Flags: review?(till)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•8 years ago
|
||
Bobby, I assumed you're OK with till just reviewing this; the xray test changes are pretty simple.
Comment 4•8 years ago
|
||
Comment on attachment 8730559 [details] [diff] [review] Fix forEach on typed arrays to work over Xrays from web extension sandboxes Review of attachment 8730559 [details] [diff] [review]: ----------------------------------------------------------------- The duplication here is indeed unfortunate. I'd be for at least factoring out the attached buffer check. The rest is more annoying, so let's just keep it as-is. ::: js/src/builtin/TypedArray.js @@ +151,5 @@ > // This function is not generic. > + var isTypedArray = IsObject(this) && IsTypedArray(this); > + > + // We want to make sure that we have an attached buffer, per spec prose. > + if (isTypedArray) { I'd very much be ok with factoring out this check into a helper function. Perhaps including the isTypedArray check, returning a bool. I can't immediately think of a good name for a helper that also does that, though. The previous checks are all slightly different, so factoring them out wouldn't be exactly as straight-forward. (Granted, it'd also not really be complicated.) @@ +162,3 @@ > } > > + // If we got here, "this" is either a typed array or a cross-compartment Uber-nit: s/"this"/`this`/ (or |this|, if you prefer that). @@ +172,5 @@ > + if (isTypedArray) { > + len = TypedArrayLength(O); > + } else { > + len = callFunction(CallTypedArrayMethodIfWrapped, O, O, > + "TypedArrayLength"); Nit: this fits into 99 cols, no neither the wrapping nor the braces are needed. @@ +271,5 @@ > + if (isTypedArray) { > + len = TypedArrayLength(O); > + } else { > + len = callFunction(CallTypedArrayMethodIfWrapped, O, O, > + "TypedArrayLength"); This also fits into 99 cols. @@ +350,5 @@ > + if (isTypedArray) { > + len = TypedArrayLength(O); > + } else { > + len = callFunction(CallTypedArrayMethodIfWrapped, O, O, > + "TypedArrayLength"); And this. @@ +403,5 @@ > + if (isTypedArray) { > + len = TypedArrayLength(O); > + } else { > + len = callFunction(CallTypedArrayMethodIfWrapped, O, O, > + "TypedArrayLength"); And this. @@ +454,5 @@ > + if (isTypedArray) { > + len = TypedArrayLength(O); > + } else { > + len = callFunction(CallTypedArrayMethodIfWrapped, O, O, > + "TypedArrayLength"); And this. @@ +664,5 @@ > + if (isTypedArray) { > + len = TypedArrayLength(O); > + } else { > + len = callFunction(CallTypedArrayMethodIfWrapped, O, O, > + "TypedArrayLength"); And this. @@ +724,5 @@ > + if (isTypedArray) { > + len = TypedArrayLength(O); > + } else { > + len = callFunction(CallTypedArrayMethodIfWrapped, O, O, > + "TypedArrayLength"); And this. @@ +781,5 @@ > + if (isTypedArray) { > + len = TypedArrayLength(O); > + } else { > + len = callFunction(CallTypedArrayMethodIfWrapped, O, O, > + "TypedArrayLength"); And this. @@ +1058,5 @@ > + if (isTypedArray) { > + len = TypedArrayLength(O); > + } else { > + len = callFunction(CallTypedArrayMethodIfWrapped, O, O, > + "TypedArrayLength"); And this. @@ +1170,5 @@ > + var bufferDetached; > + if (isTypedArray) { > + bufferDetached = IsDetachedBuffer(buffer); > + } else { > + // This is totally cheating and only works because we know "this" `this` @@ +1171,5 @@ > + if (isTypedArray) { > + bufferDetached = IsDetachedBuffer(buffer); > + } else { > + // This is totally cheating and only works because we know "this" > + // abd "buffer" are same-compartment". and `buffer`
Attachment #8730559 -
Flags: review?(till) → review+
Assignee | ||
Comment 5•8 years ago
|
||
Comment 7•8 years ago
|
||
Backed out bug 1257919, bug 1256376 and bug 1256342 for Reftest, Jit and Spidermonkey failures. https://hg.mozilla.org/integration/mozilla-inbound/rev/42c21bf6657b https://hg.mozilla.org/integration/mozilla-inbound/rev/da683a9c88cf https://hg.mozilla.org/integration/mozilla-inbound/rev/b9462b6b2785 https://hg.mozilla.org/integration/mozilla-inbound/rev/f5962f5a4e3f https://hg.mozilla.org/integration/mozilla-inbound/rev/e22b4e5d45b6 https://hg.mozilla.org/integration/mozilla-inbound/rev/037fc169fe07 https://hg.mozilla.org/integration/mozilla-inbound/rev/421122394970 https://hg.mozilla.org/integration/mozilla-inbound/rev/692110787614 https://hg.mozilla.org/integration/mozilla-inbound/rev/3946d06e581e https://hg.mozilla.org/integration/mozilla-inbound/rev/5dc49e4fb827 https://hg.mozilla.org/integration/mozilla-inbound/rev/d93dc0d5266f https://hg.mozilla.org/integration/mozilla-inbound/rev/952832165a9b Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ff81c52375ba
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 8•8 years ago
|
||
The problem was the changes to use a helper function in this bug: it broke some code that assumed that self-hosted stuff never ever uses a helper function that can throw.
Flags: needinfo?(bzbarsky)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(bzbarsky)
Keywords: leave-open
Comment 11•8 years ago
|
||
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/102696dd72b5
Assignee | ||
Comment 12•8 years ago
|
||
OK, so apart from a bug in TypedArraySort (not getting "buffer" after the refactoring to use a helper function), there is one problem remaining here: We don't implement @@species on typed arrays yet. Which means this patch changes which global map() and filter() create their return value in: without this patch we enter the compartment of the `this` value and create there, while with this patch we do the creation in our original compartment. The former behavior is what @@species support would give us by default. So I guess I should wait for bug 1165053 to land before I can land this one, so we don't temporarily regress behavior.... arai, is that just blocked on the reviews for bug 1165052?
Depends on: 1165053
Flags: needinfo?(arai.unmht)
Assignee | ||
Comment 13•8 years ago
|
||
Comment 14•8 years ago
|
||
for now, bug 1165053 is waiting for bug 1165052, to land at the same time, and bug 1165052 is blocked by bug 1233642, that self-hosts Array.prototype.concat.
Flags: needinfo?(arai.unmht)
Comment 15•8 years ago
|
||
in addition to RegExp/Array, I'll wait for Promise (bug 911216) that also has @@species
Assignee | ||
Comment 16•8 years ago
|
||
We already support @@species on Promise with DOM promises. So there's nothing to wait for.
Flags: needinfo?(bzbarsky) → needinfo?(arai.unmht)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(bzbarsky)
Comment 17•8 years ago
|
||
I totally forgot about that. Given this, let's just land @@species support in pieces whenever we have them ready. Except for DOM Promises, it looks like everything will ship in the same release anyway, which seems to be the important part.
Comment 20•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/42f6a6aa1949
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 21•8 years ago
|
||
Comment 20 is when this got fixed....
You need to log in
before you can comment on or make changes to this bug.
Description
•