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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
firefox48 --- affected

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(3 files)

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.
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.
Depends on: 1256342
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: nobody → bzbarsky
Status: NEW → ASSIGNED
Bobby, I assumed you're OK with till just reviewing this; the xray test changes are pretty simple.
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+
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)
Flags: needinfo?(bzbarsky)
Keywords: leave-open
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)
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)
in addition to RegExp/Array, I'll wait for Promise (bug 911216) that also has @@species
We already support @@species on Promise with DOM promises.  So there's nothing to wait for.
Flags: needinfo?(bzbarsky) → needinfo?(arai.unmht)
Flags: needinfo?(bzbarsky)
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.
I see, just landed them :)
Flags: needinfo?(arai.unmht)
Flags: needinfo?(bzbarsky)
Comment 20 is when this got fixed....
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.