Summary: | REGRESSION: dom/xhtml/level2/html//HTMLInputElement01.xhtml crashes | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||
Component: | DOM | Assignee: | Darin Adler <darin> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Critical | CC: | cdumez | ||||
Priority: | P1 | ||||||
Version: | 420+ | ||||||
Hardware: | Mac | ||||||
OS: | OS X 10.4 | ||||||
Attachments: |
|
Description
Eric Seidel (no email)
2005-12-15 01:29:45 PST
This is a reference counting problem in the XML tokenizer -- it doesn't do the same kinds of "reference current node" things that the HTML tokenizer does. Working on a patch. Created attachment 5093 [details]
keep current node ref'd in the XML tokenizer/parser as in the HTML parser
Comment on attachment 5093 [details]
keep current node ref'd in the XML tokenizer/parser as in the HTML parser
The spacing on
setCurrentNode
looks odd.
Read through the whole thing, but I'm just waking up, so I want to read through
it all again before I r+ it.
Comment on attachment 5093 [details]
keep current node ref'd in the XML tokenizer/parser as in the HTML parser
Reading through it a second time for real:
~XMLTokenizer() has strange spacing.
setCurrentNode() (already mentioned) has strange spacing.
If startElementImpl fails to add a node, it can just call stopParsing() or?
That woudl get rid of the need for your FIXMEs
while (m_currentNode->implicitNode())
- m_currentNode = m_currentNode->parentNode();
could be re-written to use a local, and avoid the (admittedly minimal) refcount
thrash.
I'm not sure why it's not OK to clear the parent node in this case:
+ if (NodeImpl* par = m_currentNode->parentNode())
+ setCurrentNode(par);
That code will never be reached (parsing would have aborted by now), but even
so, clearing m_currnetNode should be OK...
To solve your second FIXME in exitText, I think it's OK to stop parsing in
enterText in the failure case as well.
Otherwise looks fine. Darin should land this. r=me.
Comment on attachment 5093 [details] keep current node ref'd in the XML tokenizer/parser as in the HTML parser > ~XMLTokenizer() has strange spacing. Eric and I figured out there was nothing wrong here. > setCurrentNode() (already mentioned) has strange spacing. It looks like there are some tabs in there -- I'll fix that. > If startElementImpl fails to add a node, it can just call stopParsing()? > That would get rid of the need for your FIXMEs. > To solve your second FIXME in exitText, I think it's OK to stop parsing in > enterText in the failure case as well. Sure, it would be nice to fix those FIXMEs, but I'd prefer to not do that in this patch. I just wanted to make sure to record it. > I'm not sure why it's not OK to clear the parent node in this case: > > + if (NodeImpl* par = m_currentNode->parentNode()) > + setCurrentNode(par); Generally it's never correct to set the current node to 0; any future parsed nodes would not know where to go. But I hope we can find a way to make it so that will never happen -- not moving to the parent seems like it's almost never correct. Mass moving XML DOM bugs to the "DOM" Component. |