18859 2008-05-02 17:19:55 -0700 SVGRootInlineBox::buildTextChunks can do an invalid downcast 2008-05-06 10:38:40 -0700 1 1 1 Unclassified WebKit SVG 528+ (Nightly build) PC OS X 10.5 RESOLVED FIXED P2 Normal --- 1 myrdred webkit-unassigned oldest_to_newest 79509 0 myrdred 2008-05-02 17:19:55 -0700 rendering/SVGRootInlineBox.cpp:1382: Node* node = text->element()->parent(); if (node && node->isSVGElement()) textContent = static_cast<SVGTextContentElement*>(node); ASSERT(textContent); The problem is that the parent of the element node may not be an SVGTextContentElement. For example, in this SVG: <svg xmlns="http://www.w3.org/2000/svg"> <text> <a>Oh snap!</a> </text> </svg> ...the parent node is an SVGAElement, which doesn't inherit SVGTextContentElement. To see this more clearly, replace the code above with: Node* node = text->element()->parent(); if (node && node->isSVGElement()) { ASSERT(static_cast<SVGElement*>(node)->isTextContent()); textContent = static_cast<SVGTextContentElement*>(node); } ASSERT(textContent); Build, run Safari, load above SVG, earth-shattering kaboom. 79512 1 20937 myrdred 2008-05-02 17:22:42 -0700 Created attachment 20937 patch proposed patch 79591 2 20937 rwlbuis 2008-05-03 13:39:01 -0700 Comment on attachment 20937 patch The general idea seems good to me. The braces in the if/else are not needed since the blocks are both single line. Please include a testcase in the patch that would crash in the old situation and work fine with the patch. r- because of the missing testcase. 79676 3 20969 myrdred 2008-05-05 09:20:45 -0700 Created attachment 20969 improved patch Removed extraneous braces. I assume the braces around the body of the while loop can stay? There is no good test case for the original, unpatched code. The behavior of an invalid downcast is undefined and implementation-dependent. In the case of MSVC 8, the return value from a call to textContent->textLength() on the invalid pointer ends up pointing to the m_systemLanguage of SVGAElement::SVGTests. This usually produces innocuous if bogus values. I suppose I might be able to contrive a case where it forced an assert to trigger, but again, the behavior is undefined and there's no guarantee that the same behavior would result in an Xcode compilatior, or a gcc compilation, or even a different version of MSVC. 79723 4 20969 rwlbuis 2008-05-06 00:23:55 -0700 Comment on attachment 20969 improved patch Assuming you ran the tests and no new regressions/crashes occur, r=me 79760 5 rwlbuis 2008-05-06 10:38:40 -0700 Landed in r32907. 20937 2008-05-02 17:22:42 -0700 2008-05-05 09:11:22 -0700 patch patch text/plain 1571 myrdred SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiAzMjgyOCkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTQgQEAKKzIwMDgtMDUtMDIgIEpvbmF0aGFuIEhhYXMgIDxteXJkcmVkQGdtYWls LmNvbT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTg4NTlcCisgICAgICAgIFBy ZXZlbnRlZCBTVkdSb290SW5saW5lQm94IGZyb20gc3RhdGljX2Nhc3RpbmcgYQorICAgICAgICBu b2RlIHRvIGEgY2xhc3MgaXQgZG9lc24ndCBpbmhlcml0CisgICAgICAgIAorICAgICAgICAqIHJl bmRlcmluZy9TVkdSb290SW5saW5lQm94LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OlNWR1Jvb3RJ bmxpbmVCb3g6OmJ1aWxkVGV4dENodW5rcyk6CisKIDIwMDgtMDUtMDIgIEJlbmphbWluIE90dGUg IDxvdHRlQGdub21lLm9yZz4KIAogICAgICAgICBSZXZpZXdlZCBieSBBbHAgVG9rZXIuCkluZGV4 OiBXZWJDb3JlL3JlbmRlcmluZy9TVkdSb290SW5saW5lQm94LmNwcAo9PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBX ZWJDb3JlL3JlbmRlcmluZy9TVkdSb290SW5saW5lQm94LmNwcAkocmV2aXNpb24gMzI4MjEpCisr KyBXZWJDb3JlL3JlbmRlcmluZy9TVkdSb290SW5saW5lQm94LmNwcAkod29ya2luZyBjb3B5KQpA QCAtMTM4MCw4ICsxMzgwLDEzIEBAIHZvaWQgU1ZHUm9vdElubGluZUJveDo6YnVpbGRUZXh0Q2h1 bmtzKFYKIAogICAgICAgICAgICAgU1ZHVGV4dENvbnRlbnRFbGVtZW50KiB0ZXh0Q29udGVudCA9 IDA7CiAgICAgICAgICAgICBOb2RlKiBub2RlID0gdGV4dC0+ZWxlbWVudCgpLT5wYXJlbnQoKTsK LSAgICAgICAgICAgIGlmIChub2RlICYmIG5vZGUtPmlzU1ZHRWxlbWVudCgpKQotICAgICAgICAg ICAgICAgIHRleHRDb250ZW50ID0gc3RhdGljX2Nhc3Q8U1ZHVGV4dENvbnRlbnRFbGVtZW50Kj4o bm9kZSk7CisgICAgICAgICAgICB3aGlsZSAobm9kZSAmJiBub2RlLT5pc1NWR0VsZW1lbnQoKSAm JiAhdGV4dENvbnRlbnQpIHsKKyAgICAgICAgICAgICAgICBpZiAoc3RhdGljX2Nhc3Q8U1ZHRWxl bWVudCo+KG5vZGUpLT5pc1RleHRDb250ZW50KCkpIHsKKyAgICAgICAgICAgICAgICAgICAgdGV4 dENvbnRlbnQgPSBzdGF0aWNfY2FzdDxTVkdUZXh0Q29udGVudEVsZW1lbnQqPihub2RlKTsKKyAg ICAgICAgICAgICAgICB9IGVsc2UgeworICAgICAgICAgICAgICAgICAgICBub2RlID0gbm9kZS0+ cGFyZW50Tm9kZSgpOworICAgICAgICAgICAgICAgIH0KKyAgICAgICAgICAgIH0KICAgICAgICAg ICAgIEFTU0VSVCh0ZXh0Q29udGVudCk7CiAKICAgICAgICAgICAgIC8vIFN0YXJ0IG5ldyBjaGFy YWN0ZXIgcmFuZ2UgZm9yIHRoZSBmaXJzdCBjaHVuawo= 20969 2008-05-05 09:20:45 -0700 2008-05-06 00:23:55 -0700 improved patch patch text/plain 1546 myrdred SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiAzMjgyOCkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTQgQEAKKzIwMDgtMDUtMDIgIEpvbmF0aGFuIEhhYXMgIDxteXJkcmVkQGdtYWls LmNvbT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTg4NTlcCisgICAgICAgIFBy ZXZlbnRlZCBTVkdSb290SW5saW5lQm94IGZyb20gc3RhdGljX2Nhc3RpbmcgYQorICAgICAgICBu b2RlIHRvIGEgY2xhc3MgaXQgZG9lc24ndCBpbmhlcml0CisgICAgICAgIAorICAgICAgICAqIHJl bmRlcmluZy9TVkdSb290SW5saW5lQm94LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OlNWR1Jvb3RJ bmxpbmVCb3g6OmJ1aWxkVGV4dENodW5rcyk6CisKIDIwMDgtMDUtMDIgIEJlbmphbWluIE90dGUg IDxvdHRlQGdub21lLm9yZz4KIAogICAgICAgICBSZXZpZXdlZCBieSBBbHAgVG9rZXIuCkluZGV4 OiBXZWJDb3JlL3JlbmRlcmluZy9TVkdSb290SW5saW5lQm94LmNwcAo9PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBX ZWJDb3JlL3JlbmRlcmluZy9TVkdSb290SW5saW5lQm94LmNwcAkocmV2aXNpb24gMzI4MjEpCisr KyBXZWJDb3JlL3JlbmRlcmluZy9TVkdSb290SW5saW5lQm94LmNwcAkod29ya2luZyBjb3B5KQpA QCAtMTM4MCw4ICsxMzgwLDEyIEBAIHZvaWQgU1ZHUm9vdElubGluZUJveDo6YnVpbGRUZXh0Q2h1 bmtzKFYKIAogICAgICAgICAgICAgU1ZHVGV4dENvbnRlbnRFbGVtZW50KiB0ZXh0Q29udGVudCA9 IDA7CiAgICAgICAgICAgICBOb2RlKiBub2RlID0gdGV4dC0+ZWxlbWVudCgpLT5wYXJlbnQoKTsK LSAgICAgICAgICAgIGlmIChub2RlICYmIG5vZGUtPmlzU1ZHRWxlbWVudCgpKQotICAgICAgICAg ICAgICAgIHRleHRDb250ZW50ID0gc3RhdGljX2Nhc3Q8U1ZHVGV4dENvbnRlbnRFbGVtZW50Kj4o bm9kZSk7CisgICAgICAgICAgICB3aGlsZSAobm9kZSAmJiBub2RlLT5pc1NWR0VsZW1lbnQoKSAm JiAhdGV4dENvbnRlbnQpIHsKKyAgICAgICAgICAgICAgICBpZiAoc3RhdGljX2Nhc3Q8U1ZHRWxl bWVudCo+KG5vZGUpLT5pc1RleHRDb250ZW50KCkpCisgICAgICAgICAgICAgICAgICAgIHRleHRD b250ZW50ID0gc3RhdGljX2Nhc3Q8U1ZHVGV4dENvbnRlbnRFbGVtZW50Kj4obm9kZSk7CisgICAg ICAgICAgICAgICAgZWxzZQorICAgICAgICAgICAgICAgICAgICBub2RlID0gbm9kZS0+cGFyZW50 Tm9kZSgpOworICAgICAgICAgICAgfQogICAgICAgICAgICAgQVNTRVJUKHRleHRDb250ZW50KTsK IAogICAgICAgICAgICAgLy8gU3RhcnQgbmV3IGNoYXJhY3RlciByYW5nZSBmb3IgdGhlIGZpcnN0 IGNodW5rCg==