Exit SkBitmap::scrollRect() early if width <= 0

Exit SkBitmap::scrollRect() early if width <= 0

As suggested in email from saintlou@google.com
Committed: http://code.google.com/p/skia/source/detail?r=1948

[epoger, 2011-07-22 15:50:55]

Here is the suggestion email from saintlou@google.com :

We get in a situation where memmove gets a negative value as an arg :-) in a
skia method, so Chrome crashes. We would like to patch skia as follows (see
details of investigation from Josh)


---------- Forwarded message ----------
From: Josh Horwich <jhorwich@google.com>
Date: Thu, Jul 21, 2011 at 5:03 PM
Subject: SkBitmap::scrollRect
To: Emmanuel Saint-loubert <saintlou@google.com>


Emmanuel,
   Looking at the crash you had, and the skia code:
   If one scrolls a 100-pixel bitmap by 99 pixels in x, width will become 1
(*bytes-per-pixel) so memmove has something useful to do
   If one scrolls a 100-pixel bitmap by 100 pixels in x, width will become 0, so
memmove will (I suppose) no-op but otherwise be OK.
   However, if one scrolls a 100-pixel bitmap by 105 pixels in x, width will
become -5 (*bytes-per-pixel) so we'll call memmove with a negative number

   They implicitly handle cases where abs(dy)>height as the loop  "while (
--height>=0 )" will evaluate to false immediately the first time.
   I would suspect a simple "if (width<=0) { return true; } " after the dx-based
adjustment would cause the "right thing" (no crash / no calls to memmove with a
negative number, no wasting of time when scrolling more than the bitmaps's width
horizontally) to occur - leaving the "exposed" pixels untouched, which seems to
be consistent with this method's handling for cases where the scroll amount
actually does result in useful work occuring.

   The calling method (BackingStoreSkia::ScrollBackingStore) might be able to
easily work around this, but that seems like the wrong place to do it - if
anyone else calls skia with a condition that causes this to occur, it would be
nice if skia didn't fail in this way...

- Josh

[TomH, 2011-07-22 15:57:52]

http://codereview.appspot.com/4806047/diff/1/src/core/SkBitmap_scroll.cpp
File src/core/SkBitmap_scroll.cpp (right):

http://codereview.appspot.com/4806047/diff/1/src/core/SkBitmap_scroll.cpp#new...
src/core/SkBitmap_scroll.cpp:100: return true;
The comment at line 72 reads "If we get this far, then we need to shift the
pixels", but this change falsifies it. The new code is a really straightforward
test, so I don't want to lose it, but is there a similarly simple rewriting that
would let us move it to line 71? If not, we should fix that comment.

[epoger, 2011-07-22 16:23:37]

http://codereview.appspot.com/4806047/diff/1/src/core/SkBitmap_scroll.cpp
File src/core/SkBitmap_scroll.cpp (right):

http://codereview.appspot.com/4806047/diff/1/src/core/SkBitmap_scroll.cpp#new...
src/core/SkBitmap_scroll.cpp:100: return true;
On 2011/07/22 15:57:52, TomH wrote:
> The comment at line 72 reads "If we get this far, then we need to shift the
> pixels", but this change falsifies it. The new code is a really
straightforward
> test, so I don't want to lose it, but is there a similarly simple rewriting
that
> would let us move it to line 71? If not, we should fix that comment.

Good catch, Tom.

I'm going to add a test to exercise this change anyway... I'll make sure the
documentation is proper while I'm at it, and email out again when it's ready for
re-review.

http://codereview.appspot.com/4806047/diff/1/src/core/SkBitmap_scroll.cpp#new...
src/core/SkBitmap_scroll.cpp:100: return true;
P.S. Reading the documentation in SkBitmap.h, I'm not sure whether this should
return true or false in this case.  Any suggestions?

[JoshH, 2011-07-22 22:31:09]

Just my thoughts on Elliot's comments/question. Please keep in mind I am just
rendering my opinion, I'm not the expert by any means...

