RESOLVED FIXED 87792
SVGImageCache leaks image data 
https://bugs.webkit.org/show_bug.cgi?id=87792
Summary SVGImageCache leaks image data
Xianzhu Wang
Reported 2012-05-29 16:34:09 PDT
http://petelepage.com/scratch/breel2/ Old image buffers in SVGImageCache are not deleted when a renderer is destroyed.
Attachments
patch (1.89 KB, patch)
2012-05-29 17:04 PDT, Xianzhu Wang 		no flags
DetailsFormatted DiffDiff
The more complete patch (12.16 KB, patch)
2012-05-29 17:39 PDT, Xianzhu Wang 		buildbot: commit-queue-
DetailsFormatted DiffDiff
patch v2 (11.93 KB, patch)
2012-05-30 14:29 PDT, Xianzhu Wang 		zimmermann: review-
DetailsFormatted DiffDiff
patch (with test) (20.67 KB, patch)
2012-06-01 10:04 PDT, Xianzhu Wang 		no flags
DetailsFormatted DiffDiff
patch rebased (20.66 KB, patch)
2012-06-11 11:23 PDT, Xianzhu Wang 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Xianzhu Wang
Comment 1 2012-05-29 17:04:22 PDT
Created attachment 144642 [details] patch
Xianzhu Wang
Comment 2 2012-05-29 17:11:51 PDT
I'm also thinking of a more complete fix to avoid accidental usage of CachedImage::removeClient() (which will leak SVGImageCache): 1) Make CachedResource::addClient() and CachedResource::removeClient() virtual 2) Change the type of the key of the maps in SVGImageCache from RenderObject* to CachedResourceClient*.
Xianzhu Wang
Comment 3 2012-05-29 17:39:09 PDT
Created attachment 144662 [details] The more complete patch
Build Bot
Comment 4 2012-05-29 18:20:05 PDT
Comment on attachment 144662 [details] The more complete patch Attachment 144662 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12843661
Xianzhu Wang
Comment 5 2012-05-30 14:29:43 PDT
Created attachment 144923 [details] patch v2
Nikolas Zimmermann
Comment 6 2012-05-31 03:25:04 PDT
Comment on attachment 144923 [details] patch v2 Patch looks good to me on first sight, you still need to add a testcase though. r- for the lack of a testcase. We have leak bots at build.webkit.org/waterfall that would catch if this ever leaks again.
Xianzhu Wang
Comment 7 2012-05-31 10:15:07 PDT
(In reply to comment #6) > (From update of attachment 144923 [details]) > Patch looks good to me on first sight, you still need to add a testcase though. r- for the lack of a testcase. We have leak bots at build.webkit.org/waterfall that would catch if this ever leaks again. Thanks. However, it'll need some time to write the testcase because of my limited bandwidth. Wondering which of the following choices would look good to you: 1. Wait for my test to be added 2. File another bug for adding the test after landing this patch
Xianzhu Wang
Comment 8 2012-06-01 10:04:28 PDT
Created attachment 145334 [details] patch (with test)
Nikolas Zimmermann
Comment 9 2012-06-02 03:35:08 PDT
Comment on attachment 145334 [details] patch (with test) Looks great, r=me.
WebKit Review Bot
Comment 10 2012-06-02 22:06:27 PDT
Comment on attachment 145334 [details] patch (with test) Clearing flags on attachment: 145334 Committed r119353: <http://trac.webkit.org/changeset/119353>
WebKit Review Bot
Comment 11 2012-06-02 22:06:50 PDT
All reviewed patches have been landed. Closing bug.
Abhishek Arya
Comment 12 2012-06-09 22:55:29 PDT
Sorry, but this patch was rolled out due to dependency on http://trac.webkit.org/changeset/118618. r118618 caused a whole bunch of security crashes due to reentering the loader during tear-down of render tree objects. Xianzhu, let me ping you explaining this in more detail or if you see me (aarya@google) online, please feel free to ping me.
Xianzhu Wang
Comment 13 2012-06-11 11:23:17 PDT
Created attachment 146880 [details] patch rebased Confirmed with Abhishek that this patch is good but depends on another problematic patch which was rolled-out. Rebased the patch and re-apply.
WebKit Review Bot
Comment 14 2012-06-11 13:16:03 PDT
Comment on attachment 146880 [details] patch rebased Clearing flags on attachment: 146880 Committed r120000: <http://trac.webkit.org/changeset/120000>
WebKit Review Bot
Comment 15 2012-06-11 13:16:09 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.