859817 - Eliminate implicit conversion from raw pointer to already_AddRefed

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

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

Probably would be painful, but would avoid certain classes of bugs.

Comment 1

[(no longer active)]

Can you give an example of such bugs please?

Comment 2

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

Bug 857102 comment 26.  In that case, it caused an immediate crash on startup, so it was pretty easy to spot, but in other cases it could be harder to trigger.  Specifically, in that case I had a function that returned already_AddRefed<> and just returned the result of "new".  I was replacing an NS_NewFoo() function with just using the new operator after making the function infallible, which works fine in the common case when you're assigning to an nsCOMPtr/nsRefPtr, but in one case NS_NewFoo() was being returned, and that broke when I converted to new.

Comment 3

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

Attachment: Patch

Obviously, this needs a few minor touch-ups to the rest of the tree before it actually compiles.  But is this patch the right change to nsCOMPtr.h to make this work?  If I just added "explicit", then "return nullptr;" would fail, so I added the overload on nullptr_t, which is why I include NullPtr.h and cstddef.  I could probably get away without the cstddef if I could use decltype -- the gcc and VS versions that support nullptr_t seem to support decltype too, dunno about clang.  (The gcc version of the header just defines nullptr_t as decltype(nullptr).)

It was suggested I overload on int and MOZ_ASSERT that it's zero for other compilers, to give people with old compilers the benefits of the explicitness here.  For this patch I mostly wanted to get it working locally so I could fix all the tree breakage.  I don't know if it's worth it to add more code to support old compilers.

Comment 4

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

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

Boris wanted me to request super-review from bsmedberg on the basic concept.

Comment 5

[Benjamin Smedberg]

I must admit, I'm less concerned about the constructor size of things as I am about not leaking them going out.

How big is the treewide change, and how ugly does it make things? Probably whether this is worthwhile depends primarily on how much churn and ugly code it causes.

Comment 6

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

The treewide change is currently

 86 files changed, 283 insertions(+), 370 deletions(-)

and counting.  I foresee it as being much smaller than the nsresult changes I did, but no way to tell until I'm done.  As you can see from the stat, I'm making things shorter overall -- it's mostly just replacing raw pointers and NS_ADDREF with nsCOMPtr/nsRefPtr and associated machinery.  Almost all the changes make the code easier to understand and less error-prone anyway, IMO.

I'll flag you when I post the final patch (which isn't done yet).

Comment 7

[Boris Zbarsky [:bzbarsky]]

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

This looks fine to me.

Comment 8

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

Attachment: Introduce already_AddRefed.downcast()

You suggested it, so you get to review it.  :)

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 736804 [details] [diff] [review]
Introduce already_AddRefed.downcast()

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

Given that I wrote the function, I'm not going to also review it :)

::: widget/gtk2/nsIdleServiceGTK.h
@@ +38,3 @@
>          }
>  
> +        return idleService.forget();

This doesn't appear to belong here?

Comment 10

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

Attachment: Make NS_NewAtom return already_AddRefed

This patch actually just makes everything slightly uglier in practice, but it makes the addrefs/releases more explicit, so I suppose it's a good thing anyway.  The ugliness will disappear as things move away from nsIAtom** out-params, I guess.

Comment 11

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

Attachment: Make nsStringBuffer::Alloc return already_AddRefed

Most callers take ownership in a non-nsRefPtr way, so this also doesn't actually make things prettier in most cases, but it does make them more explicit.

Comment 12

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

(In reply to :Ms2ger from comment #9)
> ::: widget/gtk2/nsIdleServiceGTK.h
> @@ +38,3 @@
> >          }
> >  
> > +        return idleService.forget();
> 
> This doesn't appear to belong here?

I converted that to use an nsRefPtr while I was at it, because that was what necessitated the .downcast() in the first place.

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to :Aryeh Gregor from comment #12)
> (In reply to :Ms2ger from comment #9)
> > ::: widget/gtk2/nsIdleServiceGTK.h
> > @@ +38,3 @@
> > >          }
> > >  
> > > +        return idleService.forget();
> > 
> > This doesn't appear to belong here?
> 
> I converted that to use an nsRefPtr while I was at it, because that was what
> necessitated the .downcast() in the first place.

I completely missed the downcast. Never mind me :)

Comment 14

[(no longer active)]

Comment on attachment 736804 [details] [diff] [review]
Introduce already_AddRefed.downcast()

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

