Bug 229668 - [GPU Process](REGRESSION): A detached canvas leaks all the images it draws
Summary: [GPU Process](REGRESSION): A detached canvas leaks all the images it draws
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: Safari Technology Preview
Hardware: iPhone / iPad Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-08-30 07:07 PDT by Maximilian Böhm
Modified: 2021-09-01 19:11 PDT (History)
13 users (show)

See Also:


Attachments
Memory usage (218.22 KB, image/png)
2021-08-30 07:07 PDT, Maximilian Böhm
no flags Details
Patch (4.32 KB, patch)
2021-08-31 11:05 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (2.08 KB, patch)
2021-08-31 13:19 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (3.46 KB, patch)
2021-08-31 14:11 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
simpler repro test case (1.45 KB, text/html)
2021-09-01 00:25 PDT, Said Abou-Hallawa
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Maximilian Böhm 2021-08-30 07:07:55 PDT
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
Comment 1 Maximilian Böhm 2021-08-30 07:15:30 PDT
GPU Process: Canvas Rendering has to be enabled.

If it is disabled, the error does not occur.
Comment 2 Radar WebKit Bug Importer 2021-08-30 09:45:35 PDT
<rdar://problem/82532484>
Comment 3 Said Abou-Hallawa 2021-08-31 00:37:29 PDT
This bug is similar to bug 226813.
Comment 4 Said Abou-Hallawa 2021-08-31 01:23:25 PDT
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.
Comment 5 Said Abou-Hallawa 2021-08-31 01:43:31 PDT
This if-statement was added in r262498.
Comment 6 Said Abou-Hallawa 2021-08-31 11:05:55 PDT
Created attachment 436916 [details]
Patch
Comment 7 Said Abou-Hallawa 2021-08-31 13:19:33 PDT
Created attachment 436932 [details]
Patch
Comment 8 Simon Fraser (smfr) 2021-08-31 13:33:35 PDT
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.
Comment 9 Said Abou-Hallawa 2021-08-31 14:11:01 PDT
Created attachment 436941 [details]
Patch
Comment 10 Simon Fraser (smfr) 2021-08-31 15:58:46 PDT
Comment on attachment 436941 [details]
Patch

Can we write a test that detect the memory growth?
Comment 11 Said Abou-Hallawa 2021-09-01 00:24:30 PDT
(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.
Comment 12 Said Abou-Hallawa 2021-09-01 00:25:26 PDT
Created attachment 436998 [details]
simpler repro test case
Comment 13 EWS 2021-09-01 00:55:46 PDT
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].