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

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.

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.

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

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.

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

(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

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.

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.

(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 :) ]

(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!

(void**) shouldn't be needed.

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

Simplified the return statement of NS_CreateJSArgv

(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!!

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.

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

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

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

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

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