::: content/base/src/nsDocument.cpp
@@ +4731,5 @@
>                    nullptr, mDefaultElementType, getter_AddRefs(content));
>    if (rv.Failed()) {
>      return nullptr;
>    }
> +  return content.forget().downcast<Element>();

AsElement does assert to make sure the cast is valid, so please don't change these callsites here.

::: widget/gtk2/nsIdleServiceGTK.h
@@ +38,3 @@
>          }
>  
> +        return idleService.forget();

Ms2ger's right.

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #14)
> Comment on attachment 736804 [details] [diff] [review]
> Introduce already_AddRefed.downcast()
> 
> Review of attachment 736804 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/base/src/nsDocument.cpp
> @@ +4731,5 @@
> >                    nullptr, mDefaultElementType, getter_AddRefs(content));
> >    if (rv.Failed()) {
> >      return nullptr;
> >    }
> > +  return content.forget().downcast<Element>();
> 
> AsElement does assert to make sure the cast is valid, so please don't change
> these callsites here.

Adding the assertion here is good, I guess, but this remains the result of Create*Elem*.

> ::: widget/gtk2/nsIdleServiceGTK.h
> @@ +38,3 @@
> >          }
> >  
> > +        return idleService.forget();
> 
> Ms2ger's right.

No, I'm not :)

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 736813 [details] [diff] [review]
Make NS_NewAtom return already_AddRefed

r=me, but at this point NS_NewAtom and do_GetAtom are identical. Maybe a followup to get rid of one or the other?

Comment 17

[(no longer active)]

(In reply to comment #15)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #14)
> > Comment on attachment 736804 [details] [diff] [review]
> > Introduce already_AddRefed.downcast()
> > 
> > Review of attachment 736804 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: content/base/src/nsDocument.cpp
> > @@ +4731,5 @@
> > >                    nullptr, mDefaultElementType, getter_AddRefs(content));
> > >    if (rv.Failed()) {
> > >      return nullptr;
> > >    }
> > > +  return content.forget().downcast<Element>();
> > 
> > AsElement does assert to make sure the cast is valid, so please don't change
> > these callsites here.
> 
> Adding the assertion here is good, I guess, but this remains the result of
> Create*Elem*.

Although adding the assertion would work here too, I'd still prefer a call to AsElement(), since that has known semantics.

> > ::: widget/gtk2/nsIdleServiceGTK.h
> > @@ +38,3 @@
> > >          }
> > >  
> > > +        return idleService.forget();
> > 
> > Ms2ger's right.
> 
> No, I'm not :)

Oh, yeah sorry missed the previous comment.  Ignore this please.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 736824 [details] [diff] [review]
Make nsStringBuffer::Alloc return already_AddRefed

nsStringBuffer::Alloc is not infallible, so you can't use that nullcheck.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 736824 [details] [diff] [review]
Make nsStringBuffer::Alloc return already_AddRefed

So yes, please restore the null-checks in nsAttrValue::GetStringBuffer, both versions of GetCertFingerprintByOidTag, nsCSSValue::BufferFromString, and nsTSubstring_CharT::MutatePrep and maybe add one in nsDocumentEncoder::EncodeToString.

> nsHtml5Atom::nsHtml5Atom(const nsAString& aString)
>+  buf.forget();

Please add a comment that we took ownership or something?

>+++ b/security/manager/ssl/src/nsClientAuthRemember.cpp
>+  RefPtr<nsStringBuffer> fingerprint(nsStringBuffer::Alloc(hash_len).get());

That's really confusing.  Also, this seems to assume that someone will take ownership of the buffer.... but I don't see who that someone would be.  As far as I can tell this code used to leak, and this patch keeps it leaking.  Can you please get someone who owns this code to review this bit?

> AtomImpl::AtomImpl(const nsAString& aString, PLDHashNumber aKeyHash)
>+  buf.forget();

Again, comment.

r=me with all the above fixed.

Comment 20

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

