Re: Getting rid of already_AddRefed?

[Boris Zbarsky]

On 9/12/16 4:21 PM, Jim Blandy wrote:

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?


Yes, it would.

-Boris
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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?

On Mon, Sep 12, 2016 at 12:47 PM, Boris Zbarsky  wrote:

> 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 runnable = new SomeRunnable(myRef.forget());
>   someTarget->Dispatch(runnable.forget());
>
> Target thread, runnable's Run method:
>
>   RefPtr 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
>
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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 runnable = new SomeRunnable(myRef.forget());
  someTarget->Dispatch(runnable.forget());

Target thread, runnable's Run method:

  RefPtr 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

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[Jim Blandy]

On Mon, Sep 12, 2016 at 12:22 PM, Boris Zbarsky  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.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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. :)
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[Daniel Holbert]

On 09/12/2016 11:00 AM, Boris Zbarsky wrote:
> 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?

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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[Kyle Huey]

On Mon, Sep 12, 2016 at 10:53 AM, Daniel Holbert  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&& aOwner, instead of
> nsCOMPtr&&?
>
> (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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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&& aOwner, instead of
nsCOMPtr&&?

(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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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

  templateclass T class RefMemberT : public nsRefPtrT {}

and delete the conversion from RefMemberT to RefParamT, 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 RawParamT 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 nsRefPtrT won't
convert to T*, and T* won't convert to RefParamT, but functions like
this want to accept either T* or nsRefPtrT, 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.  :) )
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[Aryeh Gregor]

On Sat, Dec 27, 2014 at 5:54 AM, Karl Tomlinson mozn...@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.

 nsRefPtrT*

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.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[Aryeh Gregor]

On Mon, Dec 22, 2014 at 11:10 PM, Jeff Muizelaar jmuizel...@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 RefParamT type, which would be for use
only as a function parameter.  nsRefPtrT/nsCOMPtrT 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 nsRefPtrT/nsCOMPtrT 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 nsRefPtrT to T*
often, as far as I can think of.  It would also preserve binary
compatibility with XPCOM AFAICT, because RefParamT 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 nsRefPtrT 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 nsRefPtrT 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.akhg...@gmail.com wrote:
 Are there good use cases for having functions accept an nsRefPtrT? 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.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[smaug]

On 12/26/2014 03:08 PM, Aryeh Gregor wrote:

On Mon, Dec 22, 2014 at 11:10 PM, Jeff Muizelaar jmuizel...@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 RefParamT type, which would be for use
only as a function parameter.  nsRefPtrT/nsCOMPtrT 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 nsRefPtrT/nsCOMPtrT 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 nsRefPtrT to T*
often, as far as I can think of.  It would also preserve binary
compatibility with XPCOM AFAICT, because RefParamT 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 nsRefPtrT to T*.



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 nsRefPtrT 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.akhg...@gmail.com wrote:

Are there good use cases for having functions accept an nsRefPtrT? 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.)

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[Karl Tomlinson]

 On Tue, Dec 23, 2014 at 1:21 AM, Ehsan Akhgari ehsan.akhg...@gmail.com 
 wrote:
 Are there good use cases for having functions accept an
 nsRefPtrT? 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.

nsRefPtrT*
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[Ehsan Akhgari]

On 2014-12-23 2:52 AM, Ms2ger wrote:

-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.


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


___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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 nsRefPtrT 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
 nsRefPtrT? 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 RefPtrT? or const UniquePtrT. 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...


___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[Ehsan Akhgari]

On 2014-12-23 9:47 AM, Ehsan Akhgari wrote:

On 2014-12-23 2:52 AM, Ms2ger wrote:

-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.


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.


___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[Eric Rescorla]

On Tue, Dec 23, 2014 at 6:48 AM, Ehsan Akhgari ehsan.akhg...@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 nsRefPtrT 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
  nsRefPtrT? 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 RefPtrT? or const UniquePtrT. 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 SmartPtrT 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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[Ehsan Akhgari]

