Summary: | REGRESSION: style tag in body causes two head elements to appear in the DOM | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Kilzer (:ddkilzer) <ddkilzer> | ||||||
Component: | DOM | Assignee: | David Kilzer (:ddkilzer) <ddkilzer> | ||||||
Status: | VERIFIED FIXED | ||||||||
Severity: | Normal | Keywords: | EasyFix, Regression | ||||||
Priority: | P1 | ||||||||
Version: | 420+ | ||||||||
Hardware: | Mac | ||||||||
OS: | OS X 10.4 | ||||||||
URL: | http://software.hixie.ch/utilities/js/live-dom-viewer/?%3Chtml%3E%3Chead%3E%3C/head%3E%3Cbody%3E%3Cstyle%3E%3C/style%3E%3C/body%3E%3C/html%3E%0A | ||||||||
Attachments: |
|
Description
David Kilzer (:ddkilzer)
2006-02-16 21:48:05 PST
Checking the nightly builds, this bug is as old as the first nightly, WebKit-CVS-2005-10-01 03:27:01 GMT.dmg, so it's been around a while. The bug is in HTMLParser::headCreateErrorCheck. That function should always return false, and when it decides to create a head element it needs to store a pointer to it in "head". See formCreateErrorCheck and mapCreateErrorCheck for similar logic. The only part I'm not entirely sure about is the "current->localName() == htmlTag" clause. That means that you're allowed to create a second head if you're inside an <html> element; would be good to test that to see if it's the behavior we want. (In reply to comment #2) > The bug is in HTMLParser::headCreateErrorCheck. That function should always > return false, and when it decides to create a head element it needs to store a > pointer to it in "head". What about HTMLParser::createHead() and the duplicate head-creation code under "Step 2" in HTMLParser::handleError()? With Darin's proposed fix: bool HTMLParser::headCreateErrorCheck(Token* t, RefPtr<NodeImpl>& result) { if (!head || current->localName() == htmlTag) { head = new HTMLHeadElementImpl(document); result = head; } return false; } We must make sure that no more than one <title> and one <base> tag appear in the <head>. (This is not the case if only the above change is made.) Firefox 1.5 currently prevents multiple <title> tags, but it doesn't prevent multiple <base> tags. I haven't surveyed MSIE 6 yet. Regarding Comment #3, there seems like there might be a lot of unused code in the handleError() method, but I don't grok the HTML parser well enough (yet) to understand when all that code may be used. (In reply to comment #4) > bool HTMLParser::headCreateErrorCheck(Token* t, RefPtr<NodeImpl>& result) > { > if (!head || current->localName() == htmlTag) { > head = new HTMLHeadElementImpl(document); > result = head; > } > return false; > } This change also causes an HTML document like this to crash WebKit: <<head><style></style></head> I don't think this change will be quite as easy as Darin first expected. This fix won't crash with the test case from Comment #5, but it fails about 375 LayoutTests. More investigation will be needed. (I didn't try running LayoutTests after applying the change from Comment #5, yet.) bool HTMLParser::headCreateErrorCheck(Token* t, RefPtr<NodeImpl>& result) { createHead(); return false; } NOTE: The multiple <title> and <base> tags in <head> issue also occurs on Safari 2.0.3 on 10.4.5, so this isn't a regression. I will file a new bug (or bugs) for this issue. Darin's fix causes a crash (see Comment #5) but only fails one LayoutTest: editing/inserting/editing-empty-divs. I believe I have a fix for this. Also had to modify HTMLParser::parseToken() so that 'head' behaved the same way as 'map' and 'form'. Need a test case, though. Created attachment 6676 [details]
Patch v1
Patch v1. Maciej commented on IRC that he wanted to see my next patch, so I'm giving him first dibs on the review. :) I assume I'll get a review- because there is no layout test, but I'm looking for feedback on the patch and on how to implement a layout test.
Regarding a layout test, it seems to me that a layoutTestController.dumpDOMTree() method would be really handy for a test like this. Is this worth implementing, or is there a preferred method (such as JavaScript DOM inspection) to test that the DOM has but a single HEAD node?
FWIW, the crashes I found from previous attempts at patches (Comment #5 and Comment #7) were because HTMLParser::parseToken() was not modified to set "head = 0". In those cases, the new head node was never inserted into the tree and subsequently caused a double-free error later in the parsing when an attempt was made to add a child node to it. (In reply to comment #9) > Created an attachment (id=6676) [edit] > Patch v1 I should also note that this patch passes all current layout tests as of r12944 (except for the ones that have known issues like some of the new http tests--failures are nearly identical to buildbot--and the timezone issue with fast/js/date-constructor). (In reply to comment #9) > Maciej commented on IRC that he wanted to see my next patch, so I'm > giving him first dibs on the review. :) I assume I'll get a review- because > there is no layout test, but I'm looking for feedback on the patch and on how > to implement a layout test. The patch looks perfect to me. And I also think it's straightforward enough that it doesn't need to be Maciej who reviews it. > Regarding a layout test, it seems to me that a > layoutTestController.dumpDOMTree() method would be really handy for a test like > this. Is this worth implementing, or is there a preferred method (such as > JavaScript DOM inspection) to test that the DOM has but a single HEAD node? It's very easy to find out how many <head> elements there are. document.getElementsByTagName("head").length() So I don't think you need a new layoutTestController feature. Given the other code to special-case <head>, I'd like to see layout tests that check all the different code paths. A great way to do that is to set a breakpoint on each one and make sure every one gets tickled. I didn't mean I wanted to be the one to review necessarily, I just wanted to encourage you to send more patches. :-) The patch looks good to me too, marking r- for lack of test case. Created attachment 6721 [details]
Patch v2
Changes since Patch v1:
- Added a test that uses an iframe to load various HTML documents that test each code path in htmlparser.cpp.
Comment on attachment 6721 [details]
Patch v2
r=me
Verified fixed as of r13202 (locally built). |