1188502 - Generic auto-nulled type for refcounted outparams

Status: NEW

(Core :: XPCOM, enhancement)

Description

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

I just spent a whole bunch of time tracking down a bug in a patch that turned out it was just me assigning to an nsIContent** outparam without addrefing, i.e.,

  *aOutParam = somePointer;

The fun thing about this is it will show up errors when it crashes that cannot easily be tracked down to the problematic change (at least that I know of).  So I had to split apart the patch into tiny little pieces until I tracked down the part that actually caused the failure, and started rewriting it, until it finally dawned on me.

I suggest a class like the following (not compile-tested):

  template<typename T>
  class RefOutParam
  {
    T** mPtr;

  public:
    RefOutParam(T** aPtr)
      : mPtr(aPtr)
    {
      *mPtr = nullptr;
    }

    operator=(already_AddRefed<T> aPtr)
    {
      *mPtr = aPtr.take();
    }
  };

Actually, I would prefer to have it take nsRefPtr<T>*/nsCOMPtr<T>*/OwningNonNull<T>* instead of T**, so there's type safety in the constructor too.  But I couldn't figure out how without having a member variable for each possible input type.  Wouldn't it be nice if all our standard smart pointer classes had a common base class?

What do you think, Ehsan?

Comment 1

[Benjamin Smedberg]

Would you be using this for XPCOM outparams as well? Can we guarantee the binary compatibility needed for that with xptcall?

Comment 2

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