On 2014-12-23 10:38 AM, Eric Rescorla wrote:



On Tue, Dec 23, 2014 at 6:48 AM, Ehsan Akhgari ehsan.akhg...@gmail.com
mailto:ehsan.akhg...@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
mailto: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 nsRefPtrT 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
  nsRefPtrT? 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 RefPtrT? or const UniquePtrT. 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 SmartPtrT 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.


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


___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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 nsRefPtrDerived), 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.

-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)


signature.asc
Description: Digital signature
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[Ehsan Akhgari]

On 2014-12-23 11:36 AM, Eric Rescorla wrote:



On Tue, Dec 23, 2014 at 8:32 AM, Ehsan Akhgari ehsan.akhg...@gmail.com
mailto:ehsan.akhg...@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.akhg...@gmail.com mailto:ehsan.akhg...@gmail.com
mailto:ehsan.akhgari@gmail.__com
mailto:ehsan.akhg...@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
mailto:dba...@dbaron.org mailto:dba...@dbaron.org
 mailto:dba...@dbaron.org mailto:dba...@dbaron.org
mailto: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 nsRefPtrT 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
   nsRefPtrT? 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 RefPtrT? or const
UniquePtrT. 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 SmartPtrT 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.


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.)

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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 nsRefPtrDerived), 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=RefPtrcase=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 RefPtrBase b) {
}

void base_ptr(Base* b) {
}

void foo() {
  RefPtrBase base;
  RefPtrDerived derived;

  base_ref(base);
  base_ref(derived);

  base_ptr(base);
  base_ptr(derived)
}
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[Eric Rescorla]

On Tue, Dec 23, 2014 at 8:53 AM, Ehsan Akhgari ehsan.akhg...@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.akhg...@gmail.com
 mailto:ehsan.akhg...@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.akhg...@gmail.com mailto:ehsan.akhg...@gmail.com
 mailto:ehsan.akhgari@gmail.__com

 mailto:ehsan.akhg...@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
 mailto:dba...@dbaron.org mailto:dba...@dbaron.org
  mailto:dba...@dbaron.org mailto:dba...@dbaron.org
 mailto: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 nsRefPtrT 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
nsRefPtrT? 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 RefPtrT? or const
 UniquePtrT. 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 SmartPtrT 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.


 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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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=RefPtrcase=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!)

-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)


signature.asc
Description: Digital signature
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[Ehsan Akhgari]

On 2014-12-23 12:51 PM, Eric Rescorla wrote:



On Tue, Dec 23, 2014 at 8:53 AM, Ehsan Akhgari ehsan.akhg...@gmail.com
mailto:ehsan.akhg...@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.akhg...@gmail.com mailto:ehsan.akhg...@gmail.com
mailto:ehsan.akhgari@gmail.__com
mailto:ehsan.akhg...@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.akhg...@gmail.com
mailto:ehsan.akhg...@gmail.com
mailto:ehsan.akhgari@gmail.__com mailto:ehsan.akhg...@gmail.com
 mailto:ehsan.akhgari@gmail.
mailto:ehsan.akhgari@gmail.com

 mailto:ehsan.akhgari@gmail.__com
mailto:ehsan.akhg...@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
mailto:dba...@dbaron.org mailto:dba...@dbaron.org
 mailto:dba...@dbaron.org mailto:dba...@dbaron.org
mailto:dba...@dbaron.org mailto:dba...@dbaron.org
  mailto:dba...@dbaron.org
mailto:dba...@dbaron.org mailto:dba...@dbaron.org
mailto:dba...@dbaron.org
 mailto:dba...@dbaron.org mailto:dba...@dbaron.org
mailto: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 nsRefPtrT
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
nsRefPtrT? 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 RefPtrT? or const
 UniquePtrT. 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 SmartPtrT 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.


 Why not pass the raw pointer to the function?


