77817 2012-02-04 16:26:10 -0800 CachedResourceLoader is destroyed before CSSFontSelector is destroyed 2012-02-22 10:05:58 -0800 1 1 1 Unclassified WebKit Page Loading 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 rniwa webkit-unassigned abarth bashi beidson darin eric hyatt japhet kling koivisto mitz webkit.review.bot oldest_to_newest 549491 0 rniwa 2012-02-04 16:26:10 -0800 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. 549493 1 rniwa 2012-02-04 16:28:27 -0800 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 549507 2 125509 rniwa 2012-02-04 18:01:39 -0800 Created attachment 125509 Fixes the bug 549508 3 rniwa 2012-02-04 18:04:10 -0800 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? 549509 4 rniwa 2012-02-04 18:25:05 -0800 http://code.google.com/p/chromium/issues/detail?id=112731 549510 5 125509 rniwa 2012-02-04 18:25:44 -0800 Comment on attachment 125509 Fixes the bug Apparently our fuzz cluster had been finding crashes due to this bug. I'll add a test case using that. 549514 6 rniwa 2012-02-04 18:49:07 -0800 Actually, this isn't a use-after-free since CSSFontSelector accesses CachedResourceLoader through Document. 549530 7 125509 rniwa 2012-02-04 21:57:44 -0800 Comment on attachment 125509 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> 549553 8 125509 abarth 2012-02-05 00:34:41 -0800 Comment on attachment 125509 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? 549554 9 abarth 2012-02-05 00:36:20 -0800 Ah, I didn't see the earlier discussion. Nate might have some ideas about how to write a reliable test. 550303 10 lforschler 2012-02-06 14:44:12 -0800 <rdar://problem/10815525> 552583 11 rniwa 2012-02-08 16:07:47 -0800 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. 553376 12 125509 abarth 2012-02-09 11:51:01 -0800 Comment on attachment 125509 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. 553378 13 rniwa 2012-02-09 11:52:52 -0800 (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. 553744 14 125509 webkit.review.bot 2012-02-09 19:34:37 -0800 Comment on attachment 125509 Fixes the bug Clearing flags on attachment: 125509 Committed r107346: <http://trac.webkit.org/changeset/107346> 553745 15 webkit.review.bot 2012-02-09 19:34:43 -0800 All reviewed patches have been landed. Closing bug. 125509 2012-02-04 18:01:39 -0800 2012-02-09 19:34:37 -0800 Fixes the bug bug-77817-20120204180137.patch text/plain 1698 rniwa SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDEwNjc1MikKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE4IEBACisyMDEyLTAyLTA0ICBSeW9zdWtl IE5pd2EgIDxybml3YUB3ZWJraXQub3JnPgorCisgICAgICAgIENhY2hlZFJlc291cmNlTG9hZGVy IGlzIGRlc3Ryb3llZCBiZWZvcmUgQ1NTRm9udFNlbGVjdG9yIGlzIGRlc3Ryb3llZAorICAgICAg ICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9Nzc4MTcKKworICAgICAg ICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBFeHBsaWNpdGx5IGNsZWFy IHN0eWxlIHNlbGVjdG9yIGJlZm9yZSBkZXN0b3J5aW5nIHRoZSBjYWNoZWQgcmVzb3VyY2UgbG9h ZGVyLgorCisgICAgICAgIE5vIG5ldyB0ZXN0cyBidXQgUGVyZm9ybWFuY2VUZXN0cy9QYXJzZXIv aHRtbDUtZnVsbC1yZW5kZXIuaHRtbCB3YXMgY3Jhc2hpbmcKKyAgICAgICAgb24gcGVyZm9ybWFu Y2UgYm90cyBkdWUgdG8gdGhpcyBidWcuCisKKyAgICAgICAgKiBkb20vRG9jdW1lbnQuY3BwOgor ICAgICAgICAoV2ViQ29yZTo6RG9jdW1lbnQ6On5Eb2N1bWVudCk6CisKIDIwMTItMDItMDQgIEFu ZGVycyBDYXJsc3NvbiAgPGFuZGVyc2NhQGFwcGxlLmNvbT4KIAogICAgICAgICBSZW1vdmUgZGVh ZCBjb2RlIGZyb20gU2Nyb2xsaW5nQ29vcmRpbmF0b3IKSW5kZXg6IFNvdXJjZS9XZWJDb3JlL2Rv bS9Eb2N1bWVudC5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUvZG9tL0RvY3VtZW50 LmNwcAkocmV2aXNpb24gMTA2NzQwKQorKysgU291cmNlL1dlYkNvcmUvZG9tL0RvY3VtZW50LmNw cAkod29ya2luZyBjb3B5KQpAQCAtNTQwLDcgKzU0MCw2IEBAIERvY3VtZW50Ojp+RG9jdW1lbnQo KQogICAgIEFTU0VSVCghbV9wYXJzZXIgfHwgbV9wYXJzZXItPnJlZkNvdW50KCkgPT0gMSk7CiAg ICAgZGV0YWNoUGFyc2VyKCk7CiAgICAgbV9kb2N1bWVudCA9IDA7Ci0gICAgbV9jYWNoZWRSZXNv dXJjZUxvYWRlci5jbGVhcigpOwogCiAgICAgbV9yZW5kZXJBcmVuYS5jbGVhcigpOwogCkBAIC01 NzMsNiArNTcyLDkgQEAgRG9jdW1lbnQ6On5Eb2N1bWVudCgpCiAgICAgaWYgKG1fbWVkaWFRdWVy eU1hdGNoZXIpCiAgICAgICAgIG1fbWVkaWFRdWVyeU1hdGNoZXItPmRvY3VtZW50RGVzdHJveWVk KCk7CiAKKyAgICBjbGVhclN0eWxlU2VsZWN0b3IoKTsgLy8gV2UgbmVlZCB0byBkZXN0b3J5IENT U0ZvbnRTZWxlY3RvciBiZWZvcmUgZGVzdHJveWluZyBtX2NhY2hlZFJlc291cmNlTG9hZGVyLgor ICAgIG1fY2FjaGVkUmVzb3VyY2VMb2FkZXIuY2xlYXIoKTsKKwogICAgIC8vIFdlIG11c3QgY2Fs bCBjbGVhclJhcmVEYXRhKCkgaGVyZSBzaW5jZSBhIERvY3VtZW50IGNsYXNzIGluaGVyaXRzIFRy ZWVTY29wZQogICAgIC8vIGFzIHdlbGwgYXMgTm9kZS4gU2VlIGEgY29tbWVudCBvbiBUcmVlU2Nv cGUuaCBmb3IgdGhlIHJlYXNvbi4KICAgICBpZiAoaGFzUmFyZURhdGEoKSkK