357509 - Problem setting/using nsIDOMRange.setStartBefore

Status: VERIFIED FIXED

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

Description

[Stephen Donner [:stephend] Not actively reading bugmail]

Build ID: 2006-10-21-09 of SeaMonkey trunk under Windows XP.

This worked in last night's nightly build; I suspect bug 357445.

Steps to Reproduce:

1. Using Facebook.com, navigate to http://iusb.facebook.com/friends.php? and click [Edit Details]

Expected Results:
Link works

Actual Results:
Link fails, throwing:

Error: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIDOMRange.setStartBefore]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: http://static.facebook.com/js/socialmap.js?26159 :: ajaxLoadContent :: line 300"  data: no]
Source File: http://static.facebook.com/js/socialmap.js?26159
Line: 300

The code in question does:

function ajaxLoadContent(elem, content){
  if (navigator.appName == 'Netscape' && document.createRange) {
    rng = document.createRange();
    rng.setStartBefore(elem);
    htmlFrag = rng.createContextualFragment(content);
    while (elem.hasChildNodes()) {
      elem.removeChild(elem.lastChild); 
    }
    elem.appendChild(htmlFrag);
  } else {
    elem.innerHTML = content;
  }
}

the line in question is:

rng.setStartBefore(elem);

Comment 1

[Adam Guthrie]

Attachment: testcase

Comment 2

[Alex Vincent [:WeirdAl]]

So with document.createRange(), both end points are (document, 0).  mIsPositioned becomes true.

In SetStart(), we skip over http://lxr.mozilla.org/seamonkey/source/content/base/src/nsRange.cpp#718 because mIsPositioned is true, and the element is obviously in the same document as the document itself.  So DoSetRange doesn't get called.

Then we hit the ComparePoints check (nsRange.cpp#724), which will tell you the elem is after the document, so it returns NS_ERROR_ILLEGAL_VALUE.

DOM 2 Range Recommendation, section 2.4, states:

"The start position of a Range is guaranteed to never be after the end position. To enforce this restriction, if the start is set to be at a position after the end, the Range is collapsed to that position. Similarly, if the end is set to be at a position before the start, the Range is collapsed to that position."

Likewise, setStart* according to the spec doesn't require throwing an exception for a start node after the end node.  Errata page for the spec shows no changes to these lines.

In short, I believe this is a valid bug.

Comment 3

[Adam Guthrie]

*** Bug 357510 has been marked as a duplicate of this bug. ***

Comment 4

[Alex Vincent [:WeirdAl]]

I have a patch, but I want to test it locally first.

Comment 5

[Alex Vincent [:WeirdAl]]

Attachment: patch, v1

Comment 6

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

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

Ah, the reason this worked in the old code is because the old nsRange::IsIncreasing was busted when one of the compared nodes were a document.

>@@ -715,20 +715,21 @@ nsresult nsRange::SetStart(nsIDOMNode* a
> 
>   // Collapse if not positioned yet, or if positioned in another doc.
>   if (!mIsPositioned || !InSameDoc(parent, mEndParent)) {
>     DoSetRange(parent, aOffset, parent, aOffset);
> 
>     return NS_OK;
>   }
> 
>-  // the start must be before the end
>+  // the start must be before or equal to the end
>   if (nsContentUtils::ComparePoints(parent, aOffset,
>                                     mEndParent, mEndOffset) == 1) {

Move the test into the above if-statement instead.

with that, r/sr=sicking

Comment 7

[Alex Vincent [:WeirdAl]]

Attachment: patch with r/sr nit answered

sicking, I moved the comment into the if statement as well; I figured it was important enough to keep.  Sorry for the awkwardness...

Comment 8

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

Comment on attachment 243065 [details] [diff] [review]
patch with r/sr nit answered

Don't put the comment inline in the ifstatement. Put it with rest of the comment above.

Comment 9

[Alex Vincent [:WeirdAl]]

Attachment: patch with r/sr nit answered

and a typo fix, and the comment moved properly.

Comment 10

[Alex Vincent [:WeirdAl]]

fix checked in by sicking, please retest with Sunday's nightly builds.

Comment 11

[Robert Sayre]

we have coverage on this in bug 357523. WFM on a trunk checkout after sicking's checkin.

Comment 12

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20061021 Minefield/3.0a1 ID:2006102123 [cairo]

Bug 357510 is WFM again after this landed

Comment 13

[Alex Vincent [:WeirdAl]]

per comments, marking FIXED.  Thanks, guys!

Comment 14

[Justin Kerk]

*** Bug 357558 has been marked as a duplicate of this bug. ***

Comment 15

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using build 2006-10-22-07 of SeaMonkey trunk under Windows XP; I appreciate the quick turnaround time from both Alex and Jonas on this: thanks, guys!