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

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

One of them is: nsJSArgArray

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

I think that all that is needed for this is:
1. Delete the whole "struct HasDangerousPublicDestructor<nsJSArgArray>"
  http://mxr.mozilla.org/mozilla-central/source/dom/base/nsJSEnvironment.cpp#3089
2. Turn |ret| into an nsRefPtr<nsJSArgArray> in NS_CreateJSArgv and remove the |delete ret|.
While you are there, delete the null check on ret, because new is infallible now.

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

Comment on attachment 8452821 [details] [diff] [review]
Bug 1034910 - Remove dangerous public destructor of nsJSArgArray r=mccr8

I think you forgot to qref or something, as the patch is empty.

	Shashank VRSN Sabniveesu Assignee
	Comment 4 • 10 years ago

Funny that I submitted an empty one! I feel bad for the extra work I caused

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

Comment on attachment 8453926 [details] [diff] [review]
Fixed part-2 as part-1 seems to be fixed already

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

What you have here looks fine aside from the indentation, but you still need to:
- Delete HasDangerousPublicDestructor<nsJSArgArray> (and the forward declaration of nsJSArgArray right before it)
- Make the destructor for nsJSArgArray protected
and of course, make sure it still compiles. :)

::: dom/base/nsJSEnvironment.cpp
@@ +3237,1 @@
>                                         static_cast<JS::Value *>(argv), &rv);

You should fix the indentation here on this line with static_cast.

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

> I feel bad for the extra work I caused
Don't feel bad, it only took me a few seconds to realize it was empty and then needinfo you. :)

	Shashank VRSN Sabniveesu Assignee
	Comment 7 • 10 years ago

(In reply to Andrew McCreight [:mccr8] from comment #5)
> - Delete HasDangerousPublicDestructor<nsJSArgArray> (and the forward
> declaration of nsJSArgArray right before it)
This is what happens when one forgets to update one's codebase to the latest. Apparently I haven't updated my codebase since Jun 23, when this code was added.

You could have commented on my '(old)Patch Description' itself!

> - Make the destructor for nsJSArgArray protected
> and of course, make sure it still compiles. :)
Done. It compiles!

> ::: dom/base/nsJSEnvironment.cpp
> @@ +3237,1 @@
> >                                         static_cast<JS::Value *>(argv), &rv);
> 
> You should fix the indentation here on this line with static_cast.
I moved the 'static_cast..' part to align with arguments in its previous line

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

Comment on attachment 8454032 [details] [diff] [review]
Bug 1034910 - Remove dangerous public destructor of nsJSArgArray r=mccr8

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

Looks good to me!  I'll pass this over to Olli to review.  Thanks for the patch.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 9 • 10 years ago

Comment on attachment 8454032 [details] [diff] [review]
Bug 1034910 - Remove dangerous public destructor of nsJSArgArray r=mccr8

ret.swap(aArray); or similar would be nice (and then drop ret->QueryInterface), but this is fine too. Not performance critical at all.

	Shashank VRSN Sabniveesu Assignee
	Comment 10 • 10 years ago

(In reply to Andrew McCreight [:mccr8] from comment #8)
> Looks good to me!  I'll pass this over to Olli to review.  Thanks for the
> patch.

Thanks for the opportunity & support. I have a doubt. While I was halfway through implementing this and testing [intentionally done :-)], I received this error:

../../dist/include/nsISupportsImpl.h:91:3: error: static assertion failed: Reference-counted class nsJSArgArray should not have a public destructor. Try to make this class's destructor non-public. If that is really not possible, you can whitelist this class by providing a HasDangerousPublicDestructor specialization for it.

What are Reference-counted classes and why can't they have public destructors?
[I'm reading https://en.wikipedia.org/wiki/Reference_counting but would like to know from you; If this is not the place to ask, warn me to use IRC :) ]

	Shashank VRSN Sabniveesu Assignee
	Comment 11 • 10 years ago

(In reply to Olli Pettay [:smaug] from comment #9)
> Comment on attachment 8454032 [details] [diff] [review]
> Bug 1034910 - Remove dangerous public destructor of nsJSArgArray r=mccr8
> 
> ret.swap(aArray); or similar would be nice (and then drop
> ret->QueryInterface), but this is fine too. Not performance critical at all.

Do you mean to change - 
'return ret->QueryInterface(NS_GET_IID(nsIArray), (void **)aArray); '
to

'return ret.swap((void **)aArray);' ?
If yes, I'd do that now!

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 12 • 10 years ago

(void**) shouldn't be needed.

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

> 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.

	Shashank VRSN Sabniveesu Assignee
	Comment 14 • 10 years ago

Simplified the return statement of NS_CreateJSArgv

	Shashank VRSN Sabniveesu Assignee
	Comment 15 • 10 years ago

(In reply to Andrew McCreight [:mccr8] from comment #13)
> > 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
> ...

Woww! Well explained. That is very helpful! Now that I get an idea of what's happening with my changes, I'd look at bugs in 'Depends on' of BUG 1028132! Thank you very much mccr8!!

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 16 • 10 years ago

Comment on attachment 8454070 [details] [diff] [review]
Bug 1034910 - Remove dangerous public destructor of nsJSArgArray; simplify the return statement of NS_CreateJSArgv r=smaug

Did you compile this?
swap() doesn't return anything, so I don't quite see
how return ret.swap(aArray) could work.
I guess you're just missing return NS_OK;

And use nsCOMPtr for an interface, not nsRefPtr.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 18 • 10 years ago

Comment on attachment 8454093 [details] [diff] [review]
Bug 1034910 - Remove dangerous public destructor of nsJSArgArray; change type of 'ret' r=smaug

Thanks

	neil@parkwaycc.co.uk
	Comment 19 • 10 years ago

(In reply to Olli Pettay from comment #9)
> ret.swap(aArray); or similar would be nice

Please only use swap for true swaps! For setting outparams, ret.forget(aArray) is better (and also allows the outparam to be a base class of the smart pointer, although that's not necessary in this case).

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

I'll change it to .forget() and do a try run today.

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

try run: https://tbpl.mozilla.org/?tree=Try&rev=f136c8fe434e

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

I changed the swap to a forget. Carrying forward smaug's r+.

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c366c305105

	Carsten Book [:Tomcat]
	Comment 24 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/7c366c305105