Closed Bug 10231 Opened 21 years ago Closed 21 years ago

crash making list after select all

Categories

(Core :: DOM: Editor, defect, P3, critical)

defect

Tracking

()

VERIFIED FIXED

People

(Reporter: mozeditor, Assigned: kinmoz)

Details

Open default editor page.  Select All.  Make a List (either kind).
Crasheroonies.
Status: NEW → ASSIGNED
Target Milestone: M9
accepting, setting to m9.
Summary: crash making list after select all
setting summary
I have a fix for this. There were actually 2 problems here.

1. nsHTMLEditRules::GetPromotedPoint() didn't account for the fact that the
GetChildAt() call could return NULL for the case where aWhere == kEnd.

2. nsGenericElement::RangeAdd() did not check to see if a range was already in
it's list, so if a range had a start and end point in the same parent, that
range would be added twice, and notified twice of any DOM tree changes.

Here are the diffs for the fixes:




Index: nsHTMLEditRules.cpp
===================================================================
RCS file: /cvsroot/mozilla/editor/base/nsHTMLEditRules.cpp,v
retrieving revision 1.37
diff -c -r1.37 nsHTMLEditRules.cpp
*** nsHTMLEditRules.cpp 1999/07/26 18:05:41     1.37
--- nsHTMLEditRules.cpp 1999/08/02 18:46:58
***************
*** 1265,1271 ****
      {
        node = nsEditor::GetChildAt(parent,offset);
      }
!     offset++;  // since this is going to be used for a range _endpoint_, we wa
nt to be after the node

      // finding the real end for this point.  look up the tree for as long as w
e are the
      // last node in the container, and as long as we haven't hit the body node
.
--- 1265,1275 ----
      {
        node = nsEditor::GetChildAt(parent,offset);
      }
!
!     if (node)
!       offset++;  // since this is going to be used for a range _endpoint_, we
want to be after the node
!     else
!       node = parent;

      // finding the real end for this point.  look up the tree for as long as
we are the
      // last node in the container, and as long as we haven't hit the body
node.



Index: nsGenericElement.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/base/src/nsGenericElement.cpp,v
retrieving revision 3.46
diff -c -r3.46 nsGenericElement.cpp
*** nsGenericElement.cpp        1999/07/22 22:34:28     3.46
--- nsGenericElement.cpp        1999/08/02 18:58:16
***************
*** 821,826 ****
--- 821,836 ----
    if (nsnull == mDOMSlots->mRangeList) {
      return NS_ERROR_OUT_OF_MEMORY;
    }
+
+   // Make sure we don't add a range that is already
+   // in the list!
+   PRInt32 i = mDOMSlots->mRangeList->IndexOf(&aRange);
+   if (i >= 0) {
+     // Range is already in the list, so there
+     // is nothing to do!
+     return NS_OK;
+   }
+
    // dont need to addref - this call is made by the range object itself
    PRBool rv = mDOMSlots->mRangeList->AppendElement(&aRange);
    if (rv)  return NS_OK;
One more fix to make this work properly.
nsDOMSelection::GetPrimaryFrameForFocusNode() did not null out it's return
parameter, so if it failed to get a focus node, it might still return NS_OK and
a pointer that was pointing to lala-land.




Index: nsRangeList.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/base/src/nsRangeList.cpp,v
retrieving revision 1.117
diff -c -r1.117 nsRangeList.cpp
*** nsRangeList.cpp     1999/07/22 19:42:21     1.117
--- nsRangeList.cpp     1999/08/02 20:27:04
***************
*** 1505,1510 ****
--- 1505,1512 ----
    if (!aReturnFrame)
      return NS_ERROR_NULL_POINTER;

+   *aReturnFrame = 0;
+
    nsresult    result = NS_OK;

    nsCOMPtr<nsIDOMNode> node = dont_QueryInterface(FetchFocusNode());
***************
*** 1525,1532 ****
      {
        nsCOMPtr<nsIContent> child;
        result = content->ChildAt(offset, *getter_AddRefs(child));
!       if (NS_FAILED(result) || !child) //out of bounds?
          return result;
        content = child;//releases the focusnode
      }
    }
--- 1527,1536 ----
      {
        nsCOMPtr<nsIContent> child;
        result = content->ChildAt(offset, *getter_AddRefs(child));
!       if (NS_FAILED(result))
          return result;
+       if (!child) //out of bounds?
+         return NS_ERROR_FAILURE;
        content = child;//releases the focusnode
      }
    }
Assignee: jfrancis → kin
Status: ASSIGNED → NEW
Reassigning bug to kin@netscape.com.
Status: NEW → ASSIGNED
Accepting bug.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Fix checked in:

    mozilla/editor/base/nsHTMLEditRules.cpp    rev 1.38
      - Added check, in WillDeleteSelection(),  to see if
        endpoints of the range are in the body before calling
        GetBlockNodeParent().

    mozilla/layout/base/src/nsGenericElement.cpp    rev 3.47
      - Modified RangeAdd() so that it doesn't add
        a range if it's already in the list.

    mozilla/layout/base/src/nsRangeList.cpp    rev 1.118
      - Modified GetPrimaryFrameForFocusNode() to
        initialize aReturnFrame and to return a failure
        if ChildAt() returns NULL.
just a note that I had to make some more changes.  I added Kin's
nsGenericElement::RangeAdd() fix to nsGenericDOMDataNode::RangeAdd(), and also
modified nsRange::DoSetRange() to work with the new dom gravity rangelist
assumption of "range will be on list once or not at all", whereas before it could
be on 0, 1, or 2 times.
Status: RESOLVED → VERIFIED
verified in 8/10 build.
You need to log in before you can comment on or make changes to this bug.