Summary: | Assertion failure in DocumentImpl::removeElementById() (idCount > 0) | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexey Proskuryakov <ap> | ||||||||
Component: | DOM | Assignee: | Darin Adler <darin> | ||||||||
Status: | VERIFIED FIXED | ||||||||||
Severity: | Normal | CC: | mitz | ||||||||
Priority: | P2 | ||||||||||
Version: | 420+ | ||||||||||
Hardware: | Mac | ||||||||||
OS: | OS X 10.4 | ||||||||||
URL: | http://nontroppo.org/wiki/PatchingStartCom/edit | ||||||||||
Attachments: |
|
Description
Alexey Proskuryakov
2005-11-20 09:16:49 PST
With current (November 21) ToT, the line numer is 842. 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.
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. 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.
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.
Created attachment 4947 [details]
Use setAttribute from htmlparser; add assert in insertAttribute
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.
Comment on attachment 4947 [details]
Use setAttribute from htmlparser; add assert in insertAttribute
r=me
|