Bug 30049

Summary: Manipulating DOM from a script while parsing XHTML can cause a crash
Product: WebKit Reporter: chameleon <gessos.paul>
Component: DOMAssignee: Alexey Proskuryakov <ap>
Status: RESOLVED FIXED    
Severity: Major CC: ap, cdumez
Priority: P1 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
A whole web site which reproduce the crash
none
Static website which reproduce the crash
none
same test in zip format
none
reduced test case (will crash)
none
proposed fix darin: review+

Description chameleon 2009-10-03 13:54:11 PDT
Created attachment 40582 [details]
A whole web site which reproduce the crash

I attach a simplified website, programmed by me.
Website uses Javascript and XSLT 1.1.
Complete source code is available in attachment.

The attached website works fine on Firefox/Opera.
When load it with latest Safari, it crashes immediatelly.

So, I cannot provide more info.
Comment 1 Mark Rowe (bdash) 2009-10-03 22:36:43 PDT
Thanks for filing this bug report.  Is there any chance that you can provide your attached reproducible case in a format that would be easier to reproduce?  Test cases that require a web server are naturally more difficult to work with than a simple HTML file.
Comment 2 chameleon 2009-10-04 22:25:05 PDT
Created attachment 40612 [details]
Static website which reproduce the crash

PHP has only the header('.....'); dynamic.
All other content was static.
So I replace the attachment with a 100% static website.
Comment 3 Alexey Proskuryakov 2009-10-07 22:16:08 PDT
Created attachment 40847 [details]
same test in zip format

Confirmed with r48940.
Comment 4 Alexey Proskuryakov 2009-10-07 22:31:08 PDT
Created attachment 40850 [details]
reduced test case (will crash)
Comment 5 Alexey Proskuryakov 2009-10-07 22:35:50 PDT
The issue here is that an element is removed while it's still being parsed. I'm not sure what the right behavior would be here.
Comment 6 Alexey Proskuryakov 2009-10-07 22:36:45 PDT
<rdar://problem/7286002>
Comment 7 Alexey Proskuryakov 2009-10-26 12:49:22 PDT
Created attachment 41883 [details]
proposed fix
Comment 8 Darin Adler 2009-10-26 15:26:44 PDT
Comment on attachment 41883 [details]
proposed fix

> +        (WebCore::XMLTokenizer::setCurrentNode): Push the new node onto stack. If null is passed,
> +        then we're aborting; nuke the whole stack.

It seems strange to give setCurrentNode(0) this special behavior. Perhaps instead we could use a separate functions for this purpose. One could be called pushNode and the other could be called something else.

> +        (WebCore::XMLTokenizer::popCurrentNode): This is now called instead of setCurrentNode when
> +        exiting a node.

I'm not sure why the word "current" is needed in the name of this function.

r=me as is, but please consider getting rid of the two different meanings for setCurrentNode.
Comment 9 Alexey Proskuryakov 2009-10-26 16:13:53 PDT
Committed <http://trac.webkit.org/changeset/50110>.
Comment 10 Lucas Forschler 2019-02-06 09:04:09 PST
Mass moving XML DOM bugs to the "DOM" Component.