Getting rid of already_AddRefed?

[Benjamin Smedberg]

Just reading bug 1052477, and I'm wondering about what are intentions
are for already_AddRefed.

In that bug it's proposed to change the return type of NS_NewAtom from
already_AddRefed to nsCOMPtr. I don't think that actually saves any
addref/release pairs if done properly, since you'd typically .forget()
into the return value anyway. But it does make it slightly safer at
callsites, because the compiler will guarantee that the return value is
always released instead of us relying on every already_AddRefed being
saved into a nsCOMPtr.

But now that nsCOMPtr/nsRefPtr support proper move constructors, is
there any reason for already_AddRefed to exist at all in our codebase?
Could we replace every already_AddRefed return value with a nsCOMPtr?

--BDS

[L. David Baron]

On Tuesday 2014-08-12 10:59 -0400, Benjamin Smedberg wrote:
> Just reading bug 1052477, and I'm wondering about what are
> intentions are for already_AddRefed.
>
> In that bug it's proposed to change the return type of NS_NewAtom
> from already_AddRefed to nsCOMPtr. I don't think that actually saves
> any addref/release pairs if done properly, since you'd typically
> .forget() into the return value anyway. But it does make it slightly
> safer at callsites, because the compiler will guarantee that the
> return value is always released instead of us relying on every
> already_AddRefed being saved into a nsCOMPtr.

Bug 967364 did already add assertions to guarantee this.

> But now that nsCOMPtr/nsRefPtr support proper move constructors, is
> there any reason for already_AddRefed to exist at all in our
> codebase? Could we replace every already_AddRefed return value with
> a nsCOMPtr?

Two thoughts:

(1) I think it introduces a somewhat major coding style risk, in
that it makes it possible to accidentally assign the value of a
function that returns a reference into a raw pointer, which isn't
possible with already_AddRefed. In other words, today, you can
write:
T* t = GetT()
and know that as long as the code is following conventions
correctly and using already_AddRefed when a reference is returned,
this won't compile if an nsCOMPtr is required. If we change to
returning nsCOMPtr instead of returning already_AddRefed, this
won't be the case, and the code will end up having a dangling
pointer to a potentially-deleted object.

(2) If we agree it's a good idea from a coding style perspective,
it's probably worth having a look that the generated code is
equally efficient, given how widely used it is in our codebase.

Because of (1), I probably lean against this change (and against bug
1052477), unless I'm missing something that makes (1) not be true.

-David

--
𝄞 L. David Baron http://dbaron.org/ 𝄂
𝄢 Mozilla https://www.mozilla.org/ 𝄂
Before I built a wall I'd ask to know
What I was walling in or walling out,
And to whom I was like to give offense.
- Robert Frost, Mending Wall (1914)

[Aryeh Gregor]

On Tue, Aug 12, 2014 at 5:59 PM, Benjamin Smedberg
<benj...@smedbergs.us> wrote:
> Just reading bug 1052477, and I'm wondering about what are intentions are
> for already_AddRefed.
>
> In that bug it's proposed to change the return type of NS_NewAtom from
> already_AddRefed to nsCOMPtr. I don't think that actually saves any
> addref/release pairs if done properly, since you'd typically .forget() into
> the return value anyway. But it does make it slightly safer at callsites,
> because the compiler will guarantee that the return value is always released
> instead of us relying on every already_AddRefed being saved into a nsCOMPtr.

Correct, it doesn't save addref/release pairs. Some advantages of
converting already_AddRefed to just returning raw nsCOMPtr/nsRefPtr
are:

1) You can use the return value directly without assigning it to an
nsCOMPtr/nsRefPtr. For instance, pass it to a function that wants a
raw pointer, or compare it against a different value.

2) It's easier to use, as evidenced by the patches in bugs 1015114 and
1052477. No .forget() is needed, a raw pointer can be returned
directly, even things like do_QueryInterface can be returned directly
without a temporary nsCOMPtr (since the return type creates the
temporary for you). You can just return whatever you like and the
compiler will figure out if an addref is needed. There's a noticeable
decrease in LoC when converting to use it.

3) We can get rid of a class, which simplifies the codebase.

4) If the callee already holds a strong reference in a local variable,
it can just return that reference instead of a raw pointer. This
saves an addref/release if the caller puts the result in an nsCOMPtr.
Currently you could do this by returning an already_AddRefed, but
that's annoying because of (1) above, so people don't always do so.

5) Callees that don't care about the risk of an extra addref/release
can just return nsCOMPtr instead of raw pointers across the board.
This avoids a potentially sec-critical use-after-free vulnerability
that can occur if a function returns a raw pointer, then the caller
passes it to a function that takes a raw pointer, then the second
function does something to release it. This is very easy to do by
mistake, because the bug just looks like "SomeFunction(GetThing());".
Again, in theory you could already use already_AddRefed for this, but
this change makes it painless.

6) In theory, the compiler can use RVO to elide the construction of
the temporary altogether, although I'm not at all sure this is useful
in practice.

IMO, already_AddRefed is exactly the sort of use-case move
constructors are meant to solve, and it's no longer needed once we
have working move constructors for nsCOMPtr/nsRefPtr. I was planning
to write up an informational post once bug 1015114 landed explaining
the benefits and telling people how to convert everything.

> But now that nsCOMPtr/nsRefPtr support proper move constructors, is there
> any reason for already_AddRefed to exist at all in our codebase? Could we
> replace every already_AddRefed return value with a nsCOMPtr?

I've put some thought into this. Simple returns of already_AddRefed
can definitely be converted without much effort, and it reduces LoC
too. A bit more thought will be needed to get rid of the class
entirely, I think. For one thing, we'd need a replacement for
dont_AddRef(). Also, the move constructors themselves will need to be
rewritten, because they use .forget() (a few friend declarations
should do it).

[Benoit Jacob]

As far as I know, the only downside in replacing already_AddRefed by
nsCOMPtr would be to incur more useless calls to AddRef and Release. In the
case of "threadsafe" i.e. atomic refcounting, these use atomic
instructions, which might be expensive enough on certain ARM CPUs that this
might matter. So if you're interested, you could take a low-end ARM CPU
that we care about and see if replacing already_AddRefed by nsCOMPtr causes
any measurable performance regression.

Benoit

2014-08-12 10:59 GMT-04:00 Benjamin Smedberg <benj...@smedbergs.us>:

> Just reading bug 1052477, and I'm wondering about what are intentions are
> for already_AddRefed.
>
> In that bug it's proposed to change the return type of NS_NewAtom from
> already_AddRefed to nsCOMPtr. I don't think that actually saves any
> addref/release pairs if done properly, since you'd typically .forget() into
> the return value anyway. But it does make it slightly safer at callsites,
> because the compiler will guarantee that the return value is always
> released instead of us relying on every already_AddRefed being saved into a
> nsCOMPtr.
>

> But now that nsCOMPtr/nsRefPtr support proper move constructors, is there
> any reason for already_AddRefed to exist at all in our codebase? Could we
> replace every already_AddRefed return value with a nsCOMPtr?
>

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

[Aryeh Gregor]

On Tue, Aug 12, 2014 at 6:14 PM, L. David Baron <dba...@dbaron.org> wrote:
> Bug 967364 did already add assertions to guarantee this.

Yes, if I'm not mistaken, already_AddRefed these days is quite safe.

> Two thoughts:
>
> (1) I think it introduces a somewhat major coding style risk, in
> that it makes it possible to accidentally assign the value of a
> function that returns a reference into a raw pointer, which isn't
> possible with already_AddRefed. In other words, today, you can
> write:
> T* t = GetT()
> and know that as long as the code is following conventions
> correctly and using already_AddRefed when a reference is returned,
> this won't compile if an nsCOMPtr is required. If we change to
> returning nsCOMPtr instead of returning already_AddRefed, this
> won't be the case, and the code will end up having a dangling
> pointer to a potentially-deleted object.

For refcounted types, isn't a raw pointer in a local variable a red
flag to reviewers to begin with? If GetT() returns a raw pointer
today, like nsINode::GetFirstChild() or something, storing the result
in a raw pointer is a potential use-after-free, and that definitely
has happened already. Reviewers need to make sure that refcounted
types aren't ever kept in raw pointers in local variables, unless
perhaps it's very clear from the code that nothing can possibly call
Release() (although it still makes me nervous).

> (2) If we agree it's a good idea from a coding style perspective,
> it's probably worth having a look that the generated code is
> equally efficient, given how widely used it is in our codebase.

The implementation includes a simple test-case that verifies that
there are no unexpected addrefs when relying on move constructors.
Beyond that (actual assembly instructions) I don't know.

[Aryeh Gregor]

On Tue, Aug 12, 2014 at 6:16 PM, Benoit Jacob <jacob.b...@gmail.com> wrote:
> As far as I know, the only downside in replacing already_AddRefed by
> nsCOMPtr would be to incur more useless calls to AddRef and Release. In the
> case of "threadsafe" i.e. atomic refcounting, these use atomic instructions,
> which might be expensive enough on certain ARM CPUs that this might matter.
> So if you're interested, you could take a low-end ARM CPU that we care about
> and see if replacing already_AddRefed by nsCOMPtr causes any measurable
> performance regression.

Bug 1015114 removes those extra addrefs using C++11 move semantics, so
assuming that lands, it's not an issue. (IIRC, Boris has previously
said that excessive addref/release is a real performance problem and
needs to be avoided.)

[L. David Baron]

On Tuesday 2014-08-12 18:15 +0300, Aryeh Gregor wrote:
> 1) You can use the return value directly without assigning it to an
> nsCOMPtr/nsRefPtr. For instance, pass it to a function that wants a
> raw pointer, or compare it against a different value.

I consider this a disadvantage, as I pointed out in my previous
post. It removes one of the major safety features of nsCOMPtr.

> 4) If the callee already holds a strong reference in a local variable,
> it can just return that reference instead of a raw pointer. This
> saves an addref/release if the caller puts the result in an nsCOMPtr.
> Currently you could do this by returning an already_AddRefed, but
> that's annoying because of (1) above, so people don't always do so.

How does this save an addref/release? Is the compiler allowed to
use move constructors rather than copy constructors when
constructing the return value of a function from a local variable in
that function?

[L. David Baron]

On Tuesday 2014-08-12 18:22 +0300, Aryeh Gregor wrote:
> On Tue, Aug 12, 2014 at 6:14 PM, L. David Baron <dba...@dbaron.org> wrote:
> > Two thoughts:
> >
> > (1) I think it introduces a somewhat major coding style risk, in
> > that it makes it possible to accidentally assign the value of a
> > function that returns a reference into a raw pointer, which isn't
> > possible with already_AddRefed. In other words, today, you can
> > write:
> > T* t = GetT()
> > and know that as long as the code is following conventions
> > correctly and using already_AddRefed when a reference is returned,
> > this won't compile if an nsCOMPtr is required. If we change to
> > returning nsCOMPtr instead of returning already_AddRefed, this
> > won't be the case, and the code will end up having a dangling
> > pointer to a potentially-deleted object.
>
> For refcounted types, isn't a raw pointer in a local variable a red
> flag to reviewers to begin with? If GetT() returns a raw pointer
> today, like nsINode::GetFirstChild() or something, storing the result
> in a raw pointer is a potential use-after-free, and that definitely
> has happened already. Reviewers need to make sure that refcounted
> types aren't ever kept in raw pointers in local variables, unless
> perhaps it's very clear from the code that nothing can possibly call
> Release() (although it still makes me nervous).

This is done very commonly when we know some other data structure
(or the lifetime of |this|) owns the object, and will continue to
across the lifetime of the function.

For example, if I open content/base/src/Element.cpp, I see this
pattern 5 times in the first 325 lines of the file. (Three
nsIDocument*, one nsIContent*, one nsIPresShell*.)

[Aryeh Gregor]

On Tue, Aug 12, 2014 at 6:25 PM, L. David Baron <dba...@dbaron.org> wrote:
> On Tuesday 2014-08-12 18:15 +0300, Aryeh Gregor wrote:

>> 1) You can use the return value directly without assigning it to an>> 4) If the callee already holds a strong reference in a local variable,

>> it can just return that reference instead of a raw pointer. This
>> saves an addref/release if the caller puts the result in an nsCOMPtr.
>> Currently you could do this by returning an already_AddRefed, but
>> that's annoying because of (1) above, so people don't always do so.
>
> How does this save an addref/release? Is the compiler allowed to
> use move constructors rather than copy constructors when
> constructing the return value of a function from a local variable in
> that function?

The compiler is required to use the move constructor (if one exists)
instead of the copy constructor when constructing the return value of
a function, and also when initializing an object from the return value
of a function, or assigning the return value of a function. So if you
have

Foo GetFoo() { Foo f(1, 2, 7); /* do lots of stuff to f */ return f; }
void MyFunction() { Foo f = GetFoo(); }

the copy constructor of Foo will not get called anywhere. This is why
with the patch, you can just return an nsCOMPtr without the forget()
and it won't cause any extra addrefs.

On Tue, Aug 12, 2014 at 6:29 PM, L. David Baron <dba...@dbaron.org> wrote:
> This is done very commonly when we know some other data structure
> (or the lifetime of |this|) owns the object, and will continue to
> across the lifetime of the function.
>
> For example, if I open content/base/src/Element.cpp, I see this
> pattern 5 times in the first 325 lines of the file. (Three
> nsIDocument*, one nsIContent*, one nsIPresShell*.)

If you know something else is holding a strong reference, why is it a
problem to assign the result of a function that returns
already_AddRefed<T> to a local variable of type T*? If the local
variable's usage is otherwise legitimate, that assignment is perfectly
safe and there's no reason for any compiler errors.

[L. David Baron]

