Closed Bug 807237 Opened 12 years ago Closed 12 years ago

Add 'data' parameter to JS_StealArrayBufferContents


(Core :: JavaScript Engine, defect)

18 Branch
Windows 7
Not set



Tracking Status
firefox18 --- fixed
firefox19 --- fixed


(Reporter: roc, Assigned: roc)



(1 file)

This allows the stealer to use the data while it's not attached to an ArrayBuffer.
Attached patch fixSplinter Review
Attachment #676918 - Flags: review?(sphink)
Comment on attachment 676918 [details] [diff] [review]

Review of attachment 676918 [details] [diff] [review]:

This one still feels weird enough that I want an sr on it.

dmandelin: roc wants to use an ArrayBuffer's contents from another thread, one that does not have a JSRuntime. If it had a JSRuntime, structured clone with a Transferable ArrayBuffer argument would do the right thing -- it would un-inline the data if needed, then pass a pointer to the contents to the other thread, and re-wrap it with an ArrayBuffer object.

JS_StealArrayBufferContents almost does this, except it hands back an opaque "contents" pointer instead of the data. You're only supposed to use this to create a new ArrayBuffer. In reality, it's a malloced ElementsHeader followed immediately by the data. Once Waldo splits out elements storage, I believe we'll be able to do cool things like accept externally-managed buffers as ArrayBuffer storage, and the header information will be stored separately. (And in that case, roc can probably do what he wants in a more straightforward way.)

In the meantime, it doesn't really break anything to hand out the data pointer. (You can't get the data pointer before stealing the contents, because the pointer may change from the un-inlining.) If we do the future split, then we might want to hand back the same pointer as both the contents and data pointers. The contents pointer is defined as the thing you can pass to free(), and what you can construct an ArrayBuffer from. So that would work, though we'd still probably need to change the API if we added in the external buffer feature (to give an indication of whether it really is valid to free() the contents.)

In summary, this API change exposes more stuff, but it doesn't seem to introduce any additional problems that we weren't already going to have. I think.

::: js/src/jsapi.h
@@ +3504,5 @@
>  /*
>   * A wrapper for js_free(p) that may delay js_free(p) invocation as a
>   * performance optimization.
> + * cx may be NULL.

I talked this over with luke. His recommendation: just use free() outside of Spidermonkey.
Attachment #676918 - Flags: review?(sphink) → superreview?(dmandelin)
Comment on attachment 676918 [details] [diff] [review]

Actually, let me mark this r+ contingent on the JS_free -> free change.
Attachment #676918 - Flags: review+
Comment on attachment 676918 [details] [diff] [review]

Review of attachment 676918 [details] [diff] [review]:

Weird indeed. I agree that it doesn't seem to actually hurt anything. It seems like it would be less gross to hand back some typed thing that you could use to query the data but I guess it doesn't matter too much.
Attachment #676918 - Flags: superreview?(dmandelin) → superreview+
Maybe we should take this on Aurora, so the data-less version of the API never reaches the wild?
Comment on attachment 676918 [details] [diff] [review]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> Maybe we should take this on Aurora, so the data-less version of the API
> never reaches the wild?

Sounds good to me.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 720949

User impact if declined: temporary API would appear for one release, then change, possibly resulting in backwards-compatibility problems

Testing completed (on m-c, etc.): landed on m-i at 2012-11-02 06:10:50 PDT 

Risk to taking this patch (and alternatives if risky): very low risk. This API is only used internally by the structured clone implementation, and this change adds a parameter that is ignored by structured clone.

String or UUID changes made by this patch: none
Attachment #676918 - Flags: approval-mozilla-aurora?
Comment on attachment 676918 [details] [diff] [review]

[Triage Comment]
Justification and risk sound good to us. Approving for Aurora 18.
Attachment #676918 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.