Closed
Bug 1256342
Opened 9 years ago
Closed 9 years ago
Typed array iterators are broken over Xrays
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(2 files, 1 obsolete file)
8.69 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
1.44 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
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...
![]() |
Assignee | |
Comment 1•9 years ago
|
||
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)
![]() |
Assignee | |
Comment 2•9 years ago
|
||
![]() |
Assignee | |
Comment 4•9 years ago
|
||
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)
Comment 5•9 years ago
|
||
(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)
![]() |
Assignee | |
Comment 6•9 years ago
|
||
Attachment #8730532 -
Flags: review?(till)
Attachment #8730532 -
Flags: review?(bobbyholley)
![]() |
Assignee | |
Updated•9 years ago
|
Attachment #8730262 -
Attachment is obsolete: true
![]() |
Assignee | |
Updated•9 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment 7•9 years ago
|
||
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)
![]() |
Assignee | |
Comment 8•9 years ago
|
||
> I'll defer to Till.
OK. I just wanted your review for the test, fwiw.
> actually, and period.
Will fix.
Comment 9•9 years ago
|
||
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+
![]() |
Assignee | |
Comment 10•9 years ago
|
||
> 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...
Comment 11•9 years ago
|
||
![]() |
||
Comment 12•9 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 13•9 years ago
|
||
The problem was the fix for bug 1256376.
Flags: needinfo?(bzbarsky)
Comment 14•9 years ago
|
||
![]() |
Assignee | |
Updated•9 years ago
|
Flags: needinfo?(bzbarsky)
Comment 15•9 years ago
|
||
![]() |
Assignee | |
Comment 16•9 years ago
|
||
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 17•9 years ago
|
||
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+
Comment 18•9 years ago
|
||
Comment 19•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•