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:

DOM

Assignee:

Darin Adler <darin>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, cdumez, darin, eric, rniwa

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Repro	none
Patch	ap: review+

Description Berend-Jan Wever 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.

Comment 1 Darin Adler 2011-07-22 18:17:10 PDT

What is a NULL node?

Comment 2 Darin Adler 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.

Comment 3 Darin Adler 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.

Comment 4 Berend-Jan Wever 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.

Comment 5 Darin Adler 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.

Comment 6 Darin Adler 2011-08-20 10:55:32 PDT

Created attachment 104621 [details]
Patch

Comment 7 Darin Adler 2011-08-20 11:12:01 PDT

Committed r93481: <http://trac.webkit.org/changeset/93481>

Comment 8 Lucas Forschler 2019-02-06 09:03:05 PST

Mass moving XML DOM bugs to the "DOM" Component.