Bug 5784 - Assertion failure in DocumentImpl::removeElementById() (idCount > 0)
Summary: Assertion failure in DocumentImpl::removeElementById() (idCount > 0)
Status: VERIFIED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Darin Adler
URL: http://nontroppo.org/wiki/PatchingSta...
Keywords:
Depends on:
Blocks:
 
Reported: 2005-11-20 09:16 PST by Alexey Proskuryakov
Modified: 2005-12-26 14:08 PST (History)
1 user (show)

See Also:


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-
Details | Formatted Diff | Diff
Use setAttribute from htmlparser; add assert in insertAttribute (3.45 KB, patch)
2005-12-04 13:39 PST, mitz
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 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'
Comment 1 Alexey Proskuryakov 2005-11-20 21:58:10 PST
With current (November 21) ToT, the line numer is 842.
Comment 2 mitz 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.
Comment 3 mitz 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.
Comment 4 mitz 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.
Comment 5 Darin Adler 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.
Comment 6 mitz 2005-12-04 13:39:05 PST
Created attachment 4947 [details]
Use setAttribute from htmlparser; add assert in insertAttribute
Comment 7 Eric Seidel (no email) 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.
Comment 8 Darin Adler 2005-12-05 06:30:39 PST
Comment on attachment 4947 [details]
Use setAttribute from htmlparser; add assert in insertAttribute

r=me