Last Comment Bug 719533 - Range.extractContents() shouldn't modify the DOM if the range contains a doctype
: Range.extractContents() shouldn't modify the DOM if the range contains a doctype
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- minor (vote)
: mozilla14
Assigned To: :Aryeh Gregor (away until August 15)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-19 11:53 PST by :Aryeh Gregor (away until August 15)
Modified: 2013-04-04 13:53 PDT (History)
3 users (show)
ayg: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (2.81 KB, patch)
2012-03-21 11:45 PDT, :Aryeh Gregor (away until August 15)
bugs: review-
Details | Diff | Review
Patch v2 (3.81 KB, patch)
2012-03-21 13:40 PDT, :Aryeh Gregor (away until August 15)
bugs: review+
Details | Diff | Review

Description :Aryeh Gregor (away until August 15) 2012-01-19 11:53:40 PST
Test case:

data:text/html,<!doctype html>
<script>
try {
var range = document.createRange();
range.selectNodeContents(document);
range.extractContents();
} catch(e) {}
alert(document.childNodes.length)
</script>

IE9 and Chrome 17 dev alert "2", because they leave the original two children of the document (doctype and <html>).  Firefox 12.0a1 alerts "1", because it removes the <html> element.  Opera Next 12.00 alpha alerts "0", because it removes both.

The spec requires the IE/WebKit behavior:

"""
13. If any member of contained children is a DocumentType, throw a "HierarchyRequestError" exception and terminate these steps.
"""
http://dvcs.w3.org/hg/domcore/raw-file/tip/dom-core.html#dom-range-extractcontents

Notice that no previous step in the spec modified the DOM.  Since the method is eventually going to throw, it should do nothing at all, not move some nodes to a DocumentFragment that then isn't returned.  (Which is what Gecko does: you can get the DocumentFragment if you save the <html> element and call .parentNode.)

This causes test failures in <http://w3c-test.org/webapps/DOMCore/tests/approved/Range-extractContents.html>.  If it's fixed, it looks like Gecko will pass all tests in that file (Chrome 17 dev already does).
Comment 1 :Aryeh Gregor (away until August 15) 2012-03-21 11:45:02 PDT
Created attachment 608042 [details] [diff] [review]
Patch v1

This fixes the failures in Range-extractContents.html.  Applied on top of the patches for bug 737612, bug 433662, and bug 366944, it also fixes the last of the failures in Range-surroundContents.html.  Try URL: <https://tbpl.mozilla.org/?tree=Try&rev=f1fa94fa8421>.  The patch is probably simple enough that you can review it before the try run finishes.

