Fix caching bug in JPEGImageDecoder

Fix caching bug in JPEGImageDecoder

When decoding only the size, update the restart position and clear
jpeg_source_mgr's bytes_in_buffer and next_input_byte. If m_data gets
collapsed (outside of JPEGImageDecoder), these values may no longer be
valid, since they may point into a segment which no longer exists.

The next call to decode will be forced to call getSomeData from the
restart position.

Add a test which fails without the fix. While the fix does not depend
on intimate knowledge of the implementation of SharedBuffer, the test
does. In order to ensure that the data in the SharedBuffer gets freed,
insert the data into the SharedBuffer using

void append(const char*, unsigned)

with a length > kSegmentSize, which will force the SharedBuffer to
skip its PurgeableVector (m_buffer). After letting JPEGImageDecoder
cache a pointer into the SharedBuffer (by calling isSizeAvailable()),
collapse the data with a call to data().

BUG=467772
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=202632

[scroggo_chromium, 2015-09-18 21:07:45]

scroggo@chromium.org changed reviewers:
+ noel@chromium.org, pkasting@chromium.org, scroggo@chromium.org

[scroggo_chromium, 2015-09-18 21:07:46]

After figuring out crbug.com/530279 (that's a security bug, but you should both
have permission to view), I went back to JPEGImageDecoder to see if it had any
risks when SharedBuffer::data() is called externally. I think in most cases
JPEGImageDecoder is safe - it can only be called outside of
JPEGImageReader::decode (unless it's called from another thread, which is a
no-no). That means JPEGImageReader is one of four states:

1) decode() hasn't been called

This is safe, since fillBuffer was never called, so JPEGImageReader has not
cached anything.

2) decode() was called with sizeOnly set to true, and decode() returned true
* This is the dangerous case, which I'll go into more detail in down below.

3) decode() was called, but had to suspend.
This is safe. bytes_in_buffer was set to 0, so the next time libjpeg tries to
read anything, it will call getSomeData, which will read from the correct part
of the SharedBuffer

4) decode() succeeded, and decoded the entire image
No longer need to do any decoding, so the cache does not matter

This CL fixes the only dangerous state, 2. It changes it to be the same as 3.

https://codereview.chromium.org/1358643002/diff/1/Source/platform/image-decod...
File Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right):

https://codereview.chromium.org/1358643002/diff/1/Source/platform/image-decod...
Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:428:
m_info.src->bytes_in_buffer = 0;
I considered calling suspend() here, which shares these three lines of code. (It
also sets m_needsRestart to true, but it is guaranteed to be true at this point,
so no harm done.)

I ended up not doing it because it seems like we're not "suspending" in this
case. Maybe it should be renamed something like "clearBuffer" or "emptyBuffer",
since it does the opposite of "fillBuffer". Then it might make sense to call it
here.

[Peter Kasting, 2015-09-19 00:51:34]

LGTM

https://codereview.chromium.org/1358643002/diff/1/Source/platform/image-decod...
File Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right):

https://codereview.chromium.org/1358643002/diff/1/Source/platform/image-decod...
Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:428:
m_info.src->bytes_in_buffer = 0;
On 2015/09/18 21:07:45, scroggo_chromium wrote:
> I considered calling suspend() here, which shares these three lines of code.
(It
> also sets m_needsRestart to true, but it is guaranteed to be true at this
point,
> so no harm done.)
> 
> I ended up not doing it because it seems like we're not "suspending" in this
> case. Maybe it should be renamed something like "clearBuffer" or
"emptyBuffer",
> since it does the opposite of "fillBuffer". Then it might make sense to call
it
> here.

I like the idea of renaming this clearBuffer() and calling it here.

https://codereview.chromium.org/1358643002/diff/1/Source/platform/image-decod...
Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:528:
updateRestartPosition();
You should probably have comments here about why you need to do these.

https://codereview.chromium.org/1358643002/diff/1/Source/platform/image-decod...
File Source/platform/image-decoders/jpeg/JPEGImageDecoderTest.cpp (right):

https://codereview.chromium.org/1358643002/diff/1/Source/platform/image-decod...
Source/platform/image-decoders/jpeg/JPEGImageDecoderTest.cpp:260:
segmentedData->append(data->data(), data->size());
You should have comments in this test about what you're actually doing and why
it works.

[scroggo_chromium, 2015-09-21 18:21:28]

https://codereview.chromium.org/1358643002/diff/1/Source/platform/image-decod...
File Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right):

https://codereview.chromium.org/1358643002/diff/1/Source/platform/image-decod...
Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:428:
m_info.src->bytes_in_buffer = 0;
On 2015/09/19 00:51:34, Peter Kasting wrote:
> I like the idea of renaming this clearBuffer() and calling it here.

Done.

> On 2015/09/18 21:07:45, scroggo_chromium wrote:
> > I considered calling suspend() here, which shares these three lines of code.
> (It
> > also sets m_needsRestart to true, but it is guaranteed to be true at this
> point,
> > so no harm done.)

I was confused here - m_needsRestart is guaranteed to be false here, and we do
not want it to get set to true. So I have moved the line

    m_needsRestart = true;

outside of the newly named clearBuffer(), so it can be used in the places it
should be used, but not here.

https://codereview.chromium.org/1358643002/diff/1/Source/platform/image-decod...
Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:528:
updateRestartPosition();
On 2015/09/19 00:51:34, Peter Kasting wrote:
> You should probably have comments here about why you need to do these.

Done.

(My instinct is include lots of comments, but previous Blink code reviews have
encouraged me to remove many of them. I seem to have corrected too far in the
other direction.)

https://codereview.chromium.org/1358643002/diff/1/Source/platform/image-decod...
File Source/platform/image-decoders/jpeg/JPEGImageDecoderTest.cpp (right):

https://codereview.chromium.org/1358643002/diff/1/Source/platform/image-decod...
Source/platform/image-decoders/jpeg/JPEGImageDecoderTest.cpp:260:
segmentedData->append(data->data(), data->size());
On 2015/09/19 00:51:34, Peter Kasting wrote:
> You should have comments in this test about what you're actually doing and why
> it works.

Done.

[scroggo, 2015-09-22 13:58:34]

The CQ bit was checked by scroggo@google.com

[scroggo, 2015-09-22 13:58:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1358643002/#ps20001
(title: "Respond to pkasting's comments in patch set 1")

[commit-bot: I haz the power, 2015-09-22 13:58:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1358643002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1358643002/20001

[commit-bot: I haz the power, 2015-09-22 15:31:51]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=202632