Closed Bug 1036052 Opened 8 years ago Closed 7 years ago

Remove dangerous public destructor of DOMRect

Categories

(Core :: DOM: Core & HTML, defect)

Other Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: bjacob, Assigned: anujagarwal464, Mentored)

References

Details

(Whiteboard: [lang=c++])

Attachments

(1 file, 1 obsolete file)

In Bug 1035394 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: DOMRect
Mentor: continuation
Whiteboard: [lang=c++]
Assignee: nobody → anujagarwal464
Attached patch bug1036052.diff (obsolete) — Splinter Review
Attachment #8454907 - Flags: feedback?(bjacob)
Comment on attachment 8454907 [details] [diff] [review]
bug1036052.diff

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

Does this compile?  These classes were all marked this way because they needed additional changes beyond just making the dtor private.

I went through and looked at all of the uses of DOMRect, looking for places that were either raw pointers or just direct declarations and I found this:
  http://mxr.mozilla.org/mozilla-central/source/dom/events/ScrollAreaEvent.h#82

It looks like the field of mClientArea ScrollAreaEvent needs to be turned into an nsRefPtr<DOMRect>, though you may want to get feedback from :smaug about whether that is the right thing to do here.
Attachment #8454907 - Flags: feedback?(bjacob) → feedback-
Comment on attachment 8454907 [details] [diff] [review]
bug1036052.diff

@Andrew: Yes it compiles.
Attachment #8454907 - Flags: feedback- → feedback?(bugs)
Yes, Andrew is right. We really need to fix ScrollAreaEvent.
Comment on attachment 8454907 [details] [diff] [review]
bug1036052.diff

Anuj, did you do a full rebuild with the patch, or only content/* ?
Since I would imagine a full rebuild would have captured ScrollAreaEvent case.
Attachment #8454907 - Flags: feedback?(bugs) → feedback-
@smaug, I did only content/* because my system can't handle full rebuild. Waiting for try access.
Yeah, that's going to be a little trouble for these bugs, unless you going digging through manually.  Anyways, for the purpose of fixing this particular thing, you should just be able to also do a build of dom/events and it should show an error.
Attached patch Bug1036052.diffSplinter Review
@Andrew: Thanks!
Attachment #8454907 - Attachment is obsolete: true
Attachment #8482885 - Flags: feedback?(continuation)
Comment on attachment 8482885 [details] [diff] [review]
Bug1036052.diff

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

Looks good to me.
Attachment #8482885 - Flags: feedback?(continuation) → feedback+
Comment on attachment 8482885 [details] [diff] [review]
Bug1036052.diff

Try run looks good.
Attachment #8482885 - Flags: review?(amarchesini)
Attachment #8482885 - Flags: review?(amarchesini) → review+
https://hg.mozilla.org/mozilla-central/rev/2d687e266e65
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.