WebGL context image buffer accumulates the NativeImage contents when drawn to RemoteImageBufferProxy Causes leaks https://ciechanow.ski/internal-combustion-engine/
<rdar://77421966>
Created attachment 430975 [details] Patch
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
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
Created attachment 431055 [details] Patch
> 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).
This should be ready for review
Created attachment 431087 [details] Workaround
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.
(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
Created attachment 431096 [details] Patch
Created attachment 431099 [details] Patch for landing
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.
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+
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].