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
Added repro URL
Fix title
Created attachment 51877 [details] patch v0
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.
Don't we have a regression test verifying that a newly created DocumentType node has no document? If we don't, please add one.
Created attachment 51880 [details] v1; guarded null document
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.
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.
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
Created attachment 51994 [details] v2; add a guard on setBaseAndExtent()
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.
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.
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".
Created attachment 52021 [details] v3; used asssertion, added null checks more; enhanced test
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.
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.
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.
Created attachment 52127 [details] v4; Added redirects for some methods, added tests for that
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.
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?
(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 ;-)
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.
Created attachment 52279 [details] v5; took the feedback to respect compatibiity
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.
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 whatwg@whatwg.org.
> > + return (node->document() == m_frame->document()); > > +} > > Extra parentheses here. I'll land this patch with the above change.
Committed r56962: <http://trac.webkit.org/changeset/56962>
http://trac.webkit.org/changeset/56962 might have broken Leopard Intel Release (Tests)
Committed another test change for this. http://trac.webkit.org/changeset/56990