366944 - Range.surroundContents doesn't check node type early enough

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Alex Vincent [:WeirdAl]]

Attachment: XUL testcase: DocumentFragment

DOM 2 Range specifies throwing INVALID_NODE_TYPE_ERR if the first argument of surroundContents is an Attr, Entity, DocumentType, Notation, Document, or DocumentFragment node.  We do that, yes, but we do it after the range has acted on the document.

Comment 1

[Alex Vincent [:WeirdAl]]

Attachment: XUL testcase: Comment

We fired another error late, HIERARCHY_REQUEST_ERR, for nodes that can't have children.

Comment 2

[Alex Vincent [:WeirdAl]]

Attachment: patch, v1

This patch is pretty simple, but it says nothing about other code in nsRange failing to check parameters beforehand.  I started reading through the code, and discovered bug 366946 as well.  It would be a good idea to compare the code to the spec for errors that should be thrown before modifying the document.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 251426 [details] [diff] [review]
patch, v1

Does the spec say to throw these errors for these node types? Otherwise always throw NS_ERROR_DOM_RANGE_INVALID_NODE_TYPE_ERR IMHO.

And make it a white-list instead of a black-list, that should make it shorter

Comment 4

[Alex Vincent [:WeirdAl]]

For INVALID_NODE_TYPE_ERR, the DOM Range spec explicitly says those node types are not acceptable.  Note the URL field for a reference.

For HIERARCHY_REQUEST_ERR, these are nodes that can't have child nodes anyway.  All four listed there forward their AppendChild calls to nsGenericDOMDataNode::AppendChild, which universally returns the error code.  This is per DOM Level 3 Core, section 1.1.1:
http://www.w3.org/TR/2004/REC-DOM-Level-3-Core-20040407/core.html#ID-1590626202
and the section on Node.appendChild:
http://www.w3.org/TR/2004/REC-DOM-Level-3-Core-20040407/core.html#ID-184E7107

Because I still have to return two different error codes, I'm not sure how making it a whitelist (nsCOMPtr<nsIDOMElement> aElement = do_QueryInterface(newParent)) helps me in determining which error code to return.  How would you suggest making it shorter?

Incidentally, this still doesn't cover the cases of entity reference nodes (which we don't implement) or xpath namespace nodes.  Should we do anything about those?

Comment 5

[Alex Vincent [:WeirdAl]]

Comment on attachment 251426 [details] [diff] [review]
patch, v1

sicking and I talked it over; we might as well return INVALID_NODE_TYPE_ERR for anything that isn't an element.

Unfortunately, I'm a little stuck on how to find out if the new parent element can't be inserted (that is, if it would trigger a HIERARCHY_REQUEST_ERR).

I thought of doing it by IsAllowedAsChild, but that's a static function in nsGenericElement.cpp, and doesn't belong to the nsGenericElement class.

There's also the problem of figuring out what the refContent should be in the IsAllowedAsChild() call; otherwise, I'd just copy the function over and cut the irrelevant parts.

I'm stumped.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

I don't think this is needed for ACID3

Comment 7

[Bill Gianopoulos [:WG9s]]

