Streamline ImageData size calculations and handle out-of-memory
Created attachment 421418 [details] Patch
rdar://72090831
Comment on attachment 421418 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=421418&action=review r=me > Source/WebCore/html/ImageData.cpp:69 > RefPtr<ImageData> ImageData::create(const IntSize& size) > { > - Checked<int, RecordOverflow> dataSize = 4; > - dataSize *= size.width(); > - dataSize *= size.height(); > + auto dataSize = ImageData::dataSize(size); > if (dataSize.hasOverflowed()) > return nullptr; > - > - return adoptRef(*new ImageData(size)); > + return adoptRef(*new ImageData(size, Uint8ClampedArray::createUninitialized(dataSize.unsafeGet()))); > } It's kind of funny that this function will safely return null on absurd sizes that overflow, but crash on more modest sizes that trigger allocation failure. Should we crash on overflow too? Or return null on allocation failure instead?
Comment on attachment 421418 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=421418&action=review >> Source/WebCore/html/ImageData.cpp:69 >> } > > It's kind of funny that this function will safely return null on absurd sizes that overflow, but crash on more modest sizes that trigger allocation failure. Should we crash on overflow too? Or return null on allocation failure instead? Those thoughts occurred to me, too. I wasn’t sure enough about where and how this function is used to make a good decision. Let me research that now.
Comment on attachment 421418 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=421418&action=review >>> Source/WebCore/html/ImageData.cpp:69 >>> } >> >> It's kind of funny that this function will safely return null on absurd sizes that overflow, but crash on more modest sizes that trigger allocation failure. Should we crash on overflow too? Or return null on allocation failure instead? > > Those thoughts occurred to me, too. I wasn’t sure enough about where and how this function is used to make a good decision. Let me research that now. Looking into this has led me down a bit of a rabbit hole. I discovered that CanvasRenderingContext2D calls this in response to calls from JavaScript and thus should be using a create function that can return an exception. That needs to be fixed, which might change the ImageData class interface to add more create functions, or we might just use the JavaScript-friendly one. Maybe it needs a name different from just "create" to make it clear that it tries or to make it clear that it’s for JavaScript-exposed uses. I’m not sure I should make any behavior change to this function in this patch, though. It seems logical that returning nullptr would be OK, but I am not really sure. One of the uses is in SerializedScriptValue, and I’m not sure are what the implications there; probably fine to fail cleanly if out of memory? It might be indirectly JavaScript-exposed?
Comment on attachment 421418 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=421418&action=review >>>> Source/WebCore/html/ImageData.cpp:69 >>>> } >>> >>> It's kind of funny that this function will safely return null on absurd sizes that overflow, but crash on more modest sizes that trigger allocation failure. Should we crash on overflow too? Or return null on allocation failure instead? >> >> Those thoughts occurred to me, too. I wasn’t sure enough about where and how this function is used to make a good decision. Let me research that now. > > Looking into this has led me down a bit of a rabbit hole. > > I discovered that CanvasRenderingContext2D calls this in response to calls from JavaScript and thus should be using a create function that can return an exception. That needs to be fixed, which might change the ImageData class interface to add more create functions, or we might just use the JavaScript-friendly one. Maybe it needs a name different from just "create" to make it clear that it tries or to make it clear that it’s for JavaScript-exposed uses. > > I’m not sure I should make any behavior change to this function in this patch, though. It seems logical that returning nullptr would be OK, but I am not really sure. One of the uses is in SerializedScriptValue, and I’m not sure are what the implications there; probably fine to fail cleanly if out of memory? It might be indirectly JavaScript-exposed? Turns out that changing behavior here, to return nullptr, fixes a failing regression test. So I’m going to change as you suggested (your second suggestion, return null). There is still follow up needed to make sure we consistently raise an exception rather than failing silently or returning null in various JavaScript contexts. And I suppose some tiny risk that returning nullptr rather than crashing is worse in some way.
Created attachment 421481 [details] Patch
Committed r273505 (234582@main): <https://commits.webkit.org/234582@main>