Add test for new FrontBufferedStream behavior.

Add test for new FrontBufferedStream behavior.

Test for https://skia.googlesource.com/skia/+/dd5a1e094c19fa10202c37c50a1f799e5af5dac0

Verify that FrontBufferedStream does not attempt to read beyond the
end of its underlying stream.

Make SkStreamToCGImageSource handle an empty stream better.

Committed: https://skia.googlesource.com/skia/+/3ceef9a66aa1ae8db896b8e75496194978708476

[scroggo, 2014-10-21 19:10:34]

scroggo@google.com changed reviewers:
+ mtklein@google.com, reed@google.com

[scroggo, 2014-10-22 21:52:01]

Ping. PTAL

(I realized that since I had a link to an old review in the description, you may
have clicked on it instead of this review and thought you had already reviewed
it. I do that sometimes...)

[mtklein, 2014-10-22 22:24:13]

yes indeed, that's what happened.

lgtm

[scroggo, 2014-10-23 17:12:21]

The CQ bit was checked by scroggo@google.com

[commit-bot: I haz the power, 2014-10-23 17:12:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/641813009/1

[commit-bot: I haz the power, 2014-10-23 17:21:32]

Committed patchset #1 (id:1) as da59f05c6738dbb9a92cad21c608cdfae53a76b2

[mtklein, 2014-10-23 19:07:50]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/649553003/ by mtklein@google.com.

The reason for reverting is: Test is SkASSERTing on Macs, e.g:

http://chromegw.corp.google.com/i/client.skia/builders/Test-Mac10.6-MacMini4....

210 tasks left	1737M peak	    1ms	test
FrontBufferedStream../../src/ports/SkImageDecoder_CG.cpp:43: failed assertion
"data"

Signal 11:
_sigtramp (+0x1a)
SkStreamToCGImageSource(SkStream*) (+0x62)
SkImageDecoder_CG::onDecode(SkStream*, SkBitmap*, SkImageDecoder::Mode) (+0x2e)
SkImageDecoder::decode(SkStream*, SkBitmap*, SkColorType, SkImageDecoder::Mode)
(+0x81)
SkImageDecoder::DecodeStream(SkStreamRewindable*, SkBitmap*, SkColorType,
SkImageDecoder::Mode, SkImageDecoder::Format*) (+0xff)
SkImageDecoder::DecodeStream(SkStreamRewindable*, SkBitmap*) (+0x31)
test_ShortFrontBufferedStream(skiatest::Reporter*) (+0x97)
skiatest::ShortFrontBufferedStreamClass::onRun(skiatest::Reporter*) (+0x19)
skiatest::Test::run() (+0x7c)
DM::CpuTestTask::draw() (+0x5a)
DM::CpuTask::run() (+0x9e)
non-virtual thunk to DM::CpuTask::run() (+0x1c)
(anonymous namespace)::ThreadPool::Loop(void*) (+0xf4)
thread_start(void*) (+0x54)
_pthread_start (+0x14b)
.

[mtklein, 2014-10-23 23:38:15]

On 2014/10/23 19:07:50, mtklein wrote:
> A revert of this CL (patchset #1 id:1) has been created in
> https://codereview.chromium.org/649553003/ by mailto:mtklein@google.com.
> 
> The reason for reverting is: Test is SkASSERTing on Macs, e.g:
> 
>
http://chromegw.corp.google.com/i/client.skia/builders/Test-Mac10.6-MacMini4....
> 
> 210 tasks left	1737M peak	    1ms	test
> FrontBufferedStream../../src/ports/SkImageDecoder_CG.cpp:43: failed assertion
> "data"
> 
> Signal 11:
> _sigtramp (+0x1a)
> SkStreamToCGImageSource(SkStream*) (+0x62)
> SkImageDecoder_CG::onDecode(SkStream*, SkBitmap*, SkImageDecoder::Mode)
(+0x2e)
> SkImageDecoder::decode(SkStream*, SkBitmap*, SkColorType,
SkImageDecoder::Mode)
> (+0x81)
> SkImageDecoder::DecodeStream(SkStreamRewindable*, SkBitmap*, SkColorType,
> SkImageDecoder::Mode, SkImageDecoder::Format*) (+0xff)
> SkImageDecoder::DecodeStream(SkStreamRewindable*, SkBitmap*) (+0x31)
> test_ShortFrontBufferedStream(skiatest::Reporter*) (+0x97)
> skiatest::ShortFrontBufferedStreamClass::onRun(skiatest::Reporter*) (+0x19)
> skiatest::Test::run() (+0x7c)
> DM::CpuTestTask::draw() (+0x5a)
> DM::CpuTask::run() (+0x9e)
> non-virtual thunk to DM::CpuTask::run() (+0x1c)
> (anonymous namespace)::ThreadPool::Loop(void*) (+0xf4)
> thread_start(void*) (+0x54)
> _pthread_start (+0x14b)
> .

FYI, Valgrind also hates your guts:

558 tasks left	2111M peak	   23ms	test FrontBufferedStream==6634== Thread 8:
==6634== Conditional jump or move depends on uninitialised value(s)
==6634==    at 0x6ED093: sk_libico_dfactory(SkStreamRewindable*)
(SkImageDecoder_libico.cpp:421)
==6634==    by 0x6E9E9D: image_decoder_from_stream(SkStreamRewindable*)
(SkImageDecoder_FactoryRegistrar.cpp:25)
==6634==    by 0x6E9E38: SkImageDecoder::Factory(SkStreamRewindable*)
(SkImageDecoder_FactoryDefault.cpp:16)
==6634==    by 0x6E9C47: SkImageDecoder::DecodeStream(SkStreamRewindable*,
SkBitmap*, SkColorType, SkImageDecoder::Mode, SkImageDecoder::Format*)
(SkImageDecoder.cpp:273)
==6634==    by 0x49A9CC:
skiatest::ShortFrontBufferedStreamClass::onRun(skiatest::Reporter*)
(SkImageDecoder.h:345)
==6634==    by 0x4172F4: skiatest::Test::run() (Test.cpp:103)
==6634==    by 0x40BB57: DM::CpuTestTask::draw() (DMTestTask.cpp:36)
==6634==    by 0x40B502: DM::CpuTask::run() (DMTask.cpp:53)
==6634==    by 0x40E03E: SkTaskGroup::wait() (SkTaskGroup.cpp:51)
==6634==    by 0x40A4C0: DM::QuiltTask::draw() (SkTaskGroup.h:23)
==6634==    by 0x40B502: DM::CpuTask::run() (DMTask.cpp:53)
==6634==    by 0x40B584: DM::CpuTask::spawnChild(DM::CpuTask*) (DMTask.cpp:63)
==6634==    by 0x408DA5: DM::CpuGMTask::draw() (DMCpuGMTask.cpp:37)
==6634==    by 0x40B502: DM::CpuTask::run() (DMTask.cpp:53)
==6634==    by 0x40DCF9: (anonymous namespace)::ThreadPool::Loop(void*)
(SkTaskGroup.cpp:117)
==6634==    by 0x75DD1A: thread_start(void*) (SkThreadUtils_pthread.cpp:66)
==6634==    by 0x5693E99: start_thread (pthread_create.c:308)
==6634==    by 0x6D3FCBC: clone (clone.S:112)
==6634==  Uninitialised value was created by a heap allocation
==6634==    at 0x4C2B6CD: malloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==6634==    by 0x76AF06: sk_malloc_flags(unsigned long, unsigned int)
(SkMemory_malloc.cpp:54)
==6634==    by 0x76AF37: sk_malloc_throw(unsigned long) (SkMemory_malloc.cpp:40)
==6634==    by 0x6ECA01: is_ico(SkStreamRewindable*) (SkTypes.h:509)
==6634==    by 0x6ED08B: sk_libico_dfactory(SkStreamRewindable*)
(SkImageDecoder_libico.cpp:421)
==6634==    by 0x6E9E9D: image_decoder_from_stream(SkStreamRewindable*)
(SkImageDecoder_FactoryRegistrar.cpp:25)
==6634==    by 0x6E9E38: SkImageDecoder::Factory(SkStreamRewindable*)
(SkImageDecoder_FactoryDefault.cpp:16)
==6634==    by 0x6E9C47: SkImageDecoder::DecodeStream(SkStreamRewindable*,
SkBitmap*, SkColorType, SkImageDecoder::Mode, SkImageDecoder::Format*)
(SkImageDecoder.cpp:273)
==6634==    by 0x49A9CC:
skiatest::ShortFrontBufferedStreamClass::onRun(skiatest::Reporter*)
(SkImageDecoder.h:345)
==6634==    by 0x4172F4: skiatest::Test::run() (Test.cpp:103)
==6634==    by 0x40BB57: DM::CpuTestTask::draw() (DMTestTask.cpp:36)
==6634==    by 0x40B502: DM::CpuTask::run() (DMTask.cpp:53)
==6634==    by 0x40E03E: SkTaskGroup::wait() (SkTaskGroup.cpp:51)
==6634==    by 0x40A4C0: DM::QuiltTask::draw() (SkTaskGroup.h:23)
==6634==    by 0x40B502: DM::CpuTask::run() (DMTask.cpp:53)
==6634==    by 0x40B584: DM::CpuTask::spawnChild(DM::CpuTask*) (DMTask.cpp:63)
==6634==    by 0x408DA5: DM::CpuGMTask::draw() (DMCpuGMTask.cpp:37)
==6634==    by 0x40B502: DM::CpuTask::run() (DMTask.cpp:53)
==6634==    by 0x40DCF9: (anonymous namespace)::ThreadPool::Loop(void*)
(SkTaskGroup.cpp:117)
==6634==    by 0x75DD1A: thread_start(void*) (SkThreadUtils_pthread.cpp:66)
==6634==    by 0x5693E99: start_thread (pthread_create.c:308)
==6634==    by 0x6D3FCBC: clone (clone.S:112)
==6634==

[mtklein, 2014-10-23 23:39:08]

On 2014/10/23 23:38:15, mtklein wrote:
> On 2014/10/23 19:07:50, mtklein wrote:
> > A revert of this CL (patchset #1 id:1) has been created in
> > https://codereview.chromium.org/649553003/ by mailto:mtklein@google.com.
> > 
> > The reason for reverting is: Test is SkASSERTing on Macs, e.g:
> > 
> >
>
http://chromegw.corp.google.com/i/client.skia/builders/Test-Mac10.6-MacMini4....
> > 
> > 210 tasks left	1737M peak	    1ms	test
> > FrontBufferedStream../../src/ports/SkImageDecoder_CG.cpp:43: failed
assertion
> > "data"
> > 
> > Signal 11:
> > _sigtramp (+0x1a)
> > SkStreamToCGImageSource(SkStream*) (+0x62)
> > SkImageDecoder_CG::onDecode(SkStream*, SkBitmap*, SkImageDecoder::Mode)
> (+0x2e)
> > SkImageDecoder::decode(SkStream*, SkBitmap*, SkColorType,
> SkImageDecoder::Mode)
> > (+0x81)
> > SkImageDecoder::DecodeStream(SkStreamRewindable*, SkBitmap*, SkColorType,
> > SkImageDecoder::Mode, SkImageDecoder::Format*) (+0xff)
> > SkImageDecoder::DecodeStream(SkStreamRewindable*, SkBitmap*) (+0x31)
> > test_ShortFrontBufferedStream(skiatest::Reporter*) (+0x97)
> > skiatest::ShortFrontBufferedStreamClass::onRun(skiatest::Reporter*) (+0x19)
> > skiatest::Test::run() (+0x7c)
> > DM::CpuTestTask::draw() (+0x5a)
> > DM::CpuTask::run() (+0x9e)
> > non-virtual thunk to DM::CpuTask::run() (+0x1c)
> > (anonymous namespace)::ThreadPool::Loop(void*) (+0xf4)
> > thread_start(void*) (+0x54)
> > _pthread_start (+0x14b)
> > .
> 
> FYI, Valgrind also hates your guts:
> 
> 558 tasks left	2111M peak	   23ms	test FrontBufferedStream==6634== Thread 8:
> ==6634== Conditional jump or move depends on uninitialised value(s)
> ==6634==    at 0x6ED093: sk_libico_dfactory(SkStreamRewindable*)
> (SkImageDecoder_libico.cpp:421)
> ==6634==    by 0x6E9E9D: image_decoder_from_stream(SkStreamRewindable*)
> (SkImageDecoder_FactoryRegistrar.cpp:25)
> ==6634==    by 0x6E9E38: SkImageDecoder::Factory(SkStreamRewindable*)
> (SkImageDecoder_FactoryDefault.cpp:16)
> ==6634==    by 0x6E9C47: SkImageDecoder::DecodeStream(SkStreamRewindable*,
> SkBitmap*, SkColorType, SkImageDecoder::Mode, SkImageDecoder::Format*)
> (SkImageDecoder.cpp:273)
> ==6634==    by 0x49A9CC:
> skiatest::ShortFrontBufferedStreamClass::onRun(skiatest::Reporter*)
> (SkImageDecoder.h:345)
> ==6634==    by 0x4172F4: skiatest::Test::run() (Test.cpp:103)
> ==6634==    by 0x40BB57: DM::CpuTestTask::draw() (DMTestTask.cpp:36)
> ==6634==    by 0x40B502: DM::CpuTask::run() (DMTask.cpp:53)
> ==6634==    by 0x40E03E: SkTaskGroup::wait() (SkTaskGroup.cpp:51)
> ==6634==    by 0x40A4C0: DM::QuiltTask::draw() (SkTaskGroup.h:23)
> ==6634==    by 0x40B502: DM::CpuTask::run() (DMTask.cpp:53)
> ==6634==    by 0x40B584: DM::CpuTask::spawnChild(DM::CpuTask*) (DMTask.cpp:63)
> ==6634==    by 0x408DA5: DM::CpuGMTask::draw() (DMCpuGMTask.cpp:37)
> ==6634==    by 0x40B502: DM::CpuTask::run() (DMTask.cpp:53)
> ==6634==    by 0x40DCF9: (anonymous namespace)::ThreadPool::Loop(void*)
> (SkTaskGroup.cpp:117)
> ==6634==    by 0x75DD1A: thread_start(void*) (SkThreadUtils_pthread.cpp:66)
> ==6634==    by 0x5693E99: start_thread (pthread_create.c:308)
> ==6634==    by 0x6D3FCBC: clone (clone.S:112)
> ==6634==  Uninitialised value was created by a heap allocation
> ==6634==    at 0x4C2B6CD: malloc (in
> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==6634==    by 0x76AF06: sk_malloc_flags(unsigned long, unsigned int)
> (SkMemory_malloc.cpp:54)
> ==6634==    by 0x76AF37: sk_malloc_throw(unsigned long)
(SkMemory_malloc.cpp:40)
> ==6634==    by 0x6ECA01: is_ico(SkStreamRewindable*) (SkTypes.h:509)
> ==6634==    by 0x6ED08B: sk_libico_dfactory(SkStreamRewindable*)
> (SkImageDecoder_libico.cpp:421)
> ==6634==    by 0x6E9E9D: image_decoder_from_stream(SkStreamRewindable*)
> (SkImageDecoder_FactoryRegistrar.cpp:25)
> ==6634==    by 0x6E9E38: SkImageDecoder::Factory(SkStreamRewindable*)
> (SkImageDecoder_FactoryDefault.cpp:16)
> ==6634==    by 0x6E9C47: SkImageDecoder::DecodeStream(SkStreamRewindable*,
> SkBitmap*, SkColorType, SkImageDecoder::Mode, SkImageDecoder::Format*)
> (SkImageDecoder.cpp:273)
> ==6634==    by 0x49A9CC:
> skiatest::ShortFrontBufferedStreamClass::onRun(skiatest::Reporter*)
> (SkImageDecoder.h:345)
> ==6634==    by 0x4172F4: skiatest::Test::run() (Test.cpp:103)
> ==6634==    by 0x40BB57: DM::CpuTestTask::draw() (DMTestTask.cpp:36)
> ==6634==    by 0x40B502: DM::CpuTask::run() (DMTask.cpp:53)
> ==6634==    by 0x40E03E: SkTaskGroup::wait() (SkTaskGroup.cpp:51)
> ==6634==    by 0x40A4C0: DM::QuiltTask::draw() (SkTaskGroup.h:23)
> ==6634==    by 0x40B502: DM::CpuTask::run() (DMTask.cpp:53)
> ==6634==    by 0x40B584: DM::CpuTask::spawnChild(DM::CpuTask*) (DMTask.cpp:63)
> ==6634==    by 0x408DA5: DM::CpuGMTask::draw() (DMCpuGMTask.cpp:37)
> ==6634==    by 0x40B502: DM::CpuTask::run() (DMTask.cpp:53)
> ==6634==    by 0x40DCF9: (anonymous namespace)::ThreadPool::Loop(void*)
> (SkTaskGroup.cpp:117)
> ==6634==    by 0x75DD1A: thread_start(void*) (SkThreadUtils_pthread.cpp:66)
> ==6634==    by 0x5693E99: start_thread (pthread_create.c:308)
> ==6634==    by 0x6D3FCBC: clone (clone.S:112)
> ==6634==

Should not have said "also".  To my knowledge, only Valgrind hates your guts.

[scroggo, 2014-10-24 01:24:02]

On 2014/10/23 19:07:50, mtklein wrote:
> FrontBufferedStream../../src/ports/SkImageDecoder_CG.cpp:43: failed assertion
> "data"

Fixed in Patch Set 2

> FYI, Valgrind also hates your guts:
> 
> 558 tasks left	2111M peak	   23ms	test FrontBufferedStream==6634== Thread 8:
> ==6634== Conditional jump or move depends on uninitialised value(s)
> ==6634==    at 0x6ED093: sk_libico_dfactory(SkStreamRewindable*)
> (SkImageDecoder_libico.cpp:421)

This is actually a separate bug that happened to be revealed by my test. Fix for
that is in
https://codereview.chromium.org/656673005/

[mtklein, 2014-10-24 02:02:49]

lgtm

[mtklein, 2014-10-24 02:02:50]

lgtm

[scroggo, 2014-10-24 13:33:17]

The CQ bit was checked by scroggo@google.com

[commit-bot: I haz the power, 2014-10-24 13:34:02]

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

[mtklein, 2014-10-24 13:38:01]

The CQ bit was unchecked by mtklein@google.com

[scroggo, 2014-10-24 13:54:29]

The CQ bit was checked by scroggo@google.com

[commit-bot: I haz the power, 2014-10-24 13:55:02]

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

[commit-bot: I haz the power, 2014-10-24 13:55:15]

Committed patchset #2 (id:20001) as 3ceef9a66aa1ae8db896b8e75496194978708476