(In reply to comment #2)
> Created an attachment (id=251426) [details]
> patch, v1
> 
> This patch is pretty simple, but it says nothing about other code in nsRange
> failing to check parameters beforehand.  I started reading through the code,
> and discovered bug 366946 as well.  It would be a good idea to compare the code
> to the spec for errors that should be thrown before modifying the document.

This patch no longer applies cleanly now that the patch for Bug 454326 has been checked-in.  It is not clear to me if this code now belongs before or after the code added for that bug or if this is needed at all.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

The patch is needed, in some form, to fix this bug.
I think it doesn't really matter whether the code is before or after the check
that was added in bug 454326.

Comment 9

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 251426 [details] [diff] [review]
patch, v1

I don't know if this patch still applies, but I think some parts are outdated, and a simpler patch would suffice:

>+  switch (nodeType) {
>+    case nsIDOMNode::ATTRIBUTE_NODE:
>+    case nsIDOMNode::ENTITY_NODE:
>+    case nsIDOMNode::DOCUMENT_TYPE_NODE:
>+    case nsIDOMNode::DOCUMENT_NODE:
>+    case nsIDOMNode::DOCUMENT_FRAGMENT_NODE:
>+    case nsIDOMNode::NOTATION_NODE:
>+      return NS_ERROR_DOM_RANGE_INVALID_NODE_TYPE_ERR;

In DOM4, ATTRIBUTE_NODE, ENTITY_NODE, and NOTATION_NODE are no longer used.  Per spec, no node should ever have these node types.

>+    case nsIDOMNode::TEXT_NODE:
>+    case nsIDOMNode::CDATA_SECTION_NODE:
>+    case nsIDOMNode::PROCESSING_INSTRUCTION_NODE:
>+    case nsIDOMNode::COMMENT_NODE:
>+      return NS_ERROR_DOM_HIERARCHY_REQUEST_ERR;

In DOM4, CDATA_SECTION_NODE also doesn't exist (bug 660660).  In any event, the spec doesn't call for this behavior.  Current Gecko behavior in this case is to proceed in extracting the range's current contents and inserting the CharacterData node, and then throwing a HierarchyRequestError only when it tries to appendChild() the old contents.  This matches the spec, as well as Chrome 19 dev and IE10 Developer Preview.  This part of the patch would cause Gecko to no longer match the spec.

>+  // Owner documents must match.
>+  nsCOMPtr<nsINode> container = do_QueryInterface(aNewParent);
>+  if (!container->HasSameOwnerDoc(mStartParent) && container->GetOwnerDoc()) {
>+    return NS_ERROR_DOM_WRONG_DOCUMENT_ERR;
>+  }

DOM4 requires that the owner document be changed in this case, rather than throwing.  Thus Gecko's current behavior here matches the spec.

I'm about to attach a smaller patch that fixes only this issue.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

> >+  switch (nodeType) {
> >+    case nsIDOMNode::ATTRIBUTE_NODE:
> >+    case nsIDOMNode::ENTITY_NODE:
> >+    case nsIDOMNode::DOCUMENT_TYPE_NODE:
> >+    case nsIDOMNode::DOCUMENT_NODE:
> >+    case nsIDOMNode::DOCUMENT_FRAGMENT_NODE:
> >+    case nsIDOMNode::NOTATION_NODE:
> >+      return NS_ERROR_DOM_RANGE_INVALID_NODE_TYPE_ERR;
> 
> In DOM4, ATTRIBUTE_NODE, ENTITY_NODE, and NOTATION_NODE are no longer used. 
> Per spec, no node should ever have these node types.
But because the implementation has still ATTRIBUTE_NODE, it needs to be handled.

Comment 11

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Olli Pettay [:smaug] from comment #10)
> But because the implementation has still ATTRIBUTE_NODE, it needs to be
> handled.

Sure, but there's no spec for it, so anything is valid.  We can throw whatever we want.  It looks like we treat it like a Text/Comment/PI node right now -- wipe out contents and throw HierarchyRequestError.  IE/Chrome/Opera seem to treat it like Document/DocumentType/DocumentFragment, in throwing InvalidNodeTypeError early.  Either way is fine per spec.  I could adjust my upcoming patch to match other browsers if you want.

(Making Attr not inherit from Node is bug 611787.)

Comment 12

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch v1

Self-explanatory.  Together with the patches from bug 737612 and bug 433662, this eliminates all failures in <http://w3c-test.org/webapps/DOMCore/tests/approved/Range-surroundContents.html>, except for those caused by bug 719533.  Feel free to wait for the try run to complete before you review: <https://tbpl.mozilla.org/?tree=Try&rev=69721f440558>

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 608031 [details] [diff] [review]
Patch v1

># HG changeset patch
># User Aryeh Gregor <[e-mail]>
># Date 1332353210 14400
># Node ID 29825fc372f75075871908a62705c7f6c39f564b
># Parent  c91fddba7585a3bb9a0047b77fdf00e364ae837e
>Bug 366944 - Range.surroundContents should throw InvalidNodeTypeError early
>
>diff --git a/content/base/src/nsRange.cpp b/content/base/src/nsRange.cpp
>--- a/content/base/src/nsRange.cpp
>+++ b/content/base/src/nsRange.cpp
>@@ -2167,21 +2167,32 @@ nsRange::SurroundContents(nsIDOMNode* aN
>                     startGrandParent &&
>                     startGrandParent == mEndParent) ||
>                    (endIsText &&
>                     endGrandParent &&
>                     endGrandParent == mStartParent),
>                    NS_ERROR_DOM_INVALID_STATE_ERR);
>   }
> 
>+  // INVALID_NODE_TYPE_ERROR if aNewParent is something that can't be inserted
>+  // (Document, DocumentType, DocumentFragment)
>+  PRUint16 nodeType;
>+  nsresult res = aNewParent->GetNodeType(&nodeType);
>+  if (NS_FAILED(res)) return res;
>+  if (nodeType == nsIDOMNode::DOCUMENT_NODE ||
>+      nodeType == nsIDOMNode::DOCUMENT_TYPE_NODE ||
>+      nodeType == nsIDOMNode::DOCUMENT_FRAGMENT_NODE) {
>+    return NS_ERROR_DOM_INVALID_NODE_TYPE_ERR;
>+  }
>+
>   // Extract the contents within the range.
> 
>   nsCOMPtr<nsIDOMDocumentFragment> docFrag;
> 
>-  nsresult res = ExtractContents(getter_AddRefs(docFrag));
>+  res = ExtractContents(getter_AddRefs(docFrag));
> 
>   if (NS_FAILED(res)) return res;
>   if (!docFrag) return NS_ERROR_FAILURE;
> 
>   // Spec says we need to remove all of aNewParent's
>   // children prior to insertion.
> 
>   nsCOMPtr<nsIDOMNodeList> children;
>diff --git a/content/base/test/Makefile.in b/content/base/test/Makefile.in
>--- a/content/base/test/Makefile.in
>+++ b/content/base/test/Makefile.in
>@@ -565,16 +565,17 @@ _TEST_FILES2 = \
> 		test_bug711047.html \
> 		test_bug696301-1.html \
> 		test_bug696301-2.html \
> 		bug696301-script-1.js \
> 		bug696301-script-1.js^headers^ \
> 		bug696301-script-2.js \
> 		test_bug737612.html \
> 		test_bug433662.html \
>+		test_bug366944.html \
> 		$(NULL)
> 
> _CHROME_FILES =	\
> 		test_bug357450.js \
> 		$(NULL)
> 
> # This test fails on the Mac for some reason
> ifneq (,$(filter gtk2 windows,$(MOZ_WIDGET_TOOLKIT)))
>diff --git a/content/base/test/test_bug366944.html b/content/base/test/test_bug366944.html
>new file mode 100644
>--- /dev/null
>+++ b/content/base/test/test_bug366944.html
>@@ -0,0 +1,49 @@
>+<!doctype html>
>+<!--
>+https://bugzilla.mozilla.org/show_bug.cgi?id=366944
>+-->
>+<title>Test for Bug 366944</title>
>+<script src="/tests/SimpleTest/SimpleTest.js"></script>
>+<link rel="stylesheet" href="/tests/SimpleTest/test.css"/>
>+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=366944">Mozilla Bug 366944</a>
>+<script>
>+
>+/** Test for Bug 366944 **/
>+var testNodes = [document, document.doctype, document.createDocumentFragment()];
>+for (var i = 0; i < testNodes.length; i++) {
>+  var range = document.createRange();
>+  // If a non-Text node is partially contained, we expect to throw for that
>+  // first
>+  range.setStart(document.head, 0);
>+  range.setEnd(document.body, 0);
>+  var threw = false;
>+  var desc = " (surrounding a range with partially-contained Element "
>+    + "with " + (i == 0 ? "document" : i == 1 ? "doctype" : "docfrag") + ")";
>+  try {
>+    range.surroundContents(testNodes[i]);
>+  } catch(e) {
>+    threw = true;
>+    is(Object.getPrototypeOf(e), DOMException.prototype,
>+       "Must throw DOMException" + desc);
>+    is(e.name, "InvalidStateError", "Must throw InvalidStateError" + desc);
>+  }
>+  ok(threw, "Must throw" + desc);
>+
>+  range.setStart(document.body, 0);
>+  range.setEnd(document.body, 1);
>+  threw = false;
>+  desc = " (surrounding a regular range "
>+    + "with " + (i == 0 ? "document" : i == 1 ? "doctype" : "docfrag") + ")";
>+  try {
>+    range.surroundContents(testNodes[i]);
>+  } catch(e) {
>+    threw = true;
>+    is(Object.getPrototypeOf(e), DOMException.prototype,
>+       "Must throw DOMException" + desc);
>+    is(e.name, "InvalidNodeTypeError",
>+       "Must throw InvalidNodeTypeError" + desc);
>+  }
>+  ok(threw, "Must throw" + desc);
>+}
>+
>+</script>

Comment 14

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

The 18 mochitest-plain-1 failures in the try run are due to the bug 433662 patch, not this one, so this is good to go.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1296b59d31de

Comment 15

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/1296b59d31de