Bug 196059 - Leak of SVGFontFaceElement when RenderStyle holds onto a FontRanges which uses it
Summary: Leak of SVGFontFaceElement when RenderStyle holds onto a FontRanges which use...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
: 189460 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-03-20 23:14 PDT by Ryosuke Niwa
Modified: 2019-03-26 04:57 PDT (History)
8 users (show)

See Also:


Attachments
WIP (missing a test) (4.38 KB, patch)
2019-03-20 23:39 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
test (WIP) (2.28 KB, text/html)
2019-03-20 23:40 PDT, Ryosuke Niwa
no flags Details
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (2.65 MB, application/zip)
2019-03-21 01:40 PDT, Build Bot
no flags Details
font-leak-1.svg (1.20 KB, image/svg+xml)
2019-03-21 21:48 PDT, Ryosuke Niwa
no flags Details
font-leak-2.html (295 bytes, text/html)
2019-03-21 21:48 PDT, Ryosuke Niwa
no flags Details
Fixes the bug (4.79 KB, patch)
2019-03-21 21:51 PDT, Ryosuke Niwa
zalan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2019-03-20 23:14:05 PDT
SVGFontFaceElement keeps its RenderStyle alive via ElementRareData
but RenderStyle can hold onto FontRanges and therefore CSSFontSource,
which in turn keeps SVGFontFaceElement alive, making a reference cycle.

Complete cycle:
SVGFontFaceElement (1) -> ElementRareData -> StyleInheritedData -> FontCascade -> FontCascadeFonts (2)

FontCascadeFonts (2) -> FontRanges (3)
FontCascadeFonts (2) -> CSSFontSelector -> CSSFontFaceSet -> CSSSegmentedFontFace -> FontRanges (3)

FontRanges (3) -> CSSFontAccessor > CSSFontFace > CSSFontSource -> SVGFontFaceElement (1)

<rdar://problem/47562909>
Comment 1 Ryosuke Niwa 2019-03-20 23:39:12 PDT
Created attachment 365497 [details]
WIP (missing a test)

The patch works but still working on a test.
Comment 2 Ryosuke Niwa 2019-03-20 23:40:04 PDT
Created attachment 365498 [details]
test (WIP)

Somehow this always passes... I suspect RenderStyle isn't storing things as expected. Gonna look into it tomorrow.
Comment 3 Build Bot 2019-03-20 23:42:09 PDT
Attachment 365497 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:17:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Build Bot 2019-03-21 01:40:43 PDT
Comment on attachment 365497 [details]
WIP (missing a test)

Attachment 365497 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11595109

New failing tests:
http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html
Comment 5 Build Bot 2019-03-21 01:40:45 PDT
Created attachment 365519 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 6 Ryosuke Niwa 2019-03-21 21:41:36 PDT
Alright, writing a test here is impossible. We need to trigger select-all in a SVG document followed by a top-level navigation, etc... to trigger a leak.
Comment 7 Ryosuke Niwa 2019-03-21 21:48:16 PDT
Created attachment 365676 [details]
font-leak-1.svg
Comment 8 Ryosuke Niwa 2019-03-21 21:48:32 PDT
Created attachment 365677 [details]
font-leak-2.html
Comment 9 Ryosuke Niwa 2019-03-21 21:49:47 PDT
./Tools/Scripts/run-webkit-tests --debug svg/dom/font-leak-1.svg svg/dom/font-leak-2.html --leaks --no-retry --no-show-results --child-processes=1 would reproduce the issue. Unfortunately, navigating via location.href seems to work once but then managing the state between navigations & detecting the actual leak within a single test is basically impossible.
Comment 10 Ryosuke Niwa 2019-03-21 21:51:06 PDT
Created attachment 365678 [details]
Fixes the bug
Comment 11 Ryosuke Niwa 2019-03-21 21:58:25 PDT
*** Bug 189460 has been marked as a duplicate of this bug. ***
Comment 12 Simon Fraser (smfr) 2019-03-25 14:18:59 PDT
Comment on attachment 365678 [details]
Fixes the bug

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

> Source/WebCore/ChangeLog:18
> +        No new tests. Unfortunately, writing a test proved to be intractable. The leak can be reproduced by running
> +        svg/text/text-text-05-t.svg then svg/zoom/page/zoom-img-preserveAspectRatio-support-1.html consecutively.

Does running with --world-leaks reveal the leak (and prove the fix)?
Comment 13 Ryosuke Niwa 2019-03-25 15:20:06 PDT
(In reply to Simon Fraser (smfr) from comment #12)
> Comment on attachment 365678 [details]
> Fixes the bug
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=365678&action=review
> 
> > Source/WebCore/ChangeLog:18
> > +        No new tests. Unfortunately, writing a test proved to be intractable. The leak can be reproduced by running
> > +        svg/text/text-text-05-t.svg then svg/zoom/page/zoom-img-preserveAspectRatio-support-1.html consecutively.
> 
> Does running with --world-leaks reveal the leak (and prove the fix)?

I didn't try --world-leaks but --leaks definitely detects this leak and the fix works.
Comment 14 Ryosuke Niwa 2019-03-25 19:59:32 PDT
Confirmed that I can detect a leak with
./Tools/Scripts/run-webkit-tests --debug --world-leaks svg/text/text-text-05-t.svg
and the leak is gone with my patch!
Comment 15 Ryosuke Niwa 2019-03-25 20:34:45 PDT
Committed r243483: <https://trac.webkit.org/changeset/243483>