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: nsJSID

See bug 1028132 comment 3 and 4 for an explanation of how to fix this kind of bug.

Hi, can I work on this bug?

Absolutely!

If I make nsJSID destructor protected, it cannot be accessed by XPCJSID.cpp which gives me the following error: 1:59.52 c:\mozilla-central\js\xpconnect\src\XPCJSID.cpp(555) : error C2248: 'ns JSID::~nsJSID' : cannot access protected member declared in class 'nsJSID' I'm confused.Can someone help me out?

That's due to http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/xpcprivate.h#2814. See bug 1028132 comment 4 part b) which describes how to fix declarations like this.

Is this bug already assigned to Debkanya Mazumder? if not, I would give a try. Please let me know. Thanks!

(In reply to arul.subramaniam from comment #6) > Is this bug already assigned to Debkanya Mazumder? Yes, we should give them more than a day to fix it. :)

Comment on attachment 8464856 [details] [diff] [review] Making ~nsJSID() protected Review of attachment 8464856 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch! Other than a few minor fixups below, this looks good to me. Please address that comment, upload a new patch, and then ask :bholley for review. He's an XPConnect peer, so he can review this patch for real. ::: js/xpconnect/src/xpcprivate.h @@ +2749,5 @@ > const nsID& GetInvalidIID() const; > > protected: > + > + ~nsJSID(); The indentation here should be fixed to match the rest, plus you should remove the trailing whitespace on the previous line.

Hi Andrew, thank you for the review! Could you tell me how do I address the comment for the review?

Do you mean what is the process for addressing the comment? Update the patch to fix the whitespace and indentation, then upload the new version of the patch, and mark the old version of the patch obsolete. Let me know if that doesn't answer your question.

I've tried to fix the indentation and it looks fine there, but it does not appear the same here. I'm not sure what to do about that so any help would be appreciated. Trailing whitespace on the previous line has been removed.

Thanks. It looks like you are using a tab character to indent that, which is why it shows up here but not locally. You need to use 4 spaces instead.

Okay thank you, I think I got it.

Comment on attachment 8464932 [details] [diff] [review] Bug1034917- Remove Dangerous Public Destructor nsJSID Review of attachment 8464932 [details] [diff] [review]: ----------------------------------------------------------------- Now that mDetails is a pointer, we need to allocate it somewhere. If you fire up a build with this patch, I'd imagine that it would crash, because we never allocate mDetails.

How do I allocate mDetails?

(In reply to Debkanya Mazumder from comment #17) > How do I allocate mDetails? Add something like mDetails(new nsJSID()) to the initializer list in the constructor for nsJSCID.

Comment on attachment 8465655 [details] [diff] [review] Bug1034917 - Remove Dangerous Public Destructor nsJSID Review of attachment 8465655 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! r=bholley

try server run: https://tbpl.mozilla.org/?tree=Try&rev=99a14e37a159 I had to make the dtor virtual again or the compiler was complained.

Thanks for the patch! https://hg.mozilla.org/integration/mozilla-inbound/rev/334445a5419f

Thank you for the help!