This bug was originally reported in bug 58969 by sunsear@raketnet.nl -------- sunsear@raketnet.nl wrote: I have the following HTML: Dikke test<a onclick="alert('blaat')">blaat</A> ...more HTML. When I select both a part from the first text-node and a part from the text-node under the a-node, mozilla crashes. As long as I don't select parts or all of the A-node, I get an exception most of the times (which is not desired behavior in my opinion either, but that's a whole other matter). If I only select a part of a text-node the behavior works fine. -------- His test case can be found at: http://bugzilla.mozilla.org/showattachment.cgi?attach_id=64267 Following the steps there, I was able to crash with the stack trace below, because tFirstChild, CopyContents(), was null. There are some important notes I put in bug 58969, when doing the code review for some of the patches ... which noted that the implementation for some of the methods bug 58969 was supposed to cover would not handle scenarios where the range end points were not in the same level. Here's the stack trace: nsRange::CopyContents(nsIDOMNode * 0x0481b1ec, nsIDOMNode * 0x04b19cac, nsRange * 0x04856470) line 2592 + 47 bytes nsRange::CloneContents(nsRange * const 0x04856470, nsIDOMDocumentFragment * * 0x0012d604) line 1736 + 27 bytes nsRange::ExtractContents(nsRange * const 0x04856470, nsIDOMDocumentFragment * * 0x0012d604) line 1573 + 16 bytes XPTC_InvokeByIndex(nsISupports * 0x04856470, unsigned int 20, unsigned int 1, nsXPTCVariant * 0x0012d604) line 106 XPCWrappedNative::CallMethod(XPCCallContext & {...}, XPCWrappedNative::CallMode CALL_METHOD) line 1998 + 42 bytes XPC_WN_CallMethod(JSContext * 0x03ad28d0, JSObject * 0x0337abc8, unsigned int 0, long * 0x034b1304, long * 0x0012d8e4) line 1266 + 14 bytes js_Invoke(JSContext * 0x03ad28d0, unsigned int 0, unsigned int 0) line 832 + 23 bytes js_Interpret(JSContext * 0x03ad28d0, long * 0x0012e6d4) line 2798 + 15 bytes js_Invoke(JSContext * 0x03ad28d0, unsigned int 1, unsigned int 2) line 849 + 13 bytes js_InternalInvoke(JSContext * 0x03ad28d0, JSObject * 0x0337ad78, long 53980816, unsigned int 0, unsigned int 1, long * 0x0012e944, long * 0x0012e7fc) line 924 + 20 bytes JS_CallFunctionValue(JSContext * 0x03ad28d0, JSObject * 0x0337ad78, long 53980816, unsigned int 1, long * 0x0012e944, long * 0x0012e7fc) line 3405 + 31 bytes nsJSContext::CallEventHandler(nsJSContext * const 0x03accb50, void * 0x0337ad78, void * 0x0337ae90, unsigned int 1, void * 0x0012e944, int * 0x0012e948, int 0) line 1011 + 33 bytes nsJSEventListener::HandleEvent(nsJSEventListener * const 0x04818060, nsIDOMEvent * 0x048543f8) line 180 + 77 bytes nsEventListenerManager::HandleEventSubType(nsListenerStruct * 0x0481fca0, nsIDOMEvent * 0x048543f8, nsIDOMEventTarget * 0x04646450, unsigned int 4, unsigned int 7) line 1205 + 20 bytes nsEventListenerManager::HandleEvent(nsEventListenerManager * const 0x048180b0, nsIPresContext * 0x04642900, nsEvent * 0x0012f4d0, nsIDOMEvent * * 0x0012f160, nsIDOMEventTarget * 0x04646450, unsigned int 7, nsEventStatus * 0x0012f8b0) line 1373 + 36 bytes nsGenericElement::HandleDOMEvent(nsGenericElement * const 0x0481ffb0, nsIPresContext * 0x04642900, nsEvent * 0x0012f4d0, nsIDOMEvent * * 0x0012f160, unsigned int 1, nsEventStatus * 0x0012f8b0) line 1648 nsHTMLInputElement::HandleDOMEvent(nsHTMLInputElement * const 0x0481ffb0, nsIPresContext * 0x04642900, nsEvent * 0x0012f4d0, nsIDOMEvent * * 0x00000000, unsigned int 1, nsEventStatus * 0x0012f8b0) line 1147 + 29 bytes PresShell::HandleEventInternal(nsEvent * 0x0012f4d0, nsIView * 0x00000000, unsigned int 1, nsEventStatus * 0x0012f8b0) line 5986 + 44 bytes PresShell::HandleEventWithTarget(PresShell * const 0x04657680, nsEvent * 0x0012f4d0, nsIFrame * 0x034f5ee4, nsIContent * 0x0481ffb0, unsigned int 1, nsEventStatus * 0x0012f8b0) line 5955 + 22 bytes nsEventStateManager::CheckForAndDispatchClick(nsEventStateManager * const 0x0462db20, nsIPresContext * 0x04642900, nsMouseEvent * 0x0012f9b8, nsEventStatus * 0x0012f8b0) line 2462 + 63 bytes nsEventStateManager::PostHandleEvent(nsEventStateManager * const 0x0462db28, nsIPresContext * 0x04642900, nsEvent * 0x0012f9b8, nsIFrame * 0x034f5ee4, nsEventStatus * 0x0012f8b0, nsIView * 0x048006a0) line 1544 + 28 bytes PresShell::HandleEventInternal(nsEvent * 0x0012f9b8, nsIView * 0x048006a0, unsigned int 1, nsEventStatus * 0x0012f8b0) line 6006 + 43 bytes PresShell::HandleEvent(PresShell * const 0x04657684, nsIView * 0x048006a0, nsGUIEvent * 0x0012f9b8, nsEventStatus * 0x0012f8b0, int 0, int & 1) line 5909 + 25 bytes nsView::HandleEvent(nsView * const 0x048006a0, nsGUIEvent * 0x0012f9b8, unsigned int 0, nsEventStatus * 0x0012f8b0, int 0, int & 1) line 387 nsView::HandleEvent(nsView * const 0x04803d40, nsGUIEvent * 0x0012f9b8, unsigned int 0, nsEventStatus * 0x0012f8b0, int 0, int & 1) line 344 nsView::HandleEvent(nsView * const 0x04650250, nsGUIEvent * 0x0012f9b8, unsigned int 0, nsEventStatus * 0x0012f8b0, int 1, int & 1) line 344 nsViewManager::DispatchEvent(nsViewManager * const 0x046574c0, nsGUIEvent * 0x0012f9b8, nsEventStatus * 0x0012f8b0) line 1930 HandleEvent(nsGUIEvent * 0x0012f9b8) line 83 nsWindow::DispatchEvent(nsWindow * const 0x04803ef4, nsGUIEvent * 0x0012f9b8, nsEventStatus & nsEventStatus_eIgnore) line 850 + 10 bytes nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012f9b8) line 871 nsWindow::DispatchMouseEvent(unsigned int 301, nsPoint * 0x00000000) line 4527 + 21 bytes ChildWindow::DispatchMouseEvent(unsigned int 301, nsPoint * 0x00000000) line 4779 nsWindow::ProcessMessage(unsigned int 514, unsigned int 0, long 2359561, long * 0x0012fdac) line 3419 + 24 bytes nsWindow::WindowProc(HWND__ * 0x04180900, unsigned int 514, unsigned int 0, long 2359561) line 1115 + 27 bytes USER32! 77e71820() MOZREG! __xi_z + 7677 bytes

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

