Bug 229790 - Add a layout test to detect memory leaks when drawing images to a detached canvas
Summary: Add a layout test to detect memory leaks when drawing images to a detached ca...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-09-01 19:11 PDT by Said Abou-Hallawa
Modified: 2021-09-10 11:24 PDT (History)
8 users (show)

See Also:


Attachments
Patch (13.92 KB, patch)
2021-09-01 19:23 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (13.44 KB, patch)
2021-09-01 21:37 PDT, Said Abou-Hallawa
darin: review+
Details | Formatted Diff | Diff
Patch (12.42 KB, patch)
2021-09-02 16:05 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2021-09-01 19:11:07 PDT
This is a follow-up for bug 229668.
Comment 1 Said Abou-Hallawa 2021-09-01 19:23:10 PDT
Created attachment 437106 [details]
Patch
Comment 2 Said Abou-Hallawa 2021-09-01 21:37:42 PDT
Created attachment 437112 [details]
Patch
Comment 3 Said Abou-Hallawa 2021-09-02 10:20:04 PDT
Without r281839 the test in this patch fails with the following diff:

- PASS internals.remoteImagesCount() is 0
+ FAIL internals.remoteImagesCount() should be 0. Was 3.
Comment 4 Darin Adler 2021-09-02 13:05:12 PDT
Comment on attachment 437112 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Add a layout test to detect memory leaks when darwing images to a detached canvas

darwing

> Source/WebKit/ChangeLog:3
> +        Add a layout test to detect memory leaks when darwing images to a detached canvas

darwing

> Source/WebCore/page/Page.h:584
> +    WEBCORE_EXPORT unsigned remoteImagesCount() const;

Given this is for testing only, could we have internals go directly to the chrome client instead of putting a helper in Page?
Comment 5 Simon Fraser (smfr) 2021-09-02 13:13:47 PDT
> > Source/WebCore/page/Page.h:584
> > +    WEBCORE_EXPORT unsigned remoteImagesCount() const;
> 
> Given this is for testing only, could we have internals go directly to the
> chrome client instead of putting a helper in Page?

Let's rename it to remoteImagesCountForTesting() also.
Comment 6 Said Abou-Hallawa 2021-09-02 16:05:27 PDT
Created attachment 437209 [details]
Patch
Comment 7 EWS 2021-09-03 01:25:09 PDT
Committed r281980 (241287@main): <https://commits.webkit.org/241287@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 437209 [details].
Comment 8 Radar WebKit Bug Importer 2021-09-03 01:26:17 PDT
<rdar://problem/82713895>
Comment 9 Truitt Savell 2021-09-10 11:16:14 PDT
It looks like the test added here is failing fast/canvas/canvas-drawImage-detached-leak.html

History:
https://results.webkit.org/?suite=layout-tests&test=fast%2Fcanvas%2Fcanvas-drawImage-detached-leak.html


Diff:
--- /Volumes/Data/worker/bigsur-debug-tests-wk2/build/layout-test-results/fast/canvas/canvas-drawImage-detached-leak-expected.txt
+++ /Volumes/Data/worker/bigsur-debug-tests-wk2/build/layout-test-results/fast/canvas/canvas-drawImage-detached-leak-actual.txt
@@ -3,7 +3,7 @@
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-PASS internals.remoteImagesCountForTesting() is 0
+FAIL internals.remoteImagesCountForTesting() should be 0. Was 3.
 PASS successfullyParsed is true
 
 TEST COMPLETE
Comment 10 Darin Adler 2021-09-10 11:24:55 PDT
That’s exactly what we’d expect if the fix in r281839 did not work.