Summary: | Skia OOM error when upscaling small subsets of images by large quantities | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Rachel Blum <groby> | ||||||
Component: | WebCore Misc. | Assignee: | Rachel Blum <groby> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | jamesr, jbates, mark, ojan, pkasting, reed, senorblanco, webkit.review.bot | ||||||
Priority: | P2 | Keywords: | GoogleBug | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Mac | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Rachel Blum
2012-04-17 21:31:28 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. jamesr@: It's coming. Still waiting for update-webkit to complete :) (Tried the patch in Chromium tree first) 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. :)
Created attachment 137655 [details]
Proper patch - combined overflow & size test into one, too.
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> All reviewed patches have been landed. Closing bug. Just curious, why doesn't the destVisibleSize check a few lines later catch this case? (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). 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. (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. (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. 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? (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?) (In reply to comment #14) > (I assume under a new bug?) Yes. For the most part, webkit takes a one-patch-per-bug approach. |