1028147 - Remove dangerous public destructor of image::Decoder

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

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

Comment 1

[Andrew McCreight [:mccr8]]

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

Comment 2

[Andrew McCreight [:mccr8]]

Well, "a bunch" is two, I guess.

Comment 3

[Andrew McCreight [:mccr8]]

(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.)

Comment 4

[Anuj Agarwal [:anujagarwal464]]

Attachment: bug1028147.diff

@Andrew: Thanks!

Comment 5

[Andrew McCreight [:mccr8]]

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.

Comment 6

[Anuj Agarwal [:anujagarwal464]]

Attachment: bug1028147.diff

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

Comment 7

[Andrew McCreight [:mccr8]]

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

Comment 8

[Anuj Agarwal [:anujagarwal464]]

Try run Android: https://tbpl.mozilla.org/?tree=Try&rev=5c6e6558fb15

Comment 9

[Andrew McCreight [:mccr8]]

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.

Comment 10

[Anuj Agarwal [:anujagarwal464]]

Attachment: bug1028147.diff

Comment 11

[Anuj Agarwal [:anujagarwal464]]

Try: https://tbpl.mozilla.org/?tree=Try&rev=06a630b522a7

Comment 12

[Andrew McCreight [:mccr8]]

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.

Comment 13

[Seth Fowler [:seth] [:s2h]]

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.

Comment 14

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/09699bf795ac

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/09699bf795ac