My general theory is that smart pointers, once boxed, should
never be
unboxed.
The 

Re: Getting rid of already_AddRefed?

[L. David Baron]

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.

-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)


signature.asc
Description: Digital signature
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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 nsRefPtrDerived), 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 ArgumentPtrT (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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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 SmartPtrDerived to SmartPtrBase 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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[Jonas Sicking]

On Tue, Dec 23, 2014 at 1:03 PM, Jeff Walden jwalden+...@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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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 SmartPtrDerived to SmartPtrBase 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.

-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)


signature.asc
Description: Digital signature
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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).
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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 UniquePtrT when I can, and for obvious reasons,
  UniquePtrT doesn't do implicit conversion.

- I generally don't do a lot of passing around of SmartPtrBase,
  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 SmartPtrDerived to SmartPtrBase 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 RefPtrDerived to something that takes RefPtrBase,
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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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.

-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)


signature.asc
Description: Digital signature
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[Ehsan Akhgari]

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 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.


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.)


___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[Ehsan Akhgari]

On 2014-12-23 5:30 PM, Eric Rescorla wrote:

On Tue, Dec 23, 2014 at 2:19 PM, Ehsan Akhgari ehsan.akhg...@gmail.com
mailto:ehsan.akhg...@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
mailto:dba...@dbaron.org mailto: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.


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.


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.


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.

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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:

nsRefPtrT 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.akhg...@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 n...@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-platform@lists.mozilla.org
 https://lists.mozilla.org/listinfo/dev-platform

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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:

nsRefPtrT 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.akhg...@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 n...@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-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform




___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[Eric Rescorla]

On Mon, Dec 22, 2014 at 1:12 PM, Ehsan Akhgari ehsan.akhg...@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:

 nsRefPtrT 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.,

  nsRefPtrT 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.akhg...@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 n...@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-platform@lists.mozilla.org
 https://lists.mozilla.org/listinfo/dev-platform



 ___
 dev-platform mailing list
 dev-platform@lists.mozilla.org
 https://lists.mozilla.org/listinfo/dev-platform

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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:
 
 nsRefPtrT 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)


signature.asc
Description: Digital signature
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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:

nsRefPtrT 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.  :-)

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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 nsRefPtrT 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

-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)


signature.asc
Description: Digital signature
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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 nsRefPtrT 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 nsRefPtrT? 
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.

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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 nsRefPtrT 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
 nsRefPtrT? 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.

-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)


signature.asc
Description: Digital signature
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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 nsRefPtrT 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
nsRefPtrT? 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.  :-)
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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 nsRefPtrT 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
  nsRefPtrT? 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 RefPtrT? or const UniquePtrT. 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)

 ___
 dev-platform mailing list
 dev-platform@lists.mozilla.org
 https://lists.mozilla.org/listinfo/dev-platform


___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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-
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[Neil]

Aryeh Gregor wrote:


