Closed Bug 1034917 Opened 5 years ago Closed 5 years ago

Remove dangerous public destructor of nsJSID

Categories

(Core :: XPConnect, defect)

Other Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: bjacob, Assigned: rimjhim.mazumder17, Mentored)

References

Details

(Whiteboard: [lang=c++])

Attachments

(1 file, 3 obsolete files)

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
Component: JavaScript Engine → XPConnect
Mentor: continuation
Whiteboard: [lang=c++]
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. :)
Attached patch Making ~nsJSID() protected (obsolete) — Splinter Review
Attachment #8464856 - Flags: review?(continuation)
Assignee: nobody → rimjhim.mazumder17
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.
Attachment #8464856 - Flags: review?(continuation)
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.
Attachment #8464856 - Attachment is obsolete: true
Attachment #8464913 - Flags: review?(bobbyholley)
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.
Attachment #8464913 - Attachment is obsolete: true
Attachment #8464913 - Flags: review?(bobbyholley)
Attachment #8464932 - Flags: review?(bobbyholley)
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.
Attachment #8464932 - Flags: review?(bobbyholley) → review-
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.
Attachment #8464932 - Attachment is obsolete: true
Attachment #8465655 - Flags: review?(bobbyholley)
Comment on attachment 8465655 [details] [diff] [review]
Bug1034917 - Remove Dangerous Public Destructor nsJSID

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

Looks good! r=bholley
Attachment #8465655 - Flags: review?(bobbyholley) → review+
try server run: https://tbpl.mozilla.org/?tree=Try&rev=99a14e37a159

I had to make the dtor virtual again or the compiler was complained.
Thank you for the help!
https://hg.mozilla.org/mozilla-central/rev/334445a5419f
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.