Closed Bug 1208814 Opened 7 years ago Closed 7 years ago

Prevent the default copy constructor of refcounted objects to be used

Categories

(Developer Infrastructure :: Source Code Analysis, defect)

defect
Not set
normal

Tracking

(firefox44 fixed)

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: nika)

Details

Attachments

(2 files, 2 obsolete files)

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.
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?
(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.
(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).
Assignee: nobody → michael
I require that refcounted objects have an mRefCnt member in this patch to be considered refcounted.
Attachment #8667362 - Flags: review?(ehsan)
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.
Attachment #8667365 - Flags: review?(ehsan)
(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 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.
Attachment #8667362 - Flags: review?(ehsan) → review+
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!
Attachment #8667365 - Flags: review?(ehsan) → review?(mak77)
(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.
oops - fixed the test to actually represent the change in note message
Attachment #8667393 - Attachment is obsolete: true
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
Attachment #8667365 - Flags: review?(mak77) → review+
https://hg.mozilla.org/mozilla-central/rev/afec4036eb61
https://hg.mozilla.org/mozilla-central/rev/b80a3b03babb
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Product: Core → Firefox Build System
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.