Bug 5784

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:

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+

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-	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
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.