refcounting - which types to use ?

[Enrico Weigelt, metux IT consult]

Hi folks,


I'm still a bit confused on which types (nsCOMPtr, ...) to use when
exactly, in combination w/ IDLs.

Let's consider a case where an nsArray is created and returned
(as rval, not out-parameter):

IDL:

[scriptable, uuid(ea0d8b3d-a549-4666-82d8-3a82cee2a3f1)]
interface nsIAddrBookService : nsISupports
{
nsIArray FindRecipients(in AString name);
...
}

C++:

/* nsIArray FindRecipients (in AString name); */
NS_IMETHODIMP nsAddrBookService::FindRecipients(const nsAString & addr,
nsIArray * *_retval)
{
nsresult rv = NS_OK;
nsCOMPtr<nsIMutableArray> list =
do_CreateInstance(NS_ARRAY_CONTRACTID, &rv);
NS_ENSURE_SUCCESS(rv, rv);
*_retval = list;
return FillRecipients(addr, list);
}

I'd assume that do_CreateInstance() returns the object w/ refcnt=1.
The assignment to nsCOMPtr should increase refcount. Correct ?
When the function is left, the nsCOMPtr is destroyed, and refcnt
goes back to 1 (we now have a reference left in the caller's pointer
field).

Now the caller side: (inspired by various examples ...)

nsCOMPtr<nsIArray> list;
rv = addrBookService->FindRecipients(
splittedRecipients[j].mEmail,
getter_AddRefs(list));

I'd guess getter_AddRefs() causes refcnt to be increased again, so
we're back at 2. That could lead to a leak, when that function returns
(and doesn't pass the ref anywhere else).

So, should I use dont_AddRef() here ?

Is the situation different when using out parameter instead of rval ?


BTW: what happens when passing nsCOMPtr as a reference (w/o IDL) ?

Assume the following code:

void caller() {
nsCOMPtr<nsIFoo> ref;

callee(ref);
ref->Something();
}

void callee(nsCOMPtr & outref) {
nsCOMPtr<nsIFoo> myref = do_CreateInstance(...);
...
outref = myref;
}

I'd assume that the last line will fill the caller's ref field w/ the
pointer and increase the object's refcnt (so it's 2 now), then when
callee is left, its myref is destroyed and refcnt is back to 1.

Is that correct ?


--mtx

[Shih-Chiang Chien]

You should use |forget| to transfer the ownership of the nsIArray from list
to _retval.

