21329 2008-10-03 01:05:22 -0700 REGRESSION: crash in ScriptElement::notifyFinished 2008-10-06 06:08:01 -0700 1 1 1 Unclassified WebKit DOM 528+ (Nightly build) PC Windows XP RESOLVED FIXED http://www.kino.de/kinosuche.php4 P1 Critical --- 1 fishd fishd ap eric zimmermann oldest_to_newest 93865 0 fishd 2008-10-03 01:05:22 -0700 REGRESSION: crash in ScriptElement::notifyFinished I observed this crash on http://www.kino.de/kinosuche.php4 (you might have to load that site a couple times to repro the crash) I found that some code changed recently that probably explains the crash. Compare: -void HTMLScriptElement::notifyFinished(CachedResource* o) -{ - CachedScript* cs = static_cast<CachedScript*>(o); - - ASSERT(cs == m_cachedScript); - - // Evaluating the script could lead to a garbage collection which - // can delete the script element so we need to protect it. - RefPtr<HTMLScriptElement> protect(this); - - if (cs->errorOccurred()) - dispatchHTMLEvent(errorEvent, true, false); - else { - evaluateScript(cs->url(), cs->script()); - dispatchHTMLEvent(loadEvent, false, false); - } - - // script evaluation may have dereffed it already - if (m_cachedScript) { - m_cachedScript->removeClient(this); - m_cachedScript = 0; - } -} To: void ScriptElementData::notifyFinished(CachedResource* o) { CachedScript* cs = static_cast<CachedScript*>(o); ASSERT(cs == m_cachedScript); if (cs->errorOccurred()) m_scriptElement->dispatchErrorEvent(); else { RefPtr<Element> protector(m_element); evaluateScript(cs->url(), cs->script()); m_scriptElement->dispatchLoadEvent(); } stopLoadRequest(); } The crash I get is inside stopLoadRequest() with |this| having been trashed. m_element owns |this|. Here's the interesting portion of the crash stack: chrome.dll!WTF::HashTable<WebCore::RenderObject *,std::pair<WebCore::RenderObject *,WebCore::RenderBlock::FloatingObject *>,WTF::PairFirstExtractor<std::pair<WebCore::RenderObject *,WebCore::RenderBlock::FloatingObject *> >,WTF::PtrHash<WebCore::RenderObject *>,WTF::PairHashTraits<WTF::HashTraits<WebCore::RenderObject *>,WTF::HashTraits<WebCore::RenderBlock::FloatingObject *> >,WTF::HashTraits<WebCore::RenderObject *> >::find<WebCore::RenderObject *,WTF::IdentityHashTranslator<WebCore::RenderObject *,std::pair<WebCore::RenderObject *,WebCore::RenderBlock::FloatingObject *>,WTF::PtrHash<WebCore::RenderObject *> > >(WebCore::RenderObject * const & key=0x0232775c) Line 751 C++ chrome.dll!WebCore::CachedResource::removeClient(WebCore::CachedResourceClient * c=0x0232775c) Line 131 + 0x12 bytes C++ > chrome.dll!WebCore::ScriptElementData::notifyFinished(WebCore::CachedResource * o=0x02330e58) Line 191 + 0xd bytes C++ chrome.dll!WebCore::CachedScript::checkNotify() Line 95 + 0xa bytes C++ chrome.dll!WebCore::CachedScript::data(WTF::PassRefPtr<WebCore::SharedBuffer> data={...}, bool allDataReceived=true) Line 85 + 0xe bytes C++ chrome.dll!WebCore::Loader::Host::didFinishLoading(WebCore::SubresourceLoader * loader=0x034297f8) Line 278 C++ chrome.dll!WebCore::SubresourceLoader::didFinishLoading() Line 195 C++ chrome.dll!WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle * __formal=0x01f31cf8) Line 399 C++ Originally identified here: http://code.google.com/p/chromium/issues/detail?id=3056 In the old code, the protector was also ensuring that |this| remained valid until after stopLoadRequest() is called. I think this is an apparent goof in the recent code change. 93869 1 ap 2008-10-03 01:11:31 -0700 (In reply to comment #0) > I found that some code changed recently that probably explains the crash. <http://trac.webkit.org/changeset/35744>. 93871 2 fishd 2008-10-03 01:16:13 -0700 This patch, which essentially just rolls back part of the original change, seems to resolve the bug. I'll work on a layout test, but it may be challenging since it is dependent on GC running at the right time. Index: ScriptElement.cpp =================================================================== --- ScriptElement.cpp (revision 2802) +++ ScriptElement.cpp (working copy) @@ -180,10 +180,13 @@ CachedScript* cs = static_cast<CachedScript*>(o); ASSERT(cs == m_cachedScript); + // Evaluating the script could lead to a garbage collection which can + // delete the script element so we need to protect it. + RefPtr<Element> protector(m_element); + if (cs->errorOccurred()) m_scriptElement->dispatchErrorEvent(); else { - RefPtr<Element> protector(m_element); evaluateScript(cs->url(), cs->script()); m_scriptElement->dispatchLoadEvent(); } 93917 3 eric 2008-10-03 11:30:28 -0700 (In reply to comment #2) > This patch, which essentially just rolls back part of the original change, > seems to resolve the bug. I'll work on a layout test, but it may be > challenging since it is dependent on GC running at the right time. > > Index: ScriptElement.cpp > =================================================================== > --- ScriptElement.cpp (revision 2802) > +++ ScriptElement.cpp (working copy) > @@ -180,10 +180,13 @@ > CachedScript* cs = static_cast<CachedScript*>(o); > ASSERT(cs == m_cachedScript); > + // Evaluating the script could lead to a garbage collection which can > + // delete the script element so we need to protect it. > + RefPtr<Element> protector(m_element); > + > if (cs->errorOccurred()) > m_scriptElement->dispatchErrorEvent(); > else { > - RefPtr<Element> protector(m_element); > evaluateScript(cs->url(), cs->script()); > m_scriptElement->dispatchLoadEvent(); > } > The change looks good. We just need a changelog, and ideally a layout test. gc() or GCController.collect() should be able to kick off GC when you need. 94020 4 24076 fishd 2008-10-03 17:14:38 -0700 Created attachment 24076 v1 patch Here's the patch sans layout test. Try as I might, I could not get my Mac build of WebKit to crash upon reaching ScriptElementData::stopLoadRequest despite having a junk |this| pointer. 94023 5 24077 fishd 2008-10-03 18:08:53 -0700 Created attachment 24077 v2 patch - same as before, but includes bug title in ChangeLog I tried using GuardMalloc to help create a test, but it seems like there is something else besides this issue that is causing DRT to crash under GuardMalloc. For example, fast/dom/script-element-gc.html crashes with or without my fix under GuardMalloc. I don't know how to get a call stack when GuardMalloc detects an error, so I don't know why DRT is crashing under GuardMalloc. 94076 6 24077 eric 2008-10-04 02:59:40 -0700 Comment on attachment 24077 v2 patch - same as before, but includes bug title in ChangeLog Looks great. Thanks for fixing this. It would be better w/ a layout test, but from discussions in the channel it sounds like you were having trouble getting one to work on your Mac. I think you have commit-bit these days, so you should be able to land this yourself. 94077 7 eric 2008-10-04 03:00:37 -0700 (In reply to comment #5) > Created an attachment (id=24077) [edit] > v2 patch - same as before, but includes bug title in ChangeLog > > I tried using GuardMalloc to help create a test, but it seems like there is > something else besides this issue that is causing DRT to crash under > GuardMalloc. For example, fast/dom/script-element-gc.html crashes with or > without my fix under GuardMalloc. I don't know how to get a call stack when > GuardMalloc detects an error, so I don't know why DRT is crashing under > GuardMalloc. > We need to file any --guard crashes for run-webkit-tests as bugs. Or rather, when you see any, don't hesitate to file them. We really should have a buildbot which runs run-webkit-tests --guard once a week or something. 94142 8 fishd 2008-10-04 23:51:05 -0700 I filed bug 21380 about how "run-webkit-tests -g" just crashes for me on seemingly every test. 94223 9 zimmermann 2008-10-06 06:08:01 -0700 Oops, good catch! My fault while refactoring the code... Sorry for any headaches. 24076 2008-10-03 17:14:38 -0700 2008-10-03 18:08:53 -0700 v1 patch script-element-1.diff text/plain 1461 fishd SW5kZXg6IENoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBDaGFuZ2VMb2cJKHJldmlzaW9uIDM3MjY3 KQorKysgQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTYgQEAKKzIwMDgtMTAt MDMgIERhcmluIEZpc2hlciAgPGRhcmluQGNocm9taXVtLm9yZz4KKworICAgICAgICBSZXZpZXdl ZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBXQVJOSU5HOiBOTyBURVNUIENBU0VTIEFE REVEIE9SIENIQU5HRUQKKworICAgICAgICBGaXhlcyBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9z aG93X2J1Zy5jZ2k/aWQ9MjEzMjkKKworICAgICAgICAqIGRvbS9TY3JpcHRFbGVtZW50LmNwcDoK KyAgICAgICAgKFdlYkNvcmU6OlNjcmlwdEVsZW1lbnREYXRhOjpub3RpZnlGaW5pc2hlZCk6IFJl dmVydCBwYXJ0IG9mIHIzNTc0NCB0bworICAgICAgICBlbnN1cmUgdGhhdCB0aGUgU2NyaXB0RWxl bWVudERhdGEgb2JqZWN0IGlzIG5vdCBkZXN0cm95ZWQgcHJlbWF0dXJlbHkuCisKKwogMjAwOC0x MC0wMyAgRGFyaW4gQWRsZXIgIDxkYXJpbkBhcHBsZS5jb20+CiAKICAgICAgICAgKiBiaW5kaW5n cy9qcy9KU0luc3BlY3RlZE9iamVjdFdyYXBwZXIuY3BwOiBUcnkgdG8gZml4IGEgYnVpbGQgZmFp bHVyZQpJbmRleDogZG9tL1NjcmlwdEVsZW1lbnQuY3BwCj09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIGRvbS9TY3Jp cHRFbGVtZW50LmNwcAkocmV2aXNpb24gMzcyMzApCisrKyBkb20vU2NyaXB0RWxlbWVudC5jcHAJ KHdvcmtpbmcgY29weSkKQEAgLTE4NiwxMCArMTg2LDEzIEBAIHZvaWQgU2NyaXB0RWxlbWVudERh dGE6Om5vdGlmeUZpbmlzaGVkKEMKICAgICBDYWNoZWRTY3JpcHQqIGNzID0gc3RhdGljX2Nhc3Q8 Q2FjaGVkU2NyaXB0Kj4obyk7CiAgICAgQVNTRVJUKGNzID09IG1fY2FjaGVkU2NyaXB0KTsKIAor ICAgIC8vIEV2YWx1YXRpbmcgdGhlIHNjcmlwdCBjb3VsZCBsZWFkIHRvIGEgZ2FyYmFnZSBjb2xs ZWN0aW9uIHdoaWNoIGNhbgorICAgIC8vIGRlbGV0ZSB0aGUgc2NyaXB0IGVsZW1lbnQgc28gd2Ug bmVlZCB0byBwcm90ZWN0IGl0IGFuZCB1cyB3aXRoIGl0IQorICAgIFJlZlB0cjxFbGVtZW50PiBw cm90ZWN0b3IobV9lbGVtZW50KTsKKwogICAgIGlmIChjcy0+ZXJyb3JPY2N1cnJlZCgpKQogICAg ICAgICBtX3NjcmlwdEVsZW1lbnQtPmRpc3BhdGNoRXJyb3JFdmVudCgpOwogICAgIGVsc2Ugewot ICAgICAgICBSZWZQdHI8RWxlbWVudD4gcHJvdGVjdG9yKG1fZWxlbWVudCk7CiAgICAgICAgIGV2 YWx1YXRlU2NyaXB0KGNzLT51cmwoKSwgY3MtPnNjcmlwdCgpKTsKICAgICAgICAgbV9zY3JpcHRF bGVtZW50LT5kaXNwYXRjaExvYWRFdmVudCgpOwogICAgIH0K 24077 2008-10-03 18:08:53 -0700 2008-10-04 02:59:40 -0700 v2 patch - same as before, but includes bug title in ChangeLog script-element-2.diff text/plain 1519 fishd SW5kZXg6IENoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBDaGFuZ2VMb2cJKHJldmlzaW9uIDM3MjY3 KQorKysgQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTYgQEAKKzIwMDgtMTAt MDMgIERhcmluIEZpc2hlciAgPGRhcmluQGNocm9taXVtLm9yZz4KKworICAgICAgICBSZXZpZXdl ZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBXQVJOSU5HOiBOTyBURVNUIENBU0VTIEFE REVEIE9SIENIQU5HRUQKKworICAgICAgICBSRUdSRVNTSU9OOiBjcmFzaCBpbiBTY3JpcHRFbGVt ZW50Ojpub3RpZnlGaW5pc2hlZAorICAgICAgICBGaXhlcyBodHRwczovL2J1Z3Mud2Via2l0Lm9y Zy9zaG93X2J1Zy5jZ2k/aWQ9MjEzMjkKKworICAgICAgICAqIGRvbS9TY3JpcHRFbGVtZW50LmNw cDoKKyAgICAgICAgKFdlYkNvcmU6OlNjcmlwdEVsZW1lbnREYXRhOjpub3RpZnlGaW5pc2hlZCk6 IFJldmVydCBwYXJ0IG9mIHIzNTc0NCB0bworICAgICAgICBlbnN1cmUgdGhhdCB0aGUgU2NyaXB0 RWxlbWVudERhdGEgb2JqZWN0IGlzIG5vdCBkZXN0cm95ZWQgcHJlbWF0dXJlbHkuCisKIDIwMDgt MTAtMDMgIERhcmluIEFkbGVyICA8ZGFyaW5AYXBwbGUuY29tPgogCiAgICAgICAgICogYmluZGlu Z3MvanMvSlNJbnNwZWN0ZWRPYmplY3RXcmFwcGVyLmNwcDogVHJ5IHRvIGZpeCBhIGJ1aWxkIGZh aWx1cmUKSW5kZXg6IGRvbS9TY3JpcHRFbGVtZW50LmNwcAo9PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBkb20vU2Ny aXB0RWxlbWVudC5jcHAJKHJldmlzaW9uIDM3MjMwKQorKysgZG9tL1NjcmlwdEVsZW1lbnQuY3Bw CSh3b3JraW5nIGNvcHkpCkBAIC0xODYsMTAgKzE4NiwxMyBAQCB2b2lkIFNjcmlwdEVsZW1lbnRE YXRhOjpub3RpZnlGaW5pc2hlZChDCiAgICAgQ2FjaGVkU2NyaXB0KiBjcyA9IHN0YXRpY19jYXN0 PENhY2hlZFNjcmlwdCo+KG8pOwogICAgIEFTU0VSVChjcyA9PSBtX2NhY2hlZFNjcmlwdCk7CiAK KyAgICAvLyBFdmFsdWF0aW5nIHRoZSBzY3JpcHQgY291bGQgbGVhZCB0byBhIGdhcmJhZ2UgY29s bGVjdGlvbiB3aGljaCBjYW4KKyAgICAvLyBkZWxldGUgdGhlIHNjcmlwdCBlbGVtZW50IHNvIHdl IG5lZWQgdG8gcHJvdGVjdCBpdCBhbmQgdXMgd2l0aCBpdCEKKyAgICBSZWZQdHI8RWxlbWVudD4g cHJvdGVjdG9yKG1fZWxlbWVudCk7CisKICAgICBpZiAoY3MtPmVycm9yT2NjdXJyZWQoKSkKICAg ICAgICAgbV9zY3JpcHRFbGVtZW50LT5kaXNwYXRjaEVycm9yRXZlbnQoKTsKICAgICBlbHNlIHsK LSAgICAgICAgUmVmUHRyPEVsZW1lbnQ+IHByb3RlY3RvcihtX2VsZW1lbnQpOwogICAgICAgICBl dmFsdWF0ZVNjcmlwdChjcy0+dXJsKCksIGNzLT5zY3JpcHQoKSk7CiAgICAgICAgIG1fc2NyaXB0 RWxlbWVudC0+ZGlzcGF0Y2hMb2FkRXZlbnQoKTsKICAgICB9Cg==