Get rid of leaks in SkCodec::NewFromStream.

Get rid of leaks in SkCodec::NewFromStream.

SkCodec::NewFromStream claims to delete the passed in SkStream on
failure. This allows the caller to pass an SkStream to the function
and not worry about deleting it depending on the return value.

Most of our SkCodecs did not honor this contract though. Update them
to delete the stream on failure. Further, update SkCodec::NewFromStream
to delete the stream if it did not match any subclass, and delete the
SkCodec if we decided to return NULL because it was too big.

Add a test which tests streams which represent the beginnings of
supported format types but do not contain enough data to create an
SkCodec. The interesting part of the test is when we run it on ASAN,
which will report that we leaked something without the other changes.

BUG=skia:3257
Committed: https://skia.googlesource.com/skia/+/0a7e69cb9b4e3929d659891d152a2c0b59bff4e0

[scroggo, 2015-04-02 21:23:21]

scroggo@google.com changed reviewers:
+ msarda@chromium.org, mtklein@google.com

[mtklein, 2015-04-02 22:11:57]

An alternative is to change the docs to say "Does not take ownership of the
stream.".  Are there SkCodec implementations that really need to take ownership?
 Seems like if it's this easy to forget to take ownership, perhaps we're using
SkCodec in places where we don't really need it to take ownership anyway.  It
certainly surprised me when I read the code in DM that was going to use
NewFromStream; we had to work around the issue there too, and part of the reason
I suggested switching to NewFromData was that it seemed to have simpler lifetime
semantics.

[msarda, 2015-04-03 08:30:41]

On 2015/04/02 22:11:57, mtklein wrote:
> An alternative is to change the docs to say "Does not take ownership of the
> stream.".  Are there SkCodec implementations that really need to take
ownership?
>  Seems like if it's this easy to forget to take ownership, perhaps we're using
> SkCodec in places where we don't really need it to take ownership anyway.  It
> certainly surprised me when I read the code in DM that was going to use
> NewFromStream; we had to work around the issue there too, and part of the
reason
> I suggested switching to NewFromData was that it seemed to have simpler
lifetime
> semantics.

I do not know this code at all, so I think I am the right reviewer for this CL.

[scroggo, 2015-04-03 13:04:04]

scroggo@google.com changed reviewers:
+ msarett@google.com
- msarda@chromium.org

[scroggo, 2015-04-03 13:04:04]

On 2015/04/02 22:11:57, mtklein wrote:
> An alternative is to change the docs to say "Does not take ownership of the
> stream.".  Are there SkCodec implementations that really need to take
ownership?
>  Seems like if it's this easy to forget to take ownership, perhaps we're using
> SkCodec in places where we don't really need it to take ownership anyway.  It
> certainly surprised me when I read the code in DM that was going to use
> NewFromStream; we had to work around the issue there too, and part of the
reason
> I suggested switching to NewFromData was that it seemed to have simpler
lifetime
> semantics.

Not taking ownership of the stream made sense for SkImageDecoder, which (except
in decodeSubset mode) reads the stream and gets everything it needs in one pass.

SkCodec, on the other hand, can be created, which reads part of the stream,
passed to another object (e.g. for deferred decoding), and then used to do the
full decode later, which reads the rest of the stream. We need to make sure the
stream still exists in order to keep reading from it.

SkStream used to be ref counted. Under that system, SkCodec could have called
ref on the stream, making the lifetime semantics similar to using NewFromData. A
ref counted stream is a little strange though; since it is a stateful object, it
does not make sense for it to have multiple owners - if one caller started
reading from it, and then another caller read from it, neither owner would be
getting the data they expect.

So I removed reference counting from SkStream and made SkCodec take ownership of
the supplied SkStream. This matches the behavior of other users of SkStreams
which need to keep one around beyond one function call.

[mtklein, 2015-04-03 13:13:05]

Gotcha.  Wasn't thinking about deferred.

lgtm with one suggestion.

