352520 - Crash when deleting chars from url bar [@ nsEditor::DeleteSelectionImpl]

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[Frank Wein [:mcsmurf]]

Recently I sometimes started crashing when deleting a char from the URL in the url bar (with backspace). Possibly there are multiple ways to reproduce this, I could not find a reliable way though yet.

Stacktrace:
ChildEBP RetAddr  
0012f694 030b5859 editor!nsEditor::DeleteSelectionImpl(short aAction = 0)+0x128 [h:\mozilla\tree-main\mozilla\editor\libeditor\base\nseditor.cpp @ 4630]
0012f704 030b8c9d editor!nsPlaintextEditor::DeleteSelection(short aAction = 2)+0x183 [h:\mozilla\tree-main\mozilla\editor\libeditor\text\nsplaintexteditor.cpp @ 687]
0012f73c 01911137 editor!nsTextEditorKeyListener::KeyPress(class nsIDOMEvent * aKeyEvent = 0x085be75c)+0x1b0 [h:\mozilla\tree-main\mozilla\editor\libeditor\text\nseditoreventlisteners.cpp @ 198]
0012f788 01919170 gklayout!nsEventListenerManager::HandleEvent(class nsPresContext * aPresContext = 0x04aee5d8, class nsEvent * aEvent = 0x0012f9ec, class nsIDOMEvent ** aDOMEvent = 0x0012f858, class nsISupports * aCurrentTarget = 0x04d1d920, unsigned int aFlags = 0x206, nsEventStatus * aEventStatus = 0x0012f85c)+0x20c [h:\mozilla\tree-main\mozilla\content\events\src\nseventlistenermanager.cpp @ 1739]
0012f7c0 01919243 gklayout!nsEventTargetChainItem::HandleEvent(class nsEventChainPostVisitor * aVisitor = 0x0012f850, unsigned int aFlags = 0x206)+0x55 [h:\mozilla\tree-main\mozilla\content\events\src\nseventdispatcher.cpp @ 356]
0012f7e4 01919318 gklayout!nsEventTargetChainItem::HandleEventTargetChain(class nsEventChainPostVisitor * aVisitor = 0x0012f850, unsigned int aFlags = 0x206, class nsDispatchingCallback * aCallback = 0x0012f8a8)+0xb7 [h:\mozilla\tree-main\mozilla\content\events\src\nseventdispatcher.cpp @ 433]
0012f80c 01919518 gklayout!nsEventTargetChainItem::HandleEventTargetChain(class nsEventChainPostVisitor * aVisitor = 0x00000000, unsigned int aFlags = 6, class nsDispatchingCallback * aCallback = 0x0012f8a8)+0x18c [h:\mozilla\tree-main\mozilla\content\events\src\nseventdispatcher.cpp @ 486]
0012f878 017ed555 gklayout!nsEventDispatcher::Dispatch(class nsISupports * aTarget = 0x04d1d920, class nsPresContext * aPresContext = 0x07f8d4e8, class nsEvent * aEvent = 0x00000000, class nsIDOMEvent * aDOMEvent = 0x00000000, nsEventStatus * aEventStatus = 0x0012f920, class nsDispatchingCallback * aCallback = 0x0012f8a8, int aTargetIsChromeHandler = 0)+0x1c7 [h:\mozilla\tree-main\mozilla\content\events\src\nseventdispatcher.cpp @ 643]
0012f8c4 017ef7f2 gklayout!PresShell::HandleEventInternal(class nsEvent * aEvent = 0x00000000, class nsIView * aView = 0x07fd0f60, nsEventStatus * aStatus = 0x0012f920)+0x1e7 [h:\mozilla\tree-main\mozilla\layout\base\nspresshell.cpp @ 6263]
0012f8f8 019829d5 gklayout!PresShell::HandleEvent(class nsIView * aView = 0x07fd0f60, class nsGUIEvent * aEvent = 0x0012f9ec, nsEventStatus * aEventStatus = 0x0012f920)+0x399 [h:\mozilla\tree-main\mozilla\layout\base\nspresshell.cpp @ 6034]
0012f918 0198396f gklayout!nsViewManager::HandleEvent(class nsView * aView = 0x00000000, struct nsPoint aPoint = struct nsPoint, class nsGUIEvent * aEvent = 0x0012f9ec, int aCaptured = 0)+0x2f [h:\mozilla\tree-main\mozilla\view\src\nsviewmanager.cpp @ 1668]
0012f98c 01984780 gklayout!nsViewManager::DispatchEvent(class nsGUIEvent * aEvent = 0x3d888889, nsEventStatus * aStatus = 0x0012f9a4)+0x72e [h:\mozilla\tree-main\mozilla\view\src\nsviewmanager.cpp @ 1621]
0012f9a8 01d57c94 gklayout!HandleEvent(class nsGUIEvent * aEvent = 0x0012f9ec)+0x27 [h:\mozilla\tree-main\mozilla\view\src\nsview.cpp @ 174]

