VERIFIED FIXED 5784
Assertion failure in DocumentImpl::removeElementById() (idCount > 0) 
https://bugs.webkit.org/show_bug.cgi?id=5784
Summary Assertion failure in DocumentImpl::removeElementById() (idCount > 0)
Alexey Proskuryakov
Reported 2005-11-20 09:16:49 PST
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'
Attachments
Reduced testcase (324 bytes, text/html)
2005-11-21 07:45 PST, mitz 		no flags
Details
Call updateId() when inserting id attribute (1.08 KB, patch)
2005-11-21 10:13 PST, mitz 		darin: review-
DetailsFormatted DiffDiff
Use setAttribute from htmlparser; add assert in insertAttribute (3.45 KB, patch)
2005-12-04 13:39 PST, mitz 		darin: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Alexey Proskuryakov
Comment 1 2005-11-20 21:58:10 PST
With current (November 21) ToT, the line numer is 842.
mitz
Comment 2 2005-11-21 07:45:36 PST
Created attachment 4756 [details] Reduced testcase The required ingredients for this bug are an <html> element with an id attribute, and evaluating document.documentElement before closing the window.
mitz
Comment 3 2005-11-21 08:38:23 PST
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.
mitz
Comment 4 2005-11-21 10:13:19 PST
Created attachment 4757 [details] 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.
Darin Adler
Comment 5 2005-11-22 19:39:20 PST
Comment on attachment 4757 [details] 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.
mitz
Comment 6 2005-12-04 13:39:05 PST
Created attachment 4947 [details] Use setAttribute from htmlparser; add assert in insertAttribute
Eric Seidel (no email)
Comment 7 2005-12-05 02:29:59 PST
Comment on attachment 4947 [details] 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.
Darin Adler
Comment 8 2005-12-05 06:30:39 PST
Comment on attachment 4947 [details] Use setAttribute from htmlparser; add assert in insertAttribute r=me
Note You need to log in before you can comment on or make changes to this bug.