Force avatar and wallpaper decoding to use robust JPEG.

Force avatar and wallpaper decoding to use robust JPEG.

The next step in an upcoming CL will bind this separate path to a different JPEG library (at a minimum on CrOS).

BUG=144296
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=154001

[Emmanuel Saint-loubert-Bié, 2012-08-28 23:35:06]

Oshima,

Please perform initial review of this CL.

-- Emmanuel

[Mihai Parparita -not on Chrome, 2012-08-28 23:39:40]

https://chromiumcodereview.appspot.com/10892023/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right):

https://chromiumcodereview.appspot.com/10892023/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:65: image_decoder_ =
new ImageDecoder(this, image_data, true);
Drive-by nit: boolean parameters like this make code harder to read. A
JpegDecoder enum with DEFAULT_JPEG_DECODER, STANDARD_JPEG_DECODER values would
make this more readable.

[oshima, 2012-08-29 00:02:48]

https://chromiumcodereview.appspot.com/10892023/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right):

https://chromiumcodereview.appspot.com/10892023/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:65: image_decoder_ =
new ImageDecoder(this, image_data, true);
On 2012/08/28 23:39:40, Mihai Parparita wrote:
> Drive-by nit: boolean parameters like this make code harder to read. A
> JpegDecoder enum with DEFAULT_JPEG_DECODER, STANDARD_JPEG_DECODER values would
> make this more readable.

While I agree with the issue with the boolean, 
DEFAULT_JPEG_DECODER vs STANDARD_JPEG_DECODER is still confusing.
How about SECURE_JPEG_DECODER?

https://chromiumcodereview.appspot.com/10892023/diff/1/chrome/browser/image_d...
File chrome/browser/image_decoder.cc (right):

