965920 - Smart pointerize getters in XPConnect

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Andrew McCreight [:mccr8]]

Instead of calling ADDREF, we can assign into a smart pointer, and return a forget.  This is kind of silly, but it makes the remaining more complex uses of ADDREF stand out more, so it is kind of a code hygiene issue.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Smart pointerize getters in XPConnect.

green L64 debug try run: https://tbpl.mozilla.org/?tree=Try&rev=c1238ed7dd0a

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Smart pointerize gSharedScriptableHelperForJSIID.

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: part 3 - Smart pointerize newVariant.

No hurry on any of these...

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8369442 [details] [diff] [review]
part 1 - Smart pointerize getters in XPConnect.

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

r=bholley with comments addressed.

::: js/xpconnect/src/XPCConvert.cpp
@@ +770,5 @@
>  CreateHolderIfNeeded(HandleObject obj, MutableHandleValue d,
>                       nsIXPConnectJSObjectHolder** dest)
>  {
>      if (dest) {
> +        nsRefPtr<XPCJSObjectHolder> objHolder = XPCJSObjectHolder::newHolder(obj);

This is infallible if obj is non-null, FWIW.

::: js/xpconnect/src/XPCWrappedJSClass.cpp
@@ +420,5 @@
>  /* readonly attribute nsIVariant value; */
>  NS_IMETHODIMP xpcProperty::GetValue(nsIVariant * *aValue)
>  {
> +    nsCOMPtr<nsIVariant> rval = mValue;
> +    mValue.forget(aValue);

I think you mean rval here. Presumably, we must not have great test coverage for this.

::: js/xpconnect/src/nsXPConnect.cpp
@@ +665,3 @@
>      }
>  
> +    nsRefPtr<nsIXPConnectWrappedNative> temp = XPCWrappedNative::Get(aJSObj);

If we're using an nsRefPtr, we should use the concrete type.

Comment 5

[Andrew McCreight [:mccr8]]

I filed bug 967562 on possibly getting rid of newHolder.

> I think you mean rval here. Presumably, we must not have great test coverage for this.

Oops, good catch!  Yeah that's the problem with changing this old code.

Comment 6

[Andrew McCreight [:mccr8]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/55dcd2a5362e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ca0b05e7155f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0f26c4b3eab7

Comment 7

[Andrew McCreight [:mccr8]]

Followup to fix some magical bustage that appeared recently.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c2bba06ed58

Comment 8

[Phil Ringnalda (:philor)]

Sorry, backed 'em all out in https://hg.mozilla.org/integration/mozilla-inbound/rev/930f794bfba3 for the https://tbpl.mozilla.org/php/getParsedLog.php?id=34108227&tree=Mozilla-Inbound bustage on the followup.

Comment 9

[Phil Ringnalda (:philor)]

Sorry again, you'll find that failure incomprehensible from any of your patches because it was from bustage that landed on your bustage busted on the same builds.

Comment 10

[Phil Ringnalda (:philor)]

Relanded in

https://hg.mozilla.org/integration/mozilla-inbound/rev/e998c14ccd10
https://hg.mozilla.org/integration/mozilla-inbound/rev/36f567b48ce2
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c9ccc6fd564
https://hg.mozilla.org/integration/mozilla-inbound/rev/1436b6638ff3

Watch out for mcMerge not commenting or closing the bug when this gets merged to m-c, not sure it copes with in-out-in in a single merge-wad.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e998c14ccd10
https://hg.mozilla.org/mozilla-central/rev/36f567b48ce2
https://hg.mozilla.org/mozilla-central/rev/1c9ccc6fd564
https://hg.mozilla.org/mozilla-central/rev/1436b6638ff3