On Tuesday 2014-08-12 18:40 +0300, Aryeh Gregor wrote:
> On Tue, Aug 12, 2014 at 6:29 PM, L. David Baron <dba...@dbaron.org> wrote:
> > This is done very commonly when we know some other data structure
> > (or the lifetime of |this|) owns the object, and will continue to
> > across the lifetime of the function.
> >
> > For example, if I open content/base/src/Element.cpp, I see this
> > pattern 5 times in the first 325 lines of the file. (Three
> > nsIDocument*, one nsIContent*, one nsIPresShell*.)
>
> If you know something else is holding a strong reference, why is it a
> problem to assign the result of a function that returns
> already_AddRefed<T> to a local variable of type T*? If the local
> variable's usage is otherwise legitimate, that assignment is perfectly
> safe and there's no reason for any compiler errors.

In these cases we document that it's likely safe for callers to do
this by having those getters return raw pointers. Getters that
require reference-counting return already_AddRefed. Thus the
designer of the API makes a choice about whether the caller is
required to reference-count the result.

Our initial switch to use getters with raw pointers rather than
reference-counting getters (as part of deCOMtamination) was a
substantial performance win (probably at least 10% on general
across-the-board performance metrics).

On the other hand, getters that might return a new object must
return already_AddRefed<T> to ensure the caller takes ownership of
that reference.

[Vladimir Vukicevic]

On Tuesday, August 12, 2014 11:22:05 AM UTC-4, Aryeh Gregor wrote:
> For refcounted types, isn't a raw pointer in a local variable a red
> flag to reviewers to begin with? If GetT() returns a raw pointer
> today, like nsINode::GetFirstChild() or something, storing the result
> in a raw pointer is a potential use-after-free, and that definitely
> has happened already. Reviewers need to make sure that refcounted
> types aren't ever kept in raw pointers in local variables, unless
> perhaps it's very clear from the code that nothing can possibly call
> Release() (although it still makes me nervous).

Putting the burden on reviewers when something can be automatically checked doesn't seem like a good idea -- it requires reviewers to know that GetT() *does* return a refcounted type, for example. As dbaron pointed out, there are cases where we do actually return and keep things around as bare pointers.

It's unfortunate that we can't create a nsCOMPtr<> that will disallow assignment to a bare pointer without an explicit .get(), but will still allow conversion to a bare pointer for arg passing purposes. (Or can we? I admit my C++-fu is not that strong in this area...) It would definitely be nice to get rid of already_AddRefed<> (not least because the spelling of "Refed" always grates when I see it :).

- Vlad

[Joshua Cranmer 🐧]

The rationale for why we still had it was that:
nsIFoo *foobar = ReturnsACOMPtr();

silently leaks. I've pointed out before that we could fix this by adding
a nsCOMPtr<T>::operator T*() && = delete; operator, but that's a gcc
4.8.1/msvc 2013 November CTP minimum requirement.

--
Joshua Cranmer
Thunderbird and DXR developer
Source code archæologist

[Aryeh Gregor]

On Tue, Aug 12, 2014 at 6:46 PM, L. David Baron <dba...@dbaron.org> wrote:
> In these cases we document that it's likely safe for callers to do
> this by having those getters return raw pointers. Getters that
> require reference-counting return already_AddRefed. Thus the
> designer of the API makes a choice about whether the caller is
> required to reference-count the result.

How is this code safe?

nsIContent* child = node->GetFirstChild();
// Do some stuff with child

It compiles fine, but if any subsequent code causes the child to be
removed from its parent, it could get freed. In particular, this can
happen if anything indirectly triggers mutation observers, and I
distinctly remember a sec-critical bug caused by exactly that.
Reviewers already have to carefully scrutinize any use of such a local
variable. Does returning already_AddRefed really make such a
difference to the degree of review required?

Put it this way -- if we had rvalue references available when
already_AddRefed was invented, would anyone have suggested making a
whole new class that's significantly more cumbersome to use just to
avoid making an inherently dangerous usage pattern (raw pointers to
refcounted variables) a little less dangerous? Or is this just status
quo bias?

[Benjamin Smedberg]

On 8/12/2014 12:28 PM, Joshua Cranmer 🐧 wrote:
>
>>
>> But now that nsCOMPtr/nsRefPtr support proper move constructors, is
>> there any reason for already_AddRefed to exist at all in our
>> codebase? Could we replace every already_AddRefed return value with a
>> nsCOMPtr?
>
> The rationale for why we still had it was that:
> nsIFoo *foobar = ReturnsACOMPtr();
>
> silently leaks.

Really? I thought that in this case there would be no leak because the
(temporary-returned) nsCOMPtr destructor would properly free the object.
The problem is that `foobar` potentially points to an object which has
already been released.

--BDS

[Kyle Huey]

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

If that function returns already_AddRefed it forces the caller to root
the return value which means you do not have to review as closely for
use-after-free issues.

We use raw pointers where we can because reference counting can be
expensive, especially in tight loops. smaug can share some history
here.

- Kyle

[Aryeh Gregor]

On Tue, Aug 12, 2014 at 7:12 PM, Vladimir Vukicevic <vlad...@pobox.com> wrote:
> It's unfortunate that we can't create a nsCOMPtr<> that will disallow assignment to a bare pointer without an explicit .get(), but will still allow conversion to a bare pointer for arg passing purposes. (Or can we? I admit my C++-fu is not that strong in this area...)

We could do this if we instituted a new type and mandated that type be
used instead of raw pointers for local variables. Call it
RawPointer<T> or something. Using raw pointers like this is still
inherently dangerous for nontrivial code -- including code that's
subsequently changed to be nontrivial -- but if this bit of extra
safety is important, it would be possible to get it this way.

Of course, then review would have to check that RawPointer<T> is used
instead of T*, unless we can get static analysis to do it.

[Aryeh Gregor]

On Tue, Aug 12, 2014 at 7:37 PM, Benjamin Smedberg
<benj...@smedbergs.us> wrote:
>
> On 8/12/2014 12:28 PM, Joshua Cranmer 🐧 wrote:

>> The rationale for why we still had it was that:
>> nsIFoo *foobar = ReturnsACOMPtr();
>>
>> silently leaks.
>

> Really? I thought that in this case there would be no leak because the
> (temporary-returned) nsCOMPtr destructor would properly free the object. The
> problem is that `foobar` potentially points to an object which has already
> been released.

Correct. I assume that's what he meant.

[Joshua Cranmer 🐧]

On 8/12/2014 11:12 AM, Vladimir Vukicevic wrote:
> It's unfortunate that we can't create a nsCOMPtr<> that will disallow assignment to a bare pointer without an explicit .get(), but will still allow conversion to a bare pointer for arg passing purposes. (Or can we? I admit my C++-fu is not that strong in this area...) It would definitely be nice to get rid of already_AddRefed<> (not least because the spelling of "Refed" always grates when I see it :).

The use of a method like

operator T*() && = delete;

causes the conversion to fail if the nsCOMPtr is an rvalue (most
temporaries). It still allows T *foo = localVariable; (there's no easy
way around that).

[Joshua Cranmer 🐧]

Er, yes. I remembered there was a problem, I forgot the actual problem. :-[

[Ehsan Akhgari]

On 2014-08-12, 12:41 PM, Joshua Cranmer 🐧 wrote:
> On 8/12/2014 11:12 AM, Vladimir Vukicevic wrote:
>> It's unfortunate that we can't create a nsCOMPtr<> that will disallow
>> assignment to a bare pointer without an explicit .get(), but will
>> still allow conversion to a bare pointer for arg passing purposes.
>> (Or can we? I admit my C++-fu is not that strong in this area...) It
>> would definitely be nice to get rid of already_AddRefed<> (not least
>> because the spelling of "Refed" always grates when I see it :).
>
> The use of a method like
> operator T*() && = delete;
>
> causes the conversion to fail if the nsCOMPtr is an rvalue (most
> temporaries). It still allows T *foo = localVariable; (there's no easy
> way around that).

It could also be solved with making operator T*() explicit, but neither
of these options are something that we can use in our code base today.

[Ehsan Akhgari]

On 2014-08-12, 11:25 AM, L. David Baron wrote:
> On Tuesday 2014-08-12 18:15 +0300, Aryeh Gregor wrote:
>> 1) You can use the return value directly without assigning it to an

>> nsCOMPtr/nsRefPtr. For instance, pass it to a function that wants a
>> raw pointer, or compare it against a different value.
>
> I consider this a disadvantage, as I pointed out in my previous
> post. It removes one of the major safety features of nsCOMPtr.

I am also strongly against the removal of already_AddRefed because of
this. I am not convinced that we can rely on reviewers to enforce this.

Cheers,
Ehsan

[Neil]

Aryeh Gregor wrote:

>2) It's easier to use, as evidenced by the patches in bugs 1015114 and 1052477. No .forget() is needed, a raw pointer can be returned directly, even things like do_QueryInterface can be returned directly without a temporary nsCOMPtr (since the return type creates the temporary for you). You can just return whatever you like and the compiler will figure out if an addref is needed. There's a noticeable decrease in LoC when converting to use it.
>

Do you have to name the temporary nsCOMPtr? For example, in the code
nsCOMPtr<nsIAtom> atom = GetAtomValue()
return atom.forget();
Can this be rewritten to
return nsCOMPtr<nsIAtom>(GetAtomValue()).forget();
Or better still, if this counts as an rvalue, can we define a move
constructor from nsCOMPtr<T>&& and write
return nsCOMPtr<nsIAtom>(GetAtomValue());

--
Warning: May contain traces of nuts.

