Closed
Bug 1028147
Opened 10 years ago
Closed 10 years ago
Remove dangerous public destructor of image::Decoder
Categories
(Core :: Graphics: ImageLib, defect)
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: bjacob, Assigned: anujagarwal464, Mentored)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
seth
:
review+
|
Details | Diff | Splinter Review |
In bug 1027251 we removed all 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: image::Decoder
Flags: needinfo?(seth)
Updated•10 years ago
|
Mentor: continuation
Comment 1•10 years ago
|
||
Anuj started looking at this. From the compile error he is getting, it looks like a bunch of things are using nsAutoPtr<Decoder>. These need to be converted to nsRefPtr<Decoder>. It looks like there are two places where this is happening:
http://mxr.mozilla.org/mozilla-central/search?string=nsAutoPtr%3CDecoder%3E&filter=[Nn]sAutoPtr%3CDecoder%3E
Updated•10 years ago
|
Assignee: nobody → anujagarwal464
Flags: needinfo?(seth)
Comment 2•10 years ago
|
||
Well, "a bunch" is two, I guess.
Comment 3•10 years ago
|
||
(If you don't want to fix this, Anuj, please just unassign yourself. I just want to set you as assigned to avoid other people coming along and looking at it without being aware somebody else is working on it.)
Assignee | ||
Comment 4•10 years ago
|
||
@Andrew: Thanks!
Attachment #8483660 -
Flags: feedback?(continuation)
Comment 5•10 years ago
|
||
Comment on attachment 8483660 [details] [diff] [review]
bug1028147.diff
Review of attachment 8483660 [details] [diff] [review]:
-----------------------------------------------------------------
Looks reasonable to me.
::: image/src/Decoder.h
@@ +167,5 @@
> }
>
> protected:
> + virtual ~Decoder();
> +
nit: trailing whitespace here.
Attachment #8483660 -
Flags: feedback?(continuation) → feedback+
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8483660 -
Attachment is obsolete: true
Comment 7•10 years ago
|
||
The try run is green, but come to think of it you should do an Android run, as it looks like some of this is Android code. You can use this for the try run:
try: -b d -p android -u all -t none
Assignee | ||
Comment 8•10 years ago
|
||
Try run Android: https://tbpl.mozilla.org/?tree=Try&rev=5c6e6558fb15
Comment 9•10 years ago
|
||
That didn't succeed. From the error, it looks like that's a different Decoder class, in the MPAPI namespace, so you should be able to just remove the AndroidMediaPluginHost changes.
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8483966 -
Attachment is obsolete: true
Assignee | ||
Comment 11•10 years ago
|
||
Comment 12•10 years ago
|
||
Comment on attachment 8484253 [details] [diff] [review]
bug1028147.diff
Given that there are now no Android changes, you probably don't need to do an Android run, but I guess it doesn't hurt.
Attachment #8484253 -
Flags: review?(seth)
Comment 13•10 years ago
|
||
Comment on attachment 8484253 [details] [diff] [review]
bug1028147.diff
Review of attachment 8484253 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good to me. Thanks for taking care of this!
You should push this ASAP or you're going to have to rebase, BTW.
Attachment #8484253 -
Flags: review?(seth) → review+
Updated•10 years ago
|
Keywords: checkin-needed
Comment 14•10 years ago
|
||
Keywords: checkin-needed
Comment 15•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in
before you can comment on or make changes to this bug.
Description
•