Bug 31680 – WebCore::Document::updateLayoutIgnorePendingStylesheets NULL pointer

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Berend-Jan Wever]

Created attachment 43518 [details]
Repro

The following HTML triggers a NULL pointer in chrome!WebCore::Document::updateLayoutIgnorePendingStylesheets:

<SCRIPT>
  sel = window.getSelection();
  doc = document.implementation.createDocumentType('c');
  sel.setBaseAndExtent(doc);
</SCRIPT>

Relevant call stack:
WebCore::Document::updateLayoutIgnorePendingStylesheets(void)+0x4
WebCore::VisiblePosition::canonicalPosition(class WebCore::Position * position = 0x0012f184)+0x3a
WebCore::VisiblePosition::init(class WebCore::Position * position = 0x0012f184, WebCore::EAffinity affinity = DOWNSTREAM (1))+0x25
WebCore::VisiblePosition::VisiblePosition(class WebCore::Node * node = 0x05639fc0, int offset = 715827888, WebCore::EAffinity affinity = DOWNSTREAM (1))+0x46
WebCore::DOMSelection::setBaseAndExtent(class WebCore::Node * baseNode = 0x05639fc0, int baseOffset = 715827888, class WebCore::Node * extentNode = 0x00000000, int extentOffset = 429496759, int * ec = 0x0012f204)+0x39
WebCore::DOMSelectionInternal::setBaseAndExtentCallback(class v8::Arguments * args = 0x0112f254)+0x180

[Berend-Jan Wever]

Added repro URL

[Berend-Jan Wever]

Fix title

[Hajime Morrita]

Created attachment 51877 [details]
patch v0

[Hajime Morrita]

Instead of just guarding null access, this change make DocumentType to 
have non-null m_document.  
This should be more safe to guarantee non-null m_document than just gurding null access to it.
This is because DocumenType is a subclass of Node and certain part of codebase assumes
Node::m_document non-null.

Comment on attachment 51877 [details]
patch v0

<http://www.w3.org/TR/2004/REC-DOM-Level-3-Core-20040407/core.html#Level-2-Core-DOM-createDocType> specifies that createDocumentType() should return a node with a null ownerDocument. Unless I’m missing something, this change contradicts the specification.

[Alexey Proskuryakov]

Don't we have a regression test verifying that a newly created DocumentType node has no document? If we don't, please add one.

[Hajime Morrita]

Created attachment 51880 [details]
v1; guarded null document

[Hajime Morrita]

Mitz, thank you for reviewing really quickly!

> specifies that createDocumentType() should return a node with a null
> ownerDocument. Unless I’m missing something, this change contradicts the
> specification.
You are right.
I updated the patch that just guards null document.

@ap
>Don't we have a regression test verifying that a newly created DocumentType
>node has no document? If we don't, please add one.
The last patch even broke the regression on fast/. I tested only editing/.
I'm so sorry about that... 
Updated patch certainly passes all test, as expected.

[Alexey Proskuryakov]

Comment on attachment 51880 [details]
v1; guarded null document

This patch looks correct to me.

But it seems wrong that such a node even ends up in Position. Perhaps it would be a better fix to ensure that this doesn't happen, I'm not sure. The place to make the check would be setBaseAndExtent(), and other functions in DOMSelection may need checks (and regression tests), too.

[Darin Adler]

Comment on attachment 51880 [details]
v1; guarded null document

The fact that DocumentType nodes have no document stinks. The question is where to guard against these types of nodes. To me, canonicalPosition seems a bit low-level a place to be doing the check. I worry there are other leaf functions that lack this. It might be better to guard this incoming at places where nodes are passed in.

r=me, though

[Hajime Morrita]

Created attachment 51994 [details]
v2; add a guard on setBaseAndExtent()

[Hajime Morrita]

darin, ap, thank you for reviewing!

>But it seems wrong that such a node even ends up in Position. Perhaps it would
>be a better fix to ensure that this doesn't happen, I'm not sure. The place to
>make the check would be setBaseAndExtent(), and other functions in DOMSelection
>may need checks (and regression tests), too.

>The fact that DocumentType nodes have no document stinks. The question is where
>to guard against these types of nodes. To me, canonicalPosition seems a bit
>low-level a place to be doing the check. I worry there are other leaf functions
>that lack this. It might be better to guard this incoming at places where nodes
are passed in.

I agreed these concern and added a guard on setBaseAndExtent().
A guard on canonicalPosition() has kept as is because 
VibiblePosition is used many places on editing area and 
It looks safe to guard it anyway.

[Hajime Morrita]

