Bug 229790

Summary: Add a layout test to detect memory leaks when drawing images to a detached canvas
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: CanvasAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, dino, mmaxfield, simon.fraser, thorton, tsavell, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=229668
Attachments:
Description Flags
Patch
none
Patch
darin: review+
Patch none

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.