Benoit Jacob [:bjacob] (mostly away) Reporter
	Description • 10 years ago

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.

	Andrew McCreight [:mccr8]
	Comment 1 • 10 years ago

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.

	Benoit Jacob [:bjacob] (mostly away) Reporter
	Comment 2 • 10 years ago

Andrew: +1! Please do!

	Andrew McCreight [:mccr8]
	Comment 3 • 10 years ago

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.

	Andrew McCreight [:mccr8]
	Comment 4 • 10 years ago

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.

	Mike Hoye [:mhoye]
	Comment 5 • 10 years ago

This is great, thanks.

	Andrew McCreight [:mccr8]
	Comment 6 • 10 years ago

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.

	Andrew McCreight [:mccr8]
	Comment 7 • 10 years ago

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.

	Benoit Jacob [:bjacob] (mostly away) Reporter
	Comment 8 • 10 years ago

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

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 9 • 9 years ago

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

This patch removes HasDangerousPublicDestructor<T>.

	Andrew McCreight [:mccr8]
	Comment 10 • 9 years ago

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

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

Nice!  Thanks for finishing this off.

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 11 • 9 years ago

Please land after bug 1073219

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 12 • 9 years ago

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

	Pulsebot
	Comment 13 • 9 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 14 • 9 years ago

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