Closed Bug 1256342 Opened 9 years ago Closed 9 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+
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.

Attachment

General

Created:
Updated:
Size: