41054 2010-06-23 01:55:59 -0700 WebCore crashes when removing an object element with fallback content in a beforeload handler 2010-06-23 17:30:54 -0700 1 1 1 Unclassified WebKit Page Loading 528+ (Nightly build) All All RESOLVED FIXED InRadar P2 Critical --- 1 aestes aestes aestes ap eric mitz oldest_to_newest 241760 0 aestes 2010-06-23 01:55:59 -0700 If an image-based <object> with fallback content is removed from the DOM in a beforeload listener and the image fails to load, WebCore will crash trying to render the fallback content. Here is the relevant stacktrace: 0 com.apple.WebCore 0x00000001013fc15d WebCore::Node::createRendererIfNeeded() + 45 1 com.apple.WebCore 0x0000000100e76d30 WebCore::Element::attach() + 32 2 com.apple.WebCore 0x0000000100c8d19c WebCore::CachedImage::checkNotify() + 76 3 com.apple.WebCore 0x0000000100c8da5c WebCore::CachedImage::data(WTF::PassRefPtr<WebCore::SharedBuffer>, bool) + 364 4 com.apple.WebCore 0x00000001013c1456 WebCore::Loader::Host::didFinishLoading(WebCore::SubresourceLoader*) + 358 5 com.apple.WebCore 0x00000001015b7cc1 WebCore::SubresourceLoader::didFinishLoading() + 49 The attached .html file demonstrates the crash. 241763 1 aestes 2010-06-23 01:57:12 -0700 <rdar://problem/8107855> 241791 2 aestes 2010-06-23 03:57:24 -0700 It looks like the crash happens when removing any <object> with an invalid data URL. It isn't specific to image URLs or <object>'s with fallback content as I had originally thought. Patch forthcoming. 241795 3 59501 aestes 2010-06-23 04:15:07 -0700 Created attachment 59501 test case 241798 4 59502 aestes 2010-06-23 04:17:13 -0700 Created attachment 59502 patch 241867 5 59502 ap 2010-06-23 08:47:22 -0700 Comment on attachment 59502 patch > + (WebCore::RenderEmbeddedObject::updateWidget): If the RenderObject was > + destroyed during processing of onbeforeload, do not proceed with loading > + the object. So the same regression test covers both fixes? That's great news. > + // may cause this RenderObject to be destroyed. Even though we protect As you explain later, it's not destroyed. I'm not sure what the correct term is. > + // crashes such as the one described in <rdar://problem/8107855>. Exit It is better to give b.w.o. references, since non-Apple people can't see radar bugs. Or maybe this comment isn't needed at all, or could be made much shorter ("The node may get detached by event handler"). In retrospect, this seems pretty obvious. > + bool success = beforeLoadSuccess && frame->loader()->subframeLoader()->requestObject(this, url, objectElement->getAttribute(nameAttr), serviceType, paramNames, paramValues); I think that dispatchBeforeLoadEvent() always "succeeds", so it would be slightly better for the variable to be called something like "beforeLoadAllowedLoad". r=me assuming that both code changes are covered by the regression test. 241869 6 59502 ap 2010-06-23 08:59:10 -0700 Comment on attachment 59502 patch + <object data="invalid.png">FAIL</object> + <object data="invalid">FAIL</object> One more trivial comment - it's slightly better to use something like "does-not-exist", because "invalid" implies broken content or URL, while neither is the case here. ChangeLog is also wrong to call the URL invalid. 242005 7 aestes 2010-06-23 12:00:24 -0700 (In reply to comment #5) > (From update of attachment 59502 [details]) > > + (WebCore::RenderEmbeddedObject::updateWidget): If the RenderObject was > > + destroyed during processing of onbeforeload, do not proceed with loading > > + the object. > > So the same regression test covers both fixes? That's great news. It does. The crashing behavior is actually slightly different depending on whether or not the data URL has an image extension, so I'm testing both cases in the layout test. > > > + // may cause this RenderObject to be destroyed. Even though we protect > > As you explain later, it's not destroyed. I'm not sure what the correct term is. By 'destroyed', I mean RenderWidget::destroy() was called. Among other things, RenderWidget::destroy() calls setNode(0). So, it hasn't been deleted yet, but it has been destroyed. Perhaps I can make that more clear in the comment. > > > + // crashes such as the one described in <rdar://problem/8107855>. Exit > > It is better to give b.w.o. references, since non-Apple people can't see radar bugs. Or maybe this comment isn't needed at all, or could be made much shorter ("The node may get detached by event handler"). In retrospect, this seems pretty obvious. You're right that the comment is probably overkill; I'll just make it shorter. > > > + bool success = beforeLoadSuccess && frame->loader()->subframeLoader()->requestObject(this, url, objectElement->getAttribute(nameAttr), serviceType, paramNames, paramValues); > > I think that dispatchBeforeLoadEvent() always "succeeds", so it would be slightly better for the variable to be called something like "beforeLoadAllowedLoad". Actually, dispatchBeforeLoadEvent() will return false if the beforeload JavaScript listener returns false or calls preventDefault(). This is the way the script tells us to block the load. > > r=me assuming that both code changes are covered by the regression test. Thanks for the review! 242008 8 aestes 2010-06-23 12:12:12 -0700 (In reply to comment #7) > > > > > + bool success = beforeLoadSuccess && frame->loader()->subframeLoader()->requestObject(this, url, objectElement->getAttribute(nameAttr), serviceType, paramNames, paramValues); > > > > I think that dispatchBeforeLoadEvent() always "succeeds", so it would be slightly better for the variable to be called something like "beforeLoadAllowedLoad". > > Actually, dispatchBeforeLoadEvent() will return false if the beforeload JavaScript listener returns false or calls preventDefault(). This is the way the script tells us to block the load. Ohh duh! Sorry, I totally misunderstood your point. Yes, 'beforeLoadAllowed' makes more sense. 242014 9 ap 2010-06-23 12:17:08 -0700 > Actually, dispatchBeforeLoadEvent() will return false if the beforeload JavaScript listener returns false or > calls preventDefault(). This is the way the script tells us to block the load. Yes, but isn't returning false also a success for dispatchBeforeLoadEvent()? That's why it exists, after all. 242015 10 ap 2010-06-23 12:18:19 -0700 Sorry, I didn't notice that you already corrected yourself. 242044 11 aestes 2010-06-23 13:05:18 -0700 Committed http://trac.webkit.org/changeset/61707 242131 12 eric 2010-06-23 16:07:05 -0700 Looks like fast/dom/beforeload/remove-bad-object-in-beforeload-listener.html was landed w/o results. 242132 13 aestes 2010-06-23 16:08:20 -0700 (In reply to comment #12) > Looks like fast/dom/beforeload/remove-bad-object-in-beforeload-listener.html was landed w/o results. Bah! Fixing... 242175 14 aestes 2010-06-23 17:30:54 -0700 (In reply to comment #13) > (In reply to comment #12) > > Looks like fast/dom/beforeload/remove-bad-object-in-beforeload-listener.html was landed w/o results. > > Bah! Fixing... Committed http://trac.webkit.org/changeset/61721 with layout test expectations. 59501 2010-06-23 04:15:07 -0700 2010-06-23 04:15:07 -0700 test case object-before-load-crash.html text/html 281 aestes PGJvZHk+CjxzY3JpcHQ+CmRvY3VtZW50LmFkZEV2ZW50TGlzdGVuZXIoImJlZm9yZWxvYWQiLCBo YW5kbGVCZWZvcmVMb2FkRXZlbnQsIHRydWUpOwoKZnVuY3Rpb24gaGFuZGxlQmVmb3JlTG9hZEV2 ZW50KGV2ZW50KQp7CiAgICBjb25zdCBlbGVtZW50ID0gZXZlbnQudGFyZ2V0OwogICAgZG9jdW1l bnQuYm9keS5yZW1vdmVDaGlsZChlbGVtZW50KTsKfQo8L3NjcmlwdD4KPG9iamVjdCBkYXRhPSJo dHRwOi8vaW1hZ2VzLmFwcGxlLmNvbS9hLmpwZyI+RkFJTDwvb2JqZWN0PgoKPC9ib2R5Pgo= 59502 2010-06-23 04:17:13 -0700 2010-06-23 08:59:10 -0700 patch 41054.patch text/plain 5579 aestes SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA2MTY4MSkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMjIgQEAKKzIwMTAtMDYtMjMgIEFuZHkgRXN0ZXMgIDxhZXN0ZXNAYXBwbGUuY29t PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorICAgICAgICAKKyAgICAg ICAgPHJkYXI6Ly9wcm9ibGVtLzgxMDc4NTU+IFByZXZlbnQgYSBjcmFzaCBpbiBXZWJDb3JlIHdo ZW4gcmVtb3ZpbmcgYW4KKyAgICAgICAgb2JqZWN0IGVsZW1lbnQgd2l0aCBhbiBpbnZhbGlkIGRh dGEgVVJMIGluIGluIGEgbGlzdGVuZXIgdG8gaXRzCisgICAgICAgIGJlZm9yZWxvYWQgZXZlbnQu CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD00MTA1NAor CisgICAgICAgIFRlc3RzOiBmYXN0L2RvbS9iZWZvcmVsb2FkL3JlbW92ZS1iYWQtb2JqZWN0LWlu LWJlZm9yZWxvYWQtbGlzdGVuZXIuaHRtbAorCisgICAgICAgICogaHRtbC9IVE1MT2JqZWN0RWxl bWVudC5jcHA6CisgICAgICAgIChXZWJDb3JlOjpIVE1MT2JqZWN0RWxlbWVudDo6cmVuZGVyRmFs bGJhY2tDb250ZW50KTogRXhpdCBlYXJseSBpZiB0aGUKKyAgICAgICAgb2JqZWN0IGVsZW1lbnQg aXMgbm90IGluIHRoZSBkb2N1bWVudC4KKyAgICAgICAgKiByZW5kZXJpbmcvUmVuZGVyRW1iZWRk ZWRPYmplY3QuY3BwOgorICAgICAgICAoV2ViQ29yZTo6UmVuZGVyRW1iZWRkZWRPYmplY3Q6OnVw ZGF0ZVdpZGdldCk6IElmIHRoZSBSZW5kZXJPYmplY3Qgd2FzCisgICAgICAgIGRlc3Ryb3llZCBk dXJpbmcgcHJvY2Vzc2luZyBvZiBvbmJlZm9yZWxvYWQsIGRvIG5vdCBwcm9jZWVkIHdpdGggbG9h ZGluZworICAgICAgICB0aGUgb2JqZWN0LgorCiAyMDEwLTA2LTIzICBBZGFtIEJhcnRoICA8YWJh cnRoQHdlYmtpdC5vcmc+CiAKICAgICAgICAgVW5yZXZpZXdlZC4gIGNsZWFuLWhlYWRlci1ndWFy ZHMgdGhhdCB3ZXJlIG1hZGUgZGlydHkgYnkgcmVjZW50CkluZGV4OiBXZWJDb3JlL2h0bWwvSFRN TE9iamVjdEVsZW1lbnQuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvaHRtbC9IVE1MT2JqZWN0 RWxlbWVudC5jcHAJKHJldmlzaW9uIDYxNjY0KQorKysgV2ViQ29yZS9odG1sL0hUTUxPYmplY3RF bGVtZW50LmNwcAkod29ya2luZyBjb3B5KQpAQCAtMjQzLDYgKzI0Myw5IEBAIHZvaWQgSFRNTE9i amVjdEVsZW1lbnQ6OnJlbmRlckZhbGxiYWNrQ28KIHsKICAgICBpZiAobV91c2VGYWxsYmFja0Nv bnRlbnQpCiAgICAgICAgIHJldHVybjsKKyAgICAKKyAgICBpZiAoIWluRG9jdW1lbnQoKSkKKyAg ICAgICAgcmV0dXJuOwogCiAgICAgLy8gQmVmb3JlIHdlIGdpdmUgdXAgYW5kIHVzZSBmYWxsYmFj ayBjb250ZW50LCBjaGVjayB0byBzZWUgaWYgdGhpcyBpcyBhIE1JTUUgdHlwZSBpc3N1ZS4KICAg ICBpZiAobV9pbWFnZUxvYWRlciAmJiBtX2ltYWdlTG9hZGVyLT5pbWFnZSgpKSB7CkluZGV4OiBX ZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJFbWJlZGRlZE9iamVjdC5jcHAKPT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0g V2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyRW1iZWRkZWRPYmplY3QuY3BwCShyZXZpc2lvbiA2MTY2 NCkKKysrIFdlYkNvcmUvcmVuZGVyaW5nL1JlbmRlckVtYmVkZGVkT2JqZWN0LmNwcAkod29ya2lu ZyBjb3B5KQpAQCAtMjg0LDcgKzI4NCwxOSBAQCB2b2lkIFJlbmRlckVtYmVkZGVkT2JqZWN0Ojp1 cGRhdGVXaWRnZXQoCiAgICAgICAgICAgICAgICAgcmV0dXJuOwogICAgICAgICB9CiAKLSAgICAg ICAgYm9vbCBzdWNjZXNzID0gb2JqZWN0RWxlbWVudC0+ZGlzcGF0Y2hCZWZvcmVMb2FkRXZlbnQo dXJsKSAmJiBmcmFtZS0+bG9hZGVyKCktPnN1YmZyYW1lTG9hZGVyKCktPnJlcXVlc3RPYmplY3Qo dGhpcywgdXJsLCBvYmplY3RFbGVtZW50LT5nZXRBdHRyaWJ1dGUobmFtZUF0dHIpLCBzZXJ2aWNl VHlwZSwgcGFyYW1OYW1lcywgcGFyYW1WYWx1ZXMpOworICAgICAgICBib29sIGJlZm9yZUxvYWRT dWNjZXNzID0gb2JqZWN0RWxlbWVudC0+ZGlzcGF0Y2hCZWZvcmVMb2FkRXZlbnQodXJsKTsKKyAg ICAgICAgCisgICAgICAgIC8vIEFyYml0cmFyeSBKYXZhU2NyaXB0IGNhbiBleGVjdXRlIGluIGRp c3BhdGNoQmVmb3JlTG9hZEV2ZW50KCkgd2hpY2gKKyAgICAgICAgLy8gbWF5IGNhdXNlIHRoaXMg UmVuZGVyT2JqZWN0IHRvIGJlIGRlc3Ryb3llZC4gIEV2ZW4gdGhvdWdoIHdlIHByb3RlY3QKKyAg ICAgICAgLy8gb3V0c2VsdmVzIGZyb20gYmVpbmcgb3V0cmlnaHQgZGVsZXRlZCBieSB1c2luZyBh IHByb3RlY3QgcG9pbnRlciwgdGhlCisgICAgICAgIC8vIGRlc3RydWN0aW9uIHByb2Nlc3Mgd2ls bCBuaWwgb3V0IG91ciBOb2RlIHJlZmVyZW5jZSwgd2hpY2ggd2lsbCBjYXVzZQorICAgICAgICAv LyBjcmFzaGVzIHN1Y2ggYXMgdGhlIG9uZSBkZXNjcmliZWQgaW4gPHJkYXI6Ly9wcm9ibGVtLzgx MDc4NTU+LiAgRXhpdAorICAgICAgICAvLyBlYXJseSBpZiBiZWZvcmVsb2FkIHJlbW92ZWQgdXMg ZnJvbSB0aGUgcmVuZGVyIHRyZWUuCisgICAgICAgIGlmICghbm9kZSgpKQorICAgICAgICAgICAg cmV0dXJuOworICAgICAgICAKKyAgICAgICAgYm9vbCBzdWNjZXNzID0gYmVmb3JlTG9hZFN1Y2Nl c3MgJiYgZnJhbWUtPmxvYWRlcigpLT5zdWJmcmFtZUxvYWRlcigpLT5yZXF1ZXN0T2JqZWN0KHRo aXMsIHVybCwgb2JqZWN0RWxlbWVudC0+Z2V0QXR0cmlidXRlKG5hbWVBdHRyKSwgc2VydmljZVR5 cGUsIHBhcmFtTmFtZXMsIHBhcmFtVmFsdWVzKTsKKyAgICAKICAgICAgICAgaWYgKCFzdWNjZXNz ICYmIG1faGFzRmFsbGJhY2tDb250ZW50KQogICAgICAgICAgICAgb2JqZWN0RWxlbWVudC0+cmVu ZGVyRmFsbGJhY2tDb250ZW50KCk7CiAKSW5kZXg6IExheW91dFRlc3RzL2Zhc3QvZG9tL2JlZm9y ZWxvYWQvcmVtb3ZlLWJhZC1vYmplY3QtaW4tYmVmb3JlbG9hZC1saXN0ZW5lci5odG1sCj09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT0KLS0tIExheW91dFRlc3RzL2Zhc3QvZG9tL2JlZm9yZWxvYWQvcmVtb3ZlLWJhZC1vYmpl Y3QtaW4tYmVmb3JlbG9hZC1saXN0ZW5lci5odG1sCShyZXZpc2lvbiAwKQorKysgTGF5b3V0VGVz dHMvZmFzdC9kb20vYmVmb3JlbG9hZC9yZW1vdmUtYmFkLW9iamVjdC1pbi1iZWZvcmVsb2FkLWxp c3RlbmVyLmh0bWwJKHJldmlzaW9uIDApCkBAIC0wLDAgKzEsNDggQEAKKzxodG1sPgorPGhlYWQ+ CisgICAgPHNjcmlwdCBzcmM9InJlc291cmNlcy9wcmludC5qcyI+PC9zY3JpcHQ+CisgICAgPHNj cmlwdD4KKyAgICAgICAgaWYgKHdpbmRvdy5sYXlvdXRUZXN0Q29udHJvbGxlcikgeworICAgICAg ICAgICAgbGF5b3V0VGVzdENvbnRyb2xsZXIuZHVtcEFzVGV4dCgpOworICAgICAgICAgICAgbGF5 b3V0VGVzdENvbnRyb2xsZXIud2FpdFVudGlsRG9uZSgpOworICAgICAgICB9CisgICAgICAgIAor ICAgICAgICB2YXIgY291bnQgPSAwOworICAgICAgICB2YXIgbWF4Q291bnQgPSAxMDsKKyAgICAg ICAgdmFyIHRpbWVvdXQgPSAxMDA7CisKKyAgICAgICAgZG9jdW1lbnQuYWRkRXZlbnRMaXN0ZW5l cigiYmVmb3JlbG9hZCIsIGZ1bmN0aW9uKGV2ZW50KSB7CisgICAgICAgICAgICBldmVudC50YXJn ZXQucGFyZW50RWxlbWVudC5yZW1vdmVDaGlsZChldmVudC50YXJnZXQpOworICAgICAgICB9LCB0 cnVlKTsKKyAgICA8L3NjcmlwdD4KKzwvaGVhZD4KKzxib2R5PgorICAgIDxwPlRoaXMgcGFnZSB0 ZXN0cyB0aGF0IHlvdSBjYW4gY29ycmVjdGx5IHJlbW92ZSBhbiBvYmplY3QgZWxlbWVudCB3aXRo IGFuIGludmFsaWQgZGF0YSBVUkwgaW4gaXRzIGJlZm9yZWxvYWQgbGlzdGVuZXIgd2l0aG91dCBj YXVzaW5nIGEgY3Jhc2guPC9wPgorICAgIDxvYmplY3QgZGF0YT0iaW52YWxpZC5wbmciPkZBSUw8 L29iamVjdD4KKyAgICA8b2JqZWN0IGRhdGE9ImludmFsaWQiPkZBSUw8L29iamVjdD4KKyAgICA8 ZGl2IGlkPSJjb25zb2xlIj48L2Rpdj4KKyAgICA8c2NyaXB0PgorICAgICAgICBmdW5jdGlvbiBj aGVja09iamVjdFJlbW92YWwoKQorICAgICAgICB7CisgICAgICAgICAgICBpZiAoIWRvY3VtZW50 LmdldEVsZW1lbnRzQnlUYWdOYW1lKCJvYmplY3QiKS5sZW5ndGgpIHsKKyAgICAgICAgICAgICAg ICBwcmludCgiUEFTUyIsICJncmVlbiIpOworICAgICAgICAgICAgICAgIGlmICh3aW5kb3cubGF5 b3V0VGVzdENvbnRyb2xsZXIpCisgICAgICAgICAgICAgICAgICAgIGxheW91dFRlc3RDb250cm9s bGVyLm5vdGlmeURvbmUoKTsKKyAgICAgICAgICAgICAgICByZXR1cm47CisgICAgICAgICAgICB9 CisgICAgICAgICAgICAKKyAgICAgICAgICAgIGlmICgrK2NvdW50ID4gbWF4Q291bnQpIHsKKyAg ICAgICAgICAgICAgICBwcmludCgiRkFJTCIsICJyZWQiKTsKKyAgICAgICAgICAgICAgICBpZiAo d2luZG93LmxheW91dFRlc3RDb250cm9sbGVyKQorICAgICAgICAgICAgICAgICAgICBsYXlvdXRU ZXN0Q29udHJvbGxlci5ub3RpZnlEb25lKCk7CisgICAgICAgICAgICAgICAgcmV0dXJuOworICAg ICAgICAgICAgfQorICAgICAgICAgICAgCisgICAgICAgICAgICBzZXRUaW1lb3V0KCJjaGVja09i amVjdFJlbW92YWwoKSIsIHRpbWVvdXQpOworICAgICAgICB9CisgICAgICAgIAorICAgICAgICBz ZXRUaW1lb3V0KCJjaGVja09iamVjdFJlbW92YWwoKSIsIHRpbWVvdXQpOworICAgIDwvc2NyaXB0 PgorPC9ib2R5PgorPC9odG1sPgorCkluZGV4OiBMYXlvdXRUZXN0cy9mYXN0L2RvbS9iZWZvcmVs b2FkL3Jlc291cmNlcy9wcmludC5qcwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBMYXlvdXRUZXN0cy9mYXN0L2Rv bS9iZWZvcmVsb2FkL3Jlc291cmNlcy9wcmludC5qcwkocmV2aXNpb24gMCkKKysrIExheW91dFRl c3RzL2Zhc3QvZG9tL2JlZm9yZWxvYWQvcmVzb3VyY2VzL3ByaW50LmpzCShyZXZpc2lvbiAwKQpA QCAtMCwwICsxLDkgQEAKK2Z1bmN0aW9uIHByaW50KG1lc3NhZ2UsIGNvbG9yKSAKK3sKKyAgICB2 YXIgcGFyYWdyYXBoID0gZG9jdW1lbnQuY3JlYXRlRWxlbWVudCgiZGl2Iik7CisgICAgcGFyYWdy YXBoLmFwcGVuZENoaWxkKGRvY3VtZW50LmNyZWF0ZVRleHROb2RlKG1lc3NhZ2UpKTsKKyAgICBw YXJhZ3JhcGguc3R5bGUuZm9udEZhbWlseSA9ICJtb25vc3BhY2UiOworICAgIGlmIChjb2xvcikK KyAgICAgICAgcGFyYWdyYXBoLnN0eWxlLmNvbG9yID0gY29sb3I7CisgICAgZG9jdW1lbnQuZ2V0 RWxlbWVudEJ5SWQoImNvbnNvbGUiKS5hcHBlbmRDaGlsZChwYXJhZ3JhcGgpOworfQo=