This looks like a regression caused by Bug 344423. I found 15 crashes with this signature in the Talkback DB which were submitted in the last two days with the build from the 12th.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This started showing up as a trunk topcrash in the 2006-09-12-04 builds (was not in 2006-09-11-04).

Comment 2

[Frank Wein [:mcsmurf]]

Bug 344423 was actually checked in a few day earlier...maybe this was caused by another bug then?

Comment 3

[Mike Gao]

Frank,

Did this happen on Windows only or Linux also? I just tried on Linux and it hitted line nsEditor.cpp:4630, but did not reproduce. The condition to hit that line is (aAction == ePrevious && --deleteCharOffset < 0), so I think the condition to triger this might be: the caret is at the very first of the url bar, and press backspace.

Comment 4

[Frank Wein [:mcsmurf]]

I only tried with Windows here, but I saw a few crashes on Linux, too in the Talkback database.

Comment 5

[Mike Gao]

Still could not reproduce this on Linux or Windows(XP). Anyone found a way?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

I haven't managed to reproduce this, but I think one possible reason
for this might be that nsEditor::DeleteSelectionImpl uses nowadays 
( after Bug 344423 ) selection's focusNode. What if the focus is already 
moved to elsewhere and selection is deleted in some other way.
What I don't understand is the reason why this bug started to happen
2006-09-12-04.

Still reading the code and trying to reproduce this.

Comment 7

[Simon Montagu :smontagu]

I agree with Smaug. Notice that http://lxr.mozilla.org/seamonkey/source/layout/generic/nsSelection.cpp#4043 says that GetFocusNode() can return a null node.

Comment 8

[Simon Montagu :smontagu]

Attachment: null-check focusNode

Comment 9

[Simon Montagu :smontagu]

Attachment: null-check focusNode

I haven't been able to reproduce the bug, so I can't be sure that this will fix it, but the return value of getFocusNode ought to be null-checked, and we can see if this makes the crash go away from talkback.

Comment 10

[Aaron Leventhal]

Comment on attachment 238481 [details] [diff] [review]
null-check focusNode

Please indent and go to the next line for the if. Otherwise, both setting breakpoints and reading the code becomes more difficult.

Comment 11

[Simon Montagu :smontagu]

