Bug 5784

Summary: Assertion failure in DocumentImpl::removeElementById() (idCount > 0)
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: DOMAssignee: 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 Flags
Reduced testcase
none
Call updateId() when inserting id attribute
darin: review-
Use setAttribute from htmlparser; add assert in insertAttribute darin: review+

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