Eliminate implicit conversion from raw pointer to already_AddRefed

RESOLVED FIXED in mozilla24

Status

()

--
enhancement
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: ayg, Assigned: ayg)

Tracking

Trunk
mozilla24
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 2 obsolete attachments)

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

Comment 1

6 years ago
Can you give an example of such bugs please?
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.
Created attachment 735816 [details] [diff] [review]
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.
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Attachment #735816 - Flags: review?(bzbarsky)
Comment on attachment 735816 [details] [diff] [review]
Patch

Boris wanted me to request super-review from bsmedberg on the basic concept.
Attachment #735816 - Flags: superreview?(benjamin)

Comment 5

6 years ago
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.
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).

Updated

6 years ago
Attachment #735816 - Flags: superreview?(benjamin) → superreview+
Comment on attachment 735816 [details] [diff] [review]
Patch

This looks fine to me.
Attachment #735816 - Flags: review?(bzbarsky) → review+
Created attachment 736804 [details] [diff] [review]
Introduce already_AddRefed.downcast()

You suggested it, so you get to review it.  :)
Attachment #736804 - Flags: review?(Ms2ger)
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?
Attachment #736804 - Flags: review?(ehsan)
Attachment #736804 - Flags: review?(Ms2ger)
Attachment #736804 - Flags: feedback+
Created attachment 736813 [details] [diff] [review]
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.
Attachment #736813 - Flags: review?(bzbarsky)
Created attachment 736824 [details] [diff] [review]
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.
Attachment #736824 - Flags: review?(bzbarsky)
(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.
(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

6 years ago
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.
Attachment #736804 - Flags: review?(ehsan) → review-
(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 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?
Attachment #736813 - Flags: review?(bzbarsky) → review+

Comment 17

6 years ago
(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 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 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.
Attachment #736824 - Flags: review?(bzbarsky) → review+
(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().
malloc is fallible right now, as far as I know.  moz_xmalloc and new are infallible.

Updated

6 years ago
Blocks: 59212
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?
Attachment #736824 - Flags: review?(bsmith)
Depends on: 864126
Created attachment 740074 [details] [diff] [review]
Introduce already_AddRefed.downcast(), v2
Attachment #736804 - Attachment is obsolete: true
Attachment #740074 - Flags: review?(ehsan)
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.)
Depends on: 864286

Comment 25

6 years ago
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.
Attachment #740074 - Flags: review?(ehsan) → review+
Created attachment 741774 [details] [diff] [review]
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.
Attachment #741774 - Flags: review?(bzbarsky)
Comment on attachment 741774 [details] [diff] [review]
Fix for NS_NewAtom patch

r=me
Attachment #741774 - Flags: review?(bzbarsky) → review+
Attachment #736813 - Flags: checkin+
Attachment #740074 - Flags: checkin+
Attachment #741774 - Flags: checkin+
Depends on: 866059
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()).
Attachment #736824 - Flags: review?(bsmith) → review+
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?
Flags: needinfo?(bsmith)
Created attachment 743298 [details] [diff] [review]
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.
Attachment #743298 - Flags: review?(honzab.moz)
Attachment #743298 - Flags: feedback?(ayg)
Flags: needinfo?(bsmith)
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!
Attachment #743298 - Flags: feedback?(ayg) → feedback+
Depends on: 867092
Depends on: 867096
Depends on: 867098
Depends on: 867101
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?
Attachment #743298 - Flags: review?(honzab.moz) → review+
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.
Attachment #743298 - Flags: checkin+
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.
Attachment #736824 - Flags: checkin+
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?
Flags: needinfo?(ehsan)

Comment 40

6 years ago
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.
Flags: needinfo?(ehsan)
Created attachment 747028 [details] [diff] [review]
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).
Attachment #735816 - Attachment is obsolete: true
Attachment #747028 - Flags: superreview?(benjamin)
Attachment #747028 - Flags: review?(bzbarsky)
Comment on attachment 747028 [details] [diff] [review]
Patch v2

r=me
Attachment #747028 - Flags: review?(bzbarsky) → review+

Updated

6 years ago
Attachment #747028 - Flags: superreview?(benjamin) → superreview+
https://hg.mozilla.org/mozilla-central/rev/efa4fb561e5b
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Depends on: 877191
You need to log in before you can comment on or make changes to this bug.