Bug 82445

Summary: Leak in WebCore::SVGFontElement::create
Product: WebKit Reporter: Kenichi Ishibashi <bashi>
Component: SVGAssignee: Kenichi Ishibashi <bashi>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: jschuh, macpherson, menard, thorton, webkit.review.bot, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: http://crbug.com/120453
Attachments:
Description Flags
Patch none

Description Kenichi Ishibashi 2012-03-28 03:39:41 PDT
See the stacktrace at http://crbug.com/120453.

r76990 introduced RefPtrs to hold SVGFontElement and looks like it is the cause of the leak. I checked removing RefPtrs fixed the leak and svg/custom/use-multiple-on-nested-disallowed-font.html passed successfully (DRT didn't crash). I think zimmermann's recent changes (e.g. r109333) disallow invalid svg contents like the test and the test is no longer effective.

jschuh@, zimmermann@, Do you think it is safe to remove RefPtrs which were introduces by r76990?
Comment 1 Kenichi Ishibashi 2012-03-28 03:42:16 PDT
Created attachment 134254 [details]
Patch
Comment 2 Nikolas Zimmermann 2012-03-28 06:06:49 PDT
Comment on attachment 134254 [details]
Patch

I think they were added on purpose - don't you see any crashes now?? Maybe something has changed, but I'd be careful here.
Can you try running tests under guard malloc? nrwt --tolerance 0 -p svg -g, and/or with --gc-between-tests.
Comment 3 Kenichi Ishibashi 2012-03-28 17:23:39 PDT
(In reply to comment #2)
> (From update of attachment 134254 [details])
> I think they were added on purpose - don't you see any crashes now?? Maybe something has changed, but I'd be careful here.

I didn't see any crashes with this patch, but I agree that we should be careful. I'd like to hear jschuh's opinion.

> Can you try running tests under guard malloc? nrwt --tolerance 0 -p svg -g, and/or with --gc-between-tests.

All svg tests ran as expected with above switches (Debug/Release).
Comment 4 Tim Horton 2012-04-04 13:35:38 PDT
(In reply to comment #0)
> See the stacktrace at http://crbug.com/120453.

I don't see one.
Comment 5 Justin Schuh 2012-04-04 17:15:56 PDT
No, it's not safe to remove the RefPtr without making other changes. This is discussed in bug 66438, along with an explanation of the solution to the memory leak. I just haven't had a chance to do any WebKit work in the last few months.

*** This bug has been marked as a duplicate of bug 66438 ***
Comment 6 Kenichi Ishibashi 2012-04-04 17:39:49 PDT
Comment on attachment 134254 [details]
Patch

Thanks. Removing r?.