1208814 - Prevent the default copy constructor of refcounted objects to be used

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[(no longer active)]

Consider this code:

class RandomRefCountedClass : public nsISupports
{
  NS_DECL_ISUPPORTS
  already_AddRefed<RandomRefCountedClass> Clone() {
    nsRefPtr<RandomRefCountedClass> copy = new RandomRefCountedClass(*this);
    return copy.forget();
  }
  nsCOMPtr<nsIFoo> mFoo;
  // ...
};

Here we are using the default constructor of the object thinking that we're only copying mFoo and that's fine, but we have a hidden mRefCnt in the macro NS_DECL_ISUPPORTS that we'd copy as well, as a result, the refcount of the object is out of balance, and in this case we probably leak the cloned object.

We should prevent this general pattern.  This is actual code that I wrote.  We can do that by looking for default copy constructors that are called for a refcounted object.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Do we require that a copy constructor always be explicitly deleted or implemented, or do you want to only prevent calls to the default copy constructor?

Comment 2

[(no longer active)]

(In reply to Michael Layzell [:mystor] from comment #1)
> Do we require that a copy constructor always be explicitly deleted or
> implemented, or do you want to only prevent calls to the default copy
> constructor?

I think requiring the copy ctor being explicitly deleted is a pain since that means that you need even more boiler plate code for all refcounted objects.  Let's just prevent calls to the default copy ctor.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #2)
> (In reply to Michael Layzell [:mystor] from comment #1)
> > Do we require that a copy constructor always be explicitly deleted or
> > implemented, or do you want to only prevent calls to the default copy
> > constructor?
> 
> I think requiring the copy ctor being explicitly deleted is a pain since
> that means that you need even more boiler plate code for all refcounted
> objects.  Let's just prevent calls to the default copy ctor.

Should we also prevents calls to the default assignment operator= to prevent code like:

nsRefPtr<Foo> f1 = ...;
nsRefPtr<Foo> f2 = ...;
*f2 = *f1;

Because that is also wrong (although probably much less likely to occur).

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Add an analysis to prevent default copy constructors from being called on refcounted objects

I require that refcounted objects have an mRefCnt member in this patch to be considered refcounted.

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Don't use the default copy constructor in nsNavHistoryQuery::Clone()

I'm not sure if you're the best person to review this change - you can forward it to someone else if someone else would be more appropriate.

Comment 6

[(no longer active)]

(In reply to Michael Layzell [:mystor] from comment #3)
> (In reply to Ehsan Akhgari (don't ask for review please) from comment #2)
> > (In reply to Michael Layzell [:mystor] from comment #1)
> > > Do we require that a copy constructor always be explicitly deleted or
> > > implemented, or do you want to only prevent calls to the default copy
> > > constructor?
> > 
> > I think requiring the copy ctor being explicitly deleted is a pain since
> > that means that you need even more boiler plate code for all refcounted
> > objects.  Let's just prevent calls to the default copy ctor.
> 
> Should we also prevents calls to the default assignment operator= to prevent
> code like:
> 
> nsRefPtr<Foo> f1 = ...;
> nsRefPtr<Foo> f2 = ...;
> *f2 = *f1;
> 
> Because that is also wrong (although probably much less likely to occur).

Yes, in a separate bug!

Comment 7

[(no longer active)]

Comment on attachment 8667362 [details] [diff] [review]
Part 1: Add an analysis to prevent default copy constructors from being called on refcounted objects

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

::: build/clang-plugin/clang-plugin.cpp
@@ +1412,5 @@
> +      DiagnosticIDs::Error, "Invalid use of compiler-provided copy constructor "
> +                            "on refcounted type");
> +  unsigned NoteID = Diag.getDiagnosticIDs()->getCustomDiagID(
> +      DiagnosticIDs::Note, "This constructor will incorrectly copy the mRefCnt "
> +                           "property, and leak memory");

Can you please add "Please provide your own copy constructor which only copies the fields that need to be copied." to the note?  Also I think it would be better to replace "and leak memory" with "and cause reference count imbalance issues".

::: build/clang-plugin/tests/TestRefCountedCopyConstructor.cpp
@@ +16,5 @@
> +  int mRefCnt;
> +};
> +
> +// Implicit copy constructor which is used
> +class RC3 {

Hmm, why do you have RC3?  I think you can drop it and just use RC1.

Comment 8

[(no longer active)]

Comment on attachment 8667365 [details] [diff] [review]
Part 2: Don't use the default copy constructor in nsNavHistoryQuery::Clone()

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

::: toolkit/components/places/nsNavHistoryQuery.cpp
@@ -1325,3 @@
>    NS_ENSURE_TRUE(clone, NS_ERROR_OUT_OF_MEMORY);
>  
> -  clone->mRefCnt = 0; // the clone doesn't inherit our refcount

Ouch!

Comment 9

[Nika Layzell [:nika] (ni? for response)]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #7)
> Comment on attachment 8667362 [details] [diff] [review]
> Part 1: Add an analysis to prevent default copy constructors from being
> called on refcounted objects
> 
> Review of attachment 8667362 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: build/clang-plugin/clang-plugin.cpp
> @@ +1412,5 @@
> > +      DiagnosticIDs::Error, "Invalid use of compiler-provided copy constructor "
> > +                            "on refcounted type");
> > +  unsigned NoteID = Diag.getDiagnosticIDs()->getCustomDiagID(
> > +      DiagnosticIDs::Note, "This constructor will incorrectly copy the mRefCnt "
> > +                           "property, and leak memory");
> 
> Can you please add "Please provide your own copy constructor which only
> copies the fields that need to be copied." to the note?  Also I think it
> would be better to replace "and leak memory" with "and cause reference count
> imbalance issues".

Yup, that's a much nicer error message :)

> ::: build/clang-plugin/tests/TestRefCountedCopyConstructor.cpp
> @@ +16,5 @@
> > +  int mRefCnt;
> > +};
> > +
> > +// Implicit copy constructor which is used
> > +class RC3 {
> 
> Hmm, why do you have RC3?  I think you can drop it and just use RC1.

Umm... Good question. I think originally I thought I might need them to be separate (I wrote the tests before I wrote the patch), but in the end I didn't.

Comment 10

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Add an analysis to prevent default copy constructors from being called on refcounted objects

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=53934ef52666

Comment 11

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Add an analysis to prevent default copy constructors from being called on refcounted objects

oops - fixed the test to actually represent the change in note message

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 8667365 [details] [diff] [review]
Part 2: Don't use the default copy constructor in nsNavHistoryQuery::Clone()

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

good ol' code

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/afec4036eb61
https://hg.mozilla.org/integration/mozilla-inbound/rev/b80a3b03babb

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/afec4036eb61
https://hg.mozilla.org/mozilla-central/rev/b80a3b03babb