Bug 77817 - CachedResourceLoader is destroyed before CSSFontSelector is destroyed
Summary: CachedResourceLoader is destroyed before CSSFontSelector is destroyed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-02-04 16:26 PST by Ryosuke Niwa
Modified: 2012-02-22 10:05 PST (History)
11 users (show)

See Also:


Attachments
Fixes the bug (1.66 KB, patch)
2012-02-04 18:01 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2012-02-04 16:26:10 PST
In Document::~Document, we're explicitly calling m_cachedResourceLoader.clear(). However, CSSFontSelector, which is owned by CSSStyleSelector, calls CachedResourceLoader::decrementRequestCount at the end of the destructor, accessing the freed-memory.
Comment 1 Ryosuke Niwa 2012-02-04 16:28:27 PST
The following stack trace can be obtained on PerformanceTests/Parser/html5-full-renderer.html on debug builds of Mac port by setting a break point inside CachedResourceLoader::decrementRequestCount after hitting an assertion in CachedResourceLoader::~CachedResourceLoader.

#0	0x1014e69aa in WebCore::CachedResourceLoader::decrementRequestCount at CachedResourceLoader.cpp:714
#1	0x1015d09b6 in WebCore::CSSFontSelector::clearDocument at CSSFontSelector.cpp:586
#2	0x1016bf3f0 in WebCore::CSSStyleSelector::~CSSStyleSelector at CSSStyleSelector.cpp:572
#3	0x1017a4cc7 in WTF::deleteOwnedPtr<WebCore::CSSStyleSelector> at OwnPtrCommon.h:54
#4	0x1017a4e26 in WTF::OwnPtr<WebCore::CSSStyleSelector>::~OwnPtr at OwnPtr.h:55
#5	0x10178292d in WebCore::Document::~Document at Document.cpp:580
#6	0x101bfc883 in WebCore::HTMLDocument::~HTMLDocument at HTMLDocument.cpp:91
#7	0x1017906c6 in WebCore::Document::guardDeref at Document.h:253
#8	0x10177ac28 in WebCore::Document::removedLastRef at Document.cpp:626
#9	0x1015b94dd in WebCore::TreeShared<WebCore::ContainerNode>::deref at TreeShared.h:79
Comment 2 Ryosuke Niwa 2012-02-04 18:01:39 PST
Created attachment 125509 [details]
Fixes the bug
Comment 3 Ryosuke Niwa 2012-02-04 18:04:10 PST
It appears that making a reduction from html5-full-parser.html is pretty hard :( Maybe we can add it to LayoutTests as well as a crash-test?
Comment 5 Ryosuke Niwa 2012-02-04 18:25:44 PST
Comment on attachment 125509 [details]
Fixes the bug

Apparently our fuzz cluster had been finding crashes due to this bug. I'll add a test case using that.
Comment 6 Ryosuke Niwa 2012-02-04 18:49:07 PST
Actually, this isn't a use-after-free since CSSFontSelector accesses CachedResourceLoader through Document.
Comment 7 Ryosuke Niwa 2012-02-04 21:57:44 PST
Comment on attachment 125509 [details]
Fixes the bug

I'm sorry but turning this file into a DRT test appears to be really hard :( There appears to be some race condition in triggering this bug and it doesn't reproduce reliably in DRT. Also calling notifyDone or document.write or assigning some value to innerHTML/innerText also seem to make the test case not work :( I also tried embedding it inside an iframe and reload it several times but that doesn't seem to work either.

<script>
function check () {
 var mylink = document.getElementsByTagName('a');
 location =  mylink[0];
}
</script><style>@font-face { font-family: "A"; src: url(); }* { font-family: A;</style>
<body onload="check()"<linearGradient>A00A00A000AA0AA00AA0<a>
Comment 8 Adam Barth 2012-02-05 00:34:41 PST
Comment on attachment 125509 [details]
Fixes the bug

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

> Source/WebCore/ChangeLog:11
> +        No new tests but PerformanceTests/Parser/html5-full-render.html was crashing
> +        on performance bots due to this bug.

Can we write an explicit LayoutTest for this crash?
Comment 9 Adam Barth 2012-02-05 00:36:20 PST
Ah, I didn't see the earlier discussion.  Nate might have some ideas about how to write a reliable test.
Comment 10 Lucas Forschler 2012-02-06 14:44:12 PST
<rdar://problem/10815525>
Comment 11 Ryosuke Niwa 2012-02-08 16:07:47 PST
japhet told me that he had tried to make a reduction but was unable to. At this point, I'd like to ask reviewers to have a look at my patch again. The fix is self-evidently correct as far as I'm concerned.
Comment 12 Adam Barth 2012-02-09 11:51:01 PST
Comment on attachment 125509 [details]
Fixes the bug

Sigh.  I'm worried that shuffling around these shutdown events will cause other crashes.  Without adding tests, we don't know that we're continuously improving.  We could just be cycling through problems.
Comment 13 Ryosuke Niwa 2012-02-09 11:52:52 PST
(In reply to comment #12)
> (From update of attachment 125509 [details])
> Sigh.  I'm worried that shuffling around these shutdown events will cause other crashes.  Without adding tests, we don't know that we're continuously improving.  We could just be cycling through problems.

I know. Hopefully the comment there makes it clear that we need to destroy clearStyleSelector first. In the long term, we really need to come up with a better mechanism to test the loader code.
Comment 14 WebKit Review Bot 2012-02-09 19:34:37 PST
Comment on attachment 125509 [details]
Fixes the bug

Clearing flags on attachment: 125509

Committed r107346: <http://trac.webkit.org/changeset/107346>
Comment 15 WebKit Review Bot 2012-02-09 19:34:43 PST
All reviewed patches have been landed.  Closing bug.