for instance, here's a real-world bit of code from nsWSRunObject:

 if ((aRun-mRightType  WSType::block) 
 IsBlockNode(nsCOMPtrnsINode(GetWSBoundingParent( {

GetWSBoundingParent() returns an already_AddRefednsINode

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;
}

--
Warning: May contain traces of nuts.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[Aryeh Gregor]

On Thu, Aug 14, 2014 at 12:00 PM, Neil n...@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).
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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 n...@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-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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_AddRefedT 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 nsCOMPtrT 
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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[Neil]

Karl Tomlinson wrote:


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?
 


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


--
Warning: May contain traces of nuts.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[Aryeh Gregor]

On Wed, Aug 13, 2014 at 9:42 AM, Jeff Walden jwalden+...@mit.edu wrote:
 So at risk of adding yet another flavor of thing: why not introduce an 
 already_AddRefedT 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 nsCOMPtrT 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?
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[smaug]

On 08/12/2014 06:23 PM, Aryeh Gregor wrote:

On Tue, Aug 12, 2014 at 6:16 PM, Benoit Jacob jacob.benoi...@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.)




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

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[Aryeh Gregor]

On Wed, Aug 13, 2014 at 4:11 PM, Ehsan Akhgari ehsan.akhg...@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.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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.akhg...@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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[Aryeh Gregor]

On Wed, Aug 13, 2014 at 5:44 PM, Ehsan Akhgari ehsan.akhg...@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(nsCOMPtrnsINode(GetWSBoundingParent( {

GetWSBoundingParent() returns an already_AddRefednsINode, 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(nsCOMPtrnsIAtom(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?
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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_AddRefedT* 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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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(nsCOMPtrnsINode(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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[Ehsan Akhgari]

On 2014-08-13, 12:24 PM, Aryeh Gregor wrote:

On Wed, Aug 13, 2014 at 5:44 PM, Ehsan Akhgari ehsan.akhg...@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(nsCOMPtrnsINode(GetWSBoundingParent( {

GetWSBoundingParent() returns an already_AddRefednsINode, 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(nsCOMPtrnsIAtom(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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[smaug]

On 08/13/2014 07:24 PM, Aryeh Gregor wrote:

On Wed, Aug 13, 2014 at 5:44 PM, Ehsan Akhgari ehsan.akhg...@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(nsCOMPtrnsINode(GetWSBoundingParent( {

GetWSBoundingParent() returns an already_AddRefednsINode, 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



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(nsCOMPtrnsIAtom(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?



___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[Aryeh Gregor]

On Tue, Aug 12, 2014 at 5:59 PM, Benjamin Smedberg
benja...@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).
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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)


signature.asc
Description: Digital signature
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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 benja...@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-platform@lists.mozilla.org
 https://lists.mozilla.org/listinfo/dev-platform

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[Aryeh Gregor]

On Tue, Aug 12, 2014 at 6:16 PM, Benoit Jacob jacob.benoi...@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.)
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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?

-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)


signature.asc
Description: Digital signature
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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*.)

-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)


signature.asc
Description: Digital signature
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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_AddRefedT 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.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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_AddRefedT 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_AddRefedT to ensure the caller takes ownership of
that reference.

-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)


signature.asc
Description: Digital signature
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[Joshua Cranmer ]

On 8/12/2014 9:59 AM, 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.


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. I've pointed out before that we could fix this by adding 
a nsCOMPtrT::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

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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?
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[Kyle Huey]

On Tue, Aug 12, 2014 at 9: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.
 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?
 ___
 dev-platform mailing list
 dev-platform@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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[Aryeh Gregor]

On Tue, Aug 12, 2014 at 7:12 PM, Vladimir Vukicevic vladi...@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
RawPointerT 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 RawPointerT is used
instead of T*, unless we can get static analysis to do it.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[Aryeh Gregor]

On Tue, Aug 12, 2014 at 7:37 PM, Benjamin Smedberg
benja...@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.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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
Thunderbird and DXR developer
Source code archæologist

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[Joshua Cranmer ]

On 8/12/2014 11:40 AM, Aryeh Gregor wrote:

On Tue, Aug 12, 2014 at 7:37 PM, Benjamin Smedberg
benja...@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.

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

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

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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.


___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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
   nsCOMPtrnsIAtom atom = GetAtomValue()
   return atom.forget();
Can this be rewritten to
   return nsCOMPtrnsIAtom(GetAtomValue()).forget();
Or better still, if this counts as an rvalue, can we define a move 
constructor from nsCOMPtrT and write

   return nsCOMPtrnsIAtom(GetAtomValue());

--
Warning: May contain traces of nuts.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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 ofolo!o’o owoiololo oboeo oiono odoaonogoeoro
ooofo
otohoeo ofoioroeo ooofo ohoeololo.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Getting rid of already_AddRefed?

[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?
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform