| Summary: | [WK1] Null dereference loading Blink layout test fast/parser/xhtml-dom-character-data-modified-crash.html | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jon Honeycutt <jhoneycutt> | ||||||||||||
| Component: | DOM | Assignee: | Jiewen Tan <jiewen_tan> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | ap, cdumez, commit-queue, jhoneycutt, jiewen_tan, rniwa, webkit-bug-importer | ||||||||||||
| Priority: | P2 | Keywords: | BlinkMergeCandidate, HasReduction, InRadar | ||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Jon Honeycutt
2015-09-22 16:50:10 PDT
Created attachment 261780 [details]
subresource
Created attachment 262214 [details]
Patch
Comment on attachment 262214 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262214&action=review > Source/WebCore/xml/parser/XMLDocumentParser.cpp:165 > Vector<xmlChar> empty; > m_bufferedText.swap(empty); Not sure I understand the value of this idiom over simpler ways of writing it, like this: m_bufferedText = { }; > Source/WebCore/xml/parser/XMLDocumentParser.cpp:170 > + // Mutation event handlers executed by appendData() might detach this parser. > + return !isStopped(); This comment is super-mysterious. We make this statement, but don’t establish its relevance, nor explain what the return value of the function means. We should start by adding a comment somewhere that explains the meaning of the return value; I think it returns false if we should stop processing data. I think that’s a bit strange. In fact, why don’t the callers just check isStopped themselves? Why do we need a return value? Then the comment here would say something like this: // We need to check again whether the parser is stopped, since mutation // event handlers executed by appendData might have detached this parser. > Source/WebCore/xml/parser/XMLDocumentParser.cpp:201 > + // Do not bail out if in a stopped state, but notify document that > + // parsing has finished. This comment is confusing. I think it’s referring to not looking at the return value of updateLeafTextNode, but the phrase “notify document that parsing has finished” seems to refer to code way further down in the function. I suggest just omitting the comment unless there’s something useful to say. Maybe: // No need to bail out if the parser is in a stopped state because ... where you fill in the reason. But I think just omitting the comment is better than what you have here. Created attachment 262279 [details]
Patch
Are we still expected to dispatch synchronous mutation events here? I thought that this was going away. See bug 81920, bug 85161, bug 68729, bug 64532. Created attachment 262336 [details]
Patch
(In reply to comment #6) > Are we still expected to dispatch synchronous mutation events here? I > thought that this was going away. > > See bug 81920, bug 85161, bug 68729, bug 64532. To me it would seem a bit early to drop DOMCharacterDataModified. Even though, it is being phased out, Chrome still supports it for example (although they have a UseCounter on it). (In reply to comment #6) > Are we still expected to dispatch synchronous mutation events here? I > thought that this was going away. > > See bug 81920, bug 85161, bug 68729, bug 64532. That I don't know. What's your opinion? I will CC Jon as well. I expect that someone else CC'ed on the bug will know more about these discussions than myself. Comment on attachment 262336 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262336&action=review > Source/WebCore/xml/parser/XMLDocumentParser.cpp:156 > -void XMLDocumentParser::exitText() > +bool XMLDocumentParser::updateLeafTextNode() Some day we should return and fix this confusing idiom. It’s not at all clear what the return value means, nor does it seem that callers need this function to do the checks for them. The caller could just check isStopped(). Comment on attachment 262336 [details] Patch Clearing flags on attachment: 262336 Committed r190760: <http://trac.webkit.org/changeset/190760> All reviewed patches have been landed. Closing bug. Mass moving XML DOM bugs to the "DOM" Component. |