869526 - GC: Fix more rooting hazards in xpconnect

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Jon Coppeard (:jonco)]

Attachment: Proposed changes

Here's a patch to fix some more rooting hazards in XPConnect.

A couple of them need a little explanation:

The changes to XPCVariant::VariantDataToJS() are to fix a false positive because the rooting analysis doesn't understand that the jsval in the union can never be a hazard in this case.  I don't think it's feasible to fix the analysis for this one case, so I changed the code to use a new union that doesn't contain a jsval.  You might object to the code churn however, it's kind of an unfortunate case.

I also changed nsXPConnect::GetCurrentJSContext() and nsXPConnect::GetSafeJSContext() to use GetRuntime() rather than XPCJSRuntime::Get() as the latter can in theory case a GC since it ends up lazily initializing XPConnect if necessary, even though this is probably not going to happen in practice.

Comment 1

[Bobby Holley (:bholley)]

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

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

I think I would prefer to scope each case: block and then just locally declare whichever temporary is needed in each instance. Unless I'm misreading, the only point of the variant here is effectively just to have a typed memory buffer to use as temporary storage for each of the conversion calls, right? If so, I think locals are the way to go.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: Patch v2

Here's an updated patch with the use of the union removed and local variables used instead.

Comment 3

[Bobby Holley (:bholley)]

Comment on attachment 746986 [details] [diff] [review]
Patch v2

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

r=bholley with comment addressed.

::: js/xpconnect/src/XPCVariant.cpp
@@ +468,4 @@
>              if (NS_FAILED(variant->GetAsID(&iid)))
>                  return false;
> +            nsID *v = &iid;
> +            return XPCConvert::NativeData2JS(lccx, pJSVal, (const void*)&v, TD_WCHAR, &iid, pErr);

This should be TD_PNSIID, no?

Comment 4

[Jon Coppeard (:jonco)]

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

Yes indeed, fixed.

Comment 5

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/83459f51f500

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/83459f51f500