https://codereview.chromium.org/1058873006/diff/1/src/codec/SkCodec_libbmp.cpp
File src/codec/SkCodec_libbmp.cpp (right):

https://codereview.chromium.org/1058873006/diff/1/src/codec/SkCodec_libbmp.cp...
src/codec/SkCodec_libbmp.cpp:501: if (ReadHeader(stream, isIco, &codec)) {
Might be less error prone to have ReadHeader take ownership too:

if (ReadHeader(stream.detach(), isIco, &codec)) {
  return codec;
}

[scroggo, 2015-04-03 13:13:50]

Note:

I ran some trybots to check for leaks, and while both runs had exceptions, they
were not leaks:

valgrind is complaining about SkImageDecoder images being encoded (not sure
whether that's a problem with the decoder or due to a rowbytes that is not tight
which we include with the decode). Those were not introduced by this CL. 

ASAN had an exception in upload dm results, which was not caused by this CL.

[scroggo, 2015-04-03 13:59:53]

https://codereview.chromium.org/1058873006/diff/1/src/codec/SkCodec_libbmp.cpp
File src/codec/SkCodec_libbmp.cpp (right):

https://codereview.chromium.org/1058873006/diff/1/src/codec/SkCodec_libbmp.cp...
src/codec/SkCodec_libbmp.cpp:501: if (ReadHeader(stream, isIco, &codec)) {
On 2015/04/03 13:13:05, mtklein wrote:
> Might be less error prone to have ReadHeader take ownership too:
> 
> if (ReadHeader(stream.detach(), isIco, &codec)) {
>   return codec;
> }

Yeah, I was not sure the best way to handle this. The problem with that approach
is that we call ReadHeader in two places:

This time, it make sense for ReadHeader to take ownership and pass to the codec
if it creates one.

The other time, after a rewind, we already have a codec, and it already owns the
stream. In this case we pass NULL for codec.

We could make ReadHeader behave differently with regards to taking ownership
depending on whether a codec pointer is passed in, but that seems ugly to me.
(Maybe no worse than this decision though?)

[mtklein, 2015-04-03 14:01:28]

On 2015/04/03 13:59:53, scroggo wrote:
> https://codereview.chromium.org/1058873006/diff/1/src/codec/SkCodec_libbmp.cpp
> File src/codec/SkCodec_libbmp.cpp (right):
> 
>
https://codereview.chromium.org/1058873006/diff/1/src/codec/SkCodec_libbmp.cp...
> src/codec/SkCodec_libbmp.cpp:501: if (ReadHeader(stream, isIco, &codec)) {
> On 2015/04/03 13:13:05, mtklein wrote:
> > Might be less error prone to have ReadHeader take ownership too:
> > 
> > if (ReadHeader(stream.detach(), isIco, &codec)) {
> >   return codec;
> > }
> 
> Yeah, I was not sure the best way to handle this. The problem with that
approach
> is that we call ReadHeader in two places:
> 
> This time, it make sense for ReadHeader to take ownership and pass to the
codec
> if it creates one.
> 
> The other time, after a rewind, we already have a codec, and it already owns
the
> stream. In this case we pass NULL for codec.
> 
> We could make ReadHeader behave differently with regards to taking ownership
> depending on whether a codec pointer is passed in, but that seems ugly to me.
> (Maybe no worse than this decision though?)

Ah, I think I'm with you then.  This seems best.

[scroggo, 2015-04-03 14:10:15]

The CQ bit was checked by scroggo@google.com

[scroggo, 2015-04-03 14:10:15]

The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com
Link to the patchset: https://codereview.chromium.org/1058873006/#ps10007
(title: "Add comment that ReadHeader does not own SkStream.")

[commit-bot: I haz the power, 2015-04-03 14:10:28]

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

[commit-bot: I haz the power, 2015-04-03 14:22:25]

Committed patchset #2 (id:10007) as
https://skia.googlesource.com/skia/+/0a7e69cb9b4e3929d659891d152a2c0b59bff4e0