Closed Bug 1256342 Opened 8 years ago Closed 8 years ago

Typed array iterators are broken over Xrays

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(2 files, 1 obsolete file)

STEPS TO REPRODUCE:

1)  Open a non-e10s tab.
2)  Ensure that devtools.chrome.enabled is true in about:config
3)  Tools > Web Developer > Scratchpad
4)  In the scratchpad menu: Environment > Browser
5)  Type in "(new content.Uint8Array()).entries()"
6)  Click "Display"

EXPECTED RESULTS: [object Array Iterator]

ACTUAL RESULTS: [object Opaque]

ANALYSIS: Unlike ArrayEntries, which just creates the iterator in its current compartment, TypedArrayEntries will enter the content compartment, because it's trying to validate whether the object is in fact a typed array and does this by doing the "unwrap and check" thing.  This is not the right behavior when Xrays are involved...
OK, so I tried fixing this in the obvious way... but then it runs into:

  Error: Accessing TypedArray data over Xrays is slow, and forbidden in order
  to encourage performant code. etc

Note that this came up in bug 1250930 where we're running in a content script and basically doing:
   
  [...new TypedArray(10)]

and that's a sane thing to have work, since it's not obviously involving Xrays.  They pop up because we're in a privileged sandbox with the content window on the proto chain.... :(
Flags: needinfo?(bobbyholley)
Attached patch Attempted fix (obsolete) — Splinter Review
bz is going to investigate more.
Flags: needinfo?(bobbyholley)
OK, so for a content script specifically we _do_ allow indexed access.  See the CompartmentPrivate::Get(CurrentGlobalOrNull(cx))->isWebExtensionContentScript check in the IsTypedArrayKey && IsArrayIndex case in JSXrayTraits::resolveOwnProperty.

So this patch is correct as far as it goes; I just can't test it across non-content-script xrays....  What's a good way to test it across content-script xrays, if you know offhand?
Flags: needinfo?(bobbyholley)
Blocks: 1256376
(In reply to Boris Zbarsky [:bz] from comment #4)
> OK, so for a content script specifically we _do_ allow indexed access.  See
> the
> CompartmentPrivate::Get(CurrentGlobalOrNull(cx))-
> >isWebExtensionContentScript check in the IsTypedArrayKey && IsArrayIndex
> case in JSXrayTraits::resolveOwnProperty.

Ah yes, we solved this problem already.
> 
> So this patch is correct as far as it goes; I just can't test it across
> non-content-script xrays....  What's a good way to test it across
> content-script xrays, if you know offhand?

Pass "isWebExtensionContentScript: true" when creating a sandbox?
Flags: needinfo?(bobbyholley)
Attachment #8730532 - Flags: review?(till)
Attachment #8730532 - Flags: review?(bobbyholley)
Attachment #8730262 - Attachment is obsolete: true
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment on attachment 8730532 [details] [diff] [review]
Fix typed array iteration to work correctly over Xrays

Review of attachment 8730532 [details] [diff] [review]:
-----------------------------------------------------------------

I'll defer to Till.

::: js/src/builtin/TypedArray.js
@@ +129,5 @@
> +     * one.  In the case when we're not sure we have a typed array (e.g. we
> +     * might have a cross-compartment wrapper for one), we can go ahead and call
> +     * GetAttachedArrayBuffer via CallTypedArrayMethodIfWrapped; that will throw
> +     * if we're not actaully a wrapped typed array, or if we have a detached
> +     * array buffer/

actually, and period.
Attachment #8730532 - Flags: review?(bobbyholley)
> I'll defer to Till.

OK.  I just wanted your review for the test, fwiw.

> actually, and period.

Will fix.
Comment on attachment 8730532 [details] [diff] [review]
Fix typed array iteration to work correctly over Xrays

Review of attachment 8730532 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, sorry for the delay.

::: js/src/builtin/TypedArray.js
@@ +130,5 @@
> +     * might have a cross-compartment wrapper for one), we can go ahead and call
> +     * GetAttachedArrayBuffer via CallTypedArrayMethodIfWrapped; that will throw
> +     * if we're not actaully a wrapped typed array, or if we have a detached
> +     * array buffer/
> +     */

I'd prefer C++-style line comments here to stay in line with the file's conventions. I don't care *that* strongly though, so up to you.

@@ +522,5 @@
>      // Step 2.
>      if (!IsObject(O) || !IsTypedArray(O)) {
> +        callFunction(CallTypedArrayMethodIfWrapped, O, O, "GetAttachedArrayBuffer");
> +    } else {
> +        GetAttachedArrayBuffer(O);

(Pre-existing) nit: no braces needed for single-line bodies in all branches.

@@ +1100,1 @@
>      // Step 2-3.

Please change this to "Steps 2-6". With or without removing the 4-6 comments below, up to you.
Attachment #8730532 - Flags: review?(till) → review+
> I'd prefer C++-style line comments here to stay in line with the file's conventions.

Done.

> (Pre-existing) nit: no braces needed for single-line bodies in all branches.

Gah.  And here I thought style was just different for C++ and JS...
The problem was the fix for bug 1256376.
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bzbarsky)
The old setup had observably broken behavior even without Xrays, and this test depended on it!

Here's a simpler in-browser testcase for the behavior:

  <iframe></iframe>
  <script>
  var entries = frames[0].Int8Array.prototype.entries;
  var arr = [...entries.call(new Int8Array(2))];
  console.log(arr[0] instanceof Array);
  console.log(arr[0] instanceof frames[0].Array);
  </script>

Per spec, I believe, and with my patch, this should log "false" then "true".  Current trunk logs "true" then "false" because we enter the compartment of the `this` value when doing entries(), so create the arrays in the wrong compartment.
Flags: needinfo?(bzbarsky)
Attachment #8733637 - Flags: review?(till)
Comment on attachment 8733637 [details] [diff] [review]
Additional test fix we need

Review of attachment 8733637 [details] [diff] [review]:
-----------------------------------------------------------------

Uh, wow. We should audit all the CallFooIfWrapped usages, I guess :(
Attachment #8733637 - Flags: review?(till) → review+
https://hg.mozilla.org/mozilla-central/rev/32a6f67bd8e8
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.