Patches memory leak and crashing

Fixed memory leak for nanobench and crashing in SampleApp
NOTREECHECKS=True

BUG=skia:2830
Committed: https://skia.googlesource.com/skia/+/45f7842de7148a544008483a7829071d3dffba51

[dandov, 2014-08-14 14:26:55]

Fixed the memory issues in nanobench and fixed the crash of sample app

https://codereview.chromium.org/469333002/diff/1/src/core/SkDevice.cpp
File src/core/SkDevice.cpp (right):

https://codereview.chromium.org/469333002/diff/1/src/core/SkDevice.cpp#newcode90
src/core/SkDevice.cpp:90: if (SkPatchUtils::getVertexData(&data, cubics, colors,
texCoords, lod.width(), lod.height())) {
added condition to abort drawing in case it is not able to set up the vertex
data

https://codereview.chromium.org/469333002/diff/1/src/utils/SkPatchUtils.cpp
File src/utils/SkPatchUtils.cpp (right):

https://codereview.chromium.org/469333002/diff/1/src/utils/SkPatchUtils.cpp#n...
src/utils/SkPatchUtils.cpp:211: return false;
with really big lodX or lodY sometimes it overflows and gets a negative vertex
count which makes malloc crash

https://codereview.chromium.org/469333002/diff/1/src/utils/SkPatchUtils.cpp#n...
src/utils/SkPatchUtils.cpp:224: data->fVertexCount = (lodX + 1) * (lodY + 1);
Fixed how it adjusts lodX, lodY and vertex count; this fixes the memory issues
because in the previous version there were some scenarios in which there were
more vertices than needed and SkGpuDevice would iterate over them (uninitialized
data).

[mtklein, 2014-08-14 14:33:27]

https://codereview.chromium.org/469333002/diff/1/src/utils/SkPatchUtils.cpp
File src/utils/SkPatchUtils.cpp (right):

https://codereview.chromium.org/469333002/diff/1/src/utils/SkPatchUtils.cpp#n...
src/utils/SkPatchUtils.cpp:209: data->fVertexCount = (lodX + 1) * (lodY + 1);
This is weird, but this is undefined behavior in both C and C++.

Optimizing compilers are free to say:
  - this tests for signed integer overflow
  - signed integer overflow is undefined
  - therefore I will assume overflow did not happen and optimize away the if
statement.

The safe thing to do is bump up to another integer size (int32_t, int64_t), do
the multiplication there, and check to see if the result fits in the integer you
want.

Another option is to use unsigned integers: they are defined to wrap back around
to 0 so compilers won't mess with them.

[dandov, 2014-08-14 15:17:16]

https://codereview.chromium.org/469333002/diff/1/src/utils/SkPatchUtils.cpp
File src/utils/SkPatchUtils.cpp (right):

https://codereview.chromium.org/469333002/diff/1/src/utils/SkPatchUtils.cpp#n...
src/utils/SkPatchUtils.cpp:209: data->fVertexCount = (lodX + 1) * (lodY + 1);
On 2014/08/14 14:33:27, mtklein wrote:
> This is weird, but this is undefined behavior in both C and C++.
> 
> Optimizing compilers are free to say:
>   - this tests for signed integer overflow
>   - signed integer overflow is undefined
>   - therefore I will assume overflow did not happen and optimize away the if
> statement.
> 
> The safe thing to do is bump up to another integer size (int32_t, int64_t), do
> the multiplication there, and check to see if the result fits in the integer
you
> want.
> 
> Another option is to use unsigned integers: they are defined to wrap back
around
> to 0 so compilers won't mess with them.

I think this fixes the overflow issue, I'm not sure if there is a better way or
a problem with my solution?

[mtklein, 2014-08-14 15:32:25]

https://codereview.chromium.org/469333002/diff/20001/src/utils/SkPatchUtils.cpp
File src/utils/SkPatchUtils.cpp (right):

https://codereview.chromium.org/469333002/diff/20001/src/utils/SkPatchUtils.c...
src/utils/SkPatchUtils.cpp:208: // check for overflow in multiplication
I think what you've got will work, but here's what I'd do:

// Check for overflow of the multiplication we'll need later.
const int64_t lodX64 = (lodX+1),
              lodY64 = (lodY+1),
              mul64  = lodX64 * lodY64;
if (mul64 > SK_MaxS32) {
    return false;
}
...
  data->fVertexCount = SkToS32(mul64);

Shouldn't be possible to overflow int64s with int32 inputs, so no need to do
both the size promotion and switch to unsigned.

And, might as well use the mul64 since we've already got it laying around. 
SkToS32() will check that mul64 fits into an int32_t in debug mode, as a sort of
double-check.

[dandov, 2014-08-14 15:48:46]

Used Mike's solution.

[mtklein, 2014-08-14 15:51:34]

lgtm, of course :)

[dandov, 2014-08-14 15:52:17]

The CQ bit was checked by dandov@google.com

[commit-bot: I haz the power, 2014-08-14 15:53:07]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/dandov@google.com/469333002/60001

[egdaniel, 2014-08-15 13:06:35]

The CQ bit was unchecked by egdaniel@google.com

[egdaniel, 2014-08-15 13:06:37]

The CQ bit was checked by egdaniel@google.com

[commit-bot: I haz the power, 2014-08-15 13:06:42]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/dandov@google.com/469333002/60001

[commit-bot: I haz the power, 2014-08-15 13:06:53]

Committed patchset #4 (60001) as 45f7842de7148a544008483a7829071d3dffba51