Bug 65015 - If Range::insertNode is passed an empty document fragment, it creates a broken DOM tree
Summary: If Range::insertNode is passed an empty document fragment, it creates a broke...
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Darin Adler
Depends on:
Reported: 2011-07-22 00:41 PDT by Berend-Jan Wever
Modified: 2019-02-06 09:03 PST (History)
5 users (show)

See Also:

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+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Berend-Jan Wever 2011-07-22 00:41:28 PDT
Created attachment 101702 [details]

Chromium: https://code.google.com/p/chromium/issues/detail?id=90147

<body onload="go()"></body>
  function go() {
    var oBodyImported = document.importNode(document.body,false);
    var oRange1 = document.createRange();
    var oRange2 = document.createRange();
    var oDocumentFragment = document.createDocumentFragment();

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]
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.