Bug 84225 - Skia OOM error when upscaling small subsets of images by large quantities
Summary: Skia OOM error when upscaling small subsets of images by large quantities
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac Unspecified
: P2 Normal
Assignee: Rachel Blum
URL:
Keywords: GoogleBug
Depends on:
Blocks:
 
Reported: 2012-04-17 21:31 PDT by Rachel Blum
Modified: 2012-04-18 11:44 PDT (History)
8 users (show)

See Also:


Attachments
Suggested patch (1.31 KB, patch)
2012-04-17 21:31 PDT, Rachel Blum
no flags Details | Formatted Diff | Diff
Proper patch - combined overflow & size test into one, too. (1.74 KB, patch)
2012-04-17 22:44 PDT, Rachel Blum
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rachel Blum 2012-04-17 21:31:28 PDT
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
Comment 2 James Robinson 2012-04-17 21:47:27 PDT
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.
Comment 3 Rachel Blum 2012-04-17 21:48:49 PDT
jamesr@: It's coming. Still waiting for update-webkit to complete :) (Tried the patch in Chromium tree first)
Comment 4 David Levin 2012-04-17 22:37:34 PDT
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. :)
Comment 5 Rachel Blum 2012-04-17 22:44:07 PDT
Created attachment 137655 [details]
Proper patch - combined overflow & size test into one, too.
Comment 6 WebKit Review Bot 2012-04-17 23:36:47 PDT
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>
Comment 7 WebKit Review Bot 2012-04-17 23:36:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Ojan Vafai 2012-04-18 10:51:53 PDT
Just curious, why doesn't the destVisibleSize check a few lines later catch this case?
Comment 9 John Bates 2012-04-18 10:56:50 PDT
(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).
Comment 10 Rachel Blum 2012-04-18 10:59:43 PDT
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.
Comment 11 John Bates 2012-04-18 11:15:48 PDT
(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.
Comment 12 Rachel Blum 2012-04-18 11:23:17 PDT
(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.
Comment 13 Mark Mentovai 2012-04-18 11:27:26 PDT
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?
Comment 14 Rachel Blum 2012-04-18 11:39:54 PDT
(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?)
Comment 15 Ojan Vafai 2012-04-18 11:44:34 PDT
(In reply to comment #14)
> (I assume under a new bug?)

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