1028132 - Fix all the temporarily whitelisted dangerous public destructors of refcounted classes

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

In bug 1027251 we removed all dangerous public destructors of XPCOM-refcounted classes outside of a finite whitelist, see HasDangerousPublicDestructor. Now we are going over the entries in this whitelist.

See blockers list.

Comment 1

[Andrew McCreight [:mccr8]]

Me or somebody else should do a little writeup here about how to fix these bugs.  The basic thing is to delete the whitelist block and make it compile, but it should also have a little bit of an explanation about what we're fixing, and common patterns that will need to be fixed.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Andrew: +1! Please do!

Comment 3

[Andrew McCreight [:mccr8]]

Here's an explanation of what the goal is of this bug I gave in bug 1034910:

> What are Reference-counted classes and why can't they have public destructors?

A reference counted class does not really use the normal fully manually memory management of C++.

Normally in C++, you'd say something like
Foo* x = new Foo();
// use x
// later, you know for sure you are done with x, so you destroy it.
delete x;

With a reference counted object, you aren't not as sure when it will go away, so you just have various places claiming that they hold the object alive.  They do this by calling an Addref() method (often hidden inside the nsRefPtr<> class) when they want to hold the object alive (this increases the reference count), and calling Release() method when they are done with it (this decreases the reference count).  If the reference count becomes 0 in Release(), then we delete the object.

The problem is that if the destructor is public, then any code can call delete on the object, even if the refcount is greater than 0.  This will cause any pointers to the object to end up pointing to dead objects, which can cause security problems when another object is allocated in the same space.

The purpose of this bug, and the other bugs blocking bug 1028132, is to make it less likely that some random code will delete an object with a nonzero reference count, by making the destructor protected.  There is some fancy C++ template stuff I don't know much about that enforces that, and that HasDangerousPublicDestructor<nsJSArgArray> class you deleted tells it to not produce an error for that.

Comment 4

[Andrew McCreight [:mccr8]]

The basic approach to fixing a class Foo is:
1. Remove the HasDangerousPublicDestructor<Foo> declaration.
2. Make the destructor of Foo either private or protected.  If there isn't one, then it is using the default one, so just create a new empty destructor and make it private.
3. Fix any compilation errors that come up from places that are trying to manually destroy an instance of Foo.

Some common patterns are:

a) raw pointer delete:
Foo* x = new Foo();
...
if (something failed) {
  delete x; // under some circumstance
  return NS_ERROR_FAILURE;
}
NS_ADDREF(*aReturn = x);

In this case, you want to turn the Foo* into an nsRefPtr<>.  That will addref x when it is created, and release x when it leaves the scope, so you can get rid of the delete.  Then you can change the NS_ADDREF line to x.forget(aReturn), and it will transfer x into the return value.  This requires a little bit of knowledge of how the Firefox ref counting works.

b) stack-allocated object
In this case, the object is allocated directly on the stack:
Foo x(a,b);
When x goes out of scope, it will have its destructor called directly, which is not good.
To fix this, change it to
nsRefPtr<Foo> x = new Foo(a, b);
x will still be destroyed when it goes out of scope (probably), it will just be allocated on the heap.
x is now a pointer, not a direct reference, so you'll have to add pointer indirection to the uses of x, which the compiler should tell you about.

For some examples, check out the bugs blocking this one that have a line through them, as they have had fixes landed.

Comment 5

[Mike Hoye [:mhoye]]

This is great, thanks.

Comment 6

[Andrew McCreight [:mccr8]]

Another issue: if you convert a field mField of an object from Foo to nsRefPtr<Foo>, you need to make sure that mField is initialized to a new instance. So you have to add something like:
  mFoo(new Foo())
to the constructor.

Comment 7

[Andrew McCreight [:mccr8]]

We're down to only a handful of these in the tree.  I filed two more just now, but they are not new instances of it, just a few cases that slipped between the cracks in the initial bugstorm.

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Thanks a lot to all the contributors who have been helping and Andrew for mentoring them and fixing particularly nontrivial ones yourself!

Comment 9

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

Attachment: Remove mozilla::HasDangerousPublicDestructor<T>

When my patches for bug 1034922 and bug 1073219 land, HasDangerousPublicDestructor<T> will no longer be necessary.

This patch removes HasDangerousPublicDestructor<T>.

Comment 10

[Andrew McCreight [:mccr8]]

Comment on attachment 8630829 [details] [diff] [review]
Remove mozilla::HasDangerousPublicDestructor<T>

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

Nice!  Thanks for finishing this off.

Comment 11

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

Please land after bug 1073219

Comment 12

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

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=46afc1f50209

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/576d817e46ba

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/576d817e46ba