/* nsIArray FindRecipients (in AString name); */
NS_IMETHODIMP nsAddrBookService::FindRecipients(const nsAString & addr,
nsIArray * *_retval)
{
nsresult rv = NS_OK;
nsCOMPtr<nsIMutableArray> list =
do_CreateInstance(NS_ARRAY_CONTRACTID, &rv);
NS_ENSURE_SUCCESS(rv, rv);

if (NS_FAILED(rv = FillRecipients(addr, list)) {
return rv;
}

list.forget(_retval);
return NS_OK;

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>

[Enrico Weigelt, metux IT consult]

On 17.08.2017 04:03, Shih-Chiang Chien wrote:
> You should use |forget| to transfer the ownership of the nsIArray from
> list to _retval.

Ok, thanks. Already suspected that (found some similar things in the
code). Could we update the docs (maybe some set of examples) ?

IIRC, there're some places that do it like in my prev mail, which I took
as example (just forget where exactly that was) - should that be fixed ?

By the way: is there a difference between out parameters and rval ?
Or can I just assume, rval's are equivalent to an out parameter ?

OTOH, is there a way to create versions that really return it as rval,
so I conveniently could just call like that ?

nsCOMPtr<nsIMutableArray> list = addrbook->FindRecipients(addr);


--mtx

[Aryeh Gregor]

On Thu, Aug 17, 2017 at 7:07 AM, Enrico Weigelt, metux IT consult
<enrico....@gr13.net> wrote:
> OTOH, is there a way to create versions that really return it as rval,
> so I conveniently could just call like that ?
>
> nsCOMPtr<nsIMutableArray> list = addrbook->FindRecipients(addr);

You can do this with a return type of
already_AddRefed<nsIMutableArray>. Then instead of

list.forget(_retval);
return NS_OK;

you do just

return list.forget();

Note that in this case you cannot ignore the return value -- it must
be assigned to an nsCOMPtr or a RefPtr, or else it will leak. Ideally
we would allow a return type of nsCOMPtr<nsIMutableArray>&&, but
currently I think we don't.

[jyav...@mozilla.com]

On Thursday, August 17, 2017 at 1:02:56 PM UTC+2, Aryeh Gregor wrote:
> Note that in this case you cannot ignore the return value -- it must
> be assigned to an nsCOMPtr or a RefPtr, or else it will leak. Ideally
> we would allow a return type of nsCOMPtr<nsIMutableArray>&&, but
> currently I think we don't.

This is not entirely correct.

alreadyAddRefed<T> is marked MOZ_MUST_USE_TYPE.

If you do not use the return value, it will not compile.

[James Cheng]

How could I make it compile error?

I did write

RefPtr<Foo> xxx = yyy;
xxx.forget();

It is ok for compiling on my Mac.

The comment [1] said " Makes it a compile time error to not use the return
value of a function which has this type", but how do I make the clang
format take effect?

BTW, if you want to ignore the return value, you can use the special
version of Unused [2], but it will leak if you don't manually delete the
pointer.
[1]
http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/mfbt/Attributes.h#519-521
[2]
http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/mfbt/AlreadyAddRefed.h#112-117

[jyav...@mozilla.com]

On Thursday, August 24, 2017 at 11:56:53 AM UTC+2, James Cheng wrote:
> How could I make it compile error?
>
> I did write
>
> RefPtr<Foo> xxx = yyy;
> xxx.forget();

You need to run the static analyser.

The documentation about MOZ_MUST_USE_TYPE is explained there http://searchfox.org/mozilla-central/source/mfbt/Attributes.h#519-521.

This is why you should always do a try run, because the static analyser *will* choke on this code.

[Ehsan Akhgari]

You can also run the static analyzer locally by using a clang binary
downloaded from llvm.org, and adding the following line to your
mozconfig file:

ac_add_options --enable-clang-plugin

We will (hopefully sometime soon) make this extremely easy to do on all
platforms without needing to download a custom clang or change anything
in your mozconfig through a new mach command when
https://bugzilla.mozilla.org/show_bug.cgi?id=1328454 is finally fixed.

Cheers,
Ehsan

[Boris Zbarsky]

On 8/16/17 6:56 AM, Enrico Weigelt, metux IT consult wrote:
> I'd assume that do_CreateInstance() returns the object w/ refcnt=1.

Yes. It returns it as an already_AddRefed<> struct.

> The assignment to nsCOMPtr should increase refcount. Correct ?

No, because nsCOMPtr knows not to increase the refcount when an
already_AddRefed is assigned to it. If it _did_ increase it, that would
be a leak.

> When the function is left, the nsCOMPtr is destroyed, and refcnt
> goes back to 1 (we now have a reference left in the caller's pointer
> field).

The refcount goes to 0 and the object is destroyed. You want to either
list.forget(_retval) or NS_ADDREF(*_retval = list), with the former much
preferred.

> I'd guess getter_AddRefs() causes refcnt to be increased again

No. getter_AddRefs just nulls out the nsCOMPtr (causing it to release
whatever it's holding right now, if anything) so the direct assignment
into the nsCOMPtr's guts will not leak whatever used to be there. It
also makes it clear that there is a reference handoff here: after the
call, the nsCOMPtr is holding a ref, but that ref was originally
incremented inside the getter function being called.

> So, should I use dont_AddRef() here ?

No.

> Is the situation different when using out parameter instead of rval ?

No.

> BTW: what happens when passing nsCOMPtr as a reference (w/o IDL) ?

Nothing special. The code you suggest would work; it's just a bit hard
to reason about at the callsite.

> I'd assume that the last line will fill the caller's ref field w/ the
> pointer and increase the object's refcnt (so it's 2 now), then when
> callee is left, its myref is destroyed and refcnt is back to 1.
>
> Is that correct ?

Yes.

-Boris