Bug 286674 – [GTK][WPE] Crash loading https://webassembly.sh/ in CPU rendering mode

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Miguel Gomez]

When loading the page there's a crash that happens because the page uses an accelerated canvas that's being rendered through the DisplayList. This records the accelerated ImageBuffer into the DisplayList but when trying to replay the it, the crash happens because the renderer threads don't have a GL context to handle the accelerated ImageBuffer.

The problem here is that accelerated ImageBuffers should not be renderer through DisplayLists.

In the page, there's a canvas that's rendered to the display before the script rendered anything into it. Due to this, the canvas hasn't created its internal ImageBuffer. When rendering the canvas, we ask to it whether it's accelerated or not, which basically checks whether the internal ImageBuffer is accelerated. But there's no ImageBuffer created at that point, so it's detected as not accelerated, causing it to be rendered through HTMLCanvasElement::paint(), which is the DisplayList path. Inside that, we call CanvasRenderingContext::surfaceBufferToImageBuffer() to get the canvas content, which triggers the creation of the canvas ImageBuffer, which is accelerated due to its size. And then we have an accelerated canvas being painted through DisplayLists.

In order to fix this, we need to ensure that when we check whether the canvas is accelerated or not, we really know it and not just return false if the ImageBuffer hasn't been created.

[Miguel Gomez]

Pull request: https://github.com/WebKit/WebKit/pull/39683

[Miguel Gomez]

I tried to follow an approach based on forcing the creation of the canvas ImageBuffer when CanvasRenderingContext::delegatesDisplay() is called, which is how we detect whether the canvas is not accelerated and needs to be rendered through DisplayLists (delegatesDisplay is false)  or whether the canvas is accelerated and needs to be rendered to its own layer (delegatesDisplay is true). This fixes the problem, but there's the layout test inspector/canvas/memory.html that checks the memory used by the canvas that fails because of this approach because of this that's commented in the test:

// NOTE: the memory cost of a canvas will not be retrievable until an operation is
// performed on that canvas that requires the image buffer.  A blank canvas that was
// just created will not have a buffer, so its memory cost will be 0/NaN.

My approach breaks this because the ImageBuffer will be created with the default value 150x300, causing that the initial memory usage is not 0/NaN as expected. I need to change the approach to detect whether the canvas is accelerated or not without creating the ImageBuffer.

[Miguel Gomez]

After some back and forth with some failing tests, I found that the canvases that have not created their ImageBuffer are meant to be handled as not accelerated. When rendered, their content will be painted through HTMLCanvasElement::paint(), and there there's the condition m_context->isSurfaceBufferTransparentBlack() that checks whether the canvas has created the ImageBuffer or not in order to access it (the surface being transparent black means that the ImageBuffer was not created yet).

If the canvas doesn't have an ImageBuffer it's painted as a transparent black rectangle. When using a concrete composite mode, we don't even need to paint that cause it won't be visible. The problem happens when we do need to paint the transparent rectangle, as it relies on calling m_context->surfaceBufferToImageBuffer() to get the ImageBuffer from the canvas, which triggers the creation of the ImageBuffer. This creation can produce an accelerated ImageBuffer that will change the canvas from non accelerated to accelerated, but this happens in the middle of a paint that should not be happening for an accelerated canvas, and hence the crash. The solution here would be painting the transparent rectangle with fillRect instead of requesting the ImageBuffer. That would not force the creation of the ImageBuffer and would avoid the problem.

[EWS]

Committed 289775@main (9b2ca446ce97): <https://commits.webkit.org/289775@main>

Reviewed commits have been landed. Closing PR #39683 and removing active labels.