5784 2005-11-20 09:16:49 -0800 Assertion failure in DocumentImpl::removeElementById() (idCount > 0) 2005-12-26 14:08:35 -0800 1 1 1 Unclassified WebKit DOM 420+ Mac OS X 10.4 VERIFIED FIXED http://nontroppo.org/wiki/PatchingStartCom/edit P2 Normal --- 1 ap darin mitz oldest_to_newest 24296 0 ap 2005-11-20 09:16:49 -0800 Running ToT from 12/11/2005 Steps to reproduce: 1. Open http://nontroppo.org/wiki/PatchingStartCom/edit (an "Access not allowed" page appears) 2. Close the window Results: /Users/ap/WebKit/WebCore/khtml/xml/dom_docimpl.cpp:829: failed assertion `idCount > 0' 24317 1 ap 2005-11-20 21:58:10 -0800 With current (November 21) ToT, the line numer is 842. 24352 2 4756 mitz 2005-11-21 07:45:36 -0800 Created attachment 4756 Reduced testcase The required ingredients for this bug are an <html> element with an id attribute, and evaluating document.documentElement before closing the window. 24353 3 mitz 2005-11-21 08:38:23 -0800 An additional requirement for triggering the bug is to have something before the <html> tag (in the testcase, the <!DOCTYPE>). What happens is that if the first thing the parser sees is not <html>, it creates an implicit html element and inserts it into the document. When the real html element is encountered, it is not inserted into the document, so for example, document.getElementById("foo") returns null in the testcase, since the implicit html element doesn't have an id attribute. document.documentElement returns the "explicit" element. 24354 4 4757 mitz 2005-11-21 10:13:19 -0800 Created attachment 4757 Call updateId() when inserting id attribute I was (partly) wrong. The attributes, including "id" are copied from the explicit element to the implicit one, but insertAttribute() doesn't call updateId() when necessary. This fixes the testcase and makes document.getElementById('foo') return the html element. 24421 5 4757 darin 2005-11-22 19:39:20 -0800 Comment on attachment 4757 Call updateId() when inserting id attribute The change here is just right to fix the bug, but I'm slightly concerned about the effect this might have on performance. I'm also annoyed about that dom_elementimpl.h now includes the htmlnames.h header. Perhaps a better change would be to use a different function in htmlparser.cpp and leave this one as a simpler and faster function only to be used by htmltokenizer.cpp. We could add an assertion that element is 0. There's a chance that we could just use setAttribute in the two code paths inside htmlparser.cpp, and in fact I think that skipping incDOMTreeVersion and attributeChanged might be bad for those cases. Lets look into a patch that involves making insertAttribute assert more and get called less. 25523 6 4947 mitz 2005-12-04 13:39:05 -0800 Created attachment 4947 Use setAttribute from htmlparser; add assert in insertAttribute 25557 7 4947 eric 2005-12-05 02:29:59 -0800 Comment on attachment 4947 Use setAttribute from htmlparser; add assert in insertAttribute The change looks good to me. But I'll let darin, hyatt or maciej (those more familiar with this code path) have the final say. 25570 8 4947 darin 2005-12-05 06:30:39 -0800 Comment on attachment 4947 Use setAttribute from htmlparser; add assert in insertAttribute r=me 4756 2005-11-21 07:45:36 -0800 2005-11-21 07:45:36 -0800 Reduced testcase htmlElementWithId.html text/html 324 mitz PCFET0NUWVBFIGh0bWwgUFVCTElDICItLy9XM0MvL0RURCBYSFRNTCAxLjAgU3RyaWN0Ly9FTiIg Imh0dHA6Ly93d3cudzMub3JnL1RSL3hodG1sMS9EVEQveGh0bWwxLXN0cmljdC5kdGQiPgo8aHRt bCB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMTk5OS94aHRtbCIgeG1sOmxhbmc9ImVuIiBpZD0i Zm9vIj4KPGhlYWQ+Cjx0aXRsZT5kb21fZG9jaW1wbC5jcHA6ODQyIGFzc2VydDwvdGl0bGU+Cjxz Y3JpcHQgdHlwZT0idGV4dC9qYXZhc2NyaXB0Ij4KZG9jdW1lbnQuZG9jdW1lbnRFbGVtZW50Owo8 L3NjcmlwdD4KPC9oZWFkPgo8Ym9keT4KPC9ib2R5Pgo8L2h0bWw+ 4757 2005-11-21 10:13:19 -0800 2005-12-04 13:39:05 -0800 Call updateId() when inserting id attribute 5784_r1.patch text/plain 1101 mitz SW5kZXg6IFdlYkNvcmUva2h0bWwveG1sL2RvbV9lbGVtZW50aW1wbC5oCj09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KUkNT IGZpbGU6IC9jdnMvcm9vdC9XZWJDb3JlL2todG1sL3htbC9kb21fZWxlbWVudGltcGwuaCx2CnJl dHJpZXZpbmcgcmV2aXNpb24gMS42MwpkaWZmIC1wIC11IC1yMS42MyBXZWJDb3JlL2todG1sL3ht bC9kb21fZWxlbWVudGltcGwuaAotLS0gV2ViQ29yZS9raHRtbC94bWwvZG9tX2VsZW1lbnRpbXBs LmgJMjEgTm92IDIwMDUgMDQ6MDE6NTggLTAwMDAJMS42MworKysgV2ViQ29yZS9raHRtbC94bWwv ZG9tX2VsZW1lbnRpbXBsLmgJMjEgTm92IDIwMDUgMTg6MTU6MTggLTAwMDAKQEAgLTMxLDYgKzMx LDcgQEAKICNpbmNsdWRlICJtaXNjL3NoYXJlZC5oIgogI2luY2x1ZGUgImNzcy9jc3NfdmFsdWVp bXBsLmgiCiAjaW5jbHVkZSAiZG9tX3FuYW1lLmgiCisjaW5jbHVkZSAiaHRtbG5hbWVzLmgiCiAK ICNpZmRlZiBfX09CSkNfXwogI2RlZmluZSBpZCBpZF9BVk9JRF9LRVlXT1JECkBAIC0zMTYsOSAr MzE3LDExIEBAIHB1YmxpYzoKICAgICAvLyB1c2VkIGR1cmluZyBwYXJzaW5nOiBvbmx5IGluc2Vy dHMgaWYgbm90IGFscmVhZHkgdGhlcmUKICAgICAvLyBubyBlcnJvciBjaGVja2luZyEKICAgICB2 b2lkIGluc2VydEF0dHJpYnV0ZShBdHRyaWJ1dGVJbXBsKiBuZXdBdHRyaWJ1dGUpIHsKLSAgICAg ICAgaWYgKCFnZXRBdHRyaWJ1dGVJdGVtKG5ld0F0dHJpYnV0ZS0+bmFtZSgpKSkKKyAgICAgICAg aWYgKCFnZXRBdHRyaWJ1dGVJdGVtKG5ld0F0dHJpYnV0ZS0+bmFtZSgpKSkgeworICAgICAgICAg ICAgaWYgKGVsZW1lbnQgJiYgbmV3QXR0cmlidXRlLT5uYW1lKCkgPT0gSFRNTE5hbWVzOjppZEF0 dHIpCisgICAgICAgICAgICAgICAgZWxlbWVudC0+dXBkYXRlSWQobnVsbEF0b20sIG5ld0F0dHJp YnV0ZS0+dmFsdWUoKSk7CiAgICAgICAgICAgICBhZGRBdHRyaWJ1dGUobmV3QXR0cmlidXRlKTsK LSAgICAgICAgZWxzZQorICAgICAgICB9IGVsc2UKICAgICAgICAgICAgIG5ld0F0dHJpYnV0ZS0+ ZGVyZWYoKTsKICAgICB9CiAK 4947 2005-12-04 13:39:05 -0800 2005-12-05 06:30:39 -0800 Use setAttribute from htmlparser; add assert in insertAttribute 5784_r2.patch text/plain 3528 mitz SW5kZXg6IFdlYkNvcmUva2h0bWwvaHRtbC9odG1scGFyc2VyLmNwcAo9PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09ClJDUyBm aWxlOiAvY3ZzL3Jvb3QvV2ViQ29yZS9raHRtbC9odG1sL2h0bWxwYXJzZXIuY3BwLHYKcmV0cmll dmluZyByZXZpc2lvbiAxLjEyMwpkaWZmIC1wIC11IC1yMS4xMjMgV2ViQ29yZS9raHRtbC9odG1s L2h0bWxwYXJzZXIuY3BwCi0tLSBXZWJDb3JlL2todG1sL2h0bWwvaHRtbHBhcnNlci5jcHAJMSBE ZWMgMjAwNSAxMDozMjoxOSAtMDAwMAkxLjEyMworKysgV2ViQ29yZS9raHRtbC9odG1sL2h0bWxw YXJzZXIuY3BwCTQgRGVjIDIwMDUgMjE6MTY6NTYgLTAwMDAKQEAgLTMzOSwxNSArMzM5LDEzIEBA IGJvb2wgSFRNTFBhcnNlcjo6aGFuZGxlRXJyb3IoTm9kZUltcGwqIG4KICAgICAgICAgICAgICAg ICAgICAgLy8gd2UgaGF2ZSBhbm90aGVyIDxIVE1MPiBlbGVtZW50Li4uLiBhcHBseSBhdHRyaWJ1 dGVzIHRvIGV4aXN0aW5nIG9uZQogICAgICAgICAgICAgICAgICAgICAvLyBtYWtlIHN1cmUgd2Ug ZG9uJ3Qgb3ZlcndyaXRlIGFscmVhZHkgZXhpc3RpbmcgYXR0cmlidXRlcwogICAgICAgICAgICAg ICAgICAgICBOYW1lZEF0dHJNYXBJbXBsICptYXAgPSBzdGF0aWNfY2FzdDxFbGVtZW50SW1wbCo+ KG4pLT5hdHRyaWJ1dGVzKHRydWUpOwotICAgICAgICAgICAgICAgICAgICBOYW1lZEF0dHJNYXBJ bXBsICpibWFwID0gc3RhdGljX2Nhc3Q8RWxlbWVudEltcGwqPihkb2MoKS0+Zmlyc3RDaGlsZCgp KS0+YXR0cmlidXRlcyhmYWxzZSk7Ci0gICAgICAgICAgICAgICAgICAgIGJvb2wgY2hhbmdlZCA9 IGZhbHNlOworICAgICAgICAgICAgICAgICAgICBFbGVtZW50SW1wbCAqZXhpc3RpbmdIVE1MID0g c3RhdGljX2Nhc3Q8RWxlbWVudEltcGwqPihkb2MoKS0+Zmlyc3RDaGlsZCgpKTsKKyAgICAgICAg ICAgICAgICAgICAgTmFtZWRBdHRyTWFwSW1wbCAqYm1hcCA9IGV4aXN0aW5nSFRNTC0+YXR0cmli dXRlcyhmYWxzZSk7CiAgICAgICAgICAgICAgICAgICAgIGZvciAodW5zaWduZWQgbCA9IDA7IG1h cCAmJiBsIDwgbWFwLT5sZW5ndGgoKTsgKytsKSB7CiAgICAgICAgICAgICAgICAgICAgICAgICBB dHRyaWJ1dGVJbXBsKiBpdCA9IG1hcC0+YXR0cmlidXRlSXRlbShsKTsKLSAgICAgICAgICAgICAg ICAgICAgICAgIGNoYW5nZWQgPSAhYm1hcC0+Z2V0QXR0cmlidXRlSXRlbShpdC0+bmFtZSgpKTsK LSAgICAgICAgICAgICAgICAgICAgICAgIGJtYXAtPmluc2VydEF0dHJpYnV0ZShpdC0+Y2xvbmUo ZmFsc2UpKTsKKyAgICAgICAgICAgICAgICAgICAgICAgIGlmICghYm1hcC0+Z2V0QXR0cmlidXRl SXRlbShpdC0+bmFtZSgpKSkKKyAgICAgICAgICAgICAgICAgICAgICAgICAgICBleGlzdGluZ0hU TUwtPnNldEF0dHJpYnV0ZShpdC0+bmFtZSgpLCBpdC0+dmFsdWUoKSk7CiAgICAgICAgICAgICAg ICAgICAgIH0KLSAgICAgICAgICAgICAgICAgICAgaWYgKGNoYW5nZWQpCi0gICAgICAgICAgICAg ICAgICAgICAgICBkb2MoKS0+cmVjYWxjU3R5bGUoTm9kZUltcGw6OkluaGVyaXQpOwogICAgICAg ICAgICAgICAgIH0KICAgICAgICAgICAgICAgICByZXR1cm4gZmFsc2U7CiAgICAgICAgICAgICB9 CkBAIC0zNzYsMTUgKzM3NCwxMyBAQCBib29sIEhUTUxQYXJzZXI6OmhhbmRsZUVycm9yKE5vZGVJ bXBsKiBuCiAgICAgICAgICAgICAgICAgLy8gbWFrZSBzdXJlIHdlIGRvbid0IG92ZXJ3cml0ZSBh bHJlYWR5IGV4aXN0aW5nIGF0dHJpYnV0ZXMKICAgICAgICAgICAgICAgICAvLyBzb21lIHNpdGVz IHVzZSA8Ym9keSBiZ2NvbG9yPXJpZ2h0Y29sb3I+Li4uPGJvZHkgYmdjb2xvcj13cm9uZ2NvbG9y PgogICAgICAgICAgICAgICAgIE5hbWVkQXR0ck1hcEltcGwgKm1hcCA9IHN0YXRpY19jYXN0PEVs ZW1lbnRJbXBsKj4obiktPmF0dHJpYnV0ZXModHJ1ZSk7Ci0gICAgICAgICAgICAgICAgTmFtZWRB dHRyTWFwSW1wbCAqYm1hcCA9IGRvYygpLT5ib2R5KCktPmF0dHJpYnV0ZXMoZmFsc2UpOwotICAg ICAgICAgICAgICAgIGJvb2wgY2hhbmdlZCA9IGZhbHNlOworICAgICAgICAgICAgICAgIEVsZW1l bnRJbXBsICpleGlzdGluZ0JvZHkgPSBkb2MoKS0+Ym9keSgpOworICAgICAgICAgICAgICAgIE5h bWVkQXR0ck1hcEltcGwgKmJtYXAgPSBleGlzdGluZ0JvZHktPmF0dHJpYnV0ZXMoZmFsc2UpOwog ICAgICAgICAgICAgICAgIGZvciAodW5zaWduZWQgbCA9IDA7IG1hcCAmJiBsIDwgbWFwLT5sZW5n dGgoKTsgKytsKSB7CiAgICAgICAgICAgICAgICAgICAgIEF0dHJpYnV0ZUltcGwqIGl0ID0gbWFw LT5hdHRyaWJ1dGVJdGVtKGwpOwotICAgICAgICAgICAgICAgICAgICBjaGFuZ2VkID0gIWJtYXAt PmdldEF0dHJpYnV0ZUl0ZW0oaXQtPm5hbWUoKSk7Ci0gICAgICAgICAgICAgICAgICAgIGJtYXAt Pmluc2VydEF0dHJpYnV0ZShpdC0+Y2xvbmUoZmFsc2UpKTsKKyAgICAgICAgICAgICAgICAgICAg aWYgKCFibWFwLT5nZXRBdHRyaWJ1dGVJdGVtKGl0LT5uYW1lKCkpKQorICAgICAgICAgICAgICAg ICAgICAgICAgZXhpc3RpbmdCb2R5LT5zZXRBdHRyaWJ1dGUoaXQtPm5hbWUoKSwgaXQtPnZhbHVl KCkpOwogICAgICAgICAgICAgICAgIH0KLSAgICAgICAgICAgICAgICBpZiAoY2hhbmdlZCkKLSAg ICAgICAgICAgICAgICAgICAgZG9jKCktPnJlY2FsY1N0eWxlKE5vZGVJbXBsOjpJbmhlcml0KTsK ICAgICAgICAgICAgICAgICByZXR1cm4gZmFsc2U7CiAgICAgICAgICAgICB9CiAgICAgICAgICAg ICBlbHNlIGlmICghY3VycmVudC0+aXNEb2N1bWVudE5vZGUoKSkKSW5kZXg6IFdlYkNvcmUva2h0 bWwveG1sL2RvbV9lbGVtZW50aW1wbC5oCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KUkNTIGZpbGU6IC9jdnMvcm9vdC9X ZWJDb3JlL2todG1sL3htbC9kb21fZWxlbWVudGltcGwuaCx2CnJldHJpZXZpbmcgcmV2aXNpb24g MS42NApkaWZmIC1wIC11IC1yMS42NCBXZWJDb3JlL2todG1sL3htbC9kb21fZWxlbWVudGltcGwu aAotLS0gV2ViQ29yZS9raHRtbC94bWwvZG9tX2VsZW1lbnRpbXBsLmgJMSBEZWMgMjAwNSAxMDoz MjoyNyAtMDAwMAkxLjY0CisrKyBXZWJDb3JlL2todG1sL3htbC9kb21fZWxlbWVudGltcGwuaAk0 IERlYyAyMDA1IDIxOjE3OjAxIC0wMDAwCkBAIC0zMTYsNiArMzE2LDcgQEAgcHVibGljOgogICAg IC8vIHVzZWQgZHVyaW5nIHBhcnNpbmc6IG9ubHkgaW5zZXJ0cyBpZiBub3QgYWxyZWFkeSB0aGVy ZQogICAgIC8vIG5vIGVycm9yIGNoZWNraW5nIQogICAgIHZvaWQgaW5zZXJ0QXR0cmlidXRlKEF0 dHJpYnV0ZUltcGwqIG5ld0F0dHJpYnV0ZSkgeworICAgICAgICBhc3NlcnQoIWVsZW1lbnQpOwog ICAgICAgICBpZiAoIWdldEF0dHJpYnV0ZUl0ZW0obmV3QXR0cmlidXRlLT5uYW1lKCkpKQogICAg ICAgICAgICAgYWRkQXR0cmlidXRlKG5ld0F0dHJpYnV0ZSk7CiAgICAgICAgIGVsc2UK