That would be ideal.  I would at least like it to work for non-XPCOM outparams.  (Unless there's an existing better convention for non-XPCOM outparams that I'm not using because I don't know about it.)

Comment 3

[(no longer active)]

This is a nice idea, even if it means that we won't use it for XPCOM outparams for now.

I think you should be able to special case RefOutParam based on the smart pointer type, like this (this is just a sketch):

template <class SmartPtr>
class RefOutParam
{
  SmartPtr& mPtr;
  static_assert(detail::IsRefPtr<SmartPtr>::value, "Can only accept smart pointer types");
  // ...
};

Creating IsRefPtr should be pretty simple by partial specializing for our smart pointer types.

I definitely think we should not deal with T** here.

Comment 4

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #3)
> I think you should be able to special case RefOutParam based on the smart
> pointer type, like this (this is just a sketch):
> 
> template <class SmartPtr>
> class RefOutParam
> {
>   SmartPtr& mPtr;
>   static_assert(detail::IsRefPtr<SmartPtr>::value, "Can only accept smart
> pointer types");
>   // ...
> };

So you'd have to declare the function based on what type of smart pointer it required the outparam to be, like

  void MyFunction(nsINode& aNode, RefOutParam<nsCOMPtr<nsINode>> aOutNode);

?  That doesn't seem very nice, but it would work, I guess.

Comment 5

[(no longer active)]

(In reply to :Aryeh Gregor (working until August 13) from comment #4)
> (In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail,
> needinfo? me!) from comment #3)
> > I think you should be able to special case RefOutParam based on the smart
> > pointer type, like this (this is just a sketch):
> > 
> > template <class SmartPtr>
> > class RefOutParam
> > {
> >   SmartPtr& mPtr;
> >   static_assert(detail::IsRefPtr<SmartPtr>::value, "Can only accept smart
> > pointer types");
> >   // ...
> > };
> 
> So you'd have to declare the function based on what type of smart pointer it
> required the outparam to be, like
> 
>   void MyFunction(nsINode& aNode, RefOutParam<nsCOMPtr<nsINode>> aOutNode);
> 
> ?  That doesn't seem very nice, but it would work, I guess.

That is a good point, and I agree, that's terrible.

How about we make the constructor templated on the type of the smartptr, and the RefOutParam type templated on T itself?

Comment 6

[Nathan Froyd [:froydnj]]

mfbt/RefPtr.h has some prior art in this area, FWIW:

http://mxr.mozilla.org/mozilla-central/source/mfbt/RefPtr.h#312

Comment 7

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to :Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #5)
> How about we make the constructor templated on the type of the smartptr, and
> the RefOutParam type templated on T itself?

How would mPtr be declared?  The only way I could see is having separate member variables for each type of smart pointer, and leaving all but one null.  Or, making it a union, and having a separate enum member to store the type.  Which are both fairly horrible.

Unless we add a base class for all our existing smart pointer types, which probably is not practical unless either the classes are significantly refactored, or compilers are reliably smart about optimizing virtual methods on final classes.  (In the latter case we could make the existing classes final, and make methods like operator= virtual, and it wouldn't affect existing users of the derived classes.)

We could just bite the bullet, and use T** for the member.  Then we'd make our new class a friend of all the smart pointer classes, have the constructors steal the pointer from the source pointer, and have our class do the necessary AddRef/Release.  This is basically what getter_AddRefs does, right?  But it doesn't sound like a great idea.

Comment 8

[(no longer active)]

(In reply to :Aryeh Gregor (working until August 13) from comment #7)
> (In reply to :Ehsan Akhgari (not reviewing patches, not reading bugmail,
> needinfo? me!) from comment #5)
> > How about we make the constructor templated on the type of the smartptr, and
> > the RefOutParam type templated on T itself?
> 
> How would mPtr be declared?  The only way I could see is having separate
> member variables for each type of smart pointer, and leaving all but one
> null.  Or, making it a union, and having a separate enum member to store the
> type.  Which are both fairly horrible.

You can use MaybeOneOf (you may need to extend it to support more than two types, which should be straightforward.)

Comment 9

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

While we're at it, can you think of any way of passing a pointer to a base class?  So if a function returns nsIContent, you can pass it an nsCOMPtr<nsINode>?  (Given that we prohibit reads from the out param and only allow writes.)  I thought about it but don't see any automatic way to do it, because the caller would need to generate the proper static_casts.  But maybe I'm missing something.

Comment 10

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to :Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #8)
> You can use MaybeOneOf (you may need to extend it to support more than two
> types, which should be straightforward.)

I looked at this a bit -- you mean "straightforward using variadic templates", or did you have another way in mind?  Because variadic templates don't look straightforward to me!

Comment 11

[Nathan Froyd [:froydnj]]

(In reply to :Aryeh Gregor (working until August 13) from comment #10)
> (In reply to :Ehsan Akhgari (not reviewing patches, not reading bugmail,
> needinfo? me!) from comment #8)
> > You can use MaybeOneOf (you may need to extend it to support more than two
> > types, which should be straightforward.)
> 
> I looked at this a bit -- you mean "straightforward using variadic
> templates", or did you have another way in mind?  Because variadic templates
> don't look straightforward to me!

mfbt/Variant.h may provide a way forward for you.

Comment 12

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #11)
> mfbt/Variant.h may provide a way forward for you.

I just tried using that, and hit a snag: I need to write code like

  if (mPtr.template is<nsCOMPtr<T>*>()) {
    *mPtr.template as<nsCOMPtr<T>*>() = aOther;
  }

but this fails to compile if T is, say, mozilla::dom::Text.  The code is unreachable, but the compiler doesn't know that.

Do you know of a magical template way to avoid this problem?  Or (preferably from my perspective) would it be okay if I just went ahead and implemented it using a void* for the member and had the class track its current state manually?  The latter would be a lot simpler and easier to read, and not dangerous at all given the simplicity of the code here.  I could put in asserts to make it look safer if you like.  :)

Comment 13

[Nathan Froyd [:froydnj]]

(In reply to :Aryeh Gregor (working until August 13) from comment #12)
> (In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #11)
> > mfbt/Variant.h may provide a way forward for you.
> 
> I just tried using that, and hit a snag: I need to write code like
> 
>   if (mPtr.template is<nsCOMPtr<T>*>()) {
>     *mPtr.template as<nsCOMPtr<T>*>() = aOther;
>   }
> 
> but this fails to compile if T is, say, mozilla::dom::Text.  The code is
> unreachable, but the compiler doesn't know that.
> 
> Do you know of a magical template way to avoid this problem?

I am unclear what the problem is, beyond "the compiler complains".  Could you provide some more context?

> Or (preferably
> from my perspective) would it be okay if I just went ahead and implemented
> it using a void* for the member and had the class track its current state
> manually?  The latter would be a lot simpler and easier to read, and not
> dangerous at all given the simplicity of the code here.  I could put in
> asserts to make it look safer if you like.  :)

What are we writing here, C? ;)

I would prefer to not use void* or similar unless we absolutely had to.  But I'd need a better handle on the problem before I can make a real decision.

Comment 14

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #13)
> I am unclear what the problem is, beyond "the compiler complains".  Could
> you provide some more context?

nsCOMPtr<Text> doesn't compile, because Text is not an XPCOM class -- doesn't have the right ID or whatever (I have no idea how this stuff works).  Instantiating an nsCOMPtr for such classes will fail.  You have to use nsRefPtr.

The lazy way out would be to make a separate COMOutParam class.  Otherwise, we'd probably need scary template magic to get it to not include nsCOMPtr in the Variant if T can't be put in an nsCOMPtr, but I have no idea what that would look like.

Comment 15

[Nathan Froyd [:froydnj]]

Ah, I see what you mean now.  Thanks for the explanation.

So what you want to write is something like:

template<class T>
class OutParam
{
  Variant<nsRefPtr<T>*, RefPtr<T>*, nsCOMPtr<T>*> mHolder;
  ...

  // Various methods checking |is<...<T>*>| below.  Ex:

    if (mHolder.is<nsCOMPtr<T>*>()) {
      *mHolder.as<nsCOMPtr<T>*>() = aThing;
    } else if (mHolder.is<nsRefPtr<T>*>()) {
      *mHolder.as<nsRefPtr<T>*>() = aThing;
    } // and so forth.
};

?  It's doubly problematic, because writing the template magic for the Variant type isn't too difficult, but then (presumably) you'd need to duplicate the methods of the class for COM-ness vs. non-COM-ness.  You might be able to arrange things something like:

// Maybe OutParamWithCOM is even derived from OutParamWithoutCOM below...
class OutParamWithCOM {
  Variant<..., nsCOMPtr<T>*, ...> mHolder;
  ...
};

class OutParamWithoutCOM {
  Variant<nsRefPtr<T>*, RefPtr<T>*> mHolder;
  ...
};

class OutParam : Conditional<SupportsCOM<T>::value, OutParamWithCOM, OutParamWithoutCOM>::Type
{};

I'm not sure if there's a nice solution here that doesn't involve a lot of code duplication.  Gerald, do you have ideas here?

Comment 16

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #15)

Let me dodge the question (for now at least), and point back to your comment #6:
> mfbt/RefPtr.h has some prior art in this area, FWIW:
> http://mxr.mozilla.org/mozilla-central/source/mfbt/RefPtr.h#312

How about having a different ByRef() for each type of smart pointer, and hopefully we could have just one template for the proxy, taking both the smart pointer type and the pointee type.
E.g.:
  template<typename SP, typename T> class OutParamRef { ...magic happens... };
  template<typename T> OutParamRef<RefPtr<T>, T>   ByRef(RefPtr<T>& aPtr) { ... }
  template<typename T> OutParamRef<nsRefPtr<T>, T> ByRef(nsRefPtr<T>& aPtr) { ... }
etc.


A couple of extra suggestions:
- It could even be extended to non-smart pointers, as both a declaration of intention and something that can host a breakpoint to track when an out-parameters is assigned to.
  template <typename T> ??? ByRef(T* aRawPtr) { return ???<T>(aRawPtr); }
- How about distinguishing out-only vs in/out parameters? Out-only would prevent implicitly reading the raw pointer inside the callee.
Though these might be better placed in the callee function declaration? Probably best to have another bug if anyone else sees any value in these; I don't want to derail this bug any further.

Comment 17

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to :Aryeh Gregor (working until August 13) from comment #12)
> Or (preferably
> from my perspective) would it be okay if I just went ahead and implemented
> it using a void* for the member and had the class track its current state
> manually?

Actually, that runs into the exact same problem.  The only way I see to do this now is to just take a T** to the pointed-to thing and AddRef it ourselves instead of getting the smart pointer to do it.

(In reply to Gerald Squelart [:gerald] from comment #16)
> How about having a different ByRef() for each type of smart pointer, and
> hopefully we could have just one template for the proxy, taking both the
> smart pointer type and the pointee type.
> E.g.:
>   template<typename SP, typename T> class OutParamRef { ...magic happens...
> };
>   template<typename T> OutParamRef<RefPtr<T>, T>   ByRef(RefPtr<T>& aPtr) {
> ... }
>   template<typename T> OutParamRef<nsRefPtr<T>, T> ByRef(nsRefPtr<T>& aPtr)
> { ... }
> etc.

Doesn't that mean you'd have to specify the type of pointer you want in the function signature?

Comment 18

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch

The COMOutParam approach works, and it's pretty simple to use.  It makes sense as long as we have different types for nsRefPtr and nsCOMPtr.  Compiles locally.

Based on bug 1190823.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=744cf345d16b

Comment 19

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 8643023 [details] [diff] [review]
Patch

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

::: xpcom/base/RefOutParam.h
@@ +63,5 @@
> +    }
> +    if (mPtr.template is<RefPtr<T>*>()) {
> +      return *mPtr.template as<RefPtr<T>*>();
> +    }
> +    MOZ_ASSERT_UNREACHABLE();

These are missing arguments, I changed to MOZ_ASSERT_UNREACHABLE("Unknown type").

@@ +68,5 @@
> +    return false;
> +  }
> +
> +public:
> +  RefOutParam(decltype(nullptr))

These constructors should all be MOZ_IMPLICIT.

With those two fixes, green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d9f880a4ae3

Comment 20

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to :Aryeh Gregor (working until August 13) from comment #17)
> (In reply to Gerald Squelart [:gerald] from comment #16)
> Doesn't that mean you'd have to specify the type of pointer you want in the
> function signature?

Ah yes you're right, I now see what you're trying to do. Sorry for the distraction.
I'll need more free time to be able to help, so don't wait for me -- if you can already catch potential bugs, it's a win!

Comment 21

[Nathan Froyd [:froydnj]]

Comment on attachment 8643023 [details] [diff] [review]
Patch

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

I think this is pretty close, but I'd like to have some discussion on the comments below.  They feel a bit too big to just grant "r+ with fixes".

::: editor/libeditor/nsEditor.cpp
@@ +3699,5 @@
>  {
>    NS_ENSURE_TRUE(outStartNode && outStartOffset && aSelection, NS_ERROR_NULL_POINTER);
>  
>    nsCOMPtr<nsINode> startNode;
> +  nsresult rv = GetStartNodeAndOffset(aSelection, address_of(startNode),

I discovered address_of the other day, and I must say, I'm not a fan.  I'm sure VCS history spelunking will reveal fascinating reasons why we're using address_of rather than |&|.

Since the ultimate goal here is to use this for XPCOM outparams and replacing getter_AddRefs, WDYT about adopting a name tied to outparam-ness, rather than a generic address_of?  outParam?  byRef is kind of nice, but that's already being used...

::: xpcom/base/RefOutParam.h
@@ +29,5 @@
> + *
> + *   3) PtrType<T>* does not automatically get initialized to null.  If the
> + *   callee does not do this manually and returns without setting the parameter
> + *   to anything, the caller will treat the prior contents of the pointer as the
> + *   return value, likely causing confusing bugs.

I'm curious how much of a problem this is in practice, since our smart pointer classes typically get initialized to null anyway.  The compiler *ought* to be able to optimize out useless re-assignments of null, but if it can't, we're bloating callers with pointless null checks and assignments.  Compare getter_AddRefs, which doesn't perform this service.

@@ +37,5 @@
> + *
> + *   5) Neither of the above automatically allow the caller to pass a null
> + *   pointer to indicate that they aren't interested in the result.  This leads
> + *   callers to make up unused variables, unless the callee specifically checks
> + *   for null.

Pretty sure we have places that take |T** aOut| and do |if (aOut), NS_ADDREF(*aOut = ...);| or similar.

Bug 1181305 requested an outparam type, but specified that it acted like a non-null pointer.  I think enforcing non-nullness and fixing up places that--for one reason or another--(ab)use null would be better than perpetuating null pointer issues...

Besides, one ought to be able to just pass DontCare<T>() or nsRefPtr<T>() for such parameters, right?  Or add explicit overloads?

@@ +95,5 @@
> +  {
> +    if (aPtr) {
> +      *aPtr = nullptr;
> +    }
> +  }

Can we get away with deleting the copy/move constructor/assignment methods?  If not, it'd be better to define those explicitly.

@@ +98,5 @@
> +    }
> +  }
> +
> +protected:
> +  // You need to use nsCOMParam to access this constructor.

Nit: COMOutParam.

@@ +131,5 @@
> +  template<class U>
> +  void
> +  operator=(already_AddRefed<U>&& aOther)
> +  {
> +    if (IsNull()) {

I think we should assert non-nullness here.  Otherwise, if we have null here, I think already_AddRefed's destructor will assert when the temporary |aOther| gets destroyed, because there was no place to put the temporary value.  And asserting here is marginally less confusing than asserting in the destructor...maybe?  WDYT?

@@ +150,5 @@
> +
> +/**
> + * Same as above, but accepts nsCOMPtr as well.  We need a separate class
> + * because using this class on a non-XPCOM type will try to instantiate
> + * nsCOMPtr<T> and fail.

The second sentence of this comment is a little confusing to me.  How about:

We need a separate class for types that would be used with nsCOMPtr.  If we had a single outparam class and we wanted to use it with a non-XPCOM type, then methods like |operator=| would require instantiating |nsCOMPtr<NonXPCOMType>|, which would fail.

@@ +188,5 @@
> +
> +  COMOutParam(nsCOMPtr<T>* aPtr)
> +    : RefOutParam<T>(aPtr)
> +  {
> +  }

Same comments here about deleting the copy/move constructor/assignment methods.

Comment 22

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #21)
> I discovered address_of the other day, and I must say, I'm not a fan.  I'm
> sure VCS history spelunking will reveal fascinating reasons why we're using
> address_of rather than |&|.
> 
> Since the ultimate goal here is to use this for XPCOM outparams and
> replacing getter_AddRefs, WDYT about adopting a name tied to outparam-ness,
> rather than a generic address_of?  outParam?  byRef is kind of nice, but
> that's already being used...

I'd be happy with allowing |&| to be used on nsCOMPtr/nsRefPtr.  Failing that, the way to take their address is address_of, and the standard way to pass out params in C++ is to use pointers or references.  Why make up a new class and break the established convention?

> ::: xpcom/base/RefOutParam.h
> @@ +29,5 @@
> > + *
> > + *   3) PtrType<T>* does not automatically get initialized to null.  If the
> > + *   callee does not do this manually and returns without setting the parameter
> > + *   to anything, the caller will treat the prior contents of the pointer as the
> > + *   return value, likely causing confusing bugs.
> 
> I'm curious how much of a problem this is in practice, since our smart
> pointer classes typically get initialized to null anyway.  The compiler
> *ought* to be able to optimize out useless re-assignments of null, but if it
> can't, we're bloating callers with pointless null checks and assignments. 
> Compare getter_AddRefs, which doesn't perform this service.

getter_AddRefs does set the pointer to null.  Ehsan has told me that there have been bugs in editor before because of functions that take nsCOMPtr<T>* as an out param and don't null-initialize, and he requests that cleanup patches change that to T** with getter_AddRefs.  The problem is when the caller uses a preexisting variable that has a value as the out param, and winds up thinking its old value was returned by the function (when the function really just aborted early and didn't null the out param).

> @@ +37,5 @@
> > + *
> > + *   5) Neither of the above automatically allow the caller to pass a null
> > + *   pointer to indicate that they aren't interested in the result.  This leads
> > + *   callers to make up unused variables, unless the callee specifically checks
> > + *   for null.
> 
> Pretty sure we have places that take |T** aOut| and do |if (aOut),
> NS_ADDREF(*aOut = ...);| or similar.

They can, but they have to opt in.  With this type, it's automatic.

> Bug 1181305 requested an outparam type, but specified that it acted like a
> non-null pointer.  I think enforcing non-nullness and fixing up places
> that--for one reason or another--(ab)use null would be better than
> perpetuating null pointer issues...
> 
> Besides, one ought to be able to just pass DontCare<T>() or nsRefPtr<T>()
> for such parameters, right?  Or add explicit overloads?

When you're passing a pointer of any sort, null is a handy value to use for "ignore this".  Yes, you could pass address_of(nsRefPtr<T>()), but it's more verbose -- particularly since functions may considerately put all the out params last and default them to nullptr.  E.g., nsHTMLEditRules::SplitBlock does this.  Explicit overloads are certainly not a good solution, especially since there may be multiple out params and the caller may want to ignore only some of them.

I don't see the value in requiring the caller to pass address_of(nsRefPtr<T>()) instead of just nullptr, particularly when the latter can be defaulted and the former cannot.

Additionally, the callee may be able to save work if it knows it doesn't need to provide the out param.  Actually, come to think of it, I'll make IsNull() public.

> @@ +95,5 @@
> > +  {
> > +    if (aPtr) {
> > +      *aPtr = nullptr;
> > +    }
> > +  }
> 
> Can we get away with deleting the copy/move constructor/assignment methods? 
> If not, it'd be better to define those explicitly.

No, because sometimes a function with an out param will forward to another function with an out param.  This way you can have

  nsresult
  F(RefOutParam<T> aOut)
  {
    DoStuff();
    G(aOut);
    return DoMoreStuff();
  }

instead of

  nsresult
  F(RefOutParam<T> aOut)
  {
    DoStuff();
    nsRefPtr<T> out;
    G(address_of(out));
    aOut = out.forget();
    return DoMoreStuff();
  }

This pattern (the first) is fairly common in editor.  The existing ways of doing things (T** and nsRefPtr<T>*/nsCOMPtr<T>*) both allow this sort of out param passing, and I don't think this type should be made any worse.

Given that we do want copy constructors/assignment operators, what's wrong with leaving the defaults?  Would you like a comment noting explicitly that we want to keep them and didn't just forget about them?

> @@ +131,5 @@
> > +  template<class U>
> > +  void
> > +  operator=(already_AddRefed<U>&& aOther)
> > +  {
> > +    if (IsNull()) {
> 
> I think we should assert non-nullness here.  Otherwise, if we have null
> here, I think already_AddRefed's destructor will assert when the temporary
> |aOther| gets destroyed, because there was no place to put the temporary
> value.  And asserting here is marginally less confusing than asserting in
> the destructor...maybe?  WDYT?

We can't assert null here, because maybe the caller doesn't actually have a use for the value.  But you're right we can't throw it away.  The right thing to do would be to assign to a local nsRefPtr so it gets released properly.  Make sense?

Comment 23

[Nathan Froyd [:froydnj]]

(In reply to :Aryeh Gregor (working until August 13) from comment #22)
> (In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #21)
> > Since the ultimate goal here is to use this for XPCOM outparams and
> > replacing getter_AddRefs, WDYT about adopting a name tied to outparam-ness,
> > rather than a generic address_of?  outParam?  byRef is kind of nice, but
> > that's already being used...
> 
> I'd be happy with allowing |&| to be used on nsCOMPtr/nsRefPtr.  Failing
> that, the way to take their address is address_of, and the standard way to
> pass out params in C++ is to use pointers or references.  Why make up a new
> class and break the established convention?

I see the choice here as replacing getter_AddRefs and therefore using something that makes it more explicit at the callsite that there's a bit of magic going on.  Neither |&| or |address_of| make that clear.

And we wouldn't be making up a new class, but new functions, similar to address_of:

template<typename T>
RefOutParam<T>
outParam(nsRefPtr<T>& aPtr)
{
  return RefOutParam(aPtr);
}

That would also avoid the need to use pointers in RefOutParam, which is a nice plus.

> > ::: xpcom/base/RefOutParam.h
> > @@ +29,5 @@
> > > + *
> > > + *   3) PtrType<T>* does not automatically get initialized to null.  If the
> > > + *   callee does not do this manually and returns without setting the parameter
> > > + *   to anything, the caller will treat the prior contents of the pointer as the
> > > + *   return value, likely causing confusing bugs.
> > 
> > I'm curious how much of a problem this is in practice, since our smart
> > pointer classes typically get initialized to null anyway.  The compiler
> > *ought* to be able to optimize out useless re-assignments of null, but if it
> > can't, we're bloating callers with pointless null checks and assignments. 
> > Compare getter_AddRefs, which doesn't perform this service.
> 
> getter_AddRefs does set the pointer to null.  Ehsan has told me that there
> have been bugs in editor before because of functions that take nsCOMPtr<T>*
> as an out param and don't null-initialize, and he requests that cleanup
> patches change that to T** with getter_AddRefs.  The problem is when the
> caller uses a preexisting variable that has a value as the out param, and
> winds up thinking its old value was returned by the function (when the
> function really just aborted early and didn't null the out param).

That seems like a problem with how the function returns success vs. failure.

Regardless, you're right: getter_AddRefs does null out things.  And the null checks for the smart pointers in the constructors ought to be optimized away by the compiler...and would be completely eliminated if we used references for the storage instead.  So I guess we're no worse off here.

> > @@ +37,5 @@
> > > + *
> > > + *   5) Neither of the above automatically allow the caller to pass a null
> > > + *   pointer to indicate that they aren't interested in the result.  This leads
> > > + *   callers to make up unused variables, unless the callee specifically checks
> > > + *   for null.
> > 
> > Pretty sure we have places that take |T** aOut| and do |if (aOut),
> > NS_ADDREF(*aOut = ...);| or similar.
> 
> They can, but they have to opt in.  With this type, it's automatic.

I don't know that moving from opt-in to automatic is necessarily a good thing, though.  It seems like there's value in being able to make a distinction between "this function always expects the caller to do useful things with outparams" vs. "this function provides outparams that the caller can ignore".  Especially because if we make it automatic everywhere, every assignment to RefOutParam<T> now needs to be null-checked, which is pure code bloat in many cases...

> > @@ +95,5 @@
> > > +  {
> > > +    if (aPtr) {
> > > +      *aPtr = nullptr;
> > > +    }
> > > +  }
> > 
> > Can we get away with deleting the copy/move constructor/assignment methods? 
> > If not, it'd be better to define those explicitly.
> 
> No, because sometimes a function with an out param will forward to another
> function with an out param.

OK.

> Given that we do want copy constructors/assignment operators, what's wrong
> with leaving the defaults?  Would you like a comment noting explicitly that
> we want to keep them and didn't just forget about them?

It looks like we can use |= default| nowadays, which is even better than a comment.  Please use |= default| where appropriate and |= delete| otherwise.

> We can't assert null here, because maybe the caller doesn't actually have a
> use for the value.  But you're right we can't throw it away.  The right
> thing to do would be to assign to a local nsRefPtr so it gets released
> properly.  Make sense?

Assuming that we want to keep nullness around here, yes, that would be the way to do things.

Comment 24

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #23)
> I see the choice here as replacing getter_AddRefs and therefore using
> something that makes it more explicit at the callsite that there's a bit of
> magic going on.  Neither |&| or |address_of| make that clear.

What difference does it make to the caller whether the callee takes a pointer or RefOutParam?  The differences are only to the callee.

> I don't know that moving from opt-in to automatic is necessarily a good
> thing, though.  It seems like there's value in being able to make a
> distinction between "this function always expects the caller to do useful
> things with outparams" vs. "this function provides outparams that the caller
> can ignore".

There are some functions where it makes no sense to ignore the returned value, but in those cases the callers just won't ignore them.  What's the value in *forcing* the caller to not ignore the value (other than performance)?

> Especially because if we make it automatic everywhere, every
> assignment to RefOutParam<T> now needs to be null-checked, which is pure
> code bloat in many cases...

In those cases where the caller will never ignore the result, agreed, useless code will have to be run by the CPU.  On the other hand, the alternative is to have extra code read/written by the programmer.  It's a convenience/performance tradeoff.  Note that if the caller can pass null, the callee may be able to improve performance by testing for it and avoiding extra work, in some cases.

We could always add a variant that doesn't take null.  But that seems like significant complexity for the (I think) minority of cases where it will be relevant.

Would you be happier if the pointer was required to not be null, but the caller could pass mozilla::unused to specify they wish to ignore the return value?

> It looks like we can use |= default| nowadays, which is even better than a
> comment.  Please use |= default| where appropriate and |= delete| otherwise.

This strikes me as overly verbose.  A one-line comment seems better than four lines of code that the compiler will ignore.  Is there any established Mozilla style on this issue?  If not, perhaps we should establish some.

Comment 25

[Kelsey Gilbert [:jgilbert]]

I'm confused. The normal existing type for a refcounted outparam is `RefPtr<Foo>* const outvar`. 

What is the value of having a more complicated approach?

Comment 26

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

A comment in the patch explains the advantages.  (3) and (4) are the primary advantages over RefPtr<T>*.  Ehsan does not allow nsCOMPtr<T>* for out-params in patches he reviews because of (3), at least for editor.  (4) is also a big deal if you use OwningNonNull, which I've started to where possible.

Comment 27

[Kelsey Gilbert [:jgilbert]]

Alright, so pulling these out of the patch:

> Problems that RefOutParam/COMOutParam solve are:
>  *
>  *   1) T** allows the callee to assign pointers that have not been AddRefed or
>  *   that have been AddRefed too many times, causing use-after-free or leaks.
>  *
>  *   2) T** allows callers to pass raw pointers-to-pointers instead of the
>  *   result of getter_AddRefs, causing leaks.
>  *
>  *   3) PtrType<T>* does not automatically get initialized to null.  If the
>  *   callee does not do this manually and returns without setting the parameter
>  *   to anything, the caller will treat the prior contents of the pointer as the
>  *   return value, likely causing confusing bugs.
>  *
>  *   4) PtrType<T>* require that that type of pointer be used by the caller.
>  *   But the caller may want to use OwningNonNull, for instance.
>  *
>  *   5) Neither of the above automatically allow the caller to pass a null
>  *   pointer to indicate that they aren't interested in the result.  This leads
>  *   callers to make up unused variables, unless the callee specifically checks
>  *   for null.

1 and 2 are solved by using PtrType<T>*. I goes without saying that if you need to keep-alive the ref-counted object, you should be using a ref-pointer type.

3 doesn't seem right. nsRefPtr and RefPtr both default to null when not initialized otherwise. Thus when you pass in &localRefPtr without explicitly initilizing localRefPtr, it's already been set to null.
If you're passing in some &mMemberRefPtr, and expect it to be modified but it isn't (or vice-versa), you're using the function wrong at the callsite.
Ultimately this same issue is true for all outparams. This is true for all cases where state may-or-may-not be modified by a function.

4 is not particularly compelling to me, but ok. Most code I see uses either nsRefPtr or RefPtr, so using the correct one is pretty simple.

For 5, PtrType<T>* can always be null. The callee either handles this or it doesn't. It seems all this adds is the ability to transparent-to-the-callee make it optional.
We have to use junk vars for non-RefPtr outparams anyway, so I don't see this being a huge win.


This ultimately doesn't seem to provide value in excess of the friction that learning and using a non-standard API imparts.

I work in code that extensively uses out-params (both refcounted and otherwise), but I don't think this patch will be useful to me.


I would like to hear more from :ehsan about comment #26, and why he considers #3 to be unacceptable.

Comment 28

[(no longer active)]

(In reply to Jeff Gilbert [:jgilbert] from comment #27)
> 1 and 2 are solved by using PtrType<T>*. I goes without saying that if you
> need to keep-alive the ref-counted object, you should be using a ref-pointer
> type.

I don't know what PtrType<T>* is.  If you mean nsCOMPtr<T>*, see below.

> 3 doesn't seem right. nsRefPtr and RefPtr both default to null when not
> initialized otherwise. Thus when you pass in &localRefPtr without explicitly
> initilizing localRefPtr, it's already been set to null.

Only if it is just initialized.  The problem with passing nsCOMPtr*s around is that you cannot know that, so depending on whether the callee assigns something to it, when it returns the object may have either a null value, or something else, or may retain its original value.

On the contrary, our existing getter_AddRefs() machinery ensures that the pointer is nulled out before passing it to the function, so you can be sure that the object's state after the callee returns is only determined by the callee.  In practice, this is an important property that prevents lots of logic errors that typically only surface when the callee errors out.  As an example:

  nsCOMPtr<T> foo;
  // Do some work, possibly assign to foo.
  GetNewFoo(&foo);
  if (foo) {
    // Surely we must have a new foo in this case, right?  Nope!
  }

In the code above, if GetNewFoo fails, the if check following it will do the wrong thing!

> If you're passing in some &mMemberRefPtr, and expect it to be modified but
> it isn't (or vice-versa), you're using the function wrong at the callsite.
> Ultimately this same issue is true for all outparams. This is true for all
> cases where state may-or-may-not be modified by a function.

Not sure what's different about members here.  The same issue above exists for them too.

> 4 is not particularly compelling to me, but ok. Most code I see uses either
> nsRefPtr or RefPtr, so using the correct one is pretty simple.

It is extremely compelling, since knowing that some smart pointers cannot contain null values is very valuable.

> For 5, PtrType<T>* can always be null. The callee either handles this or it
> doesn't. It seems all this adds is the ability to transparent-to-the-callee
> make it optional.

Yes.  Not sure if I understand your objection here.  I think you're just rewording #5.  :-)

> We have to use junk vars for non-RefPtr outparams anyway, so I don't see
> this being a huge win.

Not necessarily.  Ideally we could eliminate the need for junk vars by doing something like:

  DoWork(yadda, yadda, DontCare());

or some such.  DontCare() would do the magic work of making sure that the callee still gets an out param smart ptr type, so it doesn't need to null check or anything (thus eliminating the risk of crashes when people forget to null check, which happens all the time) and the magic object would just put what you pass it into the trash for you!

> This ultimately doesn't seem to provide value in excess of the friction that
> learning and using a non-standard API imparts.
> 
> I work in code that extensively uses out-params (both refcounted and
> otherwise), but I don't think this patch will be useful to me.

I don't understand why not.  Perhaps you have different needs?  If you have additional use cases, please feel free to raise them.

Comment 29

[Kelsey Gilbert [:jgilbert]]

(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #28)
> (In reply to Jeff Gilbert [:jgilbert] from comment #27)
> > 1 and 2 are solved by using PtrType<T>*. I goes without saying that if you
> > need to keep-alive the ref-counted object, you should be using a ref-pointer
> > type.
> 
> I don't know what PtrType<T>* is.  If you mean nsCOMPtr<T>*, see below.

PtrType is one of RefPtr, nsRefPtr, nsCOMPtr, or really any other ref-pointer class.

> 
> > 3 doesn't seem right. nsRefPtr and RefPtr both default to null when not
> > initialized otherwise. Thus when you pass in &localRefPtr without explicitly
> > initilizing localRefPtr, it's already been set to null.
> 
> Only if it is just initialized.  The problem with passing nsCOMPtr*s around
> is that you cannot know that, so depending on whether the callee assigns
> something to it, when it returns the object may have either a null value, or
> something else, or may retain its original value.
> 
> On the contrary, our existing getter_AddRefs() machinery ensures that the
> pointer is nulled out before passing it to the function, so you can be sure
> that the object's state after the callee returns is only determined by the
> callee.  In practice, this is an important property that prevents lots of
> logic errors that typically only surface when the callee errors out.  As an
> example:
> 
>   nsCOMPtr<T> foo;
>   // Do some work, possibly assign to foo.
>   GetNewFoo(&foo);
>   if (foo) {
>     // Surely we must have a new foo in this case, right?  Nope!
>   }
> 
> In the code above, if GetNewFoo fails, the if check following it will do the
> wrong thing!

This is bad code, and I don't see why we should enable the writing of code like this.

GetNewFoo should either be:
bool GetNewFoo(nsCOMPtr<T>* outFoo); // Call-site should have checked return value. Consider forcing use of ret-val.
WhateverCOMPtrHasForAlreadyAddrefed<T> GetNewFoo();

If the call-site *must* rely on whether &foo is changed, then it clearly *must* pass a nulled nsCOMPtr into the func, either by nulling `foo` or by using a different nulled temporary.

> 
> > This ultimately doesn't seem to provide value in excess of the friction that
> > learning and using a non-standard API imparts.
> > 
> > I work in code that extensively uses out-params (both refcounted and
> > otherwise), but I don't think this patch will be useful to me.
> 
> I don't understand why not.  Perhaps you have different needs?  If you have
> additional use cases, please feel free to raise them.

Because writing good code doesn't require this new construct. It seems to be that the things you mention can all be handled explicitly at the callsite:

>   nsCOMPtr<T> foo;
>   // Do some work, possibly assign to foo.
>   GetNewFoo(&foo);
>   if (foo) {
>     // Surely we must have a new foo in this case, right?  Nope!
>   }
Should be:
>   nsCOMPtr<T> foo;
>   // Do some work, possibly assign to foo.
> + foo = nullptr;
>   GetNewFoo(&foo);
>   if (foo) {
> *   // foo is definitely new
>   }

Why is `foo` being reused here anyways? If it's actually mFoo, we should either check the return value for GetNewFoo, clear mFoo to null before a fallible-new, or there should be a temporary local `foo` that we use here, and only assign to mFoo if `foo` ends up being valid.

If `&foo` in `GetNewFoo(&foo,...)` being set to something non-null instead of left untouched is the primary way success is represented, then GetNewFoo should really return `foo`, instead of taking it as an out-param.


>   OwningNonNull<T> ownedQux;
>   AlwaysGetQux(ownedQux->PretendToBeSomethingElse());
vs.
> * RefPtr<Qux> qux;
> * AlwaysGetQux(&qux);
> + OwningNonNull<T> ownedQux = qux;

Extremely explicit. No tricks here. Easily understandable.


>   DoWork(yadda, yadda, DontCare());
vs.
> + RefPtr<Bar> ignoredBar;
> * DoWork(yadda, yadda, &ignoredBar);
On at least MSVC, you can even do:
> * DoWork(yadda, yadda, &RefPtr<Bar>());

I don't have additional use cases, since even my non-trivial needs are already met by existing primitives.

I am extremely curious what use cases this is meant to solve that *aren't* already solved by existing constructs, instead of adding more constructs to the codebase. More constructs means more behaviors and options to keep in mind, as well as the combinatorial complexity of adding interactions with existing constructs.

The issue mentioned in the initial comment here seems to be immediately solved with changing `nsIContent** aOutParam` to `RefPtr<nsIContent>* aOutParam`. The only thing the rest of the bug seems to add is a construct that trickily doesn't require the function decl to establish the type of the outparam. This sort of glue does not seem (to me) like a huge improvement over adding a single extra line of glue per callsite where you need a different ref-pointer type. (which is not most of them)

That said, if it's a *generic* refcounted out-param type you want, feel free. But this bug is "Type for refcounted outparams", and we already have types for those: `RefPtr<T>* outparam`

Comment 30

[Nathan Froyd [:froydnj]]

Seth originally filed bug 1181305; ni?'ing him here to see if he has comments on the last few comments pro or con.

Comment 31

[(no longer active)]

(In reply to Jeff Gilbert [:jgilbert] from comment #29)
> > > 3 doesn't seem right. nsRefPtr and RefPtr both default to null when not
> > > initialized otherwise. Thus when you pass in &localRefPtr without explicitly
> > > initilizing localRefPtr, it's already been set to null.
> > 
> > Only if it is just initialized.  The problem with passing nsCOMPtr*s around
> > is that you cannot know that, so depending on whether the callee assigns
> > something to it, when it returns the object may have either a null value, or
> > something else, or may retain its original value.
> > 
> > On the contrary, our existing getter_AddRefs() machinery ensures that the
> > pointer is nulled out before passing it to the function, so you can be sure
> > that the object's state after the callee returns is only determined by the
> > callee.  In practice, this is an important property that prevents lots of
> > logic errors that typically only surface when the callee errors out.  As an
> > example:
> > 
> >   nsCOMPtr<T> foo;
> >   // Do some work, possibly assign to foo.
> >   GetNewFoo(&foo);
> >   if (foo) {
> >     // Surely we must have a new foo in this case, right?  Nope!
> >   }
> > 
> > In the code above, if GetNewFoo fails, the if check following it will do the
> > wrong thing!
> 
> This is bad code, and I don't see why we should enable the writing of code
> like this.

The point here is to make people not write code like this.

> GetNewFoo should either be:
> bool GetNewFoo(nsCOMPtr<T>* outFoo); // Call-site should have checked return
> value. Consider forcing use of ret-val.
> WhateverCOMPtrHasForAlreadyAddrefed<T> GetNewFoo();
> 
> If the call-site *must* rely on whether &foo is changed, then it clearly
> *must* pass a nulled nsCOMPtr into the func, either by nulling `foo` or by
> using a different nulled temporary.

Yes.  But the point is that if your method takes an nsCOMPtr<T>*, it is super simple to pass in the address to a smart pointer containing a non-null pointer, as I explained above.  The value in what is being implemented here is to give you one correct way to write the code, and not rely on you checking every single call site of these functions.  Checking them all is simply not sustainable.

> > > This ultimately doesn't seem to provide value in excess of the friction that
> > > learning and using a non-standard API imparts.
> > > 
> > > I work in code that extensively uses out-params (both refcounted and
> > > otherwise), but I don't think this patch will be useful to me.
> > 
> > I don't understand why not.  Perhaps you have different needs?  If you have
> > additional use cases, please feel free to raise them.
> 
> Because writing good code doesn't require this new construct. It seems to be
> that the things you mention can all be handled explicitly at the callsite:

It seems like you're dividing the code into two categories, good and bad, and arguing that bad code deserves what it gets!  ;-)  That is not a useful way of thinking about this.  The value in the work being done here is to make it simpler to write code that will do the right thing no matter whether you get things right in every single call site until the end of times.

Also, note that this is not a niche mistake that novice programmers make.  I cannot count the number of times that I have screwed things up in code that uses nsCOMPtr<T>* as out arguments, and that doesn't even get close to the number of times that I have missed this when reviewing other people's code.

> >   nsCOMPtr<T> foo;
> >   // Do some work, possibly assign to foo.
> >   GetNewFoo(&foo);
> >   if (foo) {
> >     // Surely we must have a new foo in this case, right?  Nope!
> >   }
> Should be:
> >   nsCOMPtr<T> foo;
> >   // Do some work, possibly assign to foo.
> > + foo = nullptr;
> >   GetNewFoo(&foo);
> >   if (foo) {
> > *   // foo is definitely new
> >   }
> 
> Why is `foo` being reused here anyways?

Because that is what people do.  Either intentionally without thinking about this issue, or accidentally by moving code around, etc.

> If it's actually mFoo, we should
> either check the return value for GetNewFoo, clear mFoo to null before a
> fallible-new, or there should be a temporary local `foo` that we use here,
> and only assign to mFoo if `foo` ends up being valid.

Sure, you're explaining how we can solve these mistakes when we find them, and I understand how to do that.  We're trying to make these mistakes impossible to happen though, so that we don't have to find and fix them later.

> If `&foo` in `GetNewFoo(&foo,...)` being set to something non-null instead
> of left untouched is the primary way success is represented, then GetNewFoo
> should really return `foo`, instead of taking it as an out-param.

That is not always attainable since sometimes you need to return other things.  If this were not the case, nobody would use out params anyway.

> >   OwningNonNull<T> ownedQux;
> >   AlwaysGetQux(ownedQux->PretendToBeSomethingElse());
> vs.
> > * RefPtr<Qux> qux;
> > * AlwaysGetQux(&qux);
> > + OwningNonNull<T> ownedQux = qux;
> 
> Extremely explicit. No tricks here. Easily understandable.

Not sure if I understand what you're referring to here.

> >   DoWork(yadda, yadda, DontCare());
> vs.
> > + RefPtr<Bar> ignoredBar;
> > * DoWork(yadda, yadda, &ignoredBar);

This is the unused variable that Aryeh was mentioning in his patch!

> On at least MSVC, you can even do:
> > * DoWork(yadda, yadda, &RefPtr<Bar>());

And this is exactly like DontCare() not any better.

> I don't have additional use cases, since even my non-trivial needs are
> already met by existing primitives.

OK.  Note that I'm not trying to make you use this stuff.  :-)

> I am extremely curious what use cases this is meant to solve that *aren't*
> already solved by existing constructs, instead of adding more constructs to
> the codebase. More constructs means more behaviors and options to keep in
> mind, as well as the combinatorial complexity of adding interactions with
> existing constructs.

The goal: making it possible to write code using refcounted out params that doesn't require checking things at every single call site forever.

If you prefer the more error prone approach of forcing the programmer to do these checks in every call site, this is not going to help you.  (I'm not sure why you prefer that exactly, but that's besides the point.)

> The issue mentioned in the initial comment here seems to be immediately
> solved with changing `nsIContent** aOutParam` to `RefPtr<nsIContent>*
> aOutParam`.

Firstly, RefPtr should never be used with XPCOM classes.  I already explained the problems with nsCOMPtr<nsIContent>*.

> The only thing the rest of the bug seems to add is a construct
> that trickily doesn't require the function decl to establish the type of the
> outparam. This sort of glue does not seem (to me) like a huge improvement
> over adding a single extra line of glue per callsite where you need a
> different ref-pointer type. (which is not most of them)

I am not a fan of requiring people to check things at the call site.  And experience shows that when we do, people get things wrong *all the time*.

> That said, if it's a *generic* refcounted out-param type you want, feel
> free. But this bug is "Type for refcounted outparams", and we already have
> types for those: `RefPtr<T>* outparam`

If you think of a better summary for the bug, please update it.  I care about the work here being done, the summary of the bug doesn't matter.  :-)

Comment 32

[Kelsey Gilbert [:jgilbert]]

(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #31)
> (In reply to Jeff Gilbert [:jgilbert] from comment #29)
> > This is bad code, and I don't see why we should enable the writing of code
> > like this.
> 
> The point here is to make people not write code like this.
> 
> > GetNewFoo should either be:
> > bool GetNewFoo(nsCOMPtr<T>* outFoo); // Call-site should have checked return
> > value. Consider forcing use of ret-val.
> > WhateverCOMPtrHasForAlreadyAddrefed<T> GetNewFoo();
> > 
> > If the call-site *must* rely on whether &foo is changed, then it clearly
> > *must* pass a nulled nsCOMPtr into the func, either by nulling `foo` or by
> > using a different nulled temporary.
> 
> Yes.  But the point is that if your method takes an nsCOMPtr<T>*, it is
> super simple to pass in the address to a smart pointer containing a non-null
> pointer, as I explained above.  The value in what is being implemented here
> is to give you one correct way to write the code, and not rely on you
> checking every single call site of these functions.  Checking them all is
> simply not sustainable.

I don't think this is true. If you want to enable this behavior, change the contract of the callee so that it nulls out outparams on failure.

> 
> > > > This ultimately doesn't seem to provide value in excess of the friction that
> > > > learning and using a non-standard API imparts.
> > > > 
> > > > I work in code that extensively uses out-params (both refcounted and
> > > > otherwise), but I don't think this patch will be useful to me.
> > > 
> > > I don't understand why not.  Perhaps you have different needs?  If you have
> > > additional use cases, please feel free to raise them.
> > 
> > Because writing good code doesn't require this new construct. It seems to be
> > that the things you mention can all be handled explicitly at the callsite:
> 
> It seems like you're dividing the code into two categories, good and bad,
> and arguing that bad code deserves what it gets!  ;-)  That is not a useful
> way of thinking about this.  The value in the work being done here is to
> make it simpler to write code that will do the right thing no matter whether
> you get things right in every single call site until the end of times.

There is a point where we decide that a pattern is bad enough that we should not allow it. I do not think adding a crutch for users of these patterns is the right answer here.
> 
> Also, note that this is not a niche mistake that novice programmers make.  I
> cannot count the number of times that I have screwed things up in code that
> uses nsCOMPtr<T>* as out arguments, and that doesn't even get close to the
> number of times that I have missed this when reviewing other people's code.

Examples please then, but this class of errors seems largely solvable by MOZ_MUST_USE. I'm going to file a bug that adds a simple bool type with MOZ_MUST_USE.
If a function is telling you it failed, *never ever pretend it succeeded*.

I don't think we need a new ref-counted flavor to handle this.

> 
> > >   nsCOMPtr<T> foo;
> > >   // Do some work, possibly assign to foo.
> > >   GetNewFoo(&foo);
> > >   if (foo) {
> > >     // Surely we must have a new foo in this case, right?  Nope!
> > >   }
> > Should be:
> > >   nsCOMPtr<T> foo;
> > >   // Do some work, possibly assign to foo.
> > > + foo = nullptr;
> > >   GetNewFoo(&foo);
> > >   if (foo) {
> > > *   // foo is definitely new
> > >   }
> > 
> > Why is `foo` being reused here anyways?
> 
> Because that is what people do.  Either intentionally without thinking about
> this issue, or accidentally by moving code around, etc.

You have the same issue with any non-refcounted outparams. This seems like a pretty general bug opportunity that as writers and reviewers we should be watching for.

> 
> > If it's actually mFoo, we should
> > either check the return value for GetNewFoo, clear mFoo to null before a
> > fallible-new, or there should be a temporary local `foo` that we use here,
> > and only assign to mFoo if `foo` ends up being valid.
> 
> Sure, you're explaining how we can solve these mistakes when we find them,
> and I understand how to do that.  We're trying to make these mistakes
> impossible to happen though, so that we don't have to find and fix them
> later.

Fix it in the callee then. I don't see why a new construct is required. If someone's going to break a function contract, they're going to break a function contract. I don't think we need to go full "safety-scissors" on this.

> 
> > If `&foo` in `GetNewFoo(&foo,...)` being set to something non-null instead
> > of left untouched is the primary way success is represented, then GetNewFoo
> > should really return `foo`, instead of taking it as an out-param.
> 
> That is not always attainable since sometimes you need to return other
> things.  If this were not the case, nobody would use out params anyway.

The vast, vast majority of such functions either succeed or they do not. The simplest patterns are `already_addRefed<Foo> MaybeGetFoo(...)` and `bool MaybeGetFoo(Foo** out, ...)`. There should never be a `void MaybeGetFoo(Foo** out, ...)` which doesn't change `*out` if it fails.

If using either of the former simple patterns, to check success, you just check the truthyness of the return value.
If you call `bool MaybeGetFoo`, and use what you passed to the outvar without checking the retval, this is bad code and cannot be tolerated.
This cannot be tolerated because often there are other outvars, like `bool MaybeGetFoo(Foo** outFoo, int* outFormat)`. If you reuse an old int to pass in to `outFormat`, and access it without checking the retval of MaybeGetFoo, (and MaybeGetFoo doesn't set it on failure) you are ignoring MaybeGetFoo's contract and stepping off a cliff.

Since it seems unreasonable to make a "generic safe-defaulting type for any-typed outparams", it seems strange that we're interested in a "generic safe-defaulting type for refcounted outparams".

> 
> > >   OwningNonNull<T> ownedQux;
> > >   AlwaysGetQux(ownedQux->PretendToBeSomethingElse());
> > vs.
> > > * RefPtr<Qux> qux;
> > > * AlwaysGetQux(&qux);
> > > + OwningNonNull<T> ownedQux = qux;
> > 
> > Extremely explicit. No tricks here. Easily understandable.
> 
> Not sure if I understand what you're referring to here.

There's no need to have a fancy outparam type that transparently hands things to a ref-pointer of a different type than the callee expects.

> 
> > >   DoWork(yadda, yadda, DontCare());
> > vs.
> > > + RefPtr<Bar> ignoredBar;
> > > * DoWork(yadda, yadda, &ignoredBar);
> 
> This is the unused variable that Aryeh was mentioning in his patch!

And it's really not that bad. And can be inlined as shown below, for no extra lines.

> 
> > On at least MSVC, you can even do:
> > > * DoWork(yadda, yadda, &RefPtr<Bar>());
> 
> And this is exactly like DontCare() not any better.

It is better because it doesn't require a new construct, and we can already use it today.

> 
> > I don't have additional use cases, since even my non-trivial needs are
> > already met by existing primitives.
> 
> OK.  Note that I'm not trying to make you use this stuff.  :-)
> 
> > I am extremely curious what use cases this is meant to solve that *aren't*
> > already solved by existing constructs, instead of adding more constructs to
> > the codebase. More constructs means more behaviors and options to keep in
> > mind, as well as the combinatorial complexity of adding interactions with
> > existing constructs.
> 
> The goal: making it possible to write code using refcounted out params that
> doesn't require checking things at every single call site forever.

Write functions that null out their refcounted outparams then. Not doing so is a choice when deciding what the function's contract should be.

> 
> If you prefer the more error prone approach of forcing the programmer to do
> these checks in every call site, this is not going to help you.  (I'm not
> sure why you prefer that exactly, but that's besides the point.)

I don't see it as error-prone to require obeying the contract of a function.

> 
> > The issue mentioned in the initial comment here seems to be immediately
> > solved with changing `nsIContent** aOutParam` to `RefPtr<nsIContent>*
> > aOutParam`.
> 
> Firstly, RefPtr should never be used with XPCOM classes.  I already
> explained the problems with nsCOMPtr<nsIContent>*.

This patch better handle that then. Making the outparam `nsCOMPtr<nsIContent>*` would make this requirement very explicit.

> 
> > The only thing the rest of the bug seems to add is a construct
> > that trickily doesn't require the function decl to establish the type of the
> > outparam. This sort of glue does not seem (to me) like a huge improvement
> > over adding a single extra line of glue per callsite where you need a
> > different ref-pointer type. (which is not most of them)
> 
> I am not a fan of requiring people to check things at the call site.  And
> experience shows that when we do, people get things wrong *all the time*.

This is not checking things at the callsite. This is adapting the needs of the callsite with the choice-of-ref-pointer class of the callee.

> If you think of a better summary for the bug, please update it.  I care
> about the work here being done, the summary of the bug doesn't matter.  :-)

Done.


I think the main case of ignoring the failure of a function should be solved with a bool-like MOZ_MUST_USE type, and I will be filing a bug for this.

There is friction any time we add a new construct to our tool box. I do not think the functionality for this new ref-pointer type is worth the friction, given that the other requirements here seem almost completely met by existing constructs.

Comment 33

[Kelsey Gilbert [:jgilbert]]

Actually MOZ_WARN_UNUSED_RESULT seems to handle this for us, with warnings-as-errors. (and not on msvc)

Comment 34

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 8643023 [details] [diff] [review]
Patch

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

::: xpcom/base/RefOutParam.h
@@ +32,5 @@
> + *   to anything, the caller will treat the prior contents of the pointer as the
> + *   return value, likely causing confusing bugs.
> + *
> + *   4) PtrType<T>* require that that type of pointer be used by the caller.
> + *   But the caller may want to use OwningNonNull, for instance.

It occurs to me -- it doesn't make any sense to pass OwningNonNull to an out param!  Maybe the function will return null.  So for out params that may be null, 4 isn't a reason to use this instead of nsCOMPtr*/nsRefPtr*/RefPtr*.  For infallible functions that have out params that they can guarantee are never null, a variant on this type (NonNullRefOutParam?) may be useful so that the caller can provide either an OwningNonNull* or nsCOMPtr*/nsRefPtr*/RefPtr*.  But I'm not sure many such functions exist.

So I think OwningNonNull needs to be removed from this file, and (4) removed from the list of reasons.

Comment 35

[Nathan Froyd [:froydnj]]

(In reply to :Aryeh Gregor (working until August 13) from comment #24)
> (In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #23)
> > I see the choice here as replacing getter_AddRefs and therefore using
> > something that makes it more explicit at the callsite that there's a bit of
> > magic going on.  Neither |&| or |address_of| make that clear.
> 
> What difference does it make to the caller whether the callee takes a
> pointer or RefOutParam?  The differences are only to the callee.

The rationale I had in mind was that it makes it more obvious for the reader of the code that something is going on (e.g. auto-nulling of things).

I am now really curious in how many cases the auto-nulling actually accomplishes useful work (i.e. the pointer is not already null).

> > Especially because if we make it automatic everywhere, every
> > assignment to RefOutParam<T> now needs to be null-checked, which is pure
> > code bloat in many cases...
> 
> In those cases where the caller will never ignore the result, agreed,
> useless code will have to be run by the CPU.  On the other hand, the
> alternative is to have extra code read/written by the programmer.  It's a
> convenience/performance tradeoff.  Note that if the caller can pass null,
> the callee may be able to improve performance by testing for it and avoiding
> extra work, in some cases.

Sure.  It's not obvious to me that it's worth providing this convenience to *every* user of RefParam, when only a small subset of users will take advantage of the functionality.

> > It looks like we can use |= default| nowadays, which is even better than a
> > comment.  Please use |= default| where appropriate and |= delete| otherwise.
> 
> This strikes me as overly verbose.  A one-line comment seems better than
> four lines of code that the compiler will ignore.  Is there any established
> Mozilla style on this issue?  If not, perhaps we should establish some.

I don't think we have established style on this.  But I'd sure rather have four lines of code that the compiler will complain about when somebody goes to add copy/move things later than a comment that can be easily overlooked.  You can also comment: "We allow the copying (resp. move) member functions because of the following scenario..." on actual defined operators, more easily, I think, versus tacking it on to another comment.

Comment 36

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Updated patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c96cf5f0ade

I had a long explanation of the differences from the last patch, but it got eaten by gremlins on submission.  Here is a reconstruction from memory:

1) OwningNonNull removed per comment 34.

2) Assignment operators no longer auto-check for null, they just assert.  If the callee wants to allow null params, they must explicitly check, as with the existing options.

3) The constructors no longer check if the passed pointer is null, they just assert.  Callers may pass nullptr, but if they pass a non-nullptr_t type, it must not be null.

4) I deleted the copy assignment operator.  Instead of the default copy constructor, I wrote a constructor that nulls the out param, for consistency with the other constructors.

5) Added a constructor from nullptr, because C++ isn't smart enough to use the U* one.

(In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #35)
> The rationale I had in mind was that it makes it more obvious for the reader
> of the code that something is going on (e.g. auto-nulling of things).

The callee should *always* null the out param before doing anything else.  Why should the caller care if the callee does the nulling automatically or explicitly?  The magic is effectively on the caller side, and the caller can see the parameter is RefOutParam, so it's already explicit.

Comment 37

[(no longer active)]

(In reply to Jeff Gilbert [:jgilbert] from comment #32)
> > > > > This ultimately doesn't seem to provide value in excess of the friction that
> > > > > learning and using a non-standard API imparts.
> > > > > 
> > > > > I work in code that extensively uses out-params (both refcounted and
> > > > > otherwise), but I don't think this patch will be useful to me.
> > > > 
> > > > I don't understand why not.  Perhaps you have different needs?  If you have
> > > > additional use cases, please feel free to raise them.
> > > 
> > > Because writing good code doesn't require this new construct. It seems to be
> > > that the things you mention can all be handled explicitly at the callsite:
> > 
> > It seems like you're dividing the code into two categories, good and bad,
> > and arguing that bad code deserves what it gets!  ;-)  That is not a useful
> > way of thinking about this.  The value in the work being done here is to
> > make it simpler to write code that will do the right thing no matter whether
> > you get things right in every single call site until the end of times.
> 
> There is a point where we decide that a pattern is bad enough that we should
> not allow it. I do not think adding a crutch for users of these patterns is
> the right answer here.
> > 
> > Also, note that this is not a niche mistake that novice programmers make.  I
> > cannot count the number of times that I have screwed things up in code that
> > uses nsCOMPtr<T>* as out arguments, and that doesn't even get close to the
> > number of times that I have missed this when reviewing other people's code.
> 
> Examples please then

I don't mean to shut you down here, but I really don't have time to go and dig out examples, since the examples that you're asking me to bring will take me days to find again.  You're welcome to look at the history of editor/ for example but since the fixes have not been tagged in any way, you'd need to go back to the bugs for an analysis of problems, which is not really worth anyone's time, IMO.

I'd prefer if you believe that the problems do exist.

>, but this class of errors seems largely solvable by
> MOZ_MUST_USE. I'm going to file a bug that adds a simple bool type with
> MOZ_MUST_USE.

MOZ_MUST_USE is only useful to mark specific return _types_ as impossible to ignore at call sites.  It's not really related to the issue at hand.

> If a function is telling you it failed, *never ever pretend it succeeded*.

Functions don't always have a simple "failed" or "not-failed" state, they can do a bunch of different work, some of them succeeding some of them not.  (Arguing that functions should be small things that only do one thing is fair in the abstract, but not really relevant to Gecko in practice. ;-)

> I don't think we need a new ref-counted flavor to handle this.
> 
> > 
> > > >   nsCOMPtr<T> foo;
> > > >   // Do some work, possibly assign to foo.
> > > >   GetNewFoo(&foo);
> > > >   if (foo) {
> > > >     // Surely we must have a new foo in this case, right?  Nope!
> > > >   }
> > > Should be:
> > > >   nsCOMPtr<T> foo;
> > > >   // Do some work, possibly assign to foo.
> > > > + foo = nullptr;
> > > >   GetNewFoo(&foo);
> > > >   if (foo) {
> > > > *   // foo is definitely new
> > > >   }
> > > 
> > > Why is `foo` being reused here anyways?
> > 
> > Because that is what people do.  Either intentionally without thinking about
> > this issue, or accidentally by moving code around, etc.
> 
> You have the same issue with any non-refcounted outparams. This seems like a
> pretty general bug opportunity that as writers and reviewers we should be
> watching for.

Sure.

> > > If it's actually mFoo, we should
> > > either check the return value for GetNewFoo, clear mFoo to null before a
> > > fallible-new, or there should be a temporary local `foo` that we use here,
> > > and only assign to mFoo if `foo` ends up being valid.
> > 
> > Sure, you're explaining how we can solve these mistakes when we find them,
> > and I understand how to do that.  We're trying to make these mistakes
> > impossible to happen though, so that we don't have to find and fix them
> > later.
> 
> Fix it in the callee then. I don't see why a new construct is required. If
> someone's going to break a function contract, they're going to break a
> function contract. I don't think we need to go full "safety-scissors" on
> this.

This seems to be the gist of your argument, if I'm understanding you correctly.  I disagree very strongly.  Getting this stuff wrong will cause crashes/sec-crits, so it is absolutely worth going full safety-scrissors on this.

If you don't agree on that, then I don't think there's much else to agree on here.  :-)

Comment 38

[Kelsey Gilbert [:jgilbert]]

Getting it wrong with or without this new construct will cause crashes and sec-crits. The fix proposed here seems placebo.

I strongly suspect your ROI will be higher with MOZ_WARN_UNUSED_RESULT and rearchitecting saner failure modes.

As long as the usage of this construct won't end up being universally prescribed, I won't hold this up.

Comment 39

[Nathan Froyd [:froydnj]]

Comment on attachment 8645760 [details] [diff] [review]
Updated patch

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

Obviously it's been a while since this request has been submitted; I'm going to clear it for now.

I tried making getter_AddRefs complain if non-null smart pointers were passed to it (i.e. "why don't we just use RefPtr<T>* and rely on programmers to DTRT?" argument), and the resulting amount of fixes just to get the browser to start were substantial.  Some of the places were easy to fix, some were not; almost all of the instances were writing reasonable code and not obviously relying on the null-on-getter_AddRefs.  Nearly all sites *were* doing proper error-checking in any event.  I didn't really want to spend a bunch of time making the test suites clean with such patches.

I am starting to think that it's worthwhile to have a richer set of "annotation types" for function parameters, I just haven't been brought around on this particular patch yet.