Closed Bug 965920 Opened 6 years ago Closed 6 years ago

Smart pointerize getters in XPConnect

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: mccr8, Assigned: mccr8)

Details

Attachments

(3 files)

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.
No hurry on any of these...
Attachment #8369444 - Flags: review?(bobbyholley)
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.
Attachment #8369442 - Flags: review?(bobbyholley) → review+
Attachment #8369443 - Flags: review?(bobbyholley) → review+
Attachment #8369444 - Flags: review?(bobbyholley) → review+
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.
Followup to fix some magical bustage that appeared recently.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c2bba06ed58
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.
You need to log in before you can comment on or make changes to this bug.