Closed Bug 719533 Opened 12 years ago Closed 12 years ago

Range.extractContents() shouldn't modify the DOM if the range contains a doctype

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
minor

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla14

People

(Reporter: ayg, Assigned: ayg)

Details

Attachments

(1 file, 1 obsolete file)

Patch v2
3.81 KB, patch
smaug
: review+
Details | Diff | Splinter Review 
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).
Attached patch Patch v1 (obsolete) — — Splinter Review 
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.)
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Attachment #608042 - Flags: review?(bugs)
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;
Attachment #608042 - Flags: review?(bugs) → review-
Attached patch Patch v2 — — Splinter Review 
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.
Attachment #608042 - Attachment is obsolete: true
Attachment #608077 - Flags: review?(bugs)
Whiteboard: [autoland-try:-u all]
Whiteboard: [autoland-try:-u all] → [autoland-in-queue]
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 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?
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.
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
Whiteboard: [autoland-in-queue]
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?
Er, nm about deleteContents
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.
Attachment #608077 - Flags: review?(bugs) → review+
Both review comments fixed.  Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/38dab05abe14
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
OS: Linux → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Oops, I forgot, I shouldn't resolve yet.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/38dab05abe14
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Component: DOM: Traversal-Range → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: