866762 - GC: A few more XPConnect rooting hazards

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Jon Coppeard (:jonco)]

Attachment: Proposed changes

Here's a patch for all but one of the (current) remaining XPConnect hazards.

The changes in XPCJSID.cpp are to stop to the analysis getting confused by the shadowing of obj, but hopefully also make it slightly more obvious to human readers what's going on.

Comment 1

[Bobby Holley (:bholley)]

Comment on attachment 743127 [details] [diff] [review]
Proposed changes

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

r=bholley with the fixes below.

::: js/xpconnect/src/XPCJSID.cpp
@@ +490,5 @@
>  
>  /* bool hasInstance (in nsIXPConnectWrappedNative wrapper, in JSContextPtr cx, in JSObjectPtr obj, in jsval val, out bool bp); */
>  NS_IMETHODIMP
>  nsJSIID::HasInstance(nsIXPConnectWrappedNative *wrapper,
> +                     JSContext * cx, JSObject * /* obj */,

This is _definitely_ not clearer IMO. Please call this |unused|.

@@ +846,5 @@
>  
>  /* bool hasInstance (in nsIXPConnectWrappedNative wrapper, in JSContextPtr cx, in JSObjectPtr obj, in jsval val, out bool bp); */
>  NS_IMETHODIMP
>  nsJSCID::HasInstance(nsIXPConnectWrappedNative *wrapper,
> +                     JSContext * cx, JSObject * /* obj */,

same.

::: js/xpconnect/src/XPCWrappedNative.cpp
@@ +3012,2 @@
>  {
> +    JSContext* cx = GetScope()->GetContext()->GetJSContext();

AutoJSContext for all of these, please.

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +1035,5 @@
>          }
>  
>          bool retval = true;
> +        RootedObject pobj(cx);
> +        nsIXPCScriptable* callback = wn->GetScriptableInfo()->GetCallback();

* on the right in XPConnect.

Comment 2

[Jon Coppeard (:jonco)]

(In reply to Bobby Holley (:bholley) from comment #1)

Thanks, comments addressed.

Comment 3

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d3b020b23cd

Comment 4

[Bobby Holley (:bholley)]

> JSContext * cx, JSObject * obj,
> JSContext * cx, JSObject * /* unused */,

Er, I meant that this pattern was confusing because of the * /* */. Can you please push a followup to remove the quoting around |unused|?

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Bobby Holley (:bholley) from comment #4)
> > JSContext * cx, JSObject * obj,
> > JSContext * cx, JSObject * /* unused */,
> 
> Er, I meant that this pattern was confusing because of the * /* */. Can you
> please push a followup to remove the quoting around |unused|?

Or just remove the comment entirely; C++ supports unnamed method parameters for just this case and I'm pretty sure we use this capability elsewhere in the tree.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2d3b020b23cd