Closed Bug 861281 Opened 7 years ago Closed 7 years ago

GC: Root the wrap object API

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: jonco, Assigned: jonco)

References

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 1 obsolete file)

Attached patch Proposed changes (obsolete) — Splinter Review
Here's a patch to root the wrapping callbacks set by JS_SetWrapObjectCallbacks().

Terrence I'm running this past you first before asking for review from others.
Attachment #736883 - Flags: review?(terrence)
Comment on attachment 736883 [details] [diff] [review]
Proposed changes

I'd somehwat prefer we make ReparentWrapper (and RescueOrphans) take handles instead of using a RootedObject here.  Having the rooting at XPCWrappedNative::RescueOrphans (which has a marked object anyway) and nsNodeUtils::CloneAndAdopt (where it's getting the object out of something) would be a lot clearer.
Comment on attachment 736883 [details] [diff] [review]
Proposed changes

Review of attachment 736883 [details] [diff] [review]:
-----------------------------------------------------------------

Lets push the rooting of ReparentWrapper back one more level with this patch, as bz suggests. With that, this all looks excellent to me.
Attachment #736883 - Flags: review?(terrence) → review+
(In reply to Boris Zbarsky (:bz) from comment #1)

Thanks for the comments.

I'll make ReparentWrapper take a handle, and RescueOrphans has already been changed in the meantime.

Unfortunately we still have to have a root in ReparentWrapper because aObj is assigned to.
Attachment #736883 - Attachment is obsolete: true
Attachment #737438 - Flags: review?(bobbyholley+bmo)
I wonder how evil it would be to have ReparentWrapper take a mutable handle..

It probably doesn't matter much, though.  Might be worth a comment in ReparentWrapper about why we're rooting again.
Comment on attachment 737438 [details] [diff] [review]
Proposed changes v2

Review of attachment 737438 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/wrappers/WrapperFactory.cpp
@@ +130,2 @@
>  {
> +    RootedObject scope(cx, scopeArg);

Rather than doing the scopeArg dance here, let's just make something new to pass to PreCreate (since we need access to the original one anyway).
Attachment #737438 - Flags: review?(bobbyholley+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/3fa225a54135
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Blocks: 773686
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.