Closed
Bug 929800
Opened 11 years ago
Closed 11 years ago
GC: Handlify js/public/StructuredClone.h
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla27
People
(Reporter: terrence, Assigned: terrence)
References
Details
Attachments
(2 files)
(deleted),
patch
|
sfink
:
review+
mccr8
:
feedback+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
mccr8
:
feedback+
|
Details | Diff | Splinter Review |
These are mostly already rooted (except for one hazard), so is just a matter of pushing the handles up through the interface.
Comment 1•11 years ago
|
||
What is the signature for JS_StructuredClone itself going to be?
Assignee | ||
Comment 2•11 years ago
|
||
Meant to post this last night, but there was some build bustage after the merge with Steve's recent changes to Serialize.
This is the SpiderMonkey half, including the new interface.
Attachment #821081 -
Flags: review?(sphink)
Attachment #821081 -
Flags: feedback?(continuation)
Assignee | ||
Comment 3•11 years ago
|
||
And the browser changes to use the handlified API.
Attachment #821086 -
Flags: review?(continuation)
Comment 4•11 years ago
|
||
Comment on attachment 821081 [details] [diff] [review]
structured_clone_rooting-js-v0.diff
Review of attachment 821081 [details] [diff] [review]:
-----------------------------------------------------------------
The JS API signature looks reasonable to me.
Attachment #821081 -
Flags: feedback?(continuation) → feedback+
Comment 5•11 years ago
|
||
Comment on attachment 821081 [details] [diff] [review]
structured_clone_rooting-js-v0.diff
Review of attachment 821081 [details] [diff] [review]:
-----------------------------------------------------------------
Ugh. Thanks for reminding me how much I dislike the current structured clone API.
::: js/src/builtin/TestingFunctions.cpp
@@ +1303,5 @@
> return false;
>
> RootedValue deserialized(cx);
> if (!JS_ReadStructuredClone(cx, obj->data(), obj->nbytes(),
> + JS_STRUCTURED_CLONE_VERSION, &deserialized, NULL, NULL)) {
These should be switched to nullptr. (I notice we currently have 77 NULLs and 4185 nullptrs in the tree.)
Attachment #821081 -
Flags: review?(sphink) → review+
Comment 6•11 years ago
|
||
Comment on attachment 821086 [details] [diff] [review]
structured_clone_rooting-browser-v0.diff
Review of attachment 821086 [details] [diff] [review]:
-----------------------------------------------------------------
This looks good to me, but you should get a DOM peer to review it.
Attachment #821086 -
Flags: review?(continuation)
Attachment #821086 -
Flags: review?(bugs)
Attachment #821086 -
Flags: feedback+
Comment 7•11 years ago
|
||
Comment on attachment 821086 [details] [diff] [review]
structured_clone_rooting-browser-v0.diff
Review of attachment 821086 [details] [diff] [review]:
-----------------------------------------------------------------
::: content/base/src/nsFrameMessageManager.cpp
@@ +416,5 @@
> const JS::Value& aJSON,
> JSAutoStructuredCloneBuffer& aBuffer,
> StructuredCloneClosure& aClosure)
> {
> + JS::Rooted<JS::Value> v(aCx, aJSON);
I'd push this root up to the callers.
Comment 8•11 years ago
|
||
Comment on attachment 821086 [details] [diff] [review]
structured_clone_rooting-browser-v0.diff
> GetParamsForMessage(JSContext* aCx,
> const JS::Value& aJSON,
> JSAutoStructuredCloneBuffer& aBuffer,
> StructuredCloneClosure& aClosure)
> {
>- if (WriteStructuredClone(aCx, aJSON, aBuffer, aClosure)) {
>+ JS::Rooted<JS::Value> v(aCx, aJSON);
>+ if (WriteStructuredClone(aCx, v, aBuffer, aClosure)) {
At some point we'll need to change GetParamsForMessage to take
Handle, but I guess that could happen when we either make
messagemanager to use webidl, or make idl to use Handles.
>- if (!buffer.write(aCx, aMessage, aTransfer, &kPostMessageCallbacks, &scInfo))
>+ JS::Rooted<JS::Value> message(aCx, aMessage);
>+ JS::Rooted<JS::Value> transfer(aCx, aTransfer);
>+ if (!buffer.write(aCx, message, transfer, &kPostMessageCallbacks, &scInfo))
Same thing here, except that caller Handlefying happens when window object is moved
to webidl I guess.
So, looks ok to me.
Attachment #821086 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 9•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f6219c6fb61
(In reply to :Ms2ger from comment #7)
> Comment on attachment 821086 [details] [diff] [review]
> structured_clone_rooting-browser-v0.diff
>
> Review of attachment 821086 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: content/base/src/nsFrameMessageManager.cpp
> @@ +416,5 @@
> > const JS::Value& aJSON,
> > JSAutoStructuredCloneBuffer& aBuffer,
> > StructuredCloneClosure& aClosure)
> > {
> > + JS::Rooted<JS::Value> v(aCx, aJSON);
>
> I'd push this root up to the callers.
It looks like there are some potential performance issues with doing that and I didn't want to get this bug side tracked in that discussion.
Perhaps there is a bug for webidling this code that we can link to so that this doesn't get dropped?
Comment 10•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in
before you can comment on or make changes to this bug.
Description
•