Currently ImageBuffer asks the ImageBufferBackend to return it. And in the case of the RemoteImageBufferProxy, we have to ensure is have been created in the GPU Process and its IOSurface or ShareableBitmap has been mapped in the Web Process. This is too complicated for something which can be calculated from the backend size and the bytesPerRow of the ImageBufferBackend. Also because the memoryCost of the ImageBuffer can be asked out of the main thread, the following assertion may fire: ASSERTION FAILED: !m_impl || m_impl->wasConstructedOnMainThread() == isMainThread() /Users/mmaxfield/Build/Products/Debug/usr/local/include/wtf/WeakPtr.h(107) : T *WTF::WeakPtr<WebKit::RemoteRenderingBackendProxy>::operator->() const [T = WebKit::RemoteRenderingBackendProxy, Counter = WTF::EmptyCounter] 1 0x509af3529 WTFCrash 2 0x4f33ab30b WTFCrashWithInfo(int, char const*, char const*, int) 3 0x4f4e976fc WTF::WeakPtr<WebKit::RemoteRenderingBackendProxy, WTF::EmptyCounter>::operator->() const 4 0x4f4e97831 WebKit::RemoteImageBufferProxy<WebKit::ImageBufferShareableMappedIOSurfaceBackend>::ensureBackendCreated() const 5 0x4f3e109ba WebCore::ConcreteImageBuffer<WebKit::ImageBufferShareableMappedIOSurfaceBackend>::memoryCost() const 6 0x51f0b4358 WebCore::CanvasBase::memoryCost() const 7 0x51cac6f91 void WebCore::JSHTMLCanvasElement::visitChildrenImpl<JSC::SlotVisitor>(JSC::JSCell*, JSC::SlotVisitor&) 8 0x51cac5bbd WebCore::JSHTMLCanvasElement::visitChildren(JSC::JSCell*, JSC::SlotVisitor&) 9 0x50ad566b9 JSC::MethodTable::visitChildren(JSC::JSCell*, JSC::SlotVisitor&) const
<rdar://75648337>
Created attachment 425450 [details] Patch
Created attachment 425460 [details] Patch
Created attachment 425465 [details] Patch
Created attachment 425473 [details] Patch
Created attachment 425476 [details] Patch
Comment on attachment 425476 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425476&action=review > Source/WebCore/platform/graphics/ImageBufferBackend.cpp:40 > - IntSize backendSize = IntSize(scaledSize); > - > - Checked<unsigned, RecordOverflow> bytesPerRow = 4 * Checked<unsigned, RecordOverflow>(backendSize.width()); > - if (bytesPerRow.hasOverflowed()) > - return { }; > - > - CheckedSize numBytes = Checked<unsigned, RecordOverflow>(backendSize.height()) * bytesPerRow; > - if (numBytes.hasOverflowed()) > - return { }; > + return IntSize(scaledSize); Don't we need CheckedArithmetic to ensure that scaledSize doesn't overflow an IntSize? parameters.resolutionScale could be large. > Source/WebCore/platform/graphics/cg/ImageBufferCGBackend.cpp:69 > + Checked<unsigned, RecordOverflow> bytesPerRow = (Checked<unsigned, RecordOverflow>(backendSize.width()) * 4); > + ASSERT(!bytesPerRow.hasOverflowed()); The point of Checked<> is to catch things in release builds. > Source/WebCore/platform/graphics/cg/ImageBufferCGBitmapBackend.cpp:48 > + Checked<unsigned, RecordOverflow> bytesPerRow = 4 * Checked<unsigned, RecordOverflow>(backendSize.width()); Maybe now, certainly in the future, assuming a pixel is 4 bytes is wrong.
Comment on attachment 425476 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425476&action=review >> Source/WebCore/platform/graphics/ImageBufferBackend.cpp:40 >> + return IntSize(scaledSize); > > Don't we need CheckedArithmetic to ensure that scaledSize doesn't overflow an IntSize? parameters.resolutionScale could be large. I think we check this above in if (scaledSize.isEmpty() || !scaledSize.isExpressibleAsIntSize()) return { }; >> Source/WebCore/platform/graphics/cg/ImageBufferCGBackend.cpp:69 >> + ASSERT(!bytesPerRow.hasOverflowed()); > > The point of Checked<> is to catch things in release builds. Yes you are right. unsafeGet() below will call RecordOverflow::crash() if bytesPerRow.hasOverflowed() is true. >> Source/WebCore/platform/graphics/cg/ImageBufferCGBitmapBackend.cpp:48 >> + Checked<unsigned, RecordOverflow> bytesPerRow = 4 * Checked<unsigned, RecordOverflow>(backendSize.width()); > > Maybe now, certainly in the future, assuming a pixel is 4 bytes is wrong. There are many places in the ImageBuffer code assume a pixel is 4 bytes. I think making bytesPerPixel be color-space dependent needs a lot of changes.
Created attachment 425659 [details] Patch
Created attachment 425715 [details] Patch
Committed r275905 (236469@main): <https://commits.webkit.org/236469@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 425715 [details].