Can someone so empowered please add "DIGBug" to the status whiteboard? Thanks. Also, please note that http://bugzilla.mozilla.org/show_bug.cgi?id=122591 has a second test case for this.

Marking nsbeta1+

Testcase does not crash my Mozilla. Though it does not appear to work correctly. Tstcase in bug 122591 does not crash either.

Sorry... Testcase in bug 112591 DID crash..

This patch rewrites CloneContents() and DeleteContents() so that they: 1. Use the subtree iterator. 2. Prevent the crash that occurs when Deleting and Cloning complex selections. (Range endpoints not at the same level) 3. Cloned subtrees now contain parent contexts specified by the spec. Joe, I noticed in the debugger that the Range gravity code does not collapse the range after the delete, but I couldn't tell if the original delete code did the right thing since it crashed with most of the testcases provided in the range bugs on my list. Comments/Reviews welcome.

Patch Rev 2 is basically the same as Patch Rev 1, except that I added a new function CollapseRangeAfterDelete() which now gets called just before DeleteContents() exits, to collapse the range in the right spot. jfrancis, can you review Patch Rev 2?

Cc sfraser for future super review.

Comment on attachment 73990 [details] [diff] [review] Patch Rev 2 (Same as above + CollapseRangeAfterDelete()) recommendations: add #ifdef XP_MAC #pragma mark - #pragma mark class RangeSubtreeIterator #pragma mark - #endif just above the RangeSubtreeIterator class. Also, the RangeSubtreeIterator class cannot be recylced (reInitted) because Init, doesnt set the state to eDone if needed, or clear the member comPtrs. I dont know if you care about that or not. Indentation wrong near end of CurrentNode(). CurrentNode() does not ensure arg. In CollapseRangeAfterDelete(), you may want to comment that the range is guaranteed to be in the right place if it is already collapsed. Why is CollapseRangeAfterDelete() not a protected member, like the other other utility functions? Comment in DeletContents() at the "if (!handled)" point is a bit misleading. It still might be CData, it's just entirely inside the range. Also, fix curly brace near end of this routine. Is anyone using CloneSibsAndParents()? Can we remove it? Fix brackets in CloneContents(). Near end of CloneContents(), the if (!commonAncestor) break; Shouldn't that be an error return instead? r=jfrancis Dude, great stuff!

Patch Rev 3 addresses jfrancis' comments above and some preliminary comments from sfraser, when I walked him through the code. I also simplified the charData deletion code in CloneContents() which I ran past jfrancis already. sfraser can you take a look at this patch, and perhaps sr= it?

Comment on attachment 74363 [details] [diff] [review] Patch Rev 3 (Addresses jfrancis comments + preliminary comments from sfraser) Fine work. sr=sfraser

Comment on attachment 74363 [details] [diff] [review] Patch Rev 3 (Addresses jfrancis comments + preliminary comments from sfraser) a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Patch Rev 3 checked into the TRUNK: mozilla/content/base/src/nsRange.cpp revision 1.146 mozilla/content/base/src/nsRange.h revision 1.37