Bug 55362

Summary:

Clarify comment about potential memory leak in SVGImage

Product:

WebKit

Reporter:

Cosmin Truta <ctruta>

Component:

SVG

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Minor

CC:

commit-queue, longsonr, zimmermann

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

Cosmin Truta

Reported 2011-02-28 04:43:16 PST

There is the following comment at // FIXME: If this SVG ends up loading itself, we might leak the world. // The comment said that the Cache code does not know about CachedImages // holding Frames and won't know to break the cycle. But [code follows] I tested SVG images with various kinds of content, running testshell under valgrind, but I could not see any leak. I believe that comment is no longer applicable. (It was written in 2009.)

Attachments
Patch (3.93 KB, patch) 2011-02-28 04:54 PST, Cosmin Truta	no flags	Details Formatted Diff Diff
Patch (4.21 KB, patch) 2011-03-03 14:17 PST, Cosmin Truta	no flags	Details Formatted Diff Diff
Patch (1.58 KB, patch) 2011-03-03 15:00 PST, Cosmin Truta	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Cosmin Truta

Comment 1 2011-02-28 04:54:54 PST

Created attachment 84040 [details] Patch

Robert Longson

Comment 2 2011-02-28 10:20:15 PST

bug 15443 suggests that images loading images has not been implemented yet. If so the comment could be true when it is implemented couldn't it?

Cosmin Truta

Comment 3 2011-02-28 19:07:20 PST

(In reply to comment #2) > bug 15443 suggests that images loading images has not been implemented yet. If so the comment could be true when it is implemented couldn't it? SVG images loading other SVG images, via the <image> tag, is supported, albeit in a limited manner. You can see in the layout test that I submitted that it works. The layout test is, in fact, meant to be run under valgrind, to show that it does not leak memory.

Cosmin Truta

Comment 4 2011-02-28 21:10:08 PST

I wonder what happened, the bubbles have been purple for the whole day.

Cosmin Truta

Comment 5 2011-03-03 14:17:09 PST

Created attachment 84626 [details] Patch Forgot to use option --binary in git-diff last time. That explains why the previous patch failed to apply.

Cosmin Truta

Comment 6 2011-03-03 14:53:04 PST

Instead of removing that comment altogher, I will submit a patch explaining why it cannot currently happen, and when (i.e. at what event in the future) it will happen.

Cosmin Truta

Comment 7 2011-03-03 15:00:58 PST

Created attachment 84636 [details] Patch

WebKit Commit Bot

Comment 8 2011-03-04 13:28:25 PST

Comment on attachment 84636 [details] Patch Clearing flags on attachment: 84636 Committed r80371: <http://trac.webkit.org/changeset/80371>

WebKit Commit Bot

Comment 9 2011-03-04 13:28:30 PST

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.