84225 2012-04-17 21:31:28 -0700 Skia OOM error when upscaling small subsets of images by large quantities 2012-04-18 11:44:34 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Mac Unspecified RESOLVED FIXED GoogleBug P2 Normal --- 1 groby groby jamesr jbates mark ojan pkasting reed senorblanco webkit.review.bot oldest_to_newest 604580 0 137653 groby 2012-04-17 21:31:28 -0700 Created attachment 137653 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 604582 1 pkasting 2012-04-17 21:36:44 -0700 See http://code.google.com/p/chromium/issues/detail?id=123589#c26 604586 2 jamesr 2012-04-17 21:47:27 -0700 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. 604587 3 groby 2012-04-17 21:48:49 -0700 jamesr@: It's coming. Still waiting for update-webkit to complete :) (Tried the patch in Chromium tree first) 604598 4 137653 levin 2012-04-17 22:37:34 -0700 Comment on attachment 137653 Suggested patch cq- simply because it will need a ChangeLog before landing. I'll let someone else do a real review. :) 604601 5 137655 groby 2012-04-17 22:44:07 -0700 Created attachment 137655 Proper patch - combined overflow & size test into one, too. 604609 6 137655 webkit.review.bot 2012-04-17 23:36:47 -0700 Comment on attachment 137655 Proper patch - combined overflow & size test into one, too. Clearing flags on attachment: 137655 Committed r114487: <http://trac.webkit.org/changeset/114487> 604610 7 webkit.review.bot 2012-04-17 23:36:51 -0700 All reviewed patches have been landed. Closing bug. 604883 8 ojan 2012-04-18 10:51:53 -0700 Just curious, why doesn't the destVisibleSize check a few lines later catch this case? 604889 9 jbates 2012-04-18 10:56:50 -0700 (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). 604891 10 groby 2012-04-18 10:59:43 -0700 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. 604914 11 jbates 2012-04-18 11:15:48 -0700 (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. 604920 12 groby 2012-04-18 11:23:17 -0700 (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. 604924 13 mark 2012-04-18 11:27:26 -0700 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? 604944 14 groby 2012-04-18 11:39:54 -0700 (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?) 604950 15 ojan 2012-04-18 11:44:34 -0700 (In reply to comment #14) > (I assume under a new bug?) Yes. For the most part, webkit takes a one-patch-per-bug approach. 137653 2012-04-17 21:31:28 -0700 2012-04-17 22:44:01 -0700 Suggested patch crash-fix.patch text/plain 1342 groby ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL3NraWEvTmF0aXZl SW1hZ2VTa2lhLmNwcCBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL3NraWEvTmF0 aXZlSW1hZ2VTa2lhLmNwcAppbmRleCA1ZTY2Y2E0Li5iN2Y5MjhjIDEwMDY0NAotLS0gYS9Tb3Vy Y2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9za2lhL05hdGl2ZUltYWdlU2tpYS5jcHAKKysr IGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3Mvc2tpYS9OYXRpdmVJbWFnZVNraWEu Y3BwCkBAIC0xMjcsMTIgKzEyNywyMiBAQCBib29sIE5hdGl2ZUltYWdlU2tpYTo6c2hvdWxkQ2Fj aGVSZXNhbXBsaW5nKGNvbnN0IFNrSVJlY3QmIHNyY1N1YnNldCwKICAgICBpZiAoIWlzRGF0YUNv bXBsZXRlKCkpCiAgICAgICAgIHJldHVybiBmYWxzZTsKIAorICAgIHN0YXRpYyBjb25zdCBpbnQg a0xhcmdlRGltZW5zaW9uID0gMzI3Njg7CisgICAgLy8gV2UgY2hlY2sgZm9yIGEgbWF4aW11bSBk aW1lbnNpb24gdG8gYXZvaWQgaW50ZWdlciBvdmVyZmxvdy4KKyAgICBpZiAoZGVzdFdpZHRoID4g a0xhcmdlRGltZW5zaW9uIHx8IGRlc3RIZWlnaHQgPiBrTGFyZ2VEaW1lbnNpb24pCisgICAgICAg IHJldHVybiBmYWxzZTsKKwogICAgIC8vIElmIHRoZSBkZXN0aW5hdGlvbiBiaXRtYXAgaXMgc21h bGwsIHdlJ2xsIGFsd2F5cyBhbGxvdyBjYWNoaW5nLCBzaW5jZQogICAgIC8vIHRoZXJlIGlzIG5v dCB2ZXJ5IG11Y2ggcGVuYWx0eSBmb3IgY29tcHV0aW5nIGl0IGFuZCBpdCBtYXkgY29tZSBpbiBo YW5keS4KICAgICBzdGF0aWMgY29uc3QgaW50IGtTbWFsbEJpdG1hcFNpemUgPSA0MDk2OwogICAg IGlmIChkZXN0V2lkdGggKiBkZXN0SGVpZ2h0IDw9IGtTbWFsbEJpdG1hcFNpemUpCiAgICAgICAg IHJldHVybiB0cnVlOwogCisgICAgLy8gSWYgdGhlIGRlc3RpbmF0aW9uIGJpdG1hcCBpcyBleGNl c3NpdmVseSBsYXJnZSwgd2UnbGwgbmV2ZXIgYWxsb3cgY2FjaGluZy4KKyAgICBzdGF0aWMgY29u c3QgaW50IGtMYXJnZUJpdG1hcFNpemUgPSA0MDk2ICogNDA5NjsKKyAgICBpZiAoZGVzdFdpZHRo ICogZGVzdEhlaWdodCA+IGtMYXJnZUJpdG1hcFNpemUpCisgICAgICAgIHJldHVybiBmYWxzZTsK KwogICAgIC8vIElmICJ0b28gbWFueSIgcmVxdWVzdHMgaGF2ZSBiZWVuIG1hZGUgZm9yIHRoaXMg Yml0bWFwLCB3ZSBhc3N1bWUgdGhhdAogICAgIC8vIG1hbnkgbW9yZSB3aWxsIGJlIG1hZGUgYXMg d2VsbCwgYW5kIHdlJ2xsIGdvIGFoZWFkIGFuZCBjYWNoZSBpdC4KICAgICBzdGF0aWMgY29uc3Qg aW50IGtNYW55UmVxdWVzdFRocmVzaG9sZCA9IDQ7Cg== 137655 2012-04-17 22:44:07 -0700 2012-04-17 23:36:47 -0700 Proper patch - combined overflow & size test into one, too. bug-84225-20120417224405.patch text/plain 1783 groby U3VidmVyc2lvbiBSZXZpc2lvbjogMTE0NDgwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNDM5ZjIwOTE0ZmI0MDll ZDFmZGMxZWFhNWUwNzg1MDY3YWMzM2YzYi4uYjM2YTRjNWVlYmEzZWU2NzA0NGMwNGNmYzIzODM3 NzA0ZTBhNDk0OCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDEyLTA0LTE3ICBSYWNo ZWwgQmx1bSAgPGdyb2J5QGNocm9taXVtLm9yZz4KKworICAgICAgICBTa2lhIE9PTSBlcnJvciB3 aGVuIHVwc2NhbGluZyBzbWFsbCBzdWJzZXRzIG9mIGltYWdlcyBieSBsYXJnZSBxdWFudGl0aWVz CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD04NDIyNQor CisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRlc3RlZCB3 aXRoIG1hbnVhbCB0ZXN0cy4gCisKKyAgICAgICAgKiBwbGF0Zm9ybS9ncmFwaGljcy9za2lhL05h dGl2ZUltYWdlU2tpYS5jcHA6CisgICAgICAgIChXZWJDb3JlOjpOYXRpdmVJbWFnZVNraWE6OnNo b3VsZENhY2hlUmVzYW1wbGluZyk6CisKIDIwMTItMDQtMTcgIEpvaG4gQmF1bWFuICA8amJhdW1h bkBjaHJvbWl1bS5vcmc+CiAKICAgICAgICAgW2Nocm9taXVtXSBFbnN1cmUgUmF0ZUxpbWl0ZXIg d2FpdHMgZm9yIFN3YXBidWZmZXJzIGNvbXBsZXRpb24KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJD b3JlL3BsYXRmb3JtL2dyYXBoaWNzL3NraWEvTmF0aXZlSW1hZ2VTa2lhLmNwcCBiL1NvdXJjZS9X ZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL3NraWEvTmF0aXZlSW1hZ2VTa2lhLmNwcAppbmRleCA1 ZTY2Y2E0N2I0MzkyNDZjMmRlN2IyMDE2Nzg4YTY5YTYzZjcxYmFlLi40MjVmNTY1NzhkYTZlNDQz MzM5NWJlZmIxZGE2YjYyZjBlZjUzY2E4IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wbGF0 Zm9ybS9ncmFwaGljcy9za2lhL05hdGl2ZUltYWdlU2tpYS5jcHAKKysrIGIvU291cmNlL1dlYkNv cmUvcGxhdGZvcm0vZ3JhcGhpY3Mvc2tpYS9OYXRpdmVJbWFnZVNraWEuY3BwCkBAIC0xMjcsNiAr MTI3LDExIEBAIGJvb2wgTmF0aXZlSW1hZ2VTa2lhOjpzaG91bGRDYWNoZVJlc2FtcGxpbmcoY29u c3QgU2tJUmVjdCYgc3JjU3Vic2V0LAogICAgIGlmICghaXNEYXRhQ29tcGxldGUoKSkKICAgICAg ICAgcmV0dXJuIGZhbHNlOwogCisgICAgLy8gSWYgdGhlIGRlc3RpbmF0aW9uIGJpdG1hcCBpcyBl eGNlc3NpdmVseSBsYXJnZSwgd2UnbGwgbmV2ZXIgYWxsb3cgY2FjaGluZy4KKyAgICBzdGF0aWMg Y29uc3QgdW5zaWduZWQgbG9uZyBsb25nIGtMYXJnZUJpdG1hcFNpemUgPSA0MDk2VUxMICogNDA5 NlVMTDsKKyAgICBpZiAoKHN0YXRpY19jYXN0PHVuc2lnbmVkIGxvbmcgbG9uZz4oZGVzdFdpZHRo KSAqIHN0YXRpY19jYXN0PHVuc2lnbmVkIGxvbmcgbG9uZz4oZGVzdEhlaWdodCkpID4ga0xhcmdl Qml0bWFwU2l6ZSkKKyAgICAgICAgcmV0dXJuIGZhbHNlOworCiAgICAgLy8gSWYgdGhlIGRlc3Rp bmF0aW9uIGJpdG1hcCBpcyBzbWFsbCwgd2UnbGwgYWx3YXlzIGFsbG93IGNhY2hpbmcsIHNpbmNl CiAgICAgLy8gdGhlcmUgaXMgbm90IHZlcnkgbXVjaCBwZW5hbHR5IGZvciBjb21wdXRpbmcgaXQg YW5kIGl0IG1heSBjb21lIGluIGhhbmR5LgogICAgIHN0YXRpYyBjb25zdCBpbnQga1NtYWxsQml0 bWFwU2l6ZSA9IDQwOTY7Cg==