(In reply to Boris Zbarsky (:bz) from comment #18)
> Comment on attachment 736824 [details] [diff] [review]
> Make nsStringBuffer::Alloc return already_AddRefed
> 
> nsStringBuffer::Alloc is not infallible, so you can't use that nullcheck.

How can it fail?  All I see is it returning the result of malloc().

Comment 21

[Boris Zbarsky [:bzbarsky]]

malloc is fallible right now, as far as I know.  moz_xmalloc and new are infallible.

Comment 22

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

Comment on attachment 736824 [details] [diff] [review]
Make nsStringBuffer::Alloc return already_AddRefed

Brian, could you review the change to security/manager/ssl/src/nsClientAuthRemember.cpp here, as requested by Boris in comment 19?

Comment 23

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

Attachment: Introduce already_AddRefed.downcast(), v2

Comment 24

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

Try for first three patches: https://tbpl.mozilla.org/?tree=Try&rev=90aee32f3e2e

(I'll put further patches in dependent bugs to save bugspam here.)

Comment 25

[(no longer active)]

Comment on attachment 740074 [details] [diff] [review]
Introduce already_AddRefed.downcast(), v2

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

::: xpcom/glue/nsCOMPtr.h
@@ +197,5 @@
> +    template<class U>
> +    already_AddRefed<U> downcast()
> +    {
> +      U* tmp = static_cast<U*>(mRawPtr);
> +      mRawPtr = NULL;

Gah, why is this file using NULL?!  Please use nullptr here at least.

Comment 26

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

Attachment: Fix for NS_NewAtom patch

So I totally know I compiled my patch before I submitted it, but, um, it doesn't compile.  This fixes it, and also removes an unneeded null check that I missed.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 741774 [details] [diff] [review]
Fix for NS_NewAtom patch

r=me

Comment 28

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

Landed two patches:

https://hg.mozilla.org/integration/mozilla-inbound/rev/321ab55f16ba
https://hg.mozilla.org/integration/mozilla-inbound/rev/57a0302a88b7

Green try run for these: https://tbpl.mozilla.org/?tree=Try&rev=0a84b658935c

Note: I accidentally put this bug's number on bug 864286's patch.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/321ab55f16ba
https://hg.mozilla.org/mozilla-central/rev/57a0302a88b7
https://hg.mozilla.org/mozilla-central/rev/447cf900cd4f

Comment 30

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 736824 [details] [diff] [review]
Make nsStringBuffer::Alloc return already_AddRefed

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

::: security/manager/ssl/src/nsCertOverrideService.cpp
@@ +388,5 @@
>                             SECOidTag aOidTag, 
>                             nsCString &fp)
>  {
>    unsigned int hash_len = HASH_ResultLenByOidTag(aOidTag);
> +  nsRefPtr<nsStringBuffer> fingerprint = nsStringBuffer::Alloc(hash_len);

Please use RefPtr in PSM instead of nsRefPtr:

RefPtr<nsStringBuffer> fingerprint(nsStringBuffer::Alloc(hash_len).forget());

(PSM uses RefPtr because it is more consistent with ScopedPtr and the many variants that PSM defines for use with NSS types, compared to nsRefptr. This case shows why it is unfortunate that we have both RefPtr and nsRefPtr.)

::: security/manager/ssl/src/nsClientAuthRemember.cpp
@@ +103,5 @@
>                             SECOidTag aOidTag, 
>                             nsCString &fp)
>  {
>    unsigned int hash_len = HASH_ResultLenByOidTag(aOidTag);
> +  RefPtr<nsStringBuffer> fingerprint(nsStringBuffer::Alloc(hash_len).get());

Seems like it would be clearer as nsStringBuffer::Alloc(hash_len).forget()).

Comment 31

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

nsStringBuffer::Alloc, as of this patch, returns already_AddRefed<nsStringBuffer>.  There's no .forget() method.  Could you clarify what you mean?

Also, Boris wanted to know why the GetCertFingerprintByOidTag code that I'm changing in nsClientAuthRemember.cpp doesn't leak (both before and after the patch).  nsStringBuffer::Alloc returns an nsStringBuffer with a refcount of 1.  Assigning it to a RefPtr gives it a refcount of 2, which drops to 1 again when the function returns.  What takes ownership of the buffer and ensures that it's eventually freed?

Comment 32

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Fix GetCertFingerprintByOidTag

(In reply to :Aryeh Gregor from comment #31)
> nsStringBuffer::Alloc, as of this patch, returns
> already_AddRefed<nsStringBuffer>.  There's no .forget() method.  Could you
> clarify what you mean?

You are right. get() is OK.

> Also, Boris wanted to know why the GetCertFingerprintByOidTag code that I'm
> changing in nsClientAuthRemember.cpp doesn't leak (both before and after the
> patch).  nsStringBuffer::Alloc returns an nsStringBuffer with a refcount of
> 1.  Assigning it to a RefPtr gives it a refcount of 2, which drops to 1
> again when the function returns.  What takes ownership of the buffer and
> ensures that it's eventually freed?

Seems there's a few problems, including the leak, and including the fact that this function has two different implementations.

I rewrote this function to have one, simpler implementation that avoids these issues completely.

Comment 33

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

Comment on attachment 743298 [details] [diff] [review]
Fix GetCertFingerprintByOidTag

Well, this removes the nsStringBuffer::Alloc call, so it fixes the problem from my perspective.  Thanks!

Comment 34

[Honza Bambas (:mayhemer)]

Comment on attachment 743298 [details] [diff] [review]
Fix GetCertFingerprintByOidTag

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

::: security/manager/ssl/src/nsNSSCertHelper.cpp
@@ +2223,5 @@
> +                                 nsscert->derCert.len);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  char *tmpstr = CERT_Hexify(const_cast<SECItem*>(&digest.get()), 1);
> +  NS_ENSURE_TRUE(tmpstr, NS_ERROR_OUT_OF_MEMORY);

Just for curiosity: why isn't a method like "nsCString& Hexify()" part of Digest?

Comment 35

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 743298 [details] [diff] [review]
Fix GetCertFingerprintByOidTag

https://hg.mozilla.org/integration/mozilla-inbound/rev/696e20fda14b

Thanks for the review.

I do not think a Hexify method on Digest makes sense because there are a lot of places where we need to "hexify" things that are not digests. For example, toolkit/identity/IdentityCryptoService.cpp "hexifies" other things.

I will file a bug about switching toolkit/identity/IdentityCryptoService.cpp to use CERT_Hexify to avoid the code duplication.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/696e20fda14b

Comment 37

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

Comment on attachment 736824 [details] [diff] [review]
Make nsStringBuffer::Alloc return already_AddRefed

https://hg.mozilla.org/integration/mozilla-inbound/rev/866fa5faea3f

Green try (failures are from other patches): https://tbpl.mozilla.org/?tree=Try&rev=8cbdbebfff18

Checkin of the actual patch is now waiting on blocker bugs.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/866fa5faea3f

Comment 39

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

Do you (or anyone else) know how to explain the red on clang platforms here?

https://tbpl.mozilla.org/?tree=Try&rev=9db698a8c4bb

On OS X/Android, this breaks very quickly with the error

In file included from ../../../dist/include/nsStaticAtom.h:9:
In file included from ../../../dist/include/nsIAtom.h:18:
../../../dist/include/nsCOMPtr.h:153:27: error: no type named 'nullptr_t' in namespace 'std'
    already_AddRefed(std::nullptr_t aNullPtr)
                     ~~~~~^

The type std::nullptr_t seems to exist in my local version of clang (3.0) when I include cstddef in a simple test file, and it builds fine in gcc and VS.  Googling didn't help.  Anything else to try?

Comment 40

[(no longer active)]

std::nullptr_t only exists in libc++ (and maybe newer libstdc++'s, but definitely not in libstdc++ 4.2.1 which is what comes with OS X by default).  MOZ_HAVE_CXX11_NULLPTR is a check depending on the version of the compiler, not what's available on your system (which is one of my pet peeves with the way we do "feature detection" in MFBT, /cc Waldo), so you cannot rely on std::nullptr_t being available when MOZ_HAVE_CXX11_NULLPTR is defined.

Comment 41

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

Attachment: Patch v2

This is a new version of the patch that fixes comment 39.  From looking at online documentation, it seems all three major compilers supported decltype before or at the same time as nullptr, so this should work on all compilers.  If it doesn't, I guess I could add a separate test for decltype to mfbt, but it seems unlikely to get much use.  This builds on all our try platforms (-b d -p all).

Comment 42

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 747028 [details] [diff] [review]
Patch v2

r=me

Comment 43

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

Green try: https://tbpl.mozilla.org/?tree=Try&rev=02f0beb2edb8

https://hg.mozilla.org/integration/mozilla-inbound/rev/efa4fb561e5b

Comment 44

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/efa4fb561e5b

Comment 45

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/comm-central/rev/fd2f5a37ef5f
https://hg.mozilla.org/comm-central/rev/3f0c807df178