Closed
Bug 807237
Opened 12 years ago
Closed 12 years ago
Add 'data' parameter to JS_StealArrayBufferContents
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: roc, Assigned: roc)
Details
Attachments
(1 file)
9.09 KB,
patch
|
sfink
:
review+
dmandelin
:
superreview+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
This allows the stealer to use the data while it's not attached to an ArrayBuffer.
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #676918 -
Flags: review?(sphink)
Comment 2•12 years ago
|
||
Comment on attachment 676918 [details] [diff] [review]
fix
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 3•12 years ago
|
||
Comment on attachment 676918 [details] [diff] [review]
fix
Actually, let me mark this r+ contingent on the JS_free -> free change.
Attachment #676918 -
Flags: review+
Comment 4•12 years ago
|
||
Comment on attachment 676918 [details] [diff] [review]
fix
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+
Assignee | ||
Comment 5•12 years ago
|
||
Assignee | ||
Comment 6•12 years ago
|
||
Maybe we should take this on Aurora, so the data-less version of the API never reaches the wild?
Comment 7•12 years ago
|
||
Comment on attachment 676918 [details] [diff] [review]
fix
(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 8•12 years ago
|
||
Comment on attachment 676918 [details] [diff] [review]
fix
[Triage Comment]
Justification and risk sound good to us. Approving for Aurora 18.
Attachment #676918 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 9•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Comment 10•12 years ago
|
||
status-firefox18:
--- → fixed
status-firefox19:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•