If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

GC: Rooting for XPCCallContext

RESOLVED FIXED in mozilla23

Status

()

Core
XPConnect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

Trunk
mozilla23
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Created attachment 739644 [details] [diff] [review]
Proposed changes

I've done a couple of things here:

1) Factored out the code to choose the JSContext to use into a separate method and used this as a default argument for the constructor.  This means that there's always a JSContext passed, and we don't have to call xpc_GetSafeJSContext() three times when initializing rooted fields.

2) Make the constructor take handles rather than object pointers so that they will be rooted over things in Init that can potentially cause GC.

3) Also a couple of misc rooting hazards in this class.

I think this will address all the hazards with this class.
Attachment #739644 - Flags: review?(bobbyholley+bmo)
(Assignee)

Updated

5 years ago
Blocks: 831379
Comment on attachment 739644 [details] [diff] [review]
Proposed changes

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

::: js/xpconnect/src/XPCCallContext.cpp
@@ +17,3 @@
>  
>  XPCCallContext::XPCCallContext(XPCContext::LangType callerLanguage,
> +                               JSContext* cx,

Please add /* = GetDefaultJSContext() */ here

@@ +38,1 @@
>  {

I'm concerned about hazards like the one you found in XPCWrappedJSClass, where an explicit cx might be passed, but it might be null. It's probably all the same anyway, but please MOZ_ASSERT(cx) in the constructors.

::: js/xpconnect/src/xpcprivate.h
@@ +1147,5 @@
> +                   JS::HandleObject funobj = JS::NullPtr(),
> +                   JS::HandleId id         = JS::JSID_VOIDHANDLE,
> +                   unsigned argc           = NO_ARGS,
> +                   jsval *argv             = nullptr,
> +                   jsval *rval             = nullptr);

Isn't this going to eventually need to be a MutableHandleValue? Or are we just relying on the fact that it's automatically rooted on the JS callstack?
Attachment #739644 - Flags: review?(bobbyholley+bmo) → review+
(Assignee)

Comment 2

5 years ago
(In reply to Bobby Holley (:bholley) from comment #1)
> Isn't this going to eventually need to be a MutableHandleValue? Or are we
> just relying on the fact that it's automatically rooted on the JS callstack?

It doesn't need to be a MutableHandleValue to make it safe as long as the pointer is actually to a rooted location, although making it a MutableHandleValue would enforce that.  At the moment I'm fixing the hazards reported by the analysis so we can get a working exactly rooted browser.  I don't know whether we are planning to remove all raw pointers in the future.
(Assignee)

Comment 3

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbf65101089e
https://hg.mozilla.org/mozilla-central/rev/cbf65101089e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.