[Robert O'Callahan]

On Wed, Aug 13, 2014 at 4:35 AM, Aryeh Gregor <a...@aryeh.name> wrote:

> On Tue, Aug 12, 2014 at 6:46 PM, L. David Baron <dba...@dbaron.org> wrote:
> > In these cases we document that it's likely safe for callers to do
> > this by having those getters return raw pointers. Getters that
> > require reference-counting return already_AddRefed. Thus the
> > designer of the API makes a choice about whether the caller is
> > required to reference-count the result.
>
> How is this code safe?
>
> nsIContent* child = node->GetFirstChild();
> // Do some stuff with child
>
> It compiles fine, but if any subsequent code causes the child to be
> removed from its parent, it could get freed. In particular, this can
> happen if anything indirectly triggers mutation observers, and I
> distinctly remember a sec-critical bug caused by exactly that.
>

There are huge swathes of code where we know DOM mutation should not
happen. Reflow and painting for example.

Rob
--
oIo otoeololo oyooouo otohoaoto oaonoyooonoeo owohooo oioso oaonogoroyo
owoiotoho oao oboroootohoeoro oooro osoiosotoeoro owoiololo oboeo
osouobojoeocoto otooo ojouodogomoeonoto.o oAogoaoiono,o oaonoyooonoeo
owohooo
osoaoyoso otooo oao oboroootohoeoro oooro osoiosotoeoro,o o‘oRoaocoao,o’o
oioso
oaonosowoeoroaoboloeo otooo otohoeo ocooouoroto.o oAonodo oaonoyooonoeo
owohooo
osoaoyoso,o o‘oYooouo ofooooolo!o’o owoiololo oboeo oiono odoaonogoeoro
ooofo
otohoeo ofoioroeo ooofo ohoeololo.

[Karl Tomlinson]

Aryeh Gregor writes:

> The compiler is required to use the move constructor (if one exists)
> instead of the copy constructor when constructing the return value of
> a function, and also when initializing an object from the return value
> of a function, or assigning the return value of a function. So if you
> have
>
> Foo GetFoo() { Foo f(1, 2, 7); /* do lots of stuff to f */ return f; }
> void MyFunction() { Foo f = GetFoo(); }
>
> the copy constructor of Foo will not get called anywhere.

I guess that means that with

struct Bar {
Bar(Foo f) : mF(f) {}
Foo GetFoo() { return mF; }

Foo mF;
}

GetFoo() would give away what mF owns?

If so, can we require that be more explicit somehow?

[Jeff Walden]

On 08/12/2014 10:06 AM, Ehsan Akhgari wrote:
> It could also be solved with making operator T*() explicit, but neither of these options are something that we can use in our code base today.

So at risk of adding yet another flavor of thing: why not introduce an already_AddRefed<T> sort of struct that *does* own an addref, *will* release on destruction if not nulled out, and does *not* explicitly convert to T*? Give nsCOMPtr move constructors and move assignment operators from that class, that null out this other pointer when called, to transfer ownership at no cost. (At risk of overplaying my hand, I think I've just described what WebBlink calls PassRefPtr, except for that pertaining to concrete classes whereas nsCOMPtr<T> is for abstract T classes.)

I recognize this introduces yet another way to do things. But it enables incremental transition to a completely safe system that prohibits assignment of a method call to a raw pointer, and yet never has problems with leaks. If we work hard at enforcing this on the way in, it can quickly become second nature, I think.

Jeff

[Neil]

No, Aryeh's comment only applies to local variables.

Foo GetFoo()
{
Foo f1, f2;
if (rand() % 2)
return f2;
return f1;
}

Here C++03 compilers may find it hard to do NRVO because we're returning
different locals. But when a C++11 compiler tries and fails to do NRVO,
it then tries the move constructor.

Note that using ? : in this case does not work, since that results in a
Foo& which therefore gets copy constructed. See
http://stackoverflow.com/a/19698477

[Aryeh Gregor]

On Wed, Aug 13, 2014 at 9:42 AM, Jeff Walden <jwald...@mit.edu> wrote:
> So at risk of adding yet another flavor of thing: why not introduce an already_AddRefed<T> sort of struct that *does* own an addref, *will* release on destruction if not nulled out, and does *not* explicitly convert to T*? Give nsCOMPtr move constructors and move assignment operators from that class, that null out this other pointer when called, to transfer ownership at no cost. (At risk of overplaying my hand, I think I've just described what WebBlink calls PassRefPtr, except for that pertaining to concrete classes whereas nsCOMPtr<T> is for abstract T classes.)
>
> I recognize this introduces yet another way to do things. But it enables incremental transition to a completely safe system that prohibits assignment of a method call to a raw pointer, and yet never has problems with leaks. If we work hard at enforcing this on the way in, it can quickly become second nature, I think.

I think this can be improved upon a bit further: just change
already_AddRefed to behave more similarly to nsCOMPtr, but still not
convert to T* implicitly. So for instance:

* Change ~already_AddRefed to just release the pointer instead of
asserting. This means it would be perfectly okay to ignore the result
of a function that returns already_AddRefed.

* Make an already_AddRefed constructor from T* (which would addref).
This allows you to return a raw pointer from a function with
already_AddRefed return type, and it will do the right thing without
you having to define an nsCOMPtr temporary.

* Make an already_AddRefed constructor from nsCOMPtr/nsRefPtr (which
would addref if the source is an lvalue, and steal the value if the
source is an rvalue). This allows you to return a local nsCOMPtr or
nsRefPtr directly, without having to call .forget(). If we want to be
extra careful to avoid accidental addrefs, we could allow construction
only from rvalues, but I'm not sure this is a good idea.

* Allow conversion of already_AddRefed to bool, and comparison to T*.

This has most of the benefits of my original proposal, without the
drawback David and Ehsan object to. It has the advantage of not
having to change the return type of a few thousand functions. It has
the disadvantage that you could still not pass an already_AddRefed
directly to a function that wants T*, which is unfortunate because
it's perfectly safe to do so. I can't think of any way to avoid that
except defining new types for refcounted parameters to use instead of
raw pointers (which would have other uses as well).

David, Ehsan, what do you think?

[smaug]

AddRef/Release calls are performance issue everywhere, in hot code paths, whether or not
there is threadsafety involved.
(excluding inline non-virtual AddRef/Release)

-Olli

[Aryeh Gregor]

On Wed, Aug 13, 2014 at 3:14 PM, Aryeh Gregor <a...@aryeh.name> wrote:
> It has
> the disadvantage that you could still not pass an already_AddRefed
> directly to a function that wants T*, which is unfortunate because
> it's perfectly safe to do so. I can't think of any way to avoid that
> except defining new types for refcounted parameters to use instead of
> raw pointers (which would have other uses as well).

Actually, once already_AddRefed releases its pointer on destruction,
we could just add a get() method and use that when we do want to
convert to T* (like when passing to a function). Certainly much
shorter than explicitly constructing an nsCOMPtr<T>.

[Ehsan Akhgari]

On 2014-08-13, 8:14 AM, Aryeh Gregor wrote:
> On Wed, Aug 13, 2014 at 9:42 AM, Jeff Walden <jwald...@mit.edu> wrote:

>> So at risk of adding yet another flavor of thing: why not introduce an already_AddRefed<T> sort of struct that *does* own an addref, *will* release on destruction if not nulled out, and does *not* explicitly convert to T*? Give nsCOMPtr move constructors and move assignment operators from that class, that null out this other pointer when called, to transfer ownership at no cost. (At risk of overplaying my hand, I think I've just described what WebBlink calls PassRefPtr, except for that pertaining to concrete classes whereas nsCOMPtr<T> is for abstract T classes.)
>>
>> I recognize this introduces yet another way to do things. But it enables incremental transition to a completely safe system that prohibits assignment of a method call to a raw pointer, and yet never has problems with leaks. If we work hard at enforcing this on the way in, it can quickly become second nature, I think.
>

> I think this can be improved upon a bit further: just change
> already_AddRefed to behave more similarly to nsCOMPtr, but still not
> convert to T* implicitly. So for instance:
>
> * Change ~already_AddRefed to just release the pointer instead of
> asserting. This means it would be perfectly okay to ignore the result

> of a function that returns already_AddRefed.

What goal would this achieve? I don't understand why it's OK to ignore
the return value of a function which returns an "already AddRef'ed"
object from a conceptual perspective.

> * Make an already_AddRefed constructor from T* (which would addref).
> This allows you to return a raw pointer from a function with
> already_AddRefed return type, and it will do the right thing without
> you having to define an nsCOMPtr temporary.

already_AddRefed already has a constructor from T* which would not
addref. Essentially this will mean not supporting dont_AddRef() any
more. Why is that a good idea?

> * Make an already_AddRefed constructor from nsCOMPtr/nsRefPtr (which
> would addref if the source is an lvalue, and steal the value if the
> source is an rvalue). This allows you to return a local nsCOMPtr or
> nsRefPtr directly, without having to call .forget(). If we want to be
> extra careful to avoid accidental addrefs, we could allow construction
> only from rvalues, but I'm not sure this is a good idea.

Hmm, so this would allow you to save typing ".forget()" when returning a
COM/ref ptr? I actually think that the .forget() there is super helpful
because it makes it clear that there is an ownership transfer going on.
It's more cumbersome to type, but code is read more often than it's
written and all. :-)

> * Allow conversion of already_AddRefed to bool, and comparison to T*.

This I can get behind, but I'm not sure how useful it is without the
other parts of your proposal.

> This has most of the benefits of my original proposal, without the
> drawback David and Ehsan object to. It has the advantage of not

> having to change the return type of a few thousand functions. It has

> the disadvantage that you could still not pass an already_AddRefed
> directly to a function that wants T*, which is unfortunate because
> it's perfectly safe to do so. I can't think of any way to avoid that
> except defining new types for refcounted parameters to use instead of
> raw pointers (which would have other uses as well).
>

> David, Ehsan, what do you think?

Honestly I'm struggling to decide which problems we _should_ solve here.
I almost always prefer more explicit syntax (for example, having to
type .forget()) when it comes to things such as ownership transfers. It
makes it easier to spot things when reading the code, and it makes the
author more conscious of what they're doing. I also think that you're
essentially trying to eliminate the concept of a helper object that acts
as a pipe of sorts to deliver the information that there is an ownership
transfer happening by making already_AddRefed and nsCOM/RefPtr
indistinguishable. Neither of these seem like improvements to me. I'm
curious to know what others think about this.

Cheers,
Ehsan

[Aryeh Gregor]

On Wed, Aug 13, 2014 at 4:11 PM, Ehsan Akhgari <ehsan....@gmail.com> wrote:
> What goal would this achieve? I don't understand why it's OK to ignore the
> return value of a function which returns an "already AddRef'ed" object from
> a conceptual perspective.

You might want the effect of the function but not care about the
object it returns. Or, you might just want to know that the object is
not null, or compare to some other object, or pass it to a function,
or something else that doesn't involve keeping a named reference to
it. Currently you have to explicitly construct a temporary nsCOMPtr
(named or unnamed) in all of these cases, but I don't see why you
should have to.

> already_AddRefed already has a constructor from T* which would not addref.
> Essentially this will mean not supporting dont_AddRef() any more. Why is
> that a good idea?

It would allow returning a raw pointer directly without having to
create an nsCOMPtr. This saves a line of code in a lot of places.

> This I can get behind, but I'm not sure how useful it is without the other
> parts of your proposal.

It's not useful at all if already_AddRefed asserts in its destructor
if the pointer it holds is not null. It would still be useful to have
that part of the proposal and this, without adding any extra implicit
conversions.

> Honestly I'm struggling to decide which problems we _should_ solve here. I
> almost always prefer more explicit syntax (for example, having to type
> .forget()) when it comes to things such as ownership transfers. It makes it
> easier to spot things when reading the code, and it makes the author more
> conscious of what they're doing. I also think that you're essentially
> trying to eliminate the concept of a helper object that acts as a pipe of
> sorts to deliver the information that there is an ownership transfer
> happening by making already_AddRefed and nsCOM/RefPtr indistinguishable.
> Neither of these seem like improvements to me. I'm curious to know what
> others think about this.

Hmm, okay. I was coming from the perspective that it's better to make
things happen automatically without the coder having to think about
ownership. But if people think explicit statements of ownership
transferal are preferable to automatic ones, this sort of change
doesn't improve matters.

[Ehsan Akhgari]

On Wed, Aug 13, 2014 at 9:32 AM, Aryeh Gregor <a...@aryeh.name> wrote:

> On Wed, Aug 13, 2014 at 4:11 PM, Ehsan Akhgari <ehsan....@gmail.com>
> wrote:
> > What goal would this achieve? I don't understand why it's OK to ignore
> the
> > return value of a function which returns an "already AddRef'ed" object
> from
> > a conceptual perspective.
>
> You might want the effect of the function but not care about the
> object it returns. Or, you might just want to know that the object is
> not null, or compare to some other object, or pass it to a function,
> or something else that doesn't involve keeping a named reference to
> it. Currently you have to explicitly construct a temporary nsCOMPtr
> (named or unnamed) in all of these cases, but I don't see why you
> should have to.
>

Can't you do the following instead?

unused << MyFunction(); // I know that I'm leaking this ref, but it's ok
somehow

> > already_AddRefed already has a constructor from T* which would not
> addref.
> > Essentially this will mean not supporting dont_AddRef() any more. Why is
> > that a good idea?
>
> It would allow returning a raw pointer directly without having to
> create an nsCOMPtr. This saves a line of code in a lot of places.
>

I don't understand this. You are already able to return raw pointers from
functions. Returning an already_AddRefed is however a very different use
case, it means, I'm giving up ownership to the object I'm returning.

> > This I can get behind, but I'm not sure how useful it is without the
> other
> > parts of your proposal.
>
> It's not useful at all if already_AddRefed asserts in its destructor
> if the pointer it holds is not null. It would still be useful to have
> that part of the proposal and this, without adding any extra implicit
> conversions.
>
> > Honestly I'm struggling to decide which problems we _should_ solve here.
> I
> > almost always prefer more explicit syntax (for example, having to type
> > .forget()) when it comes to things such as ownership transfers. It
> makes it
> > easier to spot things when reading the code, and it makes the author more
> > conscious of what they're doing. I also think that you're essentially
> > trying to eliminate the concept of a helper object that acts as a pipe of
> > sorts to deliver the information that there is an ownership transfer
> > happening by making already_AddRefed and nsCOM/RefPtr indistinguishable.
> > Neither of these seem like improvements to me. I'm curious to know what
> > others think about this.
>
> Hmm, okay. I was coming from the perspective that it's better to make
> things happen automatically without the coder having to think about
> ownership. But if people think explicit statements of ownership
> transferal are preferable to automatic ones, this sort of change
> doesn't improve matters.
>

If that is the goal, then I don't agree that is a useful outcome at all. I
*do* wish that there were better *and* safer ways of doing more things
automatically but ownership transfers are inherently unsafe operations that
are expressed using various different kinds of conventions in C++
(already_AddRefed being one such convention.) In my experience, when
people are not aware of how they're transferring ownership, bugs creep in.

Cheers,
Ehsan

--
Ehsan

[Aryeh Gregor]

On Wed, Aug 13, 2014 at 5:44 PM, Ehsan Akhgari <ehsan....@gmail.com> wrote:
> Can't you do the following instead?
>
> unused << MyFunction(); // I know that I'm leaking this ref, but it's ok
> somehow

No, because the use-case is where you don't want to leak the ref --
you want it to be released automatically for you. So for instance,
here's a real-world bit of code from nsWSRunObject:

if ((aRun->mRightType & WSType::block) &&
IsBlockNode(nsCOMPtr<nsINode>(GetWSBoundingParent()))) {

GetWSBoundingParent() returns an already_AddRefed<nsINode>, but all we
want is to check if it's a block node and then throw it away (without
leaking it). We don't want to save it beyond that. With the proposed
change, this would be (assuming we re-added a .get() method, which
would now be as safe as on nsCOMPtr)

if ((aRun->mRightType & WSType::block) &&
IsBlockNode(GetWSBoundingParent().get())) {

which means the caller didn't take ownership, just used it and let the
destructor destroy it.

Similarly, if I would like to pass a string I have to a function that
wants an atom, I would like to be able to do
f(do_getAtom(string).get()) instead of
f(nsCOMPtr<nsIAtom>(do_getAtom(string))).

There are also functions in editor that do something like, say, insert
a new <br>, and return the element they inserted. I might not want to
get a reference to the element, just want it created. Currently I
have to make an nsCOMPtr to store the result and then ignore it. I
don't see the value in requiring this.

> I don't understand this. You are already able to return raw pointers from
> functions. Returning an already_AddRefed is however a very different use
> case, it means, I'm giving up ownership to the object I'm returning.

Yes, but what if the caller wants to use the object once (or not at
all) and then have it released immediately? Is there value in
requiring explicit creation of an nsCOMPtr in that case?

[Jonas Sicking]

On Wed, Aug 13, 2014 at 5:14 AM, Aryeh Gregor <a...@aryeh.name> wrote:
> I think this can be improved upon a bit further: just change
> already_AddRefed to behave more similarly to nsCOMPtr, but still not
> convert to T* implicitly. So for instance:
>
> * Change ~already_AddRefed to just release the pointer instead of
> asserting. This means it would be perfectly okay to ignore the result
> of a function that returns already_AddRefed.

What would be the goal of this? The goal of the initial email in this
thread appeared to be to reduce the number of classes in Gecko. This
doesn't seem to accomplish that.

The downside of doing this would be that it would lead to more overlap
between nsCOMPtr and already_AddRefed, which would lead to more
confusion about when to use which.

> * Make an already_AddRefed constructor from T* (which would addref).
> This allows you to return a raw pointer from a function with
> already_AddRefed return type, and it will do the right thing without
> you having to define an nsCOMPtr temporary.

A long long time ago John Keiser and I proposed adding a
|already_AddRefed<T*> do_AddRef(T*)| which would addref. It was shot
down by scc on basis that I never really understood.

I still think it would be a useful thing to have and fits in our
current architecture well.

/ Jonas

[Jonas Sicking]

On Wed, Aug 13, 2014 at 9:24 AM, Aryeh Gregor <a...@aryeh.name> wrote:
>
> No, because the use-case is where you don't want to leak the ref --
> you want it to be released automatically for you. So for instance,
> here's a real-world bit of code from nsWSRunObject:
>
> if ((aRun->mRightType & WSType::block) &&
> IsBlockNode(nsCOMPtr<nsINode>(GetWSBoundingParent()))) {

I don't think this is a common problem outside of editor. Editor code
was written using a lot of weird conventions and it doesn't surprise
me that it leads to code patterns like this one. But it's not
something I've seen be a problem elsewhere.

/ Jonas

[Jeff Walden]

On 08/13/2014 07:44 AM, Ehsan Akhgari wrote:
> If that is the goal, then I don't agree that is a useful outcome at all. I
> *do* wish that there were better *and* safer ways of doing more things
> automatically but ownership transfers are inherently unsafe operations that
> are expressed using various different kinds of conventions in C++
> (already_AddRefed being one such convention.) In my experience, when
> people are not aware of how they're transferring ownership, bugs creep in.

The very point of move semantics is that they're built into the fabric of the language, and you can't evade them except with casts (including through explicit use of Move(), which is sufficiently explicit as to even be acceptable). Having ownership transfers take advantage of this makes them inherently safer. If a mistake is made (by ignoring a return value or similar), the consequence is usually an extra addref/release, not a leak.

I lack sufficient knowledge/experience with Rust to speak too forcefully about it. But it's worth emphasizing that, by my understanding, every value in Rust is passed using move semantics. (And you need something like reference counting tacked on to evade that.) That speaks volumes for the safety of move semantics, in my mind.

Jeff

[Ehsan Akhgari]

On 2014-08-13, 12:24 PM, Aryeh Gregor wrote:
> On Wed, Aug 13, 2014 at 5:44 PM, Ehsan Akhgari <ehsan....@gmail.com> wrote:
>> Can't you do the following instead?
>>
>> unused << MyFunction(); // I know that I'm leaking this ref, but it's ok
>> somehow
>

> No, because the use-case is where you don't want to leak the ref --
> you want it to be released automatically for you. So for instance,
> here's a real-world bit of code from nsWSRunObject:
>
> if ((aRun->mRightType & WSType::block) &&
> IsBlockNode(nsCOMPtr<nsINode>(GetWSBoundingParent()))) {
>

> GetWSBoundingParent() returns an already_AddRefed<nsINode>, but all we
> want is to check if it's a block node and then throw it away (without
> leaking it). We don't want to save it beyond that. With the proposed
> change, this would be (assuming we re-added a .get() method, which
> would now be as safe as on nsCOMPtr)
>

> if ((aRun->mRightType & WSType::block) &&

> IsBlockNode(GetWSBoundingParent().get())) {
>
> which means the caller didn't take ownership, just used it and let the
> destructor destroy it.
>
> Similarly, if I would like to pass a string I have to a function that
> wants an atom, I would like to be able to do
> f(do_getAtom(string).get()) instead of
> f(nsCOMPtr<nsIAtom>(do_getAtom(string))).

Yeah, as Jonas said, this problem is sort of limited to editor/ more or
less. :/

> There are also functions in editor that do something like, say, insert
> a new <br>, and return the element they inserted. I might not want to
> get a reference to the element, just want it created. Currently I
> have to make an nsCOMPtr to store the result and then ignore it. I
> don't see the value in requiring this.

For these cases it should be easier to have a helper that does the
nsCOMPtr dance internally and just returns void, right?

>> I don't understand this. You are already able to return raw pointers from
>> functions. Returning an already_AddRefed is however a very different use
>> case, it means, I'm giving up ownership to the object I'm returning.
>
> Yes, but what if the caller wants to use the object once (or not at
> all) and then have it released immediately? Is there value in
> requiring explicit creation of an nsCOMPtr in that case?

I think all of the other things considered, that's OK.

Cheers,
Ehsan

[smaug]

On 08/13/2014 07:24 PM, Aryeh Gregor wrote:
> On Wed, Aug 13, 2014 at 5:44 PM, Ehsan Akhgari <ehsan....@gmail.com> wrote:
>> Can't you do the following instead?
>>
>> unused << MyFunction(); // I know that I'm leaking this ref, but it's ok
>> somehow
>
> No, because the use-case is where you don't want to leak the ref --
> you want it to be released automatically for you. So for instance,
> here's a real-world bit of code from nsWSRunObject:
>
> if ((aRun->mRightType & WSType::block) &&
> IsBlockNode(nsCOMPtr<nsINode>(GetWSBoundingParent()))) {
>
> GetWSBoundingParent() returns an already_AddRefed<nsINode>, but all we
> want is to check if it's a block node and then throw it away (without
> leaking it). We don't want to save it beyond that. With the proposed
> change, this would be (assuming we re-added a .get() method, which
> would now be as safe as on nsCOMPtr)
>
> if ((aRun->mRightType & WSType::block) &&
> IsBlockNode(GetWSBoundingParent().get())) {
>
> which means the caller didn't take ownership, just used it and let the
> destructor destroy it.

I'd say it would be a bug to let easily use already_Addrefed return values this way.
(Whether or not it is actually already_AddRefed or somehow hidden doesn't matter).
AddRef/Release are meaningful operations, and one should always see when they actually happen.
They should not be hidden behind some language magic.




-Olli

[Neil]

Aryeh Gregor wrote:

>for instance, here's a real-world bit of code from nsWSRunObject:
>
> if ((aRun->mRightType & WSType::block) &&
> IsBlockNode(nsCOMPtr<nsINode>(GetWSBoundingParent()))) {
>
>GetWSBoundingParent() returns an already_AddRefed<nsINode>
>

Well there's your problem: GetWSBoundingParent doesn't need to own the
nodes it works on. This is what it should do:

nsINode*
nsWSRunObject::GetWSBoundingParent()
{
NS_ENSURE_TRUE(mNode, nullptr);
nsINode* wsBoundingParent = mNode;
while (!IsBlockNode(wsBoundingParent)) {
nsINode* parent = wsBoundingParent->GetParentNode();
if (!parent || !mHTMLEditor->IsEditable(parent)) {
break;
}
wsBoundingParent = parent;
}
return wsBoundingParent;

[Aryeh Gregor]

On Thu, Aug 14, 2014 at 12:00 PM, Neil <ne...@parkwaycc.co.uk> wrote:
> Well there's your problem: GetWSBoundingParent doesn't need to own the nodes
> it works on.

Editing code is ancient, poorly maintained, and not
performance-sensitive, so personally, I don't ever use raw pointers as
local variables in editor/. Better to not have to rely on manual
review to prevent use-after-free bugs. I am aware of at least one
sec-critical bug in editor that resulted from non-use of nsCOMPtr that
slipped through review when someone was rewriting editing code.

In this case, I seem to remember I wanted to change it to return a raw
pointer instead of already_AddRefed, but IIRC, Ehsan said not to
(although I can't find that anywhere, so maybe I made it up).

[Ehsan Akhgari]

On Thu, Aug 14, 2014 at 10:02 AM, Aryeh Gregor <a...@aryeh.name> wrote:

> On Thu, Aug 14, 2014 at 12:00 PM, Neil <ne...@parkwaycc.co.uk> wrote:

> > Well there's your problem: GetWSBoundingParent doesn't need to own the
> nodes
> > it works on.
>

> Editing code is ancient, poorly maintained, and not
> performance-sensitive, so personally, I don't ever use raw pointers as
> local variables in editor/. Better to not have to rely on manual
> review to prevent use-after-free bugs. I am aware of at least one
> sec-critical bug in editor that resulted from non-use of nsCOMPtr that
> slipped through review when someone was rewriting editing code.
>
> In this case, I seem to remember I wanted to change it to return a raw
> pointer instead of already_AddRefed, but IIRC, Ehsan said not to
> (although I can't find that anywhere, so maybe I made it up).
>

I don't remember either!

--
Ehsan

[Jeff Muizelaar]

We were talking about this problem and it was a bunch of work to figure out the conclusion so I decided to write a summary:

Replacing already_AddRefed with nsRefPtr causes allows two new things:

nsRefPtr<T> getT();

1. T* p = getT(); // this is unsafe because the destructor runs immediately and p is left dangling
2. foo(getT()); // this is safe

Possible solutions would be to:
- remove implicit conversions to T*
- add operator T* explicit and operator T* && = delete // this will be available in GCC 4.8.1 and MSVC 2014 Nov CTP

-Jeff

On Aug 14, 2014, at 10:21 AM, Ehsan Akhgari <ehsan....@gmail.com> wrote:

> On Thu, Aug 14, 2014 at 10:02 AM, Aryeh Gregor <a...@aryeh.name> wrote:
>
>> On Thu, Aug 14, 2014 at 12:00 PM, Neil <ne...@parkwaycc.co.uk> wrote:

>>> Well there's your problem: GetWSBoundingParent doesn't need to own the
>> nodes
>>> it works on.
>>

>> Editing code is ancient, poorly maintained, and not
>> performance-sensitive, so personally, I don't ever use raw pointers as
>> local variables in editor/. Better to not have to rely on manual
>> review to prevent use-after-free bugs. I am aware of at least one
>> sec-critical bug in editor that resulted from non-use of nsCOMPtr that
>> slipped through review when someone was rewriting editing code.
>>
>> In this case, I seem to remember I wanted to change it to return a raw
>> pointer instead of already_AddRefed, but IIRC, Ehsan said not to
>> (although I can't find that anywhere, so maybe I made it up).
>>
>
> I don't remember either!
>
> --
> Ehsan

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

[Ehsan Akhgari]

On 2014-12-22 4:10 PM, Jeff Muizelaar wrote:
> We were talking about this problem and it was a bunch of work to figure out the conclusion so I decided to write a summary:
>
> Replacing already_AddRefed with nsRefPtr causes allows two new things:
>
> nsRefPtr<T> getT();
>
> 1. T* p = getT(); // this is unsafe because the destructor runs immediately and p is left dangling
> 2. foo(getT()); // this is safe
>
> Possible solutions would be to:
> - remove implicit conversions to T*

Perhaps we should consider doing this.

> - add operator T* explicit and operator T* && = delete // this will be available in GCC 4.8.1 and MSVC 2014 Nov CTP
>
> -Jeff
>
> On Aug 14, 2014, at 10:21 AM, Ehsan Akhgari <ehsan....@gmail.com> wrote:
>
>> On Thu, Aug 14, 2014 at 10:02 AM, Aryeh Gregor <a...@aryeh.name> wrote:
>>
>>> On Thu, Aug 14, 2014 at 12:00 PM, Neil <ne...@parkwaycc.co.uk> wrote:

>>>> Well there's your problem: GetWSBoundingParent doesn't need to own the
>>> nodes
>>>> it works on.
>>>

[Eric Rescorla]

On Mon, Dec 22, 2014 at 1:12 PM, Ehsan Akhgari <ehsan....@gmail.com>
wrote:

> On 2014-12-22 4:10 PM, Jeff Muizelaar wrote:
>
>> We were talking about this problem and it was a bunch of work to figure
>> out the conclusion so I decided to write a summary:
>>
>> Replacing already_AddRefed with nsRefPtr causes allows two new things:
>>
>> nsRefPtr<T> getT();
>>
>> 1. T* p = getT(); // this is unsafe because the destructor runs
>> immediately and p is left dangling
>> 2. foo(getT()); // this is safe
>>
>> Possible solutions would be to:
>> - remove implicit conversions to T*
>>
>
> Perhaps we should consider doing this.

+1

There are plenty of other ways to get in trouble with implicit conversion.
E.g.,

nsRefPtr<T> t(new T());
delete t;

However, in the interest of full disclosure, here's a Bugzilla thread where
this was aired a while back without resolution:

https://bugzilla.mozilla.org/show_bug.cgi?id=767178

-Ekr

>
> - add operator T* explicit and operator T* && = delete // this will be
>> available in GCC 4.8.1 and MSVC 2014 Nov CTP
>>
>> -Jeff
>>
>> On Aug 14, 2014, at 10:21 AM, Ehsan Akhgari <ehsan....@gmail.com>
>> wrote:
>>
>> On Thu, Aug 14, 2014 at 10:02 AM, Aryeh Gregor <a...@aryeh.name> wrote:
>>>
>>> On Thu, Aug 14, 2014 at 12:00 PM, Neil <ne...@parkwaycc.co.uk> wrote:
>>>>

>>>>> Well there's your problem: GetWSBoundingParent doesn't need to own the
>>>>>
>>>> nodes
>>>>
>>>>> it works on.
>>>>>
>>>>

[L. David Baron]

On Monday 2014-12-22 16:10 -0500, Jeff Muizelaar wrote:
> We were talking about this problem and it was a bunch of work to figure out the conclusion so I decided to write a summary:
>
> Replacing already_AddRefed with nsRefPtr causes allows two new things:
>
> nsRefPtr<T> getT();
>
> 1. T* p = getT(); // this is unsafe because the destructor runs immediately and p is left dangling
> 2. foo(getT()); // this is safe
>
> Possible solutions would be to:
> - remove implicit conversions to T*

> - add operator T* explicit and operator T* && = delete // this will be available in GCC 4.8.1 and MSVC 2014 Nov CTP

I think removing implicit conversions to T* will make a lot of code
in the tree uglier (".get()" everywhere). That might, in turn,
encourage people to do worse things to avoid having to write .get()
everywhere; it's worth thinking about what those things will be.

(I think it's also worth thinking about the *massive* number of
callsites that will need to be fixed if we remove implicit
conversion to T*.)

I'd also like to keep nsRefPtr and nsCOMPtr consistent with each
other in this regard, to avoid people having to learn additional
patterns. (And, on that subject, I think development practice in
MFBT has been too readily adding new and different things instead of
moving the existing things from XPCOM into MFBT and then improving
them incrementally.)

-David

--
𝄞 L. David Baron http://dbaron.org/ 𝄂
𝄢 Mozilla https://www.mozilla.org/ 𝄂
Before I built a wall I'd ask to know
What I was walling in or walling out,
And to whom I was like to give offense.
- Robert Frost, Mending Wall (1914)

[Ehsan Akhgari]

On 2014-12-22 4:56 PM, L. David Baron wrote:
> On Monday 2014-12-22 16:10 -0500, Jeff Muizelaar wrote:
>> We were talking about this problem and it was a bunch of work to figure out the conclusion so I decided to write a summary:
>>
>> Replacing already_AddRefed with nsRefPtr causes allows two new things:
>>
>> nsRefPtr<T> getT();
>>
>> 1. T* p = getT(); // this is unsafe because the destructor runs immediately and p is left dangling
>> 2. foo(getT()); // this is safe
>>
>> Possible solutions would be to:
>> - remove implicit conversions to T*
>> - add operator T* explicit and operator T* && = delete // this will be available in GCC 4.8.1 and MSVC 2014 Nov CTP
>
> I think removing implicit conversions to T* will make a lot of code
> in the tree uglier (".get()" everywhere). That might, in turn,
> encourage people to do worse things to avoid having to write .get()
> everywhere; it's worth thinking about what those things will be.

Do you have any examples of those bad things? (FWIW I'm all for making
bad things impossible.)

> (I think it's also worth thinking about the *massive* number of
> callsites that will need to be fixed if we remove implicit
> conversion to T*.)

I am planning to write a tool for this task, so the number of call sites
will not be a prohibitive issue.

> I'd also like to keep nsRefPtr and nsCOMPtr consistent with each
> other in this regard, to avoid people having to learn additional
> patterns.

Sure. I think we should remove the implicit conversion from both at the
same time. And from RefPtr too.

> (And, on that subject, I think development practice in
> MFBT has been too readily adding new and different things instead of
> moving the existing things from XPCOM into MFBT and then improving
> them incrementally.)

I completely agree. The amount of pain that RefPtr.h has caused is hard
to overstate, for example. :-)

[L. David Baron]

On Monday 2014-12-22 17:54 -0500, Ehsan Akhgari wrote:
> On 2014-12-22 4:56 PM, L. David Baron wrote:
> >I think removing implicit conversions to T* will make a lot of code
> >in the tree uglier (".get()" everywhere). That might, in turn,
> >encourage people to do worse things to avoid having to write .get()
> >everywhere; it's worth thinking about what those things will be.
>
> Do you have any examples of those bad things? (FWIW I'm all for
> making bad things impossible.)

* using raw pointers instead of smart pointers

* making functions take nsRefPtr<T>& instead of T*, leading to
unnecessary risk of mutation of the caller's pointer and extra
indirection

* ... and perhaps the same for getters

* using C-style casts (or reinterpret_cast) to make things compile

[Mike Hommey]

On Mon, Dec 22, 2014 at 04:56:12PM -0500, L. David Baron wrote:
> (And, on that subject, I think development practice in
> MFBT has been too readily adding new and different things instead of
> moving the existing things from XPCOM into MFBT and then improving
> them incrementally.)

That essentially comes from MFBT's roots in js code, as opposed to gecko
code.

Mike

[Ehsan Akhgari]

On 2014-12-22 6:07 PM, L. David Baron wrote:
> On Monday 2014-12-22 17:54 -0500, Ehsan Akhgari wrote:
>> On 2014-12-22 4:56 PM, L. David Baron wrote:
>>> I think removing implicit conversions to T* will make a lot of code
>>> in the tree uglier (".get()" everywhere). That might, in turn,
>>> encourage people to do worse things to avoid having to write .get()
>>> everywhere; it's worth thinking about what those things will be.
>>
>> Do you have any examples of those bad things? (FWIW I'm all for
>> making bad things impossible.)
>
> * using raw pointers instead of smart pointers

I am planning on making that impossible [*] in 2015.

> * making functions take nsRefPtr<T>& instead of T*, leading to
> unnecessary risk of mutation of the caller's pointer and extra
> indirection
>
> * ... and perhaps the same for getters

Are there good use cases for having functions accept an nsRefPtr<T>&?
If not, we can outlaw them.

> * using C-style casts (or reinterpret_cast) to make things compile

C-style casts would not be able to make anything compile if the class
doesn't provide an operator T*(). I'm not sure what you mean here.


[*] Obviously there will be syntax for the case where you really know
what you're doing and you really can't use a smart pointer, but
hopefully those scenarios should be few and far between.

[L. David Baron]

On Monday 2014-12-22 18:21 -0500, Ehsan Akhgari wrote:
> On 2014-12-22 6:07 PM, L. David Baron wrote:
> >On Monday 2014-12-22 17:54 -0500, Ehsan Akhgari wrote:
> >>On 2014-12-22 4:56 PM, L. David Baron wrote:
> >>>I think removing implicit conversions to T* will make a lot of code
> >>>in the tree uglier (".get()" everywhere). That might, in turn,
> >>>encourage people to do worse things to avoid having to write .get()
> >>>everywhere; it's worth thinking about what those things will be.
> >>
> >>Do you have any examples of those bad things? (FWIW I'm all for
> >>making bad things impossible.)
> >
> >* using raw pointers instead of smart pointers
>
> I am planning on making that impossible [*] in 2015.

I presume you mean making direct calls to AddRef and Release
impossible, and not raw pointers in general.

> >* making functions take nsRefPtr<T>& instead of T*, leading to
> > unnecessary risk of mutation of the caller's pointer and extra
> > indirection
> >
> > * ... and perhaps the same for getters
>
> Are there good use cases for having functions accept an
> nsRefPtr<T>&? If not, we can outlaw them.

I've seen a few, but it's probably rare. (Is that pattern still
used all over editor?)

> >* using C-style casts (or reinterpret_cast) to make things compile
>
> C-style casts would not be able to make anything compile if the
> class doesn't provide an operator T*(). I'm not sure what you mean
> here.

Sorry, originally wrote reinterpret_cast and then changed what I
wrote at the last minute because I was thinking about casting
between pointer types.

[Ehsan Akhgari]

On 2014-12-22 6:35 PM, L. David Baron wrote:
> On Monday 2014-12-22 18:21 -0500, Ehsan Akhgari wrote:
>> On 2014-12-22 6:07 PM, L. David Baron wrote:
>>> On Monday 2014-12-22 17:54 -0500, Ehsan Akhgari wrote:
>>>> On 2014-12-22 4:56 PM, L. David Baron wrote:
>>>>> I think removing implicit conversions to T* will make a lot of code
>>>>> in the tree uglier (".get()" everywhere). That might, in turn,
>>>>> encourage people to do worse things to avoid having to write .get()
>>>>> everywhere; it's worth thinking about what those things will be.
>>>>
>>>> Do you have any examples of those bad things? (FWIW I'm all for
>>>> making bad things impossible.)
>>>
>>> * using raw pointers instead of smart pointers
>>
>> I am planning on making that impossible [*] in 2015.
>
> I presume you mean making direct calls to AddRef and Release
> impossible, and not raw pointers in general.

Well, I did pick that project up but for some reason it was never
finished (don't remember what.) But that's not what I was talking
about. See bug 1114683 for an example of what I was talking about.

>>> * making functions take nsRefPtr<T>& instead of T*, leading to
>>> unnecessary risk of mutation of the caller's pointer and extra
>>> indirection
>>>
>>> * ... and perhaps the same for getters
>>
>> Are there good use cases for having functions accept an
>> nsRefPtr<T>&? If not, we can outlaw them.
>
> I've seen a few, but it's probably rare. (Is that pattern still
> used all over editor?)

It indeed is, but that doesn't make it a good use case. The editor/
stuff is mostly workarounds for the lack of things such as
already_AddRefed, getter_AddRefs and so on. I personally cannot think
of a good use case for it.

>>> * using C-style casts (or reinterpret_cast) to make things compile
>>
>> C-style casts would not be able to make anything compile if the
>> class doesn't provide an operator T*(). I'm not sure what you mean
>> here.
>
> Sorry, originally wrote reinterpret_cast and then changed what I
> wrote at the last minute because I was thinking about casting
> between pointer types.

Same thing with both types of casts. :-)

[Eric Rescorla]

On Mon, Dec 22, 2014 at 3:35 PM, L. David Baron <dba...@dbaron.org> wrote:

> On Monday 2014-12-22 18:21 -0500, Ehsan Akhgari wrote:
> > On 2014-12-22 6:07 PM, L. David Baron wrote:
> > >On Monday 2014-12-22 17:54 -0500, Ehsan Akhgari wrote:
> > >>On 2014-12-22 4:56 PM, L. David Baron wrote:
> > >>>I think removing implicit conversions to T* will make a lot of code
> > >>>in the tree uglier (".get()" everywhere). That might, in turn,
> > >>>encourage people to do worse things to avoid having to write .get()
> > >>>everywhere; it's worth thinking about what those things will be.
> > >>
> > >>Do you have any examples of those bad things? (FWIW I'm all for
> > >>making bad things impossible.)
> > >
> > >* using raw pointers instead of smart pointers
> >
> > I am planning on making that impossible [*] in 2015.
>
> I presume you mean making direct calls to AddRef and Release
> impossible, and not raw pointers in general.
>

> > >* making functions take nsRefPtr<T>& instead of T*, leading to
> > > unnecessary risk of mutation of the caller's pointer and extra
> > > indirection
> > >
> > > * ... and perhaps the same for getters
> >
> > Are there good use cases for having functions accept an
> > nsRefPtr<T>&? If not, we can outlaw them.
>
> I've seen a few, but it's probably rare. (Is that pattern still
> used all over editor?)

I frequently use const RefPtr<T>&? or const UniquePtr<T>&. Is this something
that people object to?

-Ekr

> > >* using C-style casts (or reinterpret_cast) to make things compile
> >
> > C-style casts would not be able to make anything compile if the
> > class doesn't provide an operator T*(). I'm not sure what you mean
> > here.
>
> Sorry, originally wrote reinterpret_cast and then changed what I
> wrote at the last minute because I was thinking about casting
> between pointer types.
>

> -David
>
> --
> 𝄞 L. David Baron http://dbaron.org/ 𝄂
> 𝄢 Mozilla https://www.mozilla.org/ 𝄂
> Before I built a wall I'd ask to know
> What I was walling in or walling out,
> And to whom I was like to give offense.
> - Robert Frost, Mending Wall (1914)
>

[Ms2ger]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 12/23/2014 12:45 AM, Ehsan Akhgari wrote:
> On 2014-12-22 6:35 PM, L. David Baron wrote:
>> On Monday 2014-12-22 18:21 -0500, Ehsan Akhgari wrote:
>>> On 2014-12-22 6:07 PM, L. David Baron wrote:
>>>> On Monday 2014-12-22 17:54 -0500, Ehsan Akhgari wrote:
>>>>> On 2014-12-22 4:56 PM, L. David Baron wrote:
>>>>>> I think removing implicit conversions to T* will make a
>>>>>> lot of code in the tree uglier (".get()" everywhere).
>>>>>> That might, in turn, encourage people to do worse things
>>>>>> to avoid having to write .get() everywhere; it's worth
>>>>>> thinking about what those things will be.
>>>>>
>>>>> Do you have any examples of those bad things? (FWIW I'm
>>>>> all for making bad things impossible.)
>>>>
>>>> * using raw pointers instead of smart pointers
>>>
>>> I am planning on making that impossible [*] in 2015.
>>
>> I presume you mean making direct calls to AddRef and Release
>> impossible, and not raw pointers in general.
>
> Well, I did pick that project up but for some reason it was never
> finished (don't remember what.) But that's not what I was talking
> about. See bug 1114683 for an example of what I was talking
> about.

It was finished, and then reverted. For reference:
<https://bugzilla.mozilla.org/show_bug.cgi?id=666414>,
<https://bugzilla.mozilla.org/show_bug.cgi?id=710473>.

HTH
Ms2ger
-----BEGIN PGP SIGNATURE-----

iQEcBAEBAgAGBQJUmR8xAAoJEOXgvIL+s8n2OlcH/Am6t8XxPCvZj3w/VWbPbhF8
wQRqfnWvdHwB/neOxaTzcyMWL1siQfQIHTmDjOKmbKtD6tETyybuYiWba6YmRjFQ
52URtbQkh5zRS6VawRyNEiMc3ea6gnj+TPQ9jOOgO0556wjN+CoqwWAXvcFz6mhC
AQVVh+Cm9mvo3VRJcmzFbQ6lwuIaUOruIoDE8vy8fQVKC1vYgIxntKgfpcLQIe0Z
2Oux89uN3xJOl9i+JZevUkOrdFO7tZg2LduS7YcdCg9WFZy1PNIKcLxnGLi8V1Nv
JkwEX+YpkgV/whI6etk4D7nzWT+ZDRYVEsPrZlRRf8Lph9dM0kP3y15H7pEY9Q0=
=fIbp
-----END PGP SIGNATURE-----

[Ehsan Akhgari]

Ah, right. I'll see if I can reland this now with some compiler
upgrades that have happened since then.

[Ehsan Akhgari]

On 2014-12-22 6:52 PM, Eric Rescorla wrote:

>
>
> On Mon, Dec 22, 2014 at 3:35 PM, L. David Baron <dba...@dbaron.org
> <mailto:dba...@dbaron.org>> wrote:
>
> On Monday 2014-12-22 18:21 -0500, Ehsan Akhgari wrote:
> > On 2014-12-22 6:07 PM, L. David Baron wrote:
> > >On Monday 2014-12-22 17:54 -0500, Ehsan Akhgari wrote:
> > >>On 2014-12-22 4:56 PM, L. David Baron wrote:
> > >>>I think removing implicit conversions to T* will make a lot of code
> > >>>in the tree uglier (".get()" everywhere). That might, in turn,
> > >>>encourage people to do worse things to avoid having to write .get()
> > >>>everywhere; it's worth thinking about what those things will be.
> > >>
> > >>Do you have any examples of those bad things? (FWIW I'm all for
> > >>making bad things impossible.)
> > >
> > >* using raw pointers instead of smart pointers
> >
> > I am planning on making that impossible [*] in 2015.
>
> I presume you mean making direct calls to AddRef and Release
> impossible, and not raw pointers in general.
>

> > >* making functions take nsRefPtr<T>& instead of T*, leading to
> > > unnecessary risk of mutation of the caller's pointer and extra
> > > indirection
> > >
> > > * ... and perhaps the same for getters
> >
> > Are there good use cases for having functions accept an
> > nsRefPtr<T>&? If not, we can outlaw them.
>
> I've seen a few, but it's probably rare. (Is that pattern still
> used all over editor?)
>
>
> I frequently use const RefPtr<T>&? or const UniquePtr<T>&. Is this something
> that people object to?

What is your use case? I guess it's not transferring ownership. I
don't really understand why one would use these classes as argument when
they're not trying to worry about ownership...

[Ehsan Akhgari]

On 2014-12-23 9:47 AM, Ehsan Akhgari wrote:
> On 2014-12-23 2:52 AM, Ms2ger wrote:

> Ah, right. I'll see if I can reland this now with some compiler
> upgrades that have happened since then.

... And that's impossible because of all of the final classes we have
these days. Filed bug 1114999 to write a static analysis for this instead.

[Eric Rescorla]

On Tue, Dec 23, 2014 at 6:48 AM, Ehsan Akhgari <ehsan....@gmail.com>
wrote:

> On 2014-12-22 6:52 PM, Eric Rescorla wrote:
>
>>
>>

>> On Mon, Dec 22, 2014 at 3:35 PM, L. David Baron <dba...@dbaron.org
>> <mailto:dba...@dbaron.org>> wrote:
>>
>> On Monday 2014-12-22 18:21 -0500, Ehsan Akhgari wrote:
>> > On 2014-12-22 6:07 PM, L. David Baron wrote:
>> > >On Monday 2014-12-22 17:54 -0500, Ehsan Akhgari wrote:
>> > >>On 2014-12-22 4:56 PM, L. David Baron wrote:
>> > >>>I think removing implicit conversions to T* will make a lot of
>> code
>> > >>>in the tree uglier (".get()" everywhere). That might, in turn,
>> > >>>encourage people to do worse things to avoid having to write
>> .get()
>> > >>>everywhere; it's worth thinking about what those things will be.
>> > >>
>> > >>Do you have any examples of those bad things? (FWIW I'm all for
>> > >>making bad things impossible.)
>> > >
>> > >* using raw pointers instead of smart pointers
>> >
>> > I am planning on making that impossible [*] in 2015.
>>
>> I presume you mean making direct calls to AddRef and Release
>> impossible, and not raw pointers in general.
>>

>> > >* making functions take nsRefPtr<T>& instead of T*, leading to
>> > > unnecessary risk of mutation of the caller's pointer and extra
>> > > indirection
>> > >
>> > > * ... and perhaps the same for getters
>> >
>> > Are there good use cases for having functions accept an
>> > nsRefPtr<T>&? If not, we can outlaw them.
>>
>> I've seen a few, but it's probably rare. (Is that pattern still
>> used all over editor?)
>>
>>
>> I frequently use const RefPtr<T>&? or const UniquePtr<T>&. Is this
>> something
>> that people object to?
>>
>
> What is your use case? I guess it's not transferring ownership. I don't
> really understand why one would use these classes as argument when they're
> not trying to worry about ownership...
>

You're already holding a SmartPtr<T> and you want to pass it to a function
which operates on it, but since, as you say, you're not transferring
ownership, you don't need to increment or decrement the ref ct.

-Ekr

[Ehsan Akhgari]

On 2014-12-23 10:38 AM, Eric Rescorla wrote:
>
>
> On Tue, Dec 23, 2014 at 6:48 AM, Ehsan Akhgari <ehsan....@gmail.com
> <mailto:ehsan....@gmail.com>> wrote:
>
> On 2014-12-22 6:52 PM, Eric Rescorla wrote:
>
>
>
> On Mon, Dec 22, 2014 at 3:35 PM, L. David Baron
> <dba...@dbaron.org <mailto:dba...@dbaron.org>

Why not pass the raw pointer to the function? That's what we do in most
of the places in the code.

[Eric Rescorla]

On Tue, Dec 23, 2014 at 8:32 AM, Ehsan Akhgari <ehsan....@gmail.com>
wrote:

My general theory is that smart pointers, once boxed, should never be
unboxed.
The major arguments I see for unboxing is performance, but if you pass a
ref,
then you don't have the increment/decrement cycle.

> That's what we do in most of the places in the code.
>

Yes, I think this is unwise.

-Ekr

[L. David Baron]

On Tuesday 2014-12-23 08:36 -0800, Eric Rescorla wrote:
> > Why not pass the raw pointer to the function?
>
>
> My general theory is that smart pointers, once boxed, should never be
> unboxed.
> The major arguments I see for unboxing is performance, but if you pass a
> ref,
> then you don't have the increment/decrement cycle.
>
>
>
> > That's what we do in most of the places in the code.
> >
>
> Yes, I think this is unwise.

Our convention has always been to pass raw pointers, generally with
the assumption that the caller is expected to ensure the pointer
lives across the function call.

Passing references to smart pointers doesn't work if you need
implicit conversions from derived class to base class (e.g., if the
function takes Base*, and the caller has nsRefPtr<Derived>), which
is pretty important in many areas of our codebase.

Passing smart pointers (non-references) would cause tons of extra
reference count traffic, which is particularly problematic when the
reference-counting functions are virtual and/or threadsafe.

[Ehsan Akhgari]

On 2014-12-23 11:36 AM, Eric Rescorla wrote:
>
>
> On Tue, Dec 23, 2014 at 8:32 AM, Ehsan Akhgari <ehsan....@gmail.com
> <mailto:ehsan....@gmail.com>> wrote:
>
> On 2014-12-23 10:38 AM, Eric Rescorla wrote:
>
>
>
> On Tue, Dec 23, 2014 at 6:48 AM, Ehsan Akhgari
> <ehsan....@gmail.com <mailto:ehsan....@gmail.com>

> <mailto:ehsan.akhgari@gmail.__com

> Why not pass the raw pointer to the function?
>
>
> My general theory is that smart pointers, once boxed, should never be
> unboxed.
> The major arguments I see for unboxing is performance, but if you pass a
> ref,
> then you don't have the increment/decrement cycle.

But still you'd need to double-deref the pointer to use it in the callee
(assuming that things do not get inlined.)

Also, I think that general theory is only useful if you actually manage
to stick to it all along the way. Because of the point below, that
would probably be a lost cause in our code base, and all you need to
screw things up is a single place where the smart pointer is unboxed.

That being said, I think we can and should make the compiler more aware
of the semantics of our smart pointers, to ensure that the unboxing
operation can only be performed when you hold an owning pointer to the
object. Once we get there, unboxing would be safe.

> That's what we do in most of the places in the code.
>
>
> Yes, I think this is unwise.

For the better or worse, anything that uses XPCOM needs to stick to
passing in raw interface pointers, since anything else would be an ABI
incompatible change (assuming that ABI compat with MSCOM is still a goal.)

[Eric Rescorla]

On Tue, Dec 23, 2014 at 8:48 AM, L. David Baron <dba...@dbaron.org> wrote:

> On Tuesday 2014-12-23 08:36 -0800, Eric Rescorla wrote:

> > > Why not pass the raw pointer to the function?
> >
> >
> > My general theory is that smart pointers, once boxed, should never be
> > unboxed.
> > The major arguments I see for unboxing is performance, but if you pass a
> > ref,
> > then you don't have the increment/decrement cycle.
> >
> >
> >

> > > That's what we do in most of the places in the code.
> > >
> >
> > Yes, I think this is unwise.
>

> Our convention has always been to pass raw pointers, generally with
> the assumption that the caller is expected to ensure the pointer
> lives across the function call.
>
> Passing references to smart pointers doesn't work if you need
> implicit conversions from derived class to base class (e.g., if the
> function takes Base*, and the caller has nsRefPtr<Derived>), which
> is pretty important in many areas of our codebase.
>

Hmm... This seems to work fine with RefPtr, but it *doesn't* work
with nsRefPtr (see end of message for the test code). I'm guessing
because of:

http://dxr.mozilla.org/mozilla-central/source/mfbt/RefPtr.h?from=RefPtr&case=true#239

Passing smart pointers (non-references) would cause tons of extra
> reference count traffic, which is particularly problematic when the
> reference-counting functions are virtual and/or threadsafe.

Right, that's the reason for using references.

-Ekr



class Base
{
public:
NS_INLINE_DECL_THREADSAFE_REFCOUNTING(Base);

private:
~Base() {}
};

class Derived : public Base {
};


void base_ref(const RefPtr<Base>& b) {
}

void base_ptr(Base* b) {
}

void foo() {
RefPtr<Base> base;
RefPtr<Derived> derived;

base_ref(base);
base_ref(derived);

base_ptr(base);
base_ptr(derived)
}

[Eric Rescorla]

On Tue, Dec 23, 2014 at 8:53 AM, Ehsan Akhgari <ehsan....@gmail.com>
wrote:

>> Why not pass the raw pointer to the function?
>>
>>
>> My general theory is that smart pointers, once boxed, should never be
>> unboxed.
>> The major arguments I see for unboxing is performance, but if you pass a
>> ref,
>> then you don't have the increment/decrement cycle.
>>
>

> But still you'd need to double-deref the pointer to use it in the callee
> (assuming that things do not get inlined.)
>
> Also, I think that general theory is only useful if you actually manage to
> stick to it all along the way. Because of the point below, that would
> probably be a lost cause in our code base, and all you need to screw things
> up is a single place where the smart pointer is unboxed.
>

I think this (and the issue below) may be a matter of which parts of the
code you're working in. I mostly do self-contained stuff which doesn't
need to interact with XPCOM and which uses a lot of smart pointers
internally, so I'm not overworried about having to convert to raw pointers.

-Ekr

[Ehsan Akhgari]

On 2014-12-23 12:51 PM, Eric Rescorla wrote:
>
>
> On Tue, Dec 23, 2014 at 8:53 AM, Ehsan Akhgari <ehsan....@gmail.com

> <mailto:ehsan....@gmail.com>> wrote:
>
> On 2014-12-23 11:36 AM, Eric Rescorla wrote:
>
>
>
> On Tue, Dec 23, 2014 at 8:32 AM, Ehsan Akhgari
> <ehsan....@gmail.com <mailto:ehsan....@gmail.com>

> <mailto:ehsan.akhgari@gmail.__com
> <mailto:ehsan....@gmail.com>>> wrote:
>
> On 2014-12-23 10:38 AM, Eric Rescorla wrote:
>
>
>
> On Tue, Dec 23, 2014 at 6:48 AM, Ehsan Akhgari
> <ehsan....@gmail.com
> <mailto:ehsan....@gmail.com>
> <mailto:ehsan.akhgari@gmail.__com <mailto:ehsan....@gmail.com>>

> <mailto:ehsan.akhgari@gmail.
> <mailto:ehsan.akhgari@gmail.>____com

That's fair. But I also have a tendency towards free abstractions over
ones that cost us runtime perormance...

[L. David Baron]

On Tuesday 2014-12-23 09:49 -0800, Eric Rescorla wrote:
> Hmm... This seems to work fine with RefPtr, but it *doesn't* work
> with nsRefPtr (see end of message for the test code). I'm guessing
> because of:
>
> http://dxr.mozilla.org/mozilla-central/source/mfbt/RefPtr.h?from=RefPtr&case=true#239

But that's an implicit constructor that's causing extra refcount
traffic, which is one of the things we didn't want here.

(Also, it's another implicit constructor; yikes!)

[Martin Thomson]

On Tue, Dec 23, 2014 at 9:55 AM, L. David Baron <dba...@dbaron.org> wrote:
>
> But that's an implicit constructor that's causing extra refcount
> traffic, which is one of the things we didn't want here.

I don't think that it's quite fair to complain about implicit conversion if
the whole point is to enable implicit conversion of another sort. And
maybe something could be done about the refcount traffic.

[L. David Baron]

The problem is that something unexpected is happening. People
should expect the implicit conversions that are part of the
language. They generally don't expect implicit creation of smart
pointers to cause reference counting traffic when they make a
function call.

Implicit conversions can be useful when they're carefully designed.
But C++ makes it too easy to create them by accident (by making
1-parameter constructors do implicit conversion by default). And
this one doesn't look carefully designed, given that it does things
that users don't expect, and it's not clear how to fix that.

[Jeff Walden]

On 12/23/2014 10:48 AM, L. David Baron wrote:
> Our convention has always been to pass raw pointers, generally with
> the assumption that the caller is expected to ensure the pointer
> lives across the function call.

Like Eric, I would like us to move away from this convention over time, or at least stay away from it in the places that are fresher and better-adjusted.

> Passing references to smart pointers doesn't work if you need
> implicit conversions from derived class to base class (e.g., if the
> function takes Base*, and the caller has nsRefPtr<Derived>), which
> is pretty important in many areas of our codebase.

Passing references to smart pointers is also mildly more expensive because you have an extra dereference operation, in the general case. But that almost certainly far outweighs not addrefing/releasing through even a well-predicted vtable, as you go on to say.

Really, tho, I don't think we have to forego a smart pointer argument type. We just need a separate smart pointer type ArgumentPtr<T> (name not seriously proposed, just to have a handle on the concept), implicitly constructible from the various smart pointer types of U, where U derives from T.

And concerning the binary compatibility argument -- which I don't think really matters any more, and if it does a11y could have its own MSCOM smart pointer class for its own special-snowflake uses -- it seems fairly likely to me that there's a smart pointer class that could preserve that binary compatibility. Although I haven't investigated deeply to conclude so for sure.

Jeff

[Eric Rescorla]

On Tue, Dec 23, 2014 at 10:14 AM, L. David Baron <dba...@dbaron.org> wrote:

> On Tuesday 2014-12-23 09:59 -0800, Martin Thomson wrote:
> > On Tue, Dec 23, 2014 at 9:55 AM, L. David Baron <dba...@dbaron.org>
> wrote:
> > >
> > > But that's an implicit constructor that's causing extra refcount
> > > traffic, which is one of the things we didn't want here.
> >
> >
> >
> > I don't think that it's quite fair to complain about implicit conversion
> if
> > the whole point is to enable implicit conversion of another sort. And
> > maybe something could be done about the refcount traffic.
>
> The problem is that something unexpected is happening. People
> should expect the implicit conversions that are part of the
> language. They generally don't expect implicit creation of smart
> pointers to cause reference counting traffic when they make a
> function call.
>
> Implicit conversions can be useful when they're carefully designed.
> But C++ makes it too easy to create them by accident (by making
> 1-parameter constructors do implicit conversion by default). And
> this one doesn't look carefully designed, given that it does things
> that users don't expect, and it's not clear how to fix that.

Just to be clear, is your problem the implicit conversion itself
or the reference count increment/decrement?

On the specific topic of what users expect, what you expect may not
be what I expect, but I would observe that both C++11 shared_ptr
and Boost intrusive_ptr (the analog to our RefPtrs> automatically
convert from SmartPtr<Derived> to SmartPtr<Base> and appear to
increment the reference count in the same way that RefPtr does;
this seems to suggest that at least some people do expect it to
behave this way.

-Ekr

[Jonas Sicking]

On Tue, Dec 23, 2014 at 1:03 PM, Jeff Walden <jwald...@mit.edu> wrote:
> On 12/23/2014 10:48 AM, L. David Baron wrote:
>> Our convention has always been to pass raw pointers, generally with
>> the assumption that the caller is expected to ensure the pointer
>> lives across the function call.
>
> Like Eric, I would like us to move away from this convention over time, or at least stay away from it in the places that are fresher and better-adjusted.

As long as we can do that without incurring extra overhead. An
addref/release pair is fairly expensive for objects that are cycle
collected.

I guess passing references to an nsRef/COMPtr is a way to avoid
passing a raw pointer while also avoiding extra addrefs/releases. But
it's pretty ugly and also has a runtime cost.

/ Jonas

[L. David Baron]

On Tuesday 2014-12-23 13:14 -0800, Eric Rescorla wrote:
> Just to be clear, is your problem the implicit conversion itself
> or the reference count increment/decrement?

The latter -- the problem is that there's an implicit conversion
that has surprising side-effects.

(It sounds from this discussion like you've written a bunch of code
that you wrote the way you did to avoid excess reference-counting,
yet there may have been a bunch of excess reference-counting that
you didn't know about.)

> On the specific topic of what users expect, what you expect may not
> be what I expect, but I would observe that both C++11 shared_ptr
> and Boost intrusive_ptr (the analog to our RefPtrs> automatically
> convert from SmartPtr<Derived> to SmartPtr<Base> and appear to
> increment the reference count in the same way that RefPtr does;
> this seems to suggest that at least some people do expect it to
> behave this way.

Ugh.

Then again, this wouldn't be the first thing in the standard library
that didn't make sense for users who care about performance.

[Martin Thomson]

On Tue, Dec 23, 2014 at 1:51 PM, L. David Baron <dba...@dbaron.org> wrote:
>
> > Just to be clear, is your problem the implicit conversion itself
> > or the reference count increment/decrement?
>
> The latter -- the problem is that there's an implicit conversion
> that has surprising side-effects.

Why are you surprised that when you pass a pointer to a function that the
function gets a pointer?

I agree that the performance is less than perfect. I do have to ask: at
what point do you consider the preservation of developer time more valuable
than the preservation of CPU time? We could be writing in machine code
directly, but we made some trade-offs along the way. There will always be
exceptions, and .get() exists to support those (or Waldo's proposed clever
hacks, I suppose).

[Eric Rescorla]

On Tue, Dec 23, 2014 at 1:51 PM, L. David Baron <dba...@dbaron.org> wrote:

> On Tuesday 2014-12-23 13:14 -0800, Eric Rescorla wrote:
> > Just to be clear, is your problem the implicit conversion itself
> > or the reference count increment/decrement?
>
> The latter -- the problem is that there's an implicit conversion
> that has surprising side-effects.
>

> (It sounds from this discussion like you've written a bunch of code
> that you wrote the way you did to avoid excess reference-counting,
> yet there may have been a bunch of excess reference-counting that
> you didn't know about.)

I could look, but I don't think so, for two reasons:

- I try to use UniquePtr<T> when I can, and for obvious reasons,
UniquePtr<T> doesn't do implicit conversion.

- I generally don't do a lot of passing around of SmartPtr<Base>,
just because the type of code I write tends not to have a lot of
this kind of inheritance.

> On the specific topic of what users expect, what you expect may not
> > be what I expect, but I would observe that both C++11 shared_ptr
> > and Boost intrusive_ptr (the analog to our RefPtrs> automatically
> > convert from SmartPtr<Derived> to SmartPtr<Base> and appear to
> > increment the reference count in the same way that RefPtr does;
> > this seems to suggest that at least some people do expect it to
> > behave this way.
>
> Ugh.
>
> Then again, this wouldn't be the first thing in the standard library
> that didn't make sense for users who care about performance.

This may be a much longer argument, but I'm not convinced that
sacrificing what would otherwise be good programming practice
(never unboxing your pointers) at the altar of performance is a good
idea. I'd note that you only pay this penalty when you pass an object
of type RefPtr<Derived> to something that takes RefPtr<Base>,
and depending on your code, this may happen infrequently or
at least only happen a lot of times at a fairly small number of call
sites, which can then be optimized in the usual fashion by changing
to a raw pointer call.

-Ekr

[L. David Baron]

On Tuesday 2014-12-23 13:59 -0800, Martin Thomson wrote:
> Why are you surprised that when you pass a pointer to a function that the
> function gets a pointer?

You're putting words in my mouth.

> I agree that the performance is less than perfect. I do have to ask: at
> what point do you consider the preservation of developer time more valuable
> than the preservation of CPU time?

I think that's a poor description of the problem.

Some developers spend time working on performance problems, and
hiding those performance problems is not good for their time.

Developers should be able to reason about the performance
characteristics of code that they're looking at. Many
reference-counting operations are expensive. Many developers
spend time structuring their code to avoid them. I'm just asking
that when they try to do this, they get what they expect.

[L. David Baron]

On Tuesday 2014-12-23 14:03 -0800, Eric Rescorla wrote:
> This may be a much longer argument, but I'm not convinced that
> sacrificing what would otherwise be good programming practice
> (never unboxing your pointers) at the altar of performance is a good
> idea.

Why do you think never unboxing pointers is a good programming
practice?

[Eric Rescorla]

On Tue, Dec 23, 2014 at 2:07 PM, L. David Baron <dba...@dbaron.org> wrote:

> On Tuesday 2014-12-23 14:03 -0800, Eric Rescorla wrote:
> > This may be a much longer argument, but I'm not convinced that
> > sacrificing what would otherwise be good programming practice
> > (never unboxing your pointers) at the altar of performance is a good
> > idea.
>
> Why do you think never unboxing pointers is a good programming
> practice?

Because it allows/encourages mixed ownership regimes between reference
counting (or single ownership) and explicit management.

Note that I'm not saying that there's never a time for unboxing, but it
should be done carefully not routinely.

-Ekr

[Ehsan Akhgari]

I think there is a different way of looking at things. When you pass a
conceptual pointer value to a function, sometimes you want an ownership
change to happen, and at other times you just want the callee to use the
pointer (perhaps to call functions on it, for example) without changing
the ownership. I think unboxing the pointer for the latter case is
perfectly fine, as long as enforce that the ownership on the caller side
remains valid for the duration of the call, and that the callee doesn't
do something that changes the ownership behind the scenes (the latter is
more challenging.)

[Eric Rescorla]

On Tue, Dec 23, 2014 at 2:19 PM, Ehsan Akhgari <ehsan....@gmail.com>
wrote:

Well, I certainly agree it's more challenging, since nothing stops the
callee from simple assigning the raw pointer to some location that
outlives the function call. The idea behind requiring that pointers
remain boxed is to prevent that and/or to shine a light on places
where it is possible because someone unboxed.

-Ekr

[Ehsan Akhgari]

On 2014-12-23 5:30 PM, Eric Rescorla wrote:
> On Tue, Dec 23, 2014 at 2:19 PM, Ehsan Akhgari <ehsan....@gmail.com
> <mailto:ehsan....@gmail.com>> wrote:
>
> On 2014-12-23 5:13 PM, Eric Rescorla wrote:
>
> On Tue, Dec 23, 2014 at 2:07 PM, L. David Baron
> <dba...@dbaron.org <mailto:dba...@dbaron.org>

That's actually pretty easy to detect and disallow! Filed bug 1115175.

> The idea behind requiring that pointers
> remain boxed is to prevent that and/or to shine a light on places
> where it is possible because someone unboxed.

Right. And that's one solution but it's not the only one. I'm hoping
to make our clang plugin much more aware of our ownership rules so that
it can enforce a lot of them.

[Boris Zbarsky]

On 12/23/14 1:03 PM, Jeff Walden wrote:
> And concerning the binary compatibility argument -- which I don't think really matters any more

Oh, I wish you were right.

Things like bug 1078674 suggest that you're wrong, however. We have
random binary code we don't control calling into our code, so if we
change the ABI we need to either making sure said random binary code is
updated accordingly or that it doesn't run. Otherwise we get crashes,
and possibly exploitable ones given that vtables are involved.

Note that this is talking about ABI compat with ourselves; the fact that
it happens to match MSCOM doesn't matter here.

-Boris

[Joshua Cranmer 🐧]

For what it's worth, if we are willing to do a real mass-conversion on
our codebase (to deal with XPIDL-related API), we could probably replace
a lot of our T* arguments with use of "the world's dumbest smart
pointer" (see
<http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4282.pdf>). If
we designed it right, it would be ABI-compatible for raw pointers on
argument passing (but probably not return values, if I recall my C++
ABIs properly). We could arguably also replace our T** arguments with
nsRefPtr/nsCOMPtr & arguments at the same time if we're massively
rewriting our codebase.

--
Joshua Cranmer
Thunderbird and DXR developer
Source code archæologist

[Neil]

Ehsan Akhgari wrote:

> On 2014-12-23 5:30 PM, Eric Rescorla wrote:
>
>> nothing stops the callee from simple assigning the raw pointer to
>> some location that outlives the function call.
>
> That's actually pretty easy to detect and disallow! Filed bug 1115175.

Although we want to allow it in the case of another nsCOMPtr of course.

--
Warning: May contain traces of nuts.

[Ehsan Akhgari]

On 2014-12-24 3:21 PM, Neil wrote:
> Ehsan Akhgari wrote:
>
>> On 2014-12-23 5:30 PM, Eric Rescorla wrote:
>>
>>> nothing stops the callee from simple assigning the raw pointer to
>>> some location that outlives the function call.
>>
>> That's actually pretty easy to detect and disallow! Filed bug 1115175.
>
> Although we want to allow it in the case of another nsCOMPtr of course.

Of course. This was about raw pointer assignment.

[Andrew McCreight]

On Tue, Dec 23, 2014 at 4:26 PM, Jonas Sicking <jo...@sicking.cc> wrote:

> On Tue, Dec 23, 2014 at 1:03 PM, Jeff Walden <jwald...@mit.edu> wrote:
> > On 12/23/2014 10:48 AM, L. David Baron wrote:
> >> Our convention has always been to pass raw pointers, generally with
> >> the assumption that the caller is expected to ensure the pointer
> >> lives across the function call.
> >
> > Like Eric, I would like us to move away from this convention over time,
> or at least stay away from it in the places that are fresher and
> better-adjusted.
>

> As long as we can do that without incurring extra overhead. An

> addref/release pair is fairly expensive for objects that are cycle
> collected.
>

For what it's worth, thanks to the Snow White work done by Olli Pettay,
doing an addref/release on a cycle collected pointer should be roughly as
expensive as any other addref and release.

The first addref or release of an object after a cycle collection will be
more expensive, but the rest after that just have to do a little bit of
extra bit twiddling in addition to the increment or decrement of the
refcount.

Andrew

>
> I guess passing references to an nsRef/COMPtr is a way to avoid
> passing a raw pointer while also avoiding extra addrefs/releases. But
> it's pretty ugly and also has a runtime cost.
>
> / Jonas

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

[smaug]

> hacks, I suppose).
>


I've spent quite some time to reduce extra addref/release calls in DOM code, and
at least there extra refcnt traffic is definitely no-no and won't be accepted in any
hot code paths (which many DOM code paths tend to be).
So by default the coding style / convention should reduce the number of addref/release calls and make it clear
when addref/release actually happen.
(returning nsCOMPtr/nsRefPtr makes this all less clear.)

And I haven't seen any proposal here which would "preserve developer time" vs. the current coding style
where already_AddRef is used for return values and raw pointers for params.



-Olli

[smaug]

Also, "developer time" includes also reviewer's time, and in general coding style and conventions should be
primarily for easy-to-read, not easy-to-write.

[Aryeh Gregor]

On Mon, Dec 22, 2014 at 11:10 PM, Jeff Muizelaar <jmuiz...@mozilla.com> wrote:
> Possible solutions would be to:
> - remove implicit conversions to T*

If this were done, I think we should change the calling convention for
functions that take pointers to refcounted classes. The convention is
broken anyway: the convention is supposed to be that the caller holds
a strong reference, but the parameter type is a raw pointer, which
does not enforce this. This has led to at least one sec-critical that
I know of, IIRC, where the caller did not hold a strong reference
locally, and the callee wound up freeing it. There have probably been
lots more that I don't know of.

I've thought in the past of a RefParam<T> type, which would be for use
only as a function parameter. nsRefPtr<T>/nsCOMPtr<T> would
implicitly convert to it, but raw pointers would not implicitly
convert to it. And it would implicitly convert to a raw pointer,
which is safe as long as the nsRefPtr<T>/nsCOMPtr<T> that it was
initialized with was a local variable (and not a member or global).
Thus to the callee, it would behave exactly like a raw pointer. The
only difference is the caller is not allowed to pass a raw pointer.

With this change, we wouldn't need to convert from nsRefPtr<T> to T*
often, as far as I can think of. It would also preserve binary
compatibility with XPCOM AFAICT, because RefParam<T> would have
trivial constructor and destructor and no virtual functions. It also
would add no addref/release. It would just help the compiler catch
cases where raw pointers are being passed to functions that expect the
caller to hold a strong reference, which would perhaps allow us to
sanely remove the implicit conversion from nsRefPtr<T> to T*.

> - add operator T* explicit and operator T* && = delete // this will be available in GCC 4.8.1 and MSVC 2014 Nov CTP

What would this do? I see that deleting operator T* && would prevent
a temporary nsRefPtr<T> from converting unsafely to a raw pointer,
which would address that objection to removing already_AddRefed. But
what does the first part do?

On Tue, Dec 23, 2014 at 1:21 AM, Ehsan Akhgari <ehsan....@gmail.com> wrote:
> Are there good use cases for having functions accept an nsRefPtr<T>&? If
> not, we can outlaw them.

Do we have a better convention for an in/out parameter that's a
pointer to a refcounted class? editor uses this convention in a
number of functions for "pass me a node/pointer pair as input, and
I'll update it to the new value while I'm at it." If there's a good
substitute for this, let me know and I'll use it in the future when
cleaning up editor code.

[smaug]

How would this setup help with the case when one passes
nsCOMPtr/nsRefPtr member variable as a param? I believe that has been the most common issue
with "caller-should-keep-the-parameter-alive" - one just doesn't remember to make sure the
value of the member variable can't be replaced with some other value while calling the method.

>
>> - add operator T* explicit and operator T* && = delete // this will be available in GCC 4.8.1 and MSVC 2014 Nov CTP
>
> What would this do? I see that deleting operator T* && would prevent
> a temporary nsRefPtr<T> from converting unsafely to a raw pointer,
> which would address that objection to removing already_AddRefed. But
> what does the first part do?
>
> On Tue, Dec 23, 2014 at 1:21 AM, Ehsan Akhgari <ehsan....@gmail.com> wrote:
>> Are there good use cases for having functions accept an nsRefPtr<T>&? If
>> not, we can outlaw them.
>
> Do we have a better convention for an in/out parameter that's a
> pointer to a refcounted class? editor uses this convention in a
> number of functions for "pass me a node/pointer pair as input, and
> I'll update it to the new value while I'm at it." If there's a good
> substitute for this, let me know and I'll use it in the future when
> cleaning up editor code.

nsCOM/RefPtr<>& is a good option for inouts.
(Tough one should avoid inouts when possible.)

[Karl Tomlinson]

> On Tue, Dec 23, 2014 at 1:21 AM, Ehsan Akhgari <ehsan....@gmail.com> wrote:
>> Are there good use cases for having functions accept an
>> nsRefPtr<T>&? If not, we can outlaw them.

Aryeh Gregor writes:

> Do we have a better convention for an in/out parameter that's a
> pointer to a refcounted class? editor uses this convention in a
> number of functions for "pass me a node/pointer pair as input, and
> I'll update it to the new value while I'm at it." If there's a good
> substitute for this, let me know and I'll use it in the future when
> cleaning up editor code.

nsRefPtr<T>*

[Aryeh Gregor]

On Sat, Dec 27, 2014 at 5:54 AM, Karl Tomlinson <moz...@karlt.net> wrote:
> Aryeh Gregor writes:
>
>> Do we have a better convention for an in/out parameter that's a
>> pointer to a refcounted class? editor uses this convention in a
>> number of functions for "pass me a node/pointer pair as input, and
>> I'll update it to the new value while I'm at it." If there's a good
>> substitute for this, let me know and I'll use it in the future when
>> cleaning up editor code.
>
> nsRefPtr<T>*

The advantage being that it's clear at the call site that the
parameter can be modified by the caller, right? But on the other
hand, the compiler won't help you catch when you're passing a null
parameter.

[Aryeh Gregor]

On Sat, Dec 27, 2014 at 12:34 AM, smaug <sm...@welho.com> wrote:
> How would this setup help with the case when one passes
> nsCOMPtr/nsRefPtr member variable as a param? I believe that has been the
> most common issue
> with "caller-should-keep-the-parameter-alive" - one just doesn't remember to
> make sure the
> value of the member variable can't be replaced with some other value while
> calling the method.

It wouldn't. I didn't go into it, but I did think of this, and I
think that the way to handle it would be to define

template<class T> class RefMember<T> : public nsRefPtr<T> {}

and delete the conversion from RefMember<T> to RefParam<T>, then use
static analysis to enforce that nsRefPtr/nsCOMPtr as a member variable
is a compiler error. Of course we'd also need a way to manually avoid
the addref/release in cases where the caller cares about perf and
knows it's safe, maybe a helper function like StrongRefAlreadyHeld(),
which should be a ref flag in reviews unless it's really necessary.

Also we should have a RawParam<T> that behaves like a raw pointer, but
accepts implicit conversions from any pointer type, strong or not.
This would be used for functions that care about perf and are
absolutely certain they will not release the parameter across the call
(e.g., nsINode::Contains). This is needed because nsRefPtr<T> won't
convert to T*, and T* won't convert to RefParam<T>, but functions like
this want to accept either T* or nsRefPtr<T>, so they need a new type.

> nsCOM/RefPtr<>& is a good option for inouts.
> (Tough one should avoid inouts when possible.)

There are a significant number of functions in editor where I don't
see a nicer solution than in/out parameters. (But that's not to say I
disagree with you -- one should avoid editor when possible. :) )

[Daniel Holbert]

On 08/12/2014 07:59 AM, Benjamin Smedberg wrote:
> But now that nsCOMPtr/nsRefPtr support proper move constructors, [...]
> Could we replace every already_AddRefed return value with a nsCOMPtr?

I have a variant of bsmedberg's original question here. He asked about
return values, but I'm wondering:
Could we replace every already_AddRefed *function-parameter* with a move
reference to a RefPtr/nsCOMPtr?

(Sorry if this was answered in this thread; I skimmed through it &
didn't find a definitive answer immediately. It seems like this might be
a strictly easier thing to fix up, as compared to return values.)

As a concrete example, at this usage site...
https://dxr.mozilla.org/mozilla-central/rev/1851b78b5a9673ee422f189b92e5f1e86b82a01c/dom/base/nsDOMMutationObserver.h#468
...is there any benefit to us using
already_AddRefed<nsPIDOMWindowInner>&& aOwner, instead of
nsCOMPtr<nsPIDOMWindowInner>&&?

(I believe we have the "already_AddRefed as a parameter" pattern in our
code in order to avoid an extra addref/release when ownership is being
transferred into a function. But, RefPtr/nsCOMPtr with move references
would let us do that just as well, more concisely & less arcanely.)

Thanks,
~Daniel

[Boris Zbarsky]

On 9/12/16 1:53 PM, Daniel Holbert wrote:
> (I believe we have the "already_AddRefed as a parameter" pattern in our
> code in order to avoid an extra addref/release when ownership is being
> transferred into a function.

And assertions if the function does not actually take the reference...

> But, RefPtr/nsCOMPtr with move references
> would let us do that just as well

Not the assertions part, right?

-Boris

[Kyle Huey]

On Mon, Sep 12, 2016 at 10:53 AM, Daniel Holbert <dhol...@mozilla.com> wrote:
> On 08/12/2014 07:59 AM, Benjamin Smedberg wrote:
>> But now that nsCOMPtr/nsRefPtr support proper move constructors, [...]
>> Could we replace every already_AddRefed return value with a nsCOMPtr?
>
> I have a variant of bsmedberg's original question here. He asked about
> return values, but I'm wondering:
> Could we replace every already_AddRefed *function-parameter* with a move
> reference to a RefPtr/nsCOMPtr?
>
> (Sorry if this was answered in this thread; I skimmed through it &
> didn't find a definitive answer immediately. It seems like this might be
> a strictly easier thing to fix up, as compared to return values.)
>
> As a concrete example, at this usage site...
> https://dxr.mozilla.org/mozilla-central/rev/1851b78b5a9673ee422f189b92e5f1e86b82a01c/dom/base/nsDOMMutationObserver.h#468
> ...is there any benefit to us using
> already_AddRefed<nsPIDOMWindowInner>&& aOwner, instead of
> nsCOMPtr<nsPIDOMWindowInner>&&?
>

> (I believe we have the "already_AddRefed as a parameter" pattern in our
> code in order to avoid an extra addref/release when ownership is being

> transferred into a function. But, RefPtr/nsCOMPtr with move references
> would let us do that just as well, more concisely & less arcanely.)

There's an important semantic difference: something that takes a move
reference to a smart pointer is not obligated to accept the reference.
If I do smartptr.forget() I know afterwards that smartptr is empty and
the reference that used to exist in it is Not My Problem (TM), whereas
if I call something with a move reference to smartptr I may have to
handle the error case, depending on what the callee does.

Those sorts of non-local effects, IMHO, make code harder to read.

- Kyle

[Daniel Holbert]

Right -- this sort of code with Move() and a RefPtr<>&& param would be
missing those assertions.

I don't see that as a *huge* downfall, though. Those assertions have a
few benefits for already_AddRefed:
(1) They guard against leaks from unused already_AddRefed.
(2) They guard against logic errors where we should've taken the value
but forgot to do so. (And it enforces that callees must take the value,
even if they have some error handling cases where they'd prefer not to.)

So I think (1) is irrelevant here, because with Move() the object will
always be stored in a refcounted pointer of some sort. I think (2) is
semi-relevant. It brings up one potential concern with Move: since the
callee might not take the value (intentionally or unintentionally), the
caller must *refrain* from caring about the state of its Move()'d
variable, between the Move() operation and any reassignment/cleanup.
(This seems like something that static analysis could check for us &
could guarantee is a non-issue. And it seems like a standard part of the
Move bargain that reviewers should know to watch for, as soon as they're
familiar with Move.)

I guess my bottom-line question is: which pattern should I prefer, for
new code that wants to trivially transfer ownership of a refcounted
object into a function or a constructor?

(In my current case, I'm looking at a constructor much like the
nsDOMMutationObserver example that I linked to, with a parameter that
gets directly assigned in an init list -- so, there's no actual risk of
logic preventing it from getting taken.)

I'm OK with continuing to prefer already_AddRefed, if I should; though I
was initially hoping that this is one spot where we could get rid of it
(with the upsides being brevity, consistency in type-of-smart-pointer,
and fewer mozilla-specific quirks).

Thanks,
~Daniel

[Boris Zbarsky]

On 9/12/16 3:18 PM, Daniel Holbert wrote:
> (1) They guard against leaks from unused already_AddRefed.
> (2) They guard against logic errors where we should've taken the value
> but forgot to do so. (And it enforces that callees must take the value,
> even if they have some error handling cases where they'd prefer not to.)

Right.

> So I think (1) is irrelevant here

I agree.

> It brings up one potential concern with Move: since the
> callee might not take the value (intentionally or unintentionally), the
> caller must *refrain* from caring about the state of its Move()'d
> variable, between the Move() operation and any reassignment/cleanup.

It's worse than that. For a wide range of callers (anyone handing the
ref across threads), the caller must check immediately whether the
callee actually took the value and if not make sure things are released
on the proper thread...

> I guess my bottom-line question is: which pattern should I prefer, for
> new code that wants to trivially transfer ownership of a refcounted
> object into a function or a constructor?

Are threads involved? ;)

My gut feeling is that we should use the safer pattern with
already_AddRefed just so people don't start using the other pattern in
anything resembling multithreaded code, though in your particular case
both patterns would be fine...

-Boris

[Daniel Holbert]

On 09/12/2016 12:22 PM, Boris Zbarsky wrote:
> It's worse than that. For a wide range of callers (anyone handing the
> ref across threads), the caller must check immediately whether the
> callee actually took the value and if not make sure things are released
> on the proper thread...

Ah, ok; I can see that complicating the logic in the caller
considerably, if the caller really had to worry about that.

Seems like we should enforce that callees must do the cleanup in that
sort of case (and already_AddRefed does indeed enforce that, which is
nice).

>> I guess my bottom-line question is: which pattern should I prefer, for
>> new code that wants to trivially transfer ownership of a refcounted
>> object into a function or a constructor?
>
> Are threads involved? ;)

Nope!

> My gut feeling is that we should use the safer pattern with
> already_AddRefed just so people don't start using the other pattern in
> anything resembling multithreaded code, though in your particular case
> both patterns would be fine...

OK, that's reasonable -- thanks.

~Daniel

P.S. As I realized when replying to khuey -- part of my motivation here
was for consistency with UniquePtr, where the
documentation/best-practices explicitly call for using Move() and
UniquePtr&& to transfer ownership. It seems like it'd be nice if our
other smart pointer types could have consistent best-practices for
similar operations. But, maybe that's asking too much, in the presence
of these thread complications. :)

[Jim Blandy]

On Mon, Sep 12, 2016 at 12:22 PM, Boris Zbarsky <bzba...@mit.edu> wrote:

It brings up one potential concern with Move: since the
>
>> callee might not take the value (intentionally or unintentionally), the
>> caller must *refrain* from caring about the state of its Move()'d
>> variable, between the Move() operation and any reassignment/cleanup.
>>
>
> It's worse than that. For a wide range of callers (anyone handing the ref
> across threads), the caller must check immediately whether the callee
> actually took the value and if not make sure things are released on the
> proper thread...
>

Could you go into more detail here? Either the caller will free it, or the
callee will free it --- but they're both on the same thread. If the callee
puts the reference someplace where another thread will free it, then that's
the operation that needs to be audited.

[Boris Zbarsky]

On 9/12/16 3:40 PM, Jim Blandy wrote:
> Could you go into more detail here? Either the caller will free it, or the
> callee will free it --- but they're both on the same thread.

We have this pretty common pattern for handing references around threads
safely:

Main thread:

RefPtr<SomeRunnable> runnable = new SomeRunnable(myRef.forget());
someTarget->Dispatch(runnable.forget());

Target thread, runnable's Run method:

RefPtr<ResponseRunnable> runnable = new
ResponseRunnable(mMyRef.forget());
originalThread->Dispatch(runnable.forget());

And then back on the original thread the thing in myRef is worked with
again. The idea is to keep that thing alive without ever refcounting it
on the target thread, because for example it may not have a threadsafe
refcount implementation.

> If the callee
> puts the reference someplace where another thread will free it, then that's
> the operation that needs to be audited.

Right, this all assumes that the Dispatch() calls above are infallible.
Which they are, iirc.

-Boris

[Jim Blandy]

So, in this pattern, if the target thread's execution of SomeRunnable::Run
could fail, causing ~SomeRunnable to call mMyRef's destructor and thus free
the referent on the target thread, that would be a bug?