chrome.dll!WebCore::firstPositionInNode ReadAV@NULL (b1c286808dfae0c5027c71627f71b962)

• Component: DOM
• Status: RESOLVED FIXED

[Berend-Jan Wever]

Created attachment 99631 [details]
Repro

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

Repro:
<script>
  var oSelection = window.getSelection();
  document.removeChild(document.documentElement);
  oSelection.collapse(document);
  oSelection.modify("move","backward","documentboundary");
</script>

id:             chrome.dll!WebCore::firstPositionInNode ReadAV@NULL (b1c286808dfae0c5027c71627f71b962)
description:    Attempt to read from unallocated NULL pointer+0x24 in chrome.dll!WebCore::firstPositionInNode
stack:          chrome.dll!WebCore::firstPositionInNode
                chrome.dll!WebCore::startOfDocument
                chrome.dll!WebCore::startOfDocument
                chrome.dll!WebCore::FrameSelection::modifyMovingBackward
                chrome.dll!WebCore::FrameSelection::modify
                chrome.dll!WebCore::DOMSelection::modify
                chrome.dll!WebCore::DOMSelectionInternal::modifyCallback
                ...

// 1) In the repro, the HTML element has been removed from the document.
// 2) Next, the entire document is selected and moved, causing the code to try to determine the start of the document:
// 3) startOfDocument assumes a document ALWAYS has a documentElement in webkit\source\webcore\editing\visible_units.cpp:989:
VisiblePosition startOfDocument(const Node* node)
{
    if (!node)
        return VisiblePosition();
    
    return VisiblePosition(firstPositionInNode(node->document()->documentElement()), DOWNSTREAM);
}
// 4) anchorNode is NULL in webkit\source\webcore\dom\position.h:270, leading to the crash:
inline Position firstPositionInNode(Node* anchorNode)
{
    if (anchorNode->isTextNode())
        return Position(anchorNode, 0, Position::PositionIsOffsetInAnchor);
    return Position(anchorNode, Position::PositionIsBeforeChildren);
}

[Kulanthaivel Palanichamy]

Created attachment 101491 [details]
Proposed patch.

[Eric Seidel (no email)]

Comment on attachment 101491 [details]
Proposed patch.

I wonder if the !documentElement() part of the check is necessary.  Does firstPositionInNode() handle NULL?

[Ryosuke Niwa]

(In reply to comment #2)
> (From update of attachment 101491 [details])
> I wonder if the !documentElement() part of the check is necessary.

Yes. because

> Does firstPositionInNode() handle NULL?

No.

[WebKit Review Bot]

Comment on attachment 101491 [details]
Proposed patch.

Clearing flags on attachment: 101491

Committed r91395: <http://trac.webkit.org/changeset/91395>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.