Bug 65015

Summary: If Range::insertNode is passed an empty document fragment, it creates a broken DOM tree
Product: WebKit Reporter: Berend-Jan Wever <skylined>
Component: DOMAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, cdumez, darin, eric, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Repro
none
Patch ap: review+

Berend-Jan Wever
Reported 2011-07-22 00:41:28 PDT
Created attachment 101702 [details] Repro Chromium: https://code.google.com/p/chromium/issues/detail?id=90147 Repro: <body onload="go()"></body> <script> function go() { var oBodyImported = document.importNode(document.body,false); document.body.appendChild(oBodyImported); var oRange1 = document.createRange(); var oRange2 = document.createRange(); document.body=document.activeElement; var oDocumentFragment = document.createDocumentFragment(); oRange1.setStartBefore(oBodyImported); oRange1.insertNode(oDocumentFragment); oRange2.setStart(document.body,0); oRange2.surroundContents(oBodyImported); ''+oRange1; } </script> The range oRange1 ends up containing a NULL node, among valid nodes. This is probably not supposed to happen, as some of the range's methods that access the nodes do not handle this correctly. In the repro, we end up executing this code: String Range::toString(ExceptionCode& ec) const { if (!m_start.container()) { ec = INVALID_STATE_ERR; return String(); } StringBuilder builder; Node* pastLast = pastLastNode(); for (Node* n = firstNode(); n != pastLast; n = n->traverseNextNode()) { if (n->nodeType() == Node::TEXT_NODE || n->nodeType() == Node::CDATA_SECTION_NODE) { String data = static_cast<CharacterData*>(n)->data(); int length = data.length(); int start = (n == m_start.container()) ? min(max(0, m_start.offset()), length) : 0; int end = (n == m_end.container()) ? min(max(start, m_end.offset()), length) : length; builder.append(data.characters() + start, end - start); } } return builder.toString(); } "n" goes through the nodes, which includes the NULL node. This causes a NULL ptr when the code tries to access n->nodeType(). The fact that the code loops through the nodes using n->traverseNextNode() signals that NULL nodes are probably not supposed to be part of a range.
Attachments
Repro (555 bytes, text/html)
2011-07-22 00:41 PDT, Berend-Jan Wever 		no flags
Details
Patch (5.60 KB, patch)
2011-08-20 10:55 PDT, Darin Adler 		ap: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Darin Adler
Comment 1 2011-07-22 18:17:10 PDT
What is a NULL node?
Darin Adler
Comment 2 2011-07-22 18:18:12 PDT
I think I understand. The issue is not with ranges. The issue is that insertNode does the wrong thing when passed an empty document fragment.
Darin Adler
Comment 3 2011-07-22 18:26:47 PDT
I think all that’s needed to fix this bug is code to return from Range::insertNode function and do no further work if newNode->firstChild() is zero inside the DOCUMENT_FRAGMENT_NODE section of the code.
Berend-Jan Wever
Comment 4 2011-07-26 04:32:24 PDT
@Darin; yes, with "NULL node" I meant a NULL ptr where a ptr to a node is expected. You analysis sounds correct to me, but I don't know the code, so don't take my word for it.
Darin Adler
Comment 5 2011-07-26 15:05:09 PDT
I think I’ll fix this myself, but if someone else wants to tackle it first, they are welcome to.
Darin Adler
Comment 6 2011-08-20 10:55:32 PDT
Created attachment 104621 [details] Patch
Darin Adler
Comment 7 2011-08-20 11:12:01 PDT
Committed r93481: <http://trac.webkit.org/changeset/93481>
Lucas Forschler
Comment 8 2019-02-06 09:03:05 PST
Mass moving XML DOM bugs to the "DOM" Component.
Note You need to log in before you can comment on or make changes to this bug.