Bug 226813 – WebGL context image buffer accumulates the NativeImage contents when drawn to RemoteImageBufferProxy

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

[Kimmo Kinnunen]

WebGL context image buffer accumulates the NativeImage contents when drawn to RemoteImageBufferProxy
Causes leaks
https://ciechanow.ski/internal-combustion-engine/

[Kimmo Kinnunen]

<rdar://77421966>

[Kimmo Kinnunen]

Created attachment 430975 [details]
Patch

[Chris Dumez]

Comment on attachment 430975 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430975&action=review

> Source/WebCore/platform/graphics/displaylists/DisplayListDrawingContext.cpp:69
> +    for (auto imageBuffer : m_displayList.imageBuffers().values())

auto& to avoid ref counting churn.

> Source/WebCore/platform/graphics/displaylists/DisplayListDrawingContext.cpp:71
> +    for (auto imageBuffer : m_displayList.imageBuffers().values())

ditto.

> Source/WebCore/platform/graphics/displaylists/DisplayListDrawingContext.h:41
> +    WEBCORE_EXPORT ~DrawingContext();

Do we construct / destroy a lot of DrawingContext objects? If so, we could consider putting this custom destructor in  #if ASSERT_ENABLED

[Brent Fulgham]

Looks like this introduced some nullptr crashes, perhaps due to clearing more frequently?

Application Specific Information:
CRASHING TEST: fast/canvas/canvas-as-image-hidpi.html

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x00000001318c9c60 WebCore::ImageBufferIOSurfaceBackend::context() const + 16
1   com.apple.WebKit              	0x0000000109c27757 WebCore::DisplayList::ImageBuffer<WebKit::ImageBufferShareableMappedIOSurfaceBackend>::~ImageBuffer() + 33
2   com.apple.WebKit              	0x0000000109c26a9c WebKit::RemoteImageBufferProxy<WebKit::ImageBufferShareableMappedIOSurfaceBackend>::~RemoteImageBufferProxy() + 14
3   com.apple.WebCore             	0x00000001312c0d65 WebCore::HTMLCanvasElement::reset() + 1205
4   com.apple.WebCore             	0x00000001312c1119 WebCore::HTMLCanvasElement::setSize(WebCore::IntSize const&) + 265
5   com.apple.WebCore             	0x00000001310d4001 WebCore::Document::getCSSCanvasContext(WTF::String const&, WTF::String const&, int, int) + 65
6   com.apple.WebCore             	0x0000000130351d28 WebCore::jsDocumentPrototypeFunction_getCSSCanvasContext(JSC::JSGlobalObject*, JSC::CallFrame*) + 504
7   ???                           	0x000053a3598011d8 0 + 91961046340056
8   com.apple.JavaScriptCore      	0x0000000135a05587 llint_entry + 110528
9   com.apple.JavaScriptCore      	0x00000001359ea3c6 vmEntryToJavaScript + 216
10  com.apple.JavaScriptCore      	0x000000013616960b JSC::Interpreter::executeCall(JSC::JSGlobalObject*, JSC::JSObject*, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 555
11  com.apple.JavaScriptCore      	0x00000001364149c4 JSC::profiledCall(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 164
12  com.apple.JavaScriptCore      	0x0000000135aa2fa5 JSObjectCallAsFunction + 661
13  com.apple.WebKitTestRunner.InjectedBundle	0x000000013a84c897 WTR::callTestRunnerCallback(unsigned int, unsigned long, OpaqueJSValue const* const*) + 98 (TestRunner.cpp:675)
14  com.apple.WebKit              	0x0000000109c65a35 WebKit::InjectedBundleClient::didReceiveMessageToPage(WebKit::InjectedBundle&, WebKit::WebPage&, WTF::String const&, API::Object*) + 179
15  com.apple.WebKit              	0x0000000109d697f8 WebKit::WebPage::postInjectedBundleMessage(WTF::String const&, WebKit::UserData const&) + 78
16  com.apple.WebKit              	0x0000000109d84fdd WebKit::WebPage::didReceiveWebPageMessage(IPC::Connection&, IPC::Decoder&) + 4793

[Kimmo Kinnunen]

Created attachment 431055 [details]
Patch

[Kimmo Kinnunen]

> Do we construct / destroy a lot of DrawingContext objects? If so, we could consider putting this custom destructor in  #if ASSERT_ENABLED

No, they're very few (one per ImageBuffer).

[Kimmo Kinnunen]

This should be ready for review

[Kimmo Kinnunen]

Created attachment 431087 [details]
Workaround

[Kimmo Kinnunen]

Unclear why the media/modern-media-controls/overflow-support/chapters.html would fail on Mac-AS with the patch.
Maybe it's better to merge the workaround while I debug the test.

[Wenson Hsieh]

(In reply to Kimmo Kinnunen from comment #9)
> Unclear why the media/modern-media-controls/overflow-support/chapters.html
> would fail on Mac-AS with the patch.
> Maybe it's better to merge the workaround while I debug the test.

I believe this test failure is being tracked here: https://bugs.webkit.org/show_bug.cgi?id=226828

[Kimmo Kinnunen]

Created attachment 431096 [details]
Patch

[Kimmo Kinnunen]

Created attachment 431099 [details]
Patch for landing

[Said Abou-Hallawa]

Comment on attachment 431099 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=431099&action=review

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:1225
> +            base.buffer()->flushDrawingContext();
>              m_context->paintCompositedResultsToCanvas(*base.buffer());

I think another approach might be the following:

1. Since GraphicsContextGLOpenGL::paintToCanvas() is static function, it can be a function of GraphicContext named something like GraphicContext::drawPixelBuffer(PixelBuffer&, ...). I think the difference between this function and ImageBuffer::putPixelBuffer() is this function can scale the PixelBuffer since it puts the pixels into a NativeImage which can be scaled when drawing.
2. Add a DisplayList item DrawPixelBuffer which should be similar to PutPixelBuffer
3. Let creating the NativeImage from the PixelBuffer happen in GPUP so no NativeImage caching will be needed.

[Kimmo Kinnunen]

Comment on attachment 431099 [details]
Patch for landing

The Mac-AS retry seemed to time out with substantial amount of failed and flakey tests. The failures before the test run timed out all seem unrelated. Taking the liberty to cq+

[EWS]

Committed r278756 (238717@main): <https://commits.webkit.org/238717@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 431099 [details].