636107 - Crash [@ nsEditor::CreateTxnForDeleteText ]

Status: RESOLVED DUPLICATE of bug 636074

(Core :: Editor, defect, critical)

Description

[Scoobidiver (away)]

It is a new crash signature that first appeared in 4.0b12pre/20110220.
It is #69 top crasher in 4.0b12pre over the last 3 days.

Signature	nsEditor::CreateTxnForDeleteText
UUID	ffa9cc43-ad4f-40b0-a467-418f32110222
Time 	2011-02-22 17:18:14.41817
Uptime	7451
Last Crash	137771 seconds (1.6 days) before submission
Install Age	7451 seconds (2.1 hours) since version was first installed.
Product	Firefox
Version	4.0b12pre
Build ID	20110222030357
Branch	2.0
OS	Mac OS X
OS Version	10.6.6 10J567
CPU	x86
CPU Info	family 6 model 23 stepping 6
Crash Reason	EXC_BAD_ACCESS / KERN_INVALID_ADDRESS
Crash Address	0x68ffff00
App Notes 	Renderers: 0x21a00,0x20400

Frame 	Module 	Signature [Expand] 	Source
0 		@0x68ffff00 	
1 	XUL 	nsEditor::CreateTxnForDeleteText 	nsAutoPtr.h:969
2 	XUL 	nsEditor::CreateTxnForDeleteCharacter 	editor/libeditor/base/nsEditor.cpp:4764
3 	XUL 	nsEditor::CreateTxnForDeleteInsertionPoint 	editor/libeditor/base/nsEditor.cpp:4827
4 	XUL 	nsEditor::CreateTxnForDeleteSelection 	editor/libeditor/base/nsEditor.cpp:4719
5 	XUL 	nsEditor::DeleteSelectionImpl 	editor/libeditor/base/nsEditor.cpp:4280
6 	XUL 	nsPlaintextEditor::DeleteSelection 	editor/libeditor/text/nsPlaintextEditor.cpp:788
7 	XUL 	nsEditor::HandleKeyPressEvent 	editor/libeditor/base/nsEditor.cpp:5104
8 	XUL 	nsHTMLEditor::HandleKeyPressEvent 	editor/libeditor/html/nsHTMLEditor.cpp:691
9 	XUL 	nsEditorEventListener::KeyPress 	editor/libeditor/base/nsEditorEventListener.cpp:362
10 	XUL 	nsEventListenerManager::HandleEventInternal 	content/events/src/nsEventListenerManager.cpp:175
11 	XUL 	nsEventTargetChainItem::HandleEventTargetChain 	content/events/src/nsEventListenerManager.h:146
12 	XUL 	nsEventTargetChainItem::HandleEventTargetChain 	content/events/src/nsEventDispatcher.cpp:395
13 	XUL 	nsEventDispatcher::Dispatch 	content/events/src/nsEventDispatcher.cpp:628
14 	XUL 	PresShell::HandleEventInternal 	layout/base/nsPresShell.cpp:7066
15 	XUL 	PresShell::HandleEvent 	layout/base/nsPresShell.cpp:6813
16 	XUL 	PresShell::HandleEvent 	layout/base/nsPresShell.cpp:6492
17 	XUL 	nsViewManager::DispatchEvent 	view/src/nsViewManager.cpp:1105
18 	XUL 	HandleEvent 	view/src/nsView.cpp:161
19 	XUL 	nsChildView::DispatchEvent 	widget/src/cocoa/nsChildView.mm:1807
20 	XUL 	nsChildView::DispatchWindowEvent 	widget/src/cocoa/nsChildView.mm:1817
21 	XUL 	-[ChildView processKeyDownEvent:] 	widget/src/cocoa/nsChildView.mm:5436
22 	XUL 	-[ChildView keyDown:] 	widget/src/cocoa/nsChildView.mm:5666
23 	AppKit 	-[NSWindow sendEvent:] 	
24 	XUL 	-[ToolbarWindow sendEvent:] 	widget/src/cocoa/nsCocoaWindow.mm:2356
25 	AppKit 	-[NSApplication sendEvent:] 	
26 	AppKit 	-[NSApplication run] 	
27 	XUL 	nsAppShell::Run 	widget/src/cocoa/nsAppShell.mm:746
28 	XUL 	nsAppStartup::Run 	toolkit/components/startup/src/nsAppStartup.cpp:220
29 	XUL 	XRE_main 	toolkit/xre/nsAppRunner.cpp:3766
30 	firefox-bin 	main 	browser/app/nsBrowserApp.cpp:158
31 	firefox-bin 	firefox-bin@0xa05 	
32 		@0x0 	

The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=657c2a92ee2b&tochange=e77f4eda0bad
The likely culprit is bug 633709.

More reports at:
https://crash-stats.mozilla.com/report/list?product=Firefox&range_value=4&range_unit=weeks&signature=nsEditor%3A%3ACreateTxnForDeleteText

Comment 1

[[e-mail]]

khuey is worried that this is exploitable, so marking s-g.

Comment 2

[Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)]

This is bad.  The problem here is that the local variable at http://hg.mozilla.org/mozilla-central/annotate/1da3405c74fd/editor/libeditor/base/nsEditor.cpp#l4825 is not initialized.  We pick up some random value from the stack and then pass it down a couple functions where it finally gets swapped into the RefPtr at http://hg.mozilla.org/mozilla-central/annotate/1da3405c74fd/editor/libeditor/base/nsEditor.cpp#l2708.  If it happens to non-null we end up calling Release on it when the RefPtr goes out of scope.

Calling virtual methods on a garbage ptr we got from the stack is not good.

Comment 3

[Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)]

Attachment: Patch.

Comment 4

[Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)]

Comment on attachment 514458 [details] [diff] [review]
Patch.

Remove some more raw ptr usage by editor to make this safe.

Comment 5

[Peter Van der Beken [:peterv]]

(In reply to comment #2)
> We pick up some random value from the stack and then pass
> it down a couple functions where it finally gets swapped into the RefPtr

Might have been beter to use .forget(...) instead of .swap(...).

Comment 6

[Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)]

Yeah, we should do that too.

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Please renominate if it turns out to not be sg:crit, but based on the previous bugs like this, we're assuming it is.

Comment 10

[:ehsan akhgari]

Kyle moved his patch over to bug 636074, and we'll take his patch there, FWIW.