Bug 224306 – [GPU Process] Make ImageBuffer calculate its memoryCost() from its size

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Said Abou-Hallawa]

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

[Said Abou-Hallawa]

<rdar://75648337>

[Said Abou-Hallawa]

Created attachment 425450 [details]
Patch

[Said Abou-Hallawa]

Created attachment 425460 [details]
Patch

[Said Abou-Hallawa]

Created attachment 425465 [details]
Patch

[Said Abou-Hallawa]

Created attachment 425473 [details]
Patch

[Said Abou-Hallawa]

Created attachment 425476 [details]
Patch

[Simon Fraser (smfr)]

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.

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

Created attachment 425659 [details]
Patch

[Said Abou-Hallawa]

Created attachment 425715 [details]
Patch

[EWS]

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].