https://chromiumcodereview.appspot.com/10892023/diff/1/chrome/browser/image_d...
chrome/browser/image_decoder.cc:67: if(require_standard_jpeg_) {
space after if

https://chromiumcodereview.appspot.com/10892023/diff/1/chrome/browser/image_d...
chrome/browser/image_decoder.cc:71: else {
} else {

https://chromiumcodereview.appspot.com/10892023/diff/1/chrome/browser/image_d...
File chrome/browser/image_decoder.h (right):

https://chromiumcodereview.appspot.com/10892023/diff/1/chrome/browser/image_d...
chrome/browser/image_decoder.h:65: bool require_standard_jpeg_;
use_standard_jpeg_decoder_; would be better.

[Emmanuel Saint-loubert-Bié, 2012-08-29 00:52:52]

OK I have applied all the comments. PTAL!

https://chromiumcodereview.appspot.com/10892023/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right):

https://chromiumcodereview.appspot.com/10892023/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:65: image_decoder_ =
new ImageDecoder(this, image_data, true);
Yes good idea. I will do that

https://chromiumcodereview.appspot.com/10892023/diff/1/chrome/browser/image_d...
File chrome/browser/image_decoder.h (right):

https://chromiumcodereview.appspot.com/10892023/diff/1/chrome/browser/image_d...
chrome/browser/image_decoder.h:65: bool require_standard_jpeg_;
On 2012/08/29 00:02:48, oshima wrote:
> use_standard_jpeg_decoder_; would be better.

Done.

[oshima, 2012-08-29 01:32:28]

lgtm with nit

https://chromiumcodereview.appspot.com/10892023/diff/4004/chrome/browser/imag...
File chrome/browser/image_decoder.h (right):

https://chromiumcodereview.appspot.com/10892023/diff/4004/chrome/browser/imag...
chrome/browser/image_decoder.h:39: };
document enum

https://chromiumcodereview.appspot.com/10892023/diff/4004/chrome/utility/chro...
File chrome/utility/chrome_content_utility_client.cc (right):

https://chromiumcodereview.appspot.com/10892023/diff/4004/chrome/utility/chro...
chrome/utility/chrome_content_utility_client.cc:193: scoped_ptr<SkBitmap>
decoded_image(gfx::JPEGCodec::Decode(&encoded_data[0],
align encoded_data.size() to &encoded_data[0].

If it doesn't fit to 80 chars, move &encoded_data[0] to
next line, like
    &encoded_data[0], encoded_data.size()

[Emmanuel Saint-loubert-Bié, 2012-08-29 01:57:18]

Thank you Oshima for LGTM, I have applied your last comments

for OWNER, PTAL:

nkostylev  -> chrome/browser/chromeos/*, chrome/browser/ui/webui/chromeos/*,
chrome/browser/ui/webui/options/chromeos/*

sail  -> chrome/browser/profiles/

sky -> remaining chrome pieces!

-- Emmnuel

https://chromiumcodereview.appspot.com/10892023/diff/4004/chrome/browser/imag...
File chrome/browser/image_decoder.h (right):

https://chromiumcodereview.appspot.com/10892023/diff/4004/chrome/browser/imag...
chrome/browser/image_decoder.h:39: };
On 2012/08/29 01:32:29, oshima wrote:
> document enum

Done.

https://chromiumcodereview.appspot.com/10892023/diff/4004/chrome/utility/chro...
File chrome/utility/chrome_content_utility_client.cc (right):

https://chromiumcodereview.appspot.com/10892023/diff/4004/chrome/utility/chro...
chrome/utility/chrome_content_utility_client.cc:193: scoped_ptr<SkBitmap>
decoded_image(gfx::JPEGCodec::Decode(&encoded_data[0],
On 2012/08/29 01:32:29, oshima wrote:
> align encoded_data.size() to &encoded_data[0].
> 
> If it doesn't fit to 80 chars, move &encoded_data[0] to
> next line, like
>     &encoded_data[0], encoded_data.size()

Done.

[Nikita (slow), 2012-08-29 14:20:10]

https://chromiumcodereview.appspot.com/10892023/diff/12/chrome/browser/ui/web...
File chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc
(right):

https://chromiumcodereview.appspot.com/10892023/diff/12/chrome/browser/ui/web...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc:236:
ImageDecoder::DEFAULT_CODEC);
Why do we use default codec here but in user_image_loader.cc
ONLY_IJG_LIBJPEG_CODEC?

https://chromiumcodereview.appspot.com/10892023/diff/12/chrome/browser/ui/web...
File chrome/browser/ui/webui/options/chromeos/change_picture_options_handler.cc
(right):

https://chromiumcodereview.appspot.com/10892023/diff/12/chrome/browser/ui/web...
chrome/browser/ui/webui/options/chromeos/change_picture_options_handler.cc:204:
ImageDecoder::DEFAULT_CODEC);
Same here.

[Nikita (slow), 2012-08-29 15:01:58]

lgtm with these changes

[sky, 2012-08-29 15:21:54]

https://chromiumcodereview.appspot.com/10892023/diff/12/chrome/browser/image_...
File chrome/browser/image_decoder.h (right):

https://chromiumcodereview.appspot.com/10892023/diff/12/chrome/browser/image_...
chrome/browser/image_decoder.h:70: ImageCodec image_codec_;
const

https://chromiumcodereview.appspot.com/10892023/diff/12/chrome/common/chrome_...
File chrome/common/chrome_utility_messages.h (right):

https://chromiumcodereview.appspot.com/10892023/diff/12/chrome/common/chrome_...
chrome/common/chrome_utility_messages.h:83:
IPC_MESSAGE_CONTROL1(ChromeUtilityMsg_DecodeImageWithIJGlibjpeg,
Why do we need a new message, can't we add an argument to the existing one?

[Jorge Lucangeli Obes, 2012-08-29 17:04:29]

https://chromiumcodereview.appspot.com/10892023/diff/12/chrome/common/chrome_...
File chrome/common/chrome_utility_messages.h (right):

https://chromiumcodereview.appspot.com/10892023/diff/12/chrome/common/chrome_...
chrome/common/chrome_utility_messages.h:83:
IPC_MESSAGE_CONTROL1(ChromeUtilityMsg_DecodeImageWithIJGlibjpeg,
On 2012/08/29 15:21:54, sky wrote:
> Why do we need a new message, can't we add an argument to the existing one?

The way I see this functionality, we're providing a new mechanism for more
robust (but slower) strictly-JPEG decoding.

The regular DecodeImage method supports a whole bunch of image formats. Adding a
second argument that would make most of the byte streams in the first argument
"invalid" sounds brittle.

As for the name, though, I would rather have a name that refers to the "what",
i.e. "RobustJPEGDecodeImage", rather than the "how"/implementation details, i.e.
mentioning IJG libjpeg. We might want to change/update how we do robust image
decoding, we should be able to change that in the utility process without
leaving an outdated message name here. The same would apply for an eventual
argument name.

[Emmanuel Saint-loubert-Bié, 2012-08-29 17:52:08]

Applied all comments.

sky: please review CL which has new naming suggested by jorgelo.

erg/davemoore: you are OWNER for chrome/utility/chrome_content_utility_client.cc


thanks!

https://chromiumcodereview.appspot.com/10892023/diff/12/chrome/browser/image_...
File chrome/browser/image_decoder.h (right):

https://chromiumcodereview.appspot.com/10892023/diff/12/chrome/browser/image_...
chrome/browser/image_decoder.h:70: ImageCodec image_codec_;
On 2012/08/29 15:21:54, sky wrote:
> const

Done.

https://chromiumcodereview.appspot.com/10892023/diff/12/chrome/browser/ui/web...
File chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc
(right):

https://chromiumcodereview.appspot.com/10892023/diff/12/chrome/browser/ui/web...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc:236:
ImageDecoder::DEFAULT_CODEC);
On 2012/08/29 14:20:11, Nikita Kostylev wrote:
> Why do we use default codec here but in user_image_loader.cc
> ONLY_IJG_LIBJPEG_CODEC?

Done.

https://chromiumcodereview.appspot.com/10892023/diff/12/chrome/browser/ui/web...
File chrome/browser/ui/webui/options/chromeos/change_picture_options_handler.cc
(right):

https://chromiumcodereview.appspot.com/10892023/diff/12/chrome/browser/ui/web...
chrome/browser/ui/webui/options/chromeos/change_picture_options_handler.cc:204:
ImageDecoder::DEFAULT_CODEC);
On 2012/08/29 14:20:11, Nikita Kostylev wrote:
> Same here.

Done.

[sky, 2012-08-29 21:02:14]

On Wed, Aug 29, 2012 at 10:04 AM,  <jorgelo@chromium.org> wrote:
>
>
https://chromiumcodereview.appspot.com/10892023/diff/12/chrome/common/chrome_...
> File chrome/common/chrome_utility_messages.h (right):
>
>
https://chromiumcodereview.appspot.com/10892023/diff/12/chrome/common/chrome_...
> chrome/common/chrome_utility_messages.h:83:
> IPC_MESSAGE_CONTROL1(ChromeUtilityMsg_DecodeImageWithIJGlibjpeg,
> On 2012/08/29 15:21:54, sky wrote:
>>
>> Why do we need a new message, can't we add an argument to the existing
>
> one?
>
> The way I see this functionality, we're providing a new mechanism for
> more robust (but slower) strictly-JPEG decoding.
>
> The regular DecodeImage method supports a whole bunch of image formats.
> Adding a second argument that would make most of the byte streams in the
> first argument "invalid" sounds brittle.
>
> As for the name, though, I would rather have a name that refers to the
> "what", i.e. "RobustJPEGDecodeImage", rather than the
> "how"/implementation details, i.e. mentioning IJG libjpeg. We might want
> to change/update how we do robust image decoding, we should be able to
> change that in the utility process without leaving an outdated message
> name here. The same would apply for an eventual argument name.
>
> https://chromiumcodereview.appspot.com/10892023/

What do you mean? ChromeUtilityMsg_DecodeImageBase64 takes a
std::string where as the one your adding takes a std::vector<unsigned
char>. Those are effectively the same thing.

  -Scott

[sail, 2012-08-29 21:11:37]

profiles/* LGTM

[sky, 2012-08-29 21:23:52]

On 2012/08/29 21:02:14, sky wrote:
> On Wed, Aug 29, 2012 at 10:04 AM,  <mailto:jorgelo@chromium.org> wrote:
> >
> >
>
https://chromiumcodereview.appspot.com/10892023/diff/12/chrome/common/chrome_...
> > File chrome/common/chrome_utility_messages.h (right):
> >
> >
>
https://chromiumcodereview.appspot.com/10892023/diff/12/chrome/common/chrome_...
> > chrome/common/chrome_utility_messages.h:83:
> > IPC_MESSAGE_CONTROL1(ChromeUtilityMsg_DecodeImageWithIJGlibjpeg,
> > On 2012/08/29 15:21:54, sky wrote:
> >>
> >> Why do we need a new message, can't we add an argument to the existing
> >
> > one?
> >
> > The way I see this functionality, we're providing a new mechanism for
> > more robust (but slower) strictly-JPEG decoding.
> >
> > The regular DecodeImage method supports a whole bunch of image formats.
> > Adding a second argument that would make most of the byte streams in the
> > first argument "invalid" sounds brittle.
> >
> > As for the name, though, I would rather have a name that refers to the
> > "what", i.e. "RobustJPEGDecodeImage", rather than the
> > "how"/implementation details, i.e. mentioning IJG libjpeg. We might want
> > to change/update how we do robust image decoding, we should be able to
> > change that in the utility process without leaving an outdated message
> > name here. The same would apply for an eventual argument name.
> >
> > https://chromiumcodereview.appspot.com/10892023/
> 
> What do you mean? ChromeUtilityMsg_DecodeImageBase64 takes a
> std::string where as the one your adding takes a std::vector<unsigned
> char>. Those are effectively the same thing.
> 
>   -Scott

I'll also mention that you're adding an argument to ImageDecoder, not creating a
whole new class. So, why is the additional argument ok there but not in the IPC
call?

  -Scott

[Jorge Lucangeli Obes, 2012-08-29 21:43:31]

On 2012/08/29 21:23:52, sky wrote:
> On 2012/08/29 21:02:14, sky wrote:
> > On Wed, Aug 29, 2012 at 10:04 AM,  <mailto:jorgelo@chromium.org> wrote:
> > >
> > >
> >
>
https://chromiumcodereview.appspot.com/10892023/diff/12/chrome/common/chrome_...
> > > File chrome/common/chrome_utility_messages.h (right):
> > >
> > >
> >
>
https://chromiumcodereview.appspot.com/10892023/diff/12/chrome/common/chrome_...
> > > chrome/common/chrome_utility_messages.h:83:
> > > IPC_MESSAGE_CONTROL1(ChromeUtilityMsg_DecodeImageWithIJGlibjpeg,
> > > On 2012/08/29 15:21:54, sky wrote:
> > >>
> > >> Why do we need a new message, can't we add an argument to the existing
> > >
> > > one?
> > >
> > > The way I see this functionality, we're providing a new mechanism for
> > > more robust (but slower) strictly-JPEG decoding.
> > >
> > > The regular DecodeImage method supports a whole bunch of image formats.
> > > Adding a second argument that would make most of the byte streams in the
> > > first argument "invalid" sounds brittle.
> > >
> > > As for the name, though, I would rather have a name that refers to the
> > > "what", i.e. "RobustJPEGDecodeImage", rather than the
> > > "how"/implementation details, i.e. mentioning IJG libjpeg. We might want
> > > to change/update how we do robust image decoding, we should be able to
> > > change that in the utility process without leaving an outdated message
> > > name here. The same would apply for an eventual argument name.
> > >
> > > https://chromiumcodereview.appspot.com/10892023/
> > 
> > What do you mean? ChromeUtilityMsg_DecodeImageBase64 takes a
> > std::string where as the one your adding takes a std::vector<unsigned
> > char>. Those are effectively the same thing.
> > 
> >   -Scott
> 
> I'll also mention that you're adding an argument to ImageDecoder, not creating
a
> whole new class. So, why is the additional argument ok there but not in the
IPC
> call?

I don't feel strongly about new-message vs extra-argument, and I agree that
doing a new message but not a new class might not be quite consistent; a new
message should also come with a RobustJPEGDecoder class or similar. So extra
argument is OK with me.

[Emmanuel Saint-loubert-Bié, 2012-08-29 22:58:39]

Hi All,

I am fine doing the change required by sky@. I just want to state for the record
that I hope that I am not going to get a comment saying that casting enums to
int32 and recasting them the other side is not ideal from a security standpoint
(even with range check and even though we do it elsewhere). :-)

Or I am equally happy to add another class.

-- Emmanuel

[sky, 2012-08-29 23:06:07]

Yes, casting ints isn't very nice. LGTM