A quick examination of nsRange::CompareBoundaryPoints shows it doesn't check owner documents for ranges.

Hmm... would the solution be to use something like this: nsIDocument* ourParent = ourNode->GetCurrentDoc(); nsIDocument* otherParent = otherNode->GetCurrentDoc(); if (ourParent != otherParent) return NS_ERROR_DOM_WRONG_DOCUMENT_ERR; Just after the end of the switch statement?

Patch v1 * Confirm both nodes are in the same document. This is cone by calling the |GetCurrentDoc| method for each node and then comparing the results. If they aren't in the same document then we return |NS_ERROR_DOM_WRONG_DOCUMENT_ERR|.

What I'd like to see here is a testcase.

Patch v1.1 Same patch as before, eliminated the need for temporary variables by checking |ourNode->GetCurrentDoc()| and |otherNode->GetCurrentDoc()| directly.

Comment on attachment 251798 [details] [diff] [review] Patch v1.1 Please use nsINode::HasSameOwnerDoc.

Patch v2 * Now using |ourNode->HasSameOwnerDoc()| per Peter's comment. Also fixed the incorrect spacing pointed out by Smaug.

Actually, you should read the spec (http://www.w3.org/TR/2000/REC-DOM-Level-2-Traversal-Range-20001113/ranges.html#Level2-Range-method-compareBoundaryPoints): we need to check more than just the ownerDocument, we need to check that the root ancestors of both boundary points are the same. And ideally we'd fix that without having to walk up the ancestor chain twice.

(In reply to comment #7) > Actually, you should read the spec > (http://www.w3.org/TR/2000/REC-DOM-Level-2-Traversal-Range-20001113/ranges.html#Level2-Range-method-compareBoundaryPoints): > we need to check more than just the ownerDocument, we need to check that the > root ancestors of both boundary points are the same. And ideally we'd fix that > without having to walk up the ancestor chain twice. > Hmm... wouldn't we need something similar to what we do here (http://lxr.mozilla.org/mozilla/source/content/base/src/nsContentUtils.cpp#1298) for that?

Looking at the code, you might be able to just compare the mRoot of both ranges.

Here's a testcase based upon my shaky understanding of how the Range class works. It fails under 2.0.0.1 but seems to pass with the patch applied.

Patch v3 * Checking the ranges come from the same owner document and also checking they have the same root to ensure they are in the same document fragment.

As advised by Sicking I have done some tests and they all seem to work fine. * Tested Cut, Copy, Paste, Select all. * Drag & Drop of text. * Tested rich text editing with all the above. If anyone else thinks there should be any more tests done or thinks an editor peer should take a look first then let me know and I'll wait before requesting review on the patch.

Comment on attachment 251923 [details] [diff] [review] Patch v3 >Index: content/base/public/nsIRange.h >=================================================================== >+ nsINode* mRoot; Put it in the protected section below. > protected: >Index: content/base/src/nsRange.cpp >=================================================================== >+ if (!ourNode->HasSameOwnerDoc(otherNode) >+ || (mRoot != otherRange->GetRoot())) >+ return NS_ERROR_DOM_WRONG_DOCUMENT_ERR; No need for the HasSameOwnerDoc check, if the roots are identical then the ownerDocumnent's are too. >Index: content/base/src/nsRange.h >=================================================================== >+ nsINode* GetRoot(); No need for this. Fix that and we're good to go.

Patch v3.1 * Updated to Peter's comments. Would this need a sr at all Peter?

Comment on attachment 252472 [details] [diff] [review] Patch v3.1 Sorry about the delay.

BTW, could you adapt the testcase for MochiTest? (see http://lxr.mozilla.org/mozilla/source/testing/mochitest/README.txt)

Comment on attachment 254947 [details] Mochitest v1 looks good to me

With a go from Robert on the Mochitest I'll request checkin for the patch and the Mochitest testcase.

mozilla/content/base/public/nsIRange.h 3.3 mozilla/content/base/src/nsRange.cpp 1.218 mozilla/content/base/src/nsRange.h 1.57

(I'll check in the testcase after I rebuild).

Comment on attachment 254947 [details] Mochitest v1 > // Compare range1 and range3: Should throw DOMException WRONG_DOCUMENT_ERR. > try { > var result2 = range3.compareBoundaryPoints(Range.START_TO_START, range1); > } > catch (ex) {} > > ok(result1 === 1, "range1 and range2 can be compared."); > ok(result2 === undefined, "range1 and range3 couldn't be compared as expected."); Suggested change: add a check in the catch block (I think |ok(ex.code == DOMException.WRONG_DOCUMENT_ERR);| would do it, but please double-check this) to check that ex actually is a WRONG_DOCUMENT_ERR? Right now if a different value is thrown the test would still pass.

With Waldo's comment fixed and a SimpleTest.finish call added (without it the test wouldn't run - did anybody tried to run it or the test suite for that matter?)

Comment on attachment 255478 [details] [diff] [review] updated mochitest, as a patch sayrer says this should be reviewed by a content peer.

Comment on attachment 255478 [details] [diff] [review] updated mochitest, as a patch The problem with this test is that the count of tests depends on whether exceptions are thrown. So without the patch I get PASSED 1, FAILED 1, but with the patch I get PASSED 3

Didn't think that was a problem, but OK.

Comment on attachment 255494 [details] [diff] [review] updated mochitest, as a patch v2 With + ok(result2 === undefined, "range1 and range3 couldn't be compared as expected.");

mozilla/content/base/test/Makefile.in 1.6 mozilla/content/base/test/test_bug366946.html 1.1