As ap mentioned before, ownerless node lead crashes on other DOMSelection APIs.
I filed the bug on https://bugs.webkit.org/show_bug.cgi?id=36800 and 
I'll submit regressions for that after this patch landed.

[Alexey Proskuryakov]

Comment on attachment 51994 [details]
v2; add a guard on setBaseAndExtent()

+        The fix also adds a null guard for canonicalPosition() just in case.

We don't usually add null checks just in case, especially in performance sensitive code. These can confuse people looking at the code to think that this can legitimately happen.

You could add an ASSERT - it would be useless for catching errors, because we crash soon enough anyway, but it would serve as documentation that nodes are supposed to have a non-null document here.

+    // Because we don't know how to "select" ownerless nodes, we take them as null.

I was going to suggest raising an exception instead (probably INVALID_ACCESS_ERR). I'm not sure if that's a good idea. But it made me think about nodes from different documents. Is there a check somewhere that we don't set the selection base and extent to nodes from another document?

+It is OK not to crash.

A better way to say this would be "PASSED if didn't crash".

[Hajime Morrita]

Created attachment 52021 [details]
v3; used asssertion, added null checks more; enhanced test

[Hajime Morrita]

ap, thank you for reviewing again! I updated the patch.

> We don't usually add null checks just in case, especially in performance
> sensitive code. These can confuse people looking at the code to think that this
> can legitimately happen.
> 
> You could add an ASSERT - it would be useless for catching errors, because we
> crash soon enough anyway, but it would serve as documentation that nodes are
> supposed to have a non-null document here.
Agreed. I replaced null check with ASSERT()

> 
> I was going to suggest raising an exception instead (probably
> INVALID_ACCESS_ERR). I'm not sure if that's a good idea.
Fixed.

> But it made me think
> about nodes from different documents. Is there a check somewhere that we don't
> set the selection base and extent to nodes from another document?
For setBaseAndExtent(), there are 2 patterns of foreign document.
And both somewhat work.

Pattern 1: owner document of selection and of argument node differ
document of argument node is used. so selection of argument node's document change.

Pattern 2: owner document of baseNode  and of extentNode differ:
extentNode is replaced by baseNode.

> +It is OK not to crash.
> 
> A better way to say this would be "PASSED if didn't crash".
Fixed.

This fix also cares Bug 36800 because both has same root cause and 
I think it is better to fix them at single commit. for change cleanness.

[Darin Adler]

Comment on attachment 52021 [details]
v3; used asssertion, added null checks more; enhanced test

Alexey pointed out what we should really be doing. This should not just be about null checking.

> +    if (node && !node->document()) {
> +        ec = INVALID_ACCESS_ERR;
> +        return;
> +    }

Instead of checking for null we should check that the node is in the document of m_frame. It's not OK to set this to a node in another document.

> +    if ((baseNode && !baseNode->document()) 
> +        || (extentNode && !extentNode->document())) {
> +        // We don't know how to "select" ownerless nodes.
> +        ec = INVALID_ACCESS_ERR;
> +        return;
> +    }

Same comment.

> +    if (node && !node->document()) {
> +        ec = INVALID_ACCESS_ERR;
> +        return;
> +    }

Same comment.

> +    if (!node->document()) {
> +        ec = INVALID_ACCESS_ERR;
> +        return;
> +    }

Ditto.

> -    if (!n || selection->isNone())
> +    if (!n || !n->document() || selection->isNone())
>          return false;

Again.

> +    if (!n->document()) {
> +        ec = INVALID_ACCESS_ERR;
> +        return;
> +    }

Again.

[Alexey Proskuryakov]

A style nit: there is no need to split short conditions like this into two lines.

