Bug 77817

Summary: CachedResourceLoader is destroyed before CSSFontSelector is destroyed
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: Page LoadingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, bashi, beidson, darin, eric, hyatt, japhet, kling, koivisto, mitz, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Fixes the bug none

Ryosuke Niwa
Reported 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.
Attachments
Fixes the bug (1.66 KB, patch)
2012-02-04 18:01 PST, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Ryosuke Niwa
Comment 1 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
Ryosuke Niwa
Comment 2 2012-02-04 18:01:39 PST
Created attachment 125509 [details] Fixes the bug
Ryosuke Niwa
Comment 3 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?
Ryosuke Niwa
Comment 5 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.
Ryosuke Niwa
Comment 6 2012-02-04 18:49:07 PST
Actually, this isn't a use-after-free since CSSFontSelector accesses CachedResourceLoader through Document.
Ryosuke Niwa
Comment 7 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>
Adam Barth
Comment 8 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?
Adam Barth
Comment 9 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.
Lucas Forschler
Comment 10 2012-02-06 14:44:12 PST
<rdar://problem/10815525>
Ryosuke Niwa
Comment 11 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.
Adam Barth
Comment 12 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.
Ryosuke Niwa
Comment 13 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.
WebKit Review Bot
Comment 14 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>
WebKit Review Bot
Comment 15 2012-02-09 19:34:43 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.