Closed
Bug 1036052
Opened 10 years ago
Closed 10 years ago
Remove dangerous public destructor of DOMRect
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: bjacob, Assigned: anujagarwal464, Mentored)
References
Details
(Whiteboard: [lang=c++])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
baku
:
review+
mccr8
:
feedback+
|
Details | Diff | Splinter Review |
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
Updated•10 years ago
|
Mentor: continuation
Whiteboard: [lang=c++]
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → anujagarwal464
Comment 2•10 years ago
|
||
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-
Assignee | ||
Comment 3•10 years ago
|
||
Comment on attachment 8454907 [details] [diff] [review] bug1036052.diff @Andrew: Yes it compiles.
Attachment #8454907 -
Flags: feedback- → feedback?(bugs)
Comment 5•10 years ago
|
||
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-
Assignee | ||
Comment 6•10 years ago
|
||
@smaug, I did only content/* because my system can't handle full rebuild. Waiting for try access.
Comment 7•10 years ago
|
||
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.
Assignee | ||
Comment 8•10 years ago
|
||
@Andrew: Thanks!
Attachment #8454907 -
Attachment is obsolete: true
Attachment #8482885 -
Flags: feedback?(continuation)
Comment 9•10 years ago
|
||
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 11•10 years ago
|
||
Comment on attachment 8482885 [details] [diff] [review] Bug1036052.diff Try run looks good.
Attachment #8482885 -
Flags: review?(amarchesini)
Updated•10 years ago
|
Attachment #8482885 -
Flags: review?(amarchesini) → review+
Updated•10 years ago
|
Keywords: checkin-needed
Comment 12•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d687e266e65
Keywords: checkin-needed
Comment 13•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2d687e266e65
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•