(Oops, I have the wrong --post-to-bugzilla on these try runs.  Oh well, I'll copy the results here when they arrive.)
Comment 2 Olli Pettay [:smaug] 2012-03-21 12:14:54 PDT
Comment on attachment 608042 [details] [diff] [review]
Patch v1

># HG changeset patch
># User Aryeh Gregor <ayg@aryeh.name>
># Date 1332355269 14400
># Node ID 9cf8f01eaca4135f74931cac91a34bb0a7a50aae
># Parent  9c3a9f9fecf7f7b2e1956f8d1e00cd67dbe5f42b
>Bug 719533 - Range.extractContents() shouldn't modify the DOM if the range contains a doctype
>
>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
>@@ -1542,16 +1542,32 @@ nsresult nsRange::CutContents(nsIDOMDocu
>     // There's nothing for us to delete.
>     rv = CollapseRangeAfterDelete(this);
>     if (NS_SUCCEEDED(rv) && aFragment) {
>       NS_ADDREF(*aFragment = retval);
>     }
>     return rv;
>   }
> 
>+  if (retval) {
>+    // For extractContents(), abort early if there's a doctype (bug 719533)
>+    while (!iter.IsDone()) {
>+      nsCOMPtr<nsIDOMNode> node(iter.GetCurrentNode());
>+      PRUint16 nodeType;
>+      node->GetNodeType(&nodeType);
>+      if (nodeType == nsIDOMNode::DOCUMENT_TYPE_NODE) {
>+        return NS_ERROR_DOM_HIERARCHY_REQUEST_ERR;
>+      }
>+      iter.Next();
>+    }
>+
>+    rv = iter.Init(this);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }
So, this is slow. You should limit the loop to only those cases when there can be dtd.
I guess that means to cases when mStartParent == doc, or mStartParent->NodeType() == nsIDOMNode::DOCUMENT_TYPE_NODE;
Comment 3 :Aryeh Gregor (away until August 15) 2012-03-21 13:40:13 PDT
Created attachment 608077 [details] [diff] [review]
Patch v2

Okay, I've rewritten it.  The only way a doctype could be contained is if the commonAncestorContainer is a document, and in that case we just need to check whether that document has a doctype and the doctype is contained in the range.  Same test as before, and seemingly the same effects.
Comment 4 Mozilla RelEng Bot 2012-03-21 13:45:03 PDT
Autoland Patchset:
	Patches: 608077
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=69bb4c4ed707
Try run started, revision 69bb4c4ed707. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=69bb4c4ed707
Comment 5 Olli Pettay [:smaug] 2012-03-21 13:54:47 PDT
Comment on attachment 608077 [details] [diff] [review]
Patch v2


>+  if (retval) {
>+    // For extractContents(), abort early if there's a doctype (bug 719533).
>+    // This can happen only if the common ancestor is a document, in which case
>+    // we just need to find its doctype child and check if that's in the range.
>+    nsCOMPtr<nsIDOMDocument> commonAncestorDocument(do_QueryInterface(commonAncestor));
>+    if (commonAncestorDocument) {
>+      nsCOMPtr<nsIDOMDocumentType> doctype = nsnull;
>+      rv = commonAncestorDocument->GetDoctype(getter_AddRefs(doctype));
>+      NS_ENSURE_SUCCESS(rv, rv);
>+
>+      if (doctype &&
>+          nsContentUtils::ComparePoints(startContainer, startOffset,
>+                                        doctype.get(), 0) < 0 &&
>+          nsContentUtils::ComparePoints(doctype.get(), 0,
>+                                        endContainer, endOffset) < 0) {
Why is offset 0 right for doctype?
Comment 6 :Aryeh Gregor (away until August 15) 2012-03-21 14:05:31 PDT
The offset makes no difference in the case of a doctype.  If you look at the source of ComparePoints, the only time aOffset1 is used is if aParent1 and aParent2 are the same, or if aParent1 is an ancestor of aParent2:

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp#1933

They can't be the same here, because the start/end node of a range can't be a doctype.  And a doctype can't be the ancestor of anything.  Likewise for aOffset2.  The offset could be -1 or 75 and it would work the same.
Comment 7 Mozilla RelEng Bot 2012-03-22 02:04:28 PDT
Try run for 69bb4c4ed707 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=69bb4c4ed707
Results (out of 219 total builds):
    exception: 1
    success: 176
    warnings: 27
    failure: 15
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-69bb4c4ed707
Comment 8 Olli Pettay [:smaug] 2012-03-26 12:52:12 PDT
Comment on attachment 608077 [details] [diff] [review]
Patch v2

>@@ -1524,16 +1525,36 @@ nsresult nsRange::CutContents(nsIDOMDocu
>   // Save the range end points locally to avoid interference
>   // of Range gravity during our edits!
> 
>   nsCOMPtr<nsIDOMNode> startContainer = do_QueryInterface(mStartParent);
>   PRInt32              startOffset = mStartOffset;
>   nsCOMPtr<nsIDOMNode> endContainer = do_QueryInterface(mEndParent);
>   PRInt32              endOffset = mEndOffset;
> 
>+  if (retval) {
Hmm, how should deleteContents work?
Comment 9 Olli Pettay [:smaug] 2012-03-26 16:35:35 PDT
Er, nm about deleteContents
Comment 10 Olli Pettay [:smaug] 2012-03-26 16:40:55 PDT
Comment on attachment 608077 [details] [diff] [review]
Patch v2


>+  if (retval) {
>+    // For extractContents(), abort early if there's a doctype (bug 719533).
>+    // This can happen only if the common ancestor is a document, in which case
>+    // we just need to find its doctype child and check if that's in the range.
>+    nsCOMPtr<nsIDOMDocument> commonAncestorDocument(do_QueryInterface(commonAncestor));
>+    if (commonAncestorDocument) {
>+      nsCOMPtr<nsIDOMDocumentType> doctype = nsnull;
No need to set doctype to null.


>+/** Test for Bug 719544 **/
>+var threw = false;
>+var origLength = document.childNodes.length;
>+try {
>+  var range = document.createRange();
>+  range.selectNodeContents(document);
>+  range.extractContents();
>+} catch(e) {
>+  threw = true;
>+}
>+ok(threw, "Must throw");
>+is(document.childNodes.length, origLength, "Must preserve original children");
Could you check that the right exception is thrown.
Comment 11 :Aryeh Gregor (away until August 15) 2012-04-01 04:46:29 PDT
Both review comments fixed.  Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/38dab05abe14
Comment 12 :Aryeh Gregor (away until August 15) 2012-04-01 05:05:43 PDT
Oops, I forgot, I shouldn't resolve yet.
Comment 13 Matt Brubeck (:mbrubeck) 2012-04-02 11:04:29 PDT
https://hg.mozilla.org/mozilla-central/rev/38dab05abe14

Note You need to log in before you can comment on or make changes to this bug.