Mac Flash: Fix scaling bitmap

Mac Flash: Fix scaling bitmap

There was a bug in the Flash code that caused a render crash. To reproduce the crash you had to do the following:
  - run on a Retina device
  - disable accelerated compositing
  - enable click to play
In this case playing a flash video would crash the renderer.

The problem was that we were incorrectly calculating the scaled rect of a bitmap.

For example, when the pixel size of the bitmap is 1x1 the scaled size should be 0x0. Instead we were rounding up and returning a scaled valeu of 1x1.

Fix was to use ToEnclosedRect instead of ToEnclosingRect.

BUG=155309
TBR=brettw@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162384

[Scott Hess - ex-Googler, 2012-10-14 21:43:46]

lgtm

[Nico, 2012-10-14 22:08:01]

test?

[danakj, 2012-10-16 00:54:52]

I'm not sure what I should review here. You used ToEnclosedRect correctly, so
lgtm? But I don't understand why you would want the result of a 1x1 rect scaled
to be 0x0. A test demonstrating the problem would go a long way if you want
feedback.

[sail, 2012-10-16 18:44:27]

Added test, please take another look.

[sail, 2012-10-16 18:45:25]

+brettw for test changes

[sail, 2012-10-16 18:46:30]

On 2012/10/16 00:54:52, danakj wrote:
> I'm not sure what I should review here. You used ToEnclosedRect correctly, so
> lgtm? But I don't understand why you would want the result of a 1x1 rect
scaled
> to be 0x0. A test demonstrating the problem would go a long way if you want
> feedback.

Hi danakj, I added a test to demonstrate this. The main reason is that the rect
is later to used to access the bitmap memory. If it's scaled to be larger than
the bitmap then we have a crash. Thanks.

[danakj, 2012-10-16 18:52:56]

On 2012/10/16 18:46:30, sail wrote:
> On 2012/10/16 00:54:52, danakj wrote:
> > I'm not sure what I should review here. You used ToEnclosedRect correctly,
so
> > lgtm? But I don't understand why you would want the result of a 1x1 rect
> scaled
> > to be 0x0. A test demonstrating the problem would go a long way if you want
> > feedback.
> 
> Hi danakj, I added a test to demonstrate this. The main reason is that the
rect
> is later to used to access the bitmap memory. If it's scaled to be larger than
> the bitmap then we have a crash. Thanks.

Thanks for the explanation. Is the place that decides the size of the bitmap
also using enclosed rect? Where does that happen?

[sail, 2012-10-16 19:15:34]

On 2012/10/16 18:52:56, danakj wrote:
> On 2012/10/16 18:46:30, sail wrote:
> > On 2012/10/16 00:54:52, danakj wrote:
> > > I'm not sure what I should review here. You used ToEnclosedRect correctly,
> so
> > > lgtm? But I don't understand why you would want the result of a 1x1 rect
> > scaled
> > > to be 0x0. A test demonstrating the problem would go a long way if you
want
> > > feedback.
> > 
> > Hi danakj, I added a test to demonstrate this. The main reason is that the
> rect
> > is later to used to access the bitmap memory. If it's scaled to be larger
than
> > the bitmap then we have a crash. Thanks.
> 
> Thanks for the explanation. Is the place that decides the size of the bitmap
> also using enclosed rect? Where does that happen?

Weird, I thought the change to use ToEnclosingRect() was made by you but it
looks like it was made by vollick@ instead.

Anyways, here's the other place where we compute the bitmap size:
https://chromiumcodereview.appspot.com/10996037/diff/9010/content/browser/ren...

This place uses ToEnclosing rect which I think is correct since we're going from
size to pixel size.

[sail, 2012-10-16 19:15:56]

+vollick ^^ See above conversation.

[sail, 2012-10-16 21:31:38]

