Fix array overrun and add test.

Fix buffer overrun, bit overrun and add a test.

BUG=539691
Committed: https://skia.googlesource.com/skia/+/5520dede2968372e79c572e37b7f325524ee0739

[herb_g, 2015-11-17 15:33:38]

Description was changed from

==========
Fix array overrun and add test.

BUG=539691
==========

to

==========
Fix buffer overrun, bit overrun and add a test.

BUG=539691
==========

[herb_g, 2015-11-17 15:33:38]

herb@google.com changed reviewers:
+ reed@google.com

[herb_g, 2015-11-17 15:33:53]

The CQ bit was checked by herb@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-11-17 15:34:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1453163002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1453163002/1

[commit-bot: I haz the power, 2015-11-17 15:36:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-17 15:36:04]

Dry run: Try jobs failed on following builders:
  Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)

[herb_g, 2015-11-17 15:48:34]

The CQ bit was checked by herb@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-11-17 15:48:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1453163002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1453163002/40001

[commit-bot: I haz the power, 2015-11-17 15:51:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-17 15:51:01]

Dry run: Try jobs failed on following builders:
  Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)

[herb_g, 2015-11-17 15:55:08]

remove stack alloc array

[herb_g, 2015-11-17 15:55:34]

The CQ bit was checked by herb@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-11-17 15:55:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1453163002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1453163002/60001

[commit-bot: I haz the power, 2015-11-17 16:09:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-17 16:09:16]

Dry run: This issue passed the CQ dry run.

[herb_g, 2015-11-17 16:13:52]

fix long line

[reed1, 2015-11-17 16:24:32]

hoooray! so glad you took the time to find/fix this.

https://codereview.chromium.org/1453163002/diff/80001/src/core/SkBlitter.cpp
File src/core/SkBlitter.cpp (right):

https://codereview.chromium.org/1453163002/diff/80001/src/core/SkBlitter.cpp#...
src/core/SkBlitter.cpp:137: U8CPU rightMask = 0xFF << (8 - (clip.width() & 7));
note: this will be bigger than a byte, something you fixed below for rite_mask
with &. Do we need these two lines to be different?

https://codereview.chromium.org/1453163002/diff/80001/tests/BlitMaskClip.cpp
File tests/BlitMaskClip.cpp (right):

https://codereview.chromium.org/1453163002/diff/80001/tests/BlitMaskClip.cpp#...
tests/BlitMaskClip.cpp:18: virtual void blitH(int x, int y, int width) override
{
our new convention is to *not* say virtual when we say override.

https://codereview.chromium.org/1453163002/diff/80001/tests/BlitMaskClip.cpp#...
tests/BlitMaskClip.cpp:30: 
Perhaps a small block-comment?

// Exercise all combinations of clips against a BW mask, to ensure we catch any
asserts or out-of-bounds reads (from old bugs)  < or something similar >

[herb_g, 2015-11-17 19:11:57]

rewrite

[herb_g, 2015-11-17 19:26:45]

fix rowbytes

[herb_g, 2015-11-17 20:14:03]

https://codereview.chromium.org/1453163002/diff/80001/src/core/SkBlitter.cpp
File src/core/SkBlitter.cpp (right):

https://codereview.chromium.org/1453163002/diff/80001/src/core/SkBlitter.cpp#...
src/core/SkBlitter.cpp:137: U8CPU rightMask = 0xFF << (8 - (clip.width() & 7));
On 2015/11/17 16:24:32, reed1 wrote:
> note: this will be bigger than a byte, something you fixed below for rite_mask
> with &. Do we need these two lines to be different?

This ends up working because the bits_to_run code end up morally and'ing when
when checking test. It would be a bug below because of the rite_mask comparison
to zero. I rewrote the code.

https://codereview.chromium.org/1453163002/diff/80001/tests/BlitMaskClip.cpp
File tests/BlitMaskClip.cpp (right):

https://codereview.chromium.org/1453163002/diff/80001/tests/BlitMaskClip.cpp#...
tests/BlitMaskClip.cpp:18: virtual void blitH(int x, int y, int width) override
{
On 2015/11/17 16:24:32, reed1 wrote:
> our new convention is to *not* say virtual when we say override.

Done.

https://codereview.chromium.org/1453163002/diff/80001/tests/BlitMaskClip.cpp#...
tests/BlitMaskClip.cpp:30: 
On 2015/11/17 16:24:32, reed1 wrote:
> Perhaps a small block-comment?
> 
> // Exercise all combinations of clips against a BW mask, to ensure we catch
any
> asserts or out-of-bounds reads (from old bugs)  < or something similar >

Done.

[herb_g, 2015-11-17 20:14:16]

The CQ bit was checked by herb@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-11-17 20:14:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1453163002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1453163002/120001

[commit-bot: I haz the power, 2015-11-17 20:16:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-17 20:16:21]

Dry run: Try jobs failed on following builders:
  Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)

[mtklein, 2015-11-17 20:33:02]

mtklein@google.com changed reviewers:
+ mtklein@google.com

[mtklein, 2015-11-17 20:33:02]

May want to keep an eye on gold diffs too:
https://gold.skia.org/search2?issue=1453163002&unt=true&query=source_type%3Dg...

[herb_g, 2015-11-17 21:19:17]

had x and y swapped

[herb_g, 2015-11-17 21:19:39]

The CQ bit was checked by herb@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-11-17 21:19:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1453163002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1453163002/140001

[commit-bot: I haz the power, 2015-11-17 21:33:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-17 21:34:00]

Dry run: This issue passed the CQ dry run.

[herb_g, 2015-11-17 21:41:39]

The CQ bit was checked by herb@google.com

[commit-bot: I haz the power, 2015-11-17 21:41:49]

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

[commit-bot: I haz the power, 2015-11-17 21:41:51]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from valid reviewer(s) till 2015-11-18 03:41 UTC

[herb_g, 2015-11-17 21:42:41]

Ok. The presubmits and gold look good. PTAL

Trybot results:
https://gold.skia.org/search2?issue=1453163002&unt=true&query=source_type%3Dg...

[commit-bot: I haz the power, 2015-11-18 03:41:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-18 03:41:01]

No LGTM from a valid reviewer yet. Please ask for an LGTM from a
full Skia committer

[reed1, 2015-11-18 18:40:14]

lgtm

[herb_g, 2015-11-18 18:49:45]

The CQ bit was checked by herb@google.com

[commit-bot: I haz the power, 2015-11-18 18:49:53]

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

[commit-bot: I haz the power, 2015-11-18 18:50:37]

Committed patchset #8 (id:140001) as
https://skia.googlesource.com/skia/+/5520dede2968372e79c572e37b7f325524ee0739