- Josh

http://codereview.appspot.com/4806047/diff/1/src/core/SkBitmap_scroll.cpp
File src/core/SkBitmap_scroll.cpp (right):

http://codereview.appspot.com/4806047/diff/1/src/core/SkBitmap_scroll.cpp#new...
src/core/SkBitmap_scroll.cpp:100: return true;
On 2011/07/22 16:23:37, epoger wrote:
> On 2011/07/22 15:57:52, TomH wrote:
> > The comment at line 72 reads "If we get this far, then we need to shift the
> > pixels", but this change falsifies it. The new code is a really
> straightforward
> > test, so I don't want to lose it, but is there a similarly simple rewriting
> that
> > would let us move it to line 71? If not, we should fix that comment.
> 
> Good catch, Tom.
> 
> I'm going to add a test to exercise this change anyway... I'll make sure the
> documentation is proper while I'm at it, and email out again when it's ready
for
> re-review.

FWIW, I think you'd only exercise this change if inval is null - otherwise the
inval computation code would have already exited (returning true and setting
inval to the entire bounds) due to lack of any intersection (line 56-58). The
code that

http://codereview.appspot.com/4806047/diff/1/src/core/SkBitmap_scroll.cpp#new...
src/core/SkBitmap_scroll.cpp:100: return true;
On 2011/07/22 16:23:37, epoger wrote:
> P.S. Reading the documentation in SkBitmap.h, I'm not sure whether this should
> return true or false in this case.  Any suggestions?

My suggestion, for what it's worth: return true. This is consistent with what
happens if there are no pixels to actually copy due to height<=0
post-dy-computation, or if the caller had passed in a non-null inval parameter. 

Side note, examining the code makes me think there is a different inconsistency
for the return value and the documentation. If the caller passes in a non-null
subset, then anything that triggers a false result from extractSubset - such as
a null fPixels, or even a subset completely out of the bitmap's bounds - will a)
cause scrollRect to return false and b) leave inval unchanged. This is not in
line with the doxygen comment in SkBitmap.h which says "if no pixels are present
(i.e. getPixels() returns false) inval will still be updated, and true will be
returned."

Second side note, since I'm on a roll... if someone passes in a subset that
doesn't have the top,left at (0,0), and they pass an inval to be computed, the
inval is not offset to compensate for the fact that the recursive call is on an
extractSubset bitmap whose 0,0 is at top/left of the original bitmap. e.g.
myBitmap->scrollRect( {x=10,y=0,width, height}, 1, 0, &myInval ) would - I think
- result in the correct pixels to be moved, but myInval being a 1-pixel-wide
region from 0,0 to 1,height rather than (10,0) to (11,height). Perhaps we need
to be translating the inval properly (by subset's origin) after calling
tmp.scrollRect but before returning to the original caller...

With all that said, the code we have that discovered this bug does not pass in
an inval, nor does it even look at the return value.

[epoger, 2011-07-25 13:39:39]

Update: On Friday I created some new tests for this code within Skia's "gm" but
I was unable to reproduce this particular error.  My top priority for today is
to reproduce and correct the error.  More news later.

[epoger, 2011-07-25 15:44:24]

Added a new "gm" test case that reproduces the segfault, and demonstrates the
scroll operations while we are at it (so we can guard against regressions in
scroll behavior).

Please re-review.

[TomH, 2011-07-25 15:56:23]

Yay, new tests.

LGTM

[reed1, 2011-07-25 15:58:57]

Can we write a unittest for this, to verify the bool result for various
parameters?

[epoger, 2011-07-25 16:34:07]

On 2011/07/25 15:58:57, reed1 wrote:
> Can we write a unittest for this, to verify the bool result for various
> parameters?

I have gone ahead and committed this change for now, so that Chromium can get
the bug fix.  I have filed this bug to track the remaining work:
http://code.google.com/p/skia/issues/detail?id=327 ('add unittest for
SkBitmap::scrollRect()')