Bug 229668 – [GPU Process](REGRESSION): A detached canvas leaks all the images it draws

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

[Maximilian Böhm]

Created attachment 436766 [details]
Memory usage

Basically we are having a video (getUserMedia) and mirror it into a canvas. As long as the canvas is not appended to the DOM, we have a memory leak.

After 40 seconds Safari reloads the page...

Funny, we are having 18446744073,98 GB of RAM! (See screenshot in attachment)


Reproduction: https://jsfiddle.net/mkn2tb3h/
Device: iPad 8
iOS: 15.0 (19A5337a)

Kind regards,
Maximilian Böhm

[Maximilian Böhm]

GPU Process: Canvas Rendering has to be enabled.

If it is disabled, the error does not occur.

[Radar WebKit Bug Importer]

<rdar://problem/82532484>

[Said Abou-Hallawa]

This bug is similar to bug 226813.

[Said Abou-Hallawa]

The reason of this bug is the if-statement in this loop in Document::prepareCanvasesForDisplayIfNeeded():

    for (auto& canvas : canvases) {
        // However, if they are not in the document body, then they won't
        // be composited and thus don't need preparation. Unfortunately they
        // can't tell at the time they were added to the list, since they
        // could be inserted or removed from the document body afterwards.
        if (!canvas->isInTreeScope())
            continue;
        canvas->prepareForDisplay();
    }

So if the canvas was not added to the DOM, prepareForDisplay() will not be called. This causes the images of the video to be accumulated in the WebProcess side and hence they are never released in the GPUProcess as well.

[Said Abou-Hallawa]

This if-statement was added in r262498.

[Said Abou-Hallawa]

Created attachment 436916 [details]
Patch

[Said Abou-Hallawa]

Created attachment 436932 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 436932 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:8942
> +        if (canvas->isInTreeScope() || (canvas->renderingContext() && canvas->renderingContext()->is2d()))
> +            canvas->prepareForDisplay();

We instead always call preprareForDisplay() but inside the implementation of that function do the isInTreeScope() check when appropriate.

[Said Abou-Hallawa]

Created attachment 436941 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 436941 [details]
Patch

Can we write a test that detect the memory growth?

[Said Abou-Hallawa]

(In reply to Simon Fraser (smfr) from comment #10)
> Comment on attachment 436941 [details]
> Patch
> 
> Can we write a test that detect the memory growth?

There is no easy way to check the WebProcess memory status before and after drawing images to the canvas. There should be an Internals API to facilitate this check.

I think this can be done in a separate patch. I am going to attach a simpler repro test case which can be added as a layout test once the Internals API is there.

[Said Abou-Hallawa]

Created attachment 436998 [details]
simpler repro test case

[EWS]

Committed r281839 (241172@main): <https://commits.webkit.org/241172@main>

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