Bug 46437 - Skia image decoder needs to check whether bitmap copying succeeded
Summary: Skia image decoder needs to check whether bitmap copying succeeded
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Peter Kasting
URL: http://asset.soup.io/asset/1056/8367_...
Depends on:
Reported: 2010-09-23 17:36 PDT by Peter Kasting
Modified: 2010-09-27 16:45 PDT (History)
0 users

See Also:

patch v1 (30.78 KB, patch)
2010-09-23 18:34 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
patch v2 (29.46 KB, patch)
2010-09-27 14:05 PDT, Peter Kasting
jamesr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Kasting 2010-09-23 17:36:09 PDT
Visiting the provided URL tries to display an extremely large animated GIF.  On my machine, Skia fails to allocate when it reached frame 3, but we don't notice because we don't check the return value of NativeImageSkia::copyTo().  Most other platforms presumably just crash directly when they can't allocate the memory trying to do the Vector copy, which is fine, but Skia uses an allocator that doesn't crash on failure.  So this means we can corrupt memory.

The fix is simple: check the relevant return code.
Comment 1 Peter Kasting 2010-09-23 18:34:42 PDT
Created attachment 68638 [details]
patch v1

The only bad thing about this patch is that the layout test waits for 15 seconds.  This was because the test case took anywhere from 7 to 11 seconds to crash on my Dev channel build of Chrome.  I'm not sure how to speed this up :(
Comment 2 Peter Kasting 2010-09-24 09:12:15 PDT
Hmm... another problem is that I bet non-Skia platforms that use the open-source image decoders will crash on this testcase once it runs them out of RAM.

Maybe I should make the layout test a manual test instead?
Comment 3 Peter Kasting 2010-09-27 14:05:45 PDT
Created attachment 68954 [details]
patch v2

This makes the test a manual test at dglazkov's suggestion.

I also fixed a missing resource in a similar test while I was at it.  I can commit that part separately, but I threw it in here so I could get two r+s for the price of one.
Comment 4 James Robinson 2010-09-27 14:13:27 PDT
Comment on attachment 68954 [details]
patch v2

R=me. Please make sure the images aren't svn:executable before landing.
Comment 5 Peter Kasting 2010-09-27 16:45:01 PDT
Fixed in r68446.