spamming more webkit/plugin/ppapi/* OWNERS

[danakj, 2012-10-16 21:35:50]

On 2012/10/16 19:15:34, sail wrote:
> On 2012/10/16 18:52:56, danakj wrote:
> > On 2012/10/16 18:46:30, sail wrote:
> > > On 2012/10/16 00:54:52, danakj wrote:
> > > > I'm not sure what I should review here. You used ToEnclosedRect
correctly,
> > so
> > > > lgtm? But I don't understand why you would want the result of a 1x1 rect
> > > scaled
> > > > to be 0x0. A test demonstrating the problem would go a long way if you
> want
> > > > feedback.
> > > 
> > > Hi danakj, I added a test to demonstrate this. The main reason is that the
> > rect
> > > is later to used to access the bitmap memory. If it's scaled to be larger
> than
> > > the bitmap then we have a crash. Thanks.
> > 
> > Thanks for the explanation. Is the place that decides the size of the bitmap
> > also using enclosed rect? Where does that happen?
> 
> Weird, I thought the change to use ToEnclosingRect() was made by you but it
> looks like it was made by vollick@ instead.
> 
> Anyways, here's the other place where we compute the bitmap size:
>
https://chromiumcodereview.appspot.com/10996037/diff/9010/content/browser/ren...
> 
> This place uses ToEnclosing rect which I think is correct since we're going
from
> size to pixel size.

If that code uses enclosing, then how do you ever get a 1x1 pixel-space rect to
start with? If the scale is 2, shouldn't any pixel buffer be a multiple of 2?

[sail, 2012-10-16 21:44:10]

On 2012/10/16 21:35:50, danakj wrote:
> On 2012/10/16 19:15:34, sail wrote:
> > On 2012/10/16 18:52:56, danakj wrote:
> > > On 2012/10/16 18:46:30, sail wrote:
> > > > On 2012/10/16 00:54:52, danakj wrote:
> > > > > I'm not sure what I should review here. You used ToEnclosedRect
> correctly,
> > > so
> > > > > lgtm? But I don't understand why you would want the result of a 1x1
rect
> > > > scaled
> > > > > to be 0x0. A test demonstrating the problem would go a long way if you
> > want
> > > > > feedback.
> > > > 
> > > > Hi danakj, I added a test to demonstrate this. The main reason is that
the
> > > rect
> > > > is later to used to access the bitmap memory. If it's scaled to be
larger
> > than
> > > > the bitmap then we have a crash. Thanks.
> > > 
> > > Thanks for the explanation. Is the place that decides the size of the
bitmap
> > > also using enclosed rect? Where does that happen?
> > 
> > Weird, I thought the change to use ToEnclosingRect() was made by you but it
> > looks like it was made by vollick@ instead.
> > 
> > Anyways, here's the other place where we compute the bitmap size:
> >
>
https://chromiumcodereview.appspot.com/10996037/diff/9010/content/browser/ren...
> > 
> > This place uses ToEnclosing rect which I think is correct since we're going
> from
> > size to pixel size.
> 
> If that code uses enclosing, then how do you ever get a 1x1 pixel-space rect
to
> start with? If the scale is 2, shouldn't any pixel buffer be a multiple of 2?

When the bitmap is first initialized it's pixel rect is 1x1.

[danakj, 2012-10-16 21:55:16]

> When the bitmap is first initialized it's pixel rect is 1x1.

Consider 1.5 scale factor. If the plugin has a size of 5x5, then the backing
store bitmap will have a size of 8x8. But this change here is going to give you
7x7?

[Nico, 2012-10-16 22:08:58]

Thanks for adding a test. This lgtm as crash fix.

Medium term, it'd be good if image_data and scale_factor would never be out of
sync. The problem here is that the scale factor is updated, then this code runs,
and only then the image data pointer is updated. In the software case, bitmap
data and scale factor are always changed "atomically" (not in a threads sense,
just in the same function, without anything happening in between). I think
that'd be more robust if that could be done here too.

[sail, 2012-10-16 22:17:34]

On 2012/10/16 21:55:16, danakj wrote:
> > When the bitmap is first initialized it's pixel rect is 1x1.
> 
> Consider 1.5 scale factor. If the plugin has a size of 5x5, then the backing
> store bitmap will have a size of 8x8. But this change here is going to give
you
> 7x7?

Hi danakj, the as it's currently written will not handle non-integer scale
factors. That's because we pass integer rects up to the browser process. The
integer rect is then used to address into the bitmap memory.

To support a 1.5 scale factor without crashing we would have to change the code
path to use RectF.

[danakj, 2012-10-16 22:30:15]

I see, thanks for the explanations. I'm glad Nico is pointing toward
where this code should go in the future, as changes like this can
become difficult to untangle for non-integer device scales. If it
wouldn't be less correct for integers, consider switching them both to
be ToEnclosedRect to make the code consistent.

lgtm

[piman, 2012-10-16 23:31:15]

lgtm

[commit-bot: I haz the power, 2012-10-16 23:36:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/11146009/8001

[commit-bot: I haz the power, 2012-10-16 23:36:21]

Presubmit check for 11146009-8001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    webkit

Presubmit checks took 1.4s to calculate.

[commit-bot: I haz the power, 2012-10-16 23:38:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/11146009/8001

[commit-bot: I haz the power, 2012-10-17 11:16:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/11146009/8001

[commit-bot: I haz the power, 2012-10-17 13:22:35]

Change committed as 162384