1036052 - Remove dangerous public destructor of DOMRect

Status: RESOLVED FIXED

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

Description

[Benoit Jacob [:bjacob] (mostly away)]

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

Comment 1

[Anuj Agarwal [:anujagarwal464]]

Attachment: bug1036052.diff

Comment 2

[Andrew McCreight [:mccr8]]

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.

Comment 3

[Anuj Agarwal [:anujagarwal464]]

Comment on attachment 8454907 [details] [diff] [review]
bug1036052.diff

@Andrew: Yes it compiles.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Yes, Andrew is right. We really need to fix ScrollAreaEvent.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 6

[Anuj Agarwal [:anujagarwal464]]

@smaug, I did only content/* because my system can't handle full rebuild. Waiting for try access.

Comment 7

[Andrew McCreight [:mccr8]]

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.

Comment 8

[Anuj Agarwal [:anujagarwal464]]

Attachment: Bug1036052.diff

@Andrew: Thanks!

Comment 9

[Andrew McCreight [:mccr8]]

Comment on attachment 8482885 [details] [diff] [review]
Bug1036052.diff

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

Looks good to me.

Comment 10

[Anuj Agarwal [:anujagarwal464]]

Try:
https://tbpl.mozilla.org/?tree=Try&rev=2edf6a7b2387

Comment 11

[Andrew McCreight [:mccr8]]

Comment on attachment 8482885 [details] [diff] [review]
Bug1036052.diff

Try run looks good.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d687e266e65

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2d687e266e65