Closed Bug 866762 Opened 7 years ago Closed 7 years ago

GC: A few more XPConnect rooting hazards

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(1 file)

Attached patch Proposed changesSplinter Review
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.
Attachment #743127 - Flags: review?(bobbyholley+bmo)
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.
Attachment #743127 - Flags: review?(bobbyholley+bmo) → review+
(In reply to Bobby Holley (:bholley) from comment #1)

Thanks, comments addressed.
> 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|?
(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.
https://hg.mozilla.org/mozilla-central/rev/2d3b020b23cd
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.