Closed
Bug 1256342
Opened 8 years ago
Closed 8 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•8 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•8 years ago
|
||
Assignee | ||
Comment 4•8 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•8 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•8 years ago
|
||
Attachment #8730532 -
Flags: review?(till)
Attachment #8730532 -
Flags: review?(bobbyholley)
Assignee | ||
Updated•8 years ago
|
Attachment #8730262 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment 7•8 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•8 years ago
|
||
> I'll defer to Till. OK. I just wanted your review for the test, fwiw. > actually, and period. Will fix.
Comment 9•8 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•8 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 12•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 13•8 years ago
|
||
The problem was the fix for bug 1256376.
Flags: needinfo?(bzbarsky)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(bzbarsky)
Comment 15•8 years ago
|
||
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/f5ee47a13b2d
Assignee | ||
Comment 16•8 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•8 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 19•8 years ago
|
||
bugherder |
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.
Description
•