(In reply to comment #10)
> (From update of attachment 238481 [details] [diff] [review] [edit])
> Please indent and go to the next line for the if. Otherwise, both setting
> breakpoints and reading the code becomes more difficult.
 
I was trying to follow local style, but sure, I'll change that before checking in.

Comment 12

[Aaron Leventhal]

I suppose there is a question of whether the character should still get deleted, even though we can't report it correctly. Or, is there another more reliable way to get the node for the deletion? Will we now get "backspace/delete key didn't work" bugs?

Comment 13

[Frank Wein [:mcsmurf]]

I found a way to reproduce this (at least in SeaMonkey and on Windows): Go to any URL/website. Now press the Tab key a few times so that the URL in the url bar gets selected (use Shift+Tab to move the focus backwards, this should be faster). Now press the right arrow key so that the selection disappears and the cursor is at the end of the URL. Press Backspace and SeaMonkey should crash.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #13)
> I found a way to reproduce this (at least in SeaMonkey and on Windows): Go to
> any URL/website. Now press the Tab key a few times so that the URL in the url
> bar gets selected (use Shift+Tab to move the focus backwards, this should be
> faster). Now press the right arrow key so that the selection disappears and the
> cursor is at the end of the URL. Press Backspace and SeaMonkey should crash.

This 'works' also in FF/Linux

Comment 15

[Simon Montagu :smontagu]

Comment on attachment 238481 [details] [diff] [review]
null-check focusNode

With those steps to reproduce, there is still a crash with this patch.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

http://lxr.mozilla.org/seamonkey/source/editor/libeditor/base/nsEditor.cpp#4623
is doing probably something wrong. Is the offset actually right?

Comment 17

[Mike Gao]

This can also be reproduced by one of the following:
1. alt + d, right arrow, backspace
2. focus on location bar, ctr + a, right arrow, backspace

I also noticed

http://lxr.mozilla.org/seamonkey/source/editor/libeditor/base/nsEditor.cpp#4619
deleteCharData = do_QueryInterface(deleteNode);

deleteCharData is null when this problem happen. 

It should not be null?

Comment 18

[Mike Gao]

(In reply to comment #16)
> http://lxr.mozilla.org/seamonkey/source/editor/libeditor/base/nsEditor.cpp#4623
> is doing probably something wrong. Is the offset actually right?
> 
4618       selection->GetFocusNode(getter_AddRefs(deleteNode));
4619       deleteCharData = do_QueryInterface(deleteNode);
4620       selection->GetFocusOffset(&deleteCharOffset);
4621       if (!deleteCharData) {
4622         nsCOMPtr<nsINode> parentNode(do_QueryInterface(deleteNode));
4623         deleteNode = do_QueryInterface(parentNode->GetChildAt(deleteCharOffset));
4624         deleteCharData = do_QueryInterface(deleteNode);
4625         deleteCharOffset = 0;
4626       }
4627       if (aAction == ePrevious && --deleteCharOffset < 0) {
4628         // Backspace pressed at beginning of node, so get the previous node
4629         nsCOMPtr<nsIDOMNode> previousNode;
4630         deleteNode->GetPreviousSibling(getter_AddRefs(previousNode));
4631         deleteNode.swap(previousNode);

the offset is re-set at 
4625         deleteCharOffset = 0;

is this right?

And at line 4620, deleteCharOffset is 1.

Comment 19

[Simon Montagu :smontagu]

The right arrow is moving the caret out of the location field. This is most likely a regression from bug 300131.

Comment 20

[Uri Bernstein]

(In reply to comment #19)
> The right arrow is moving the caret out of the location field. This is most
> likely a regression from bug 300131.

Except that I can reproduce it in a 2006-08-31 build, way before the bug 300131 patch landed.

Comment 21

[Simon Montagu :smontagu]

Uri, what exactly is the "it" you can reproduce in a 2006-08-31 build? The crash?

Comment 22

[Uri Bernstein]

(In reply to comment #21)
> Uri, what exactly is the "it" you can reproduce in a 2006-08-31 build? The
> crash?
> 

Yes, with the steps described in comment 13. Are we talking of something else now?

Comment 23

[Simon Montagu :smontagu]

It still seems likely from the talkback data that bug 300131 triggered some reproduction scenario. I won't be able to follow this up for the next 24 hours, though.

Comment 24

[Uri Bernstein]

To me, the talkback data strongly suggests that bug 344423 is the culprit.

Unfortunately, I'm not at a machine where I can debug, and I won't be at one for the following week+ (and then I won't have time to debug).

Comment 25

[Mike Gao]

(In reply to comment #19)
> The right arrow is moving the caret out of the location field. This is most
> likely a regression from bug 300131.
> 

I tested the code without patch for bug 300131 on Linux, the problem disappeared.

BTW: I did not reproduce this bug with 2006-09-11 and 2006-08-31 build on Windows.

Still debugging.

Comment 26

[Mike Gao]

> 
> I tested the code without patch for bug 300131 on Linux, the problem
> disappeared.
> 
> BTW: I did not reproduce this bug with 2006-09-11 and 2006-08-31 build on
> Windows.
> 
> Still debugging.
> 

http://lxr.mozilla.org/seamonkey/source/editor/libeditor/base/nsEditor.cpp#4619
4619       deleteCharData = do_QueryInterface(deleteNode);
4620       selection->GetFocusOffset(&deleteCharOffset);
I reversed the patch for bug 300131. With the steps in comment #13 or #17, deleteCharData is not null and deleteCharOffset gets the right value.

Comment 27

[Uri Bernstein]

If I set layout.selection.caret_style to 1 (which is the way to get the default Linux/Windows behavior on Mac), I also don't crash with the 2006-08-31 build.

Try setting layout.selection.caret_style to 2, and you'll probably be able to reproduce the crash with that build on Windows.

So while the patch on bug 300131 might have changed something that affects the behavior with the default Windows/Linux setting, I still think that the root problem is with bug 344423.

Comment 28

[Simon Montagu :smontagu]

Here's a brain dump of what I've been able to work out about this bug.

There are two slightly different scenarii here, though they both end up crashing in the same place for the same reason.

Notice first that when the whole of a text control is selected, either by focusing the control or by "Select All", the anchor and focus of the selection are in the root node (nsHTMLDivElement), not the text node(s) within the text control (see nsTextControlFrame::SelectAllContents()) This is by design, per bug 188440 comment 8.

With layout.selection.caret_style set to 2, pressing left or right arrow when there is a (non-collapsed) selection collapses the selection to the beginning or end without moving the caret.

When the whole text control is selected, this leaves the selection in the nsHTMLDivElement and the offset at 0 when collapsed to the beginning, or 1 when collapsed to the end (assuming there is only one text node in the control).

With layout.selection.caret_style set to 1, pressing left or right arrow when there is a (non-collapsed) selection collapses the selection and attempts to move the caret left or right. Since we are at the end of the text control in the right arrow case, this leads to a call to GetFrameFromDirection() from PeekOffset which fails. Before bug 300131, PeekOffset would then leave the selection at the end of the text node, and return NS_OK. Since bug 300131, PeekOffset returns NS_ERROR_FAILURE when GetFrameFromDirection fails, and then nsFrameSelection::MoveCaret restores the original selection nodes and offset.

So, in both these cases nsEditor::DeleteSelectionImpl receives a selection with the nsHTMLDivElement as focusNode, and focusOffset set to 1. This means that the call to GetChildAt() at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/editor/libeditor/base/nsEditor.cpp&rev=1.475&mark=4623#4615 returns null, which causes the crash.

Comment 29

[Simon Montagu :smontagu]

Mats, I see from bug 299417 comment 13 that you have covered similar ground in the past. What do you think about changing nsTextControlFrame::SelectAllContents() to select the text nodes? In other words, to restore the original behaviour, which bug 88049 removed, and bug 188440 changed to the current behaviour of selecting the root node.

It would be more expensive than the current behaviour, but I think it would simplify a lot of code (where we currently have special-casing) and avoid bugs like this one (where we don't).

Comment 30

[Jesse Ruderman]

Related to bug 352759, "Right arrow key (to collapse selection to right) does not scroll textbox to show caret"?

Comment 31

[Adam Guthrie]

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

Comment 32

[Adam Guthrie]

Bug 353515 has reliable steps to reproduce that yield a similar crash.

Comment 33

[(not reading, please use [e-mail] instead)]

I ran into this, but I'm not sure if I was deleting chars from the URL bar, or from the find in page text box at the bottom of the browser.


also, I was on my own, mac, trunk, debug build: 

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a1) Gecko/20060921 Minefield/3.0a1

I'll attach the crash log.

Comment 34

[(not reading, please use [e-mail] instead)]

Attachment: crash report

Comment 35

[Simon Montagu :smontagu]

Attachment: Wallpaper patch

This prevents the crash, but the deletion won't be reported correctly when following the steps to reproduce. It also won't be reported correctly with the following (non-crashing) steps:

Alt-D to select URL field
Press left arrow
Press Delete

I think that there are two possible long-term solutions: either change the behaviour of nsTextControlFrame::SelectAllContents() as I suggest in comment 29, or somehow drill down from the root node to the text nodes in this code. Aaron, would you rather wait for the long-term solution, or take this wallpaper patch and make that a new bug?

Comment 36

[(not reading, please use [e-mail] instead)]

see also bug #353515, for other steps to reproduce (to consistently cause the crasher on the trunk)

Comment 37

[Simon Montagu :smontagu]

Yeah, I should have mentioned that attachment 239570 [details] [diff] [review] also prevents the crash with the steps to reproduce from bug 353515.

Comment 38

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I've been hitting this roughly once every 1-2 hours of browsing.  We need to either get the wallpaper patch in or back out bug 344423; Minefield builds are painful to use because of this.

Comment 39

[Aaron Leventhal]

Comment on attachment 239570 [details] [diff] [review]
Wallpaper patch

So if deleteCharOffset >=childCount does that mean we're at the end of the text? Can some comment be added to the code, perhaps along with // XXX Here's what we need to fix?

Comment 40

[Aaron Leventhal]

Attachment: Fix the special case where focusOffset is past the end

I tried several ways of conserving code for this special case, in order to save a few lines, but it actually just made things too confusing.

Comment 41

[[e-mail]]

Comment on attachment 240044 [details] [diff] [review]
Fix the special case where focusOffset is past the end

So, having hit this bug once and forgotten to debug it to verify my suspicion, does the following hold:
a) We're deleting backwards
b) The caret is at the end of an element
In that case, I would change
deleteNode->GetPreviousSibling(getter_AddRefs(previousNode));
into
deleteNode = do_QueryInterface(parentNode->GetChildAt(deleteCharOffset - 1));
Or am I assuming too much?

Comment 42

[Aaron Leventhal]

Something like that might work. I'm not sure what happens if you hit the delete key (forward delete) and you're at the end. I'll play around with that.

Comment 43

[Aaron Leventhal]

Actually, it's not so easy to just change that, because the deleteCharOffset can be:
1) a character offset when the parent node is a text node
or 
2) a node offset when the parent node is an element

So, playing around with that line I quickly realized it's not so easy to simplify using that trick. You'd have to break out nsINode *parentNode and define it differently depending on what the deleteNode is. Then you'd have to have 2 separate offset variables, 1 for char offset and 1 for node offset. Then you'd actually have to calculate the node offset in the textnode/charoffset case.

So I'm back to the latest patch.

Comment 44

[Jesse Ruderman]

This patch fixes the crash but does not fix the other regression (bug 352759).

Comment 45

[Aaron Leventhal]

(In reply to comment #44)
> This patch fixes the crash but does not fix the other regression (bug 352759).
Jesse, I think that's unrelated to bug 344423, which only affected character deletion via Delete or Backsapce. That other regression is for right arrow key.

Comment 46

[[e-mail]]

Attachment: Reuse existing code to locate the character that actually gets deleted.

Rather than trying, and failing (see CreateTxnForDeleteCharater et al), I thought it would be easier to reuse the existing logic by returning the deleted node. Unfortunately my refcounting seems to be out, I'm still crashing :-(

Comment 47

[Håkan Waara]

I just hit this crash too, fwiw, on OS X.

Comment 48

[Steve England [:stevee]]

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

Comment 49

[[e-mail]]

Comment on attachment 240304 [details] [diff] [review]
Reuse existing code to locate the character that actually gets deleted.

I must remember to link after compiling :-[

Comment 50

[Aaron Leventhal]

Comment on attachment 240304 [details] [diff] [review]
Reuse existing code to locate the character that actually gets deleted.

Neil, can't we just use the brainless patch first so people can work with trunk without crashing? To review this properly I need to have more time than I have today.

Comment 51

[[e-mail]]

Comment on attachment 240044 [details] [diff] [review]
Fix the special case where focusOffset is past the end

I'd accept wallpaper e.g. calling Will/DidDeleteSelection if you can't easily find what really gets deleted.

Comment 52

[[e-mail]]

Oh, and you'll need a big // XXX FIX ME

Comment 53

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Guys, please back out bug 344423 if you cannot fix or at least wallpaper over this today, otherwise I will back out the patch for bug 344423 tonight.

Comment 54

[Aaron Leventhal]

Comment on attachment 240304 [details] [diff] [review]
Reuse existing code to locate the character that actually gets deleted.

This works in my tests and the code looks good.

One thing, I think thre should be a comment before the altered methods which explains that *aOffset and *aLength are not set unless *aNode is text. In fact, it might be a good idea to initialize to something at the start of each method, so that they're not random. Your call on that one.

Comment 55

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 240304 [details] [diff] [review]
Reuse existing code to locate the character that actually gets deleted.

Excellent!

Comment 56

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Comment on attachment 240304 [details] [diff] [review]
Reuse existing code to locate the character that actually gets deleted.

I don't like the "friend nsEditor;" in DeleteTextTxn.h and I would prefer two public helpers to retrieve the members you need. Since you don't deal with an nsITransaction but a DeleteTxtTxn, that is ok.

More generally, that's probably a good thing to add to all transactions if we want an Undo/Redo system "à la GIMP" with very informational details for each transaction.

Comment 57

[[e-mail]]

Comment on attachment 240304 [details] [diff] [review]
Reuse existing code to locate the character that actually gets deleted.

glazou unset aaronlev's + instead of -ing his ?

Comment 58

[[e-mail]]

Attachment: Updated for bitrot and review comment

Comment 59

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Comment on attachment 241160 [details] [diff] [review]
Updated for bitrot and review comment

yep! r=[e-mail]

Comment 60

[[e-mail]]

Fix checked in.

Comment 61

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

Verified FIXED using build 2006-10-14-13 of SeaMonkey trunk under Windows XP; no crash.