Bug 84225 – Skia OOM error when upscaling small subsets of images by large quantities

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Rachel Blum]

Created attachment 137653 [details]
Suggested patch

When upscaling a small subsection of an image by a large amount, Skia can erroneously decide to upscale and cache the entire image. This can lead to OOM issues. See http://code.google.com/p/chromium/issues/detail?id=123589

Suggested patch included

[Peter Kasting]

See http://code.google.com/p/chromium/issues/detail?id=123589#c26

[James Robinson]

Try using the Tools/Scripts/webkit-patch script to upload a patch - "webkit-patch upload" from the root of the WebKit checkout should do what you want, just follow the prompts.

[Rachel Blum]

jamesr@: It's coming. Still waiting for update-webkit to complete :) (Tried the patch in Chromium tree first)

[David Levin]

Comment on attachment 137653 [details]
Suggested patch

cq- simply because it will need a ChangeLog before landing. I'll let someone else do a real review. :)

[Rachel Blum]

Created attachment 137655 [details]
Proper patch - combined overflow & size test into one, too.

[WebKit Review Bot]

Comment on attachment 137655 [details]
Proper patch - combined overflow & size test into one, too.

Clearing flags on attachment: 137655

Committed r114487: <http://trac.webkit.org/changeset/114487>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ojan Vafai]

Just curious, why doesn't the destVisibleSize check a few lines later catch this case?

[John Bates]

(In reply to comment #8)
> Just curious, why doesn't the destVisibleSize check a few lines later catch this case?

It probably does at first, but if the same resize is requested kManyRequestThreshold (4) times, it will cache it anyway (see code above destVisibleSize).

[Rachel Blum]

Three reasons why that final check doesn't work by itself

1) It's not overflow-proof. 
2) It happens after we check for repeated requests
3) It doesn't prevent requests for insanely large destSizes if the destVisibleSize is very large, too.

[John Bates]

(In reply to comment #10)
> Three reasons why that final check doesn't work by itself
> 
> 1) It's not overflow-proof. 

Shouldn't overflow be handled before getting here?

> 2) It happens after we check for repeated requests
> 3) It doesn't prevent requests for insanely large destSizes if the destVisibleSize is very large, too.

[Rachel Blum]

(In reply to comment #11)
> (In reply to comment #10)
> > Three reasons why that final check doesn't work by itself
> > 
> > 1) It's not overflow-proof. 
> 
> Shouldn't overflow be handled before getting here?

To be clear, I'm talking about integer overflow when multiplying destWidth * destHeight.

And no, it's not handled earlier - that's why the above patch does the ugly cast to ULL. The overflow happens if you take a source image (say, of the entire world) and blow it up insanely (down to street level). In that use case, we only want a tiny part of that giant image - the amount that fits on our screen.

If we decide this should be cached (as the code originally did, and as it still would according to #2), this would lead to OOM since we always cache the full image. (In the repro case, it went up to sizes of 130Kx130K pixels, IIRC)

And since that multiplication can overflow, there's a chance it erroneously decides to cache because the code thinks it's a small image. Nothing in the original code prevented that.

[Mark Mentovai]

This function now computes destWidth * destHeight three different times, one of which happens at a different bit width than the others. Does it need to be done three times?

[Rachel Blum]

(In reply to comment #13)
> This function now computes destWidth * destHeight three different times, one of which happens at a different bit width than the others. Does it need to be done three times?

Obviously not, but I wanted to get the patch in quickly, so this is as minimal a change set as possible.

Happy to add another patch to clean that up. (I assume under a new bug?)

[Ojan Vafai]

(In reply to comment #14)
> (I assume under a new bug?)

Yes. For the most part, webkit takes a one-patch-per-bug approach.