> +    if ((baseNode && !baseNode->document()) 
> +        || (extentNode && !extentNode->document())) {

I'm very glad that you found and fixed many more problematic cases, besides the one originally reported here.

[Hajime Morrita]

Created attachment 52127 [details]
v4; Added redirects for some methods, added tests for that

[Hajime Morrita]

Darin, ap, thank you for reviewing and for your suggestion!
I updated the patch.

> (From update of attachment 52021 [details])
> Alexey pointed out what we should really be doing. This should not just be
> about null checking.
> (snip)
> Instead of checking for null we should check that the node is in the document
> of m_frame. It's not OK to set this to a node in another document.
Requiring m_frame->document() == node->document()  is too strict and
Doing so will break following tests: 

- editing/selection/4960137.html
- editing/selection/drag-in-iframe.html

These tests expect DOMSelection::setBaseAndExtent() to redirect operations to owner 
document of the argument node, instead of the document of the DOMSelection. 
Although these redirection was done inside SelectionController::setSelection(), 
This change do it inside DOMSelection to clarify intension.
This change also redirect other DOMSelection API with Node as an argument for consistency.
(And added DOMSelection::findControllerFor() to choose the destination of  the redirection.)

I hope I would remove code for implicit redirection from SelectionController and 
allow SelectionController to assume its operation closed inside the single document.
But SelectionController is called from many place (especially editing) and 
such cleanup looks difficult at this time. So This change keeps it as is.

[Darin Adler]

Comment on attachment 52127 [details]
v4; Added redirects for some methods, added tests for that

> +    SelectionController* controller = findControllerFor(node);
> +    if (!controller) {
> +        ec = INVALID_ACCESS_ERR;
> +        return;
> +    }
> +
> +    controller->moveTo(VisiblePosition(node, offset, DOWNSTREAM));

Is this the behavior we want or not? You are making a call on a particular window's selection object. Should this affect the selection in another window? Does any other browser implement this? What does that browser do? Is there any documentation for this? What does the documentation say? Does the HTML5 specification or another W3C specification mention this? What does it say?

[Darin Adler]

(In reply to comment #21)
> Is this the behavior we want or not? You are making a call on a particular
> window's selection object. Should this affect the selection in another window?
> Does any other browser implement this? What does that browser do? Is there any
> documentation for this? What does the documentation say? Does the HTML5
> specification or another W3C specification mention this? What does it say?

You mentioned two specific regression tests, but I think we need a little more research than that.

And where I said "You" I meant the JavaScript code, not you specifically ;-)

[Hajime Morrita]

Darin, thank you for reviewing.
And I'm sorry for my short of attention to the standard compatibility.
I updated the patch.

> Does the HTML5
> specification or another W3C specification mention this? What does it say?
HTML5 says that we should throw exceptions for nodes from foreign documents, as you mentioned.
http://www.whatwg.org/specs/web-apps/current-work/#selection

> Does any other browser implement this? What does that browser do? Is there any
> documentation for this? What does the documentation say? 
Firefox accepts foreign nodes. But its behavior is different from us.
In Firefox, selection of certain window (or document) can hold nodes from another document.
But rendering doesn't reflect it.
For example, when selection.collapse(foreignNode, 0), selection.anchorNode is set to foreignNode.
with nothing selected(highlighted) on the foreign document (nor the document of the selection).
This behavior is incompatible to HTML5 standard,
It is also hard to follow for WebKit due to our VisiblePosition design
that requires the valid nodes of selection's own document.

IE doesn't support selection API.

Updated patch goes between Firefox and HTML5 spec.
The patch just ignores foreign node, without throwing an exception.
Doing so, applications will keep running somehow even if they assume Firefox model, 
in contrast to throwing exception that will stop the program.

[Hajime Morrita]

Created attachment 52279 [details]
v5; took the feedback to respect compatibiity

[Darin Adler]

Comment on attachment 52279 [details]
v5; took the feedback to respect compatibiity

> -    if (!n || selection->isNone())
> +    if (!n || m_frame->document() != n->document() || selection->isNone())
>          return false;

Why did you choose not to use your isValidForPosition helper function here?

> +bool DOMSelection::isValidForPosition(Node* node) const
> +{
> +    ASSERT(m_frame);
> +    if (!node)
> +        return true;
> +    return (node->document() == m_frame->document());
> +}

Extra parentheses here.

r=me

Lets send the feedback to the HTML5 folks about not the need to not raise exceptions in these cases for compatibility.

[Hajime Morrita]

Darin, thank you for reviewing!

(In reply to comment #25)
> (From update of attachment 52279 [details])
> > -    if (!n || selection->isNone())
> > +    if (!n || m_frame->document() != n->document() || selection->isNone())
> >          return false;
> 
> Why did you choose not to use your isValidForPosition helper function here?
This is because isValidForPosition() returns true even if node is null.

> Lets send the feedback to the HTML5 folks about not the need to not raise
> exceptions in these cases for compatibility.
OK, I'll post the issue to [e-mail].

[Kent Tamura]

> > +    return (node->document() == m_frame->document());
> > +}
> 
> Extra parentheses here.

I'll land this patch with the above change.

[Kent Tamura]

Committed r56962: <http://trac.webkit.org/changeset/56962>

[WebKit Review Bot]

http://trac.webkit.org/changeset/56962 might have broken Leopard Intel Release (Tests)

[Kent Tamura]

Committed another test change for this.
http://trac.webkit.org/changeset/56990