759748 - crash in nsHTMLEditRules::WillDoAction

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[Scoobidiver (away)]

It first appeared in 15.0a1/20120527. The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e4574b46f0ba&tochange=133aa3a2ef0a
It's likely a regression from bug 748307.

Signature 	nsHTMLEditRules::WillDoAction(nsTypedSelection*, nsRulesInfo*, bool*, bool*) More Reports Search
UUID	cf244791-46d1-46fa-916d-cad542120529
Date Processed	2012-05-29 20:47:16
Uptime	146
Install Age	1.1 hours since version was first installed.
Install Time	2012-05-29 19:43:31
Product	Firefox
Version	15.0a1
Build ID	20120529030518
Release Channel	nightly
OS	Windows NT
OS Version	6.1.7601 Service Pack 1
Build Architecture	x86
Build Architecture Info	GenuineIntel family 6 model 37 stepping 2
Crash Reason	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address	0x10
App Notes 	
AdapterVendorID: 0x1002, AdapterDeviceID: 0x68e0, AdapterSubsysID: 033d1025, AdapterDriverVersion: 8.951.0.0
D2D? D2D+ DWrite? DWrite+ D3D10 Layers? D3D10 Layers+ 
EMCheckCompatibility	True	
Total Virtual Memory	4294836224
Available Virtual Memory	3685588992
System Memory Use Percentage	47
Available Page File	6080536576
Available Physical Memory	2182213632

Frame 	Module 	Signature 	Source
0 	xul.dll 	nsHTMLEditRules::WillDoAction 	editor/libeditor/html/nsHTMLEditRules.cpp:540
1 	xul.dll 	nsPlaintextEditor::InsertText 	editor/libeditor/text/nsPlaintextEditor.cpp:791
2 	xul.dll 	nsPlaintextEditor::TypedText 	editor/libeditor/text/nsPlaintextEditor.cpp:403
3 	xul.dll 	nsHTMLEditor::TypedText 	editor/libeditor/html/nsHTMLEditor.cpp:1241
4 	xul.dll 	nsHTMLEditor::HandleKeyPressEvent 	editor/libeditor/html/nsHTMLEditor.cpp:692
5 	xul.dll 	nsEditorEventListener::KeyPress 	editor/libeditor/base/nsEditorEventListener.cpp:458
6 	xul.dll 	nsEditorEventListener::HandleEvent 	editor/libeditor/base/nsEditorEventListener.cpp:295
7 	xul.dll 	nsEventListenerManager::HandleEventInternal 	content/events/src/nsEventListenerManager.cpp:868
8 	xul.dll 	nsEventTargetChainItem::HandleEventTargetChain 	content/events/src/nsEventDispatcher.cpp:340
9 	xul.dll 	nsEventTargetChainItem::HandleEventTargetChain 	content/events/src/nsEventDispatcher.cpp:372
10 	xul.dll 	nsEventDispatcher::Dispatch 	content/events/src/nsEventDispatcher.cpp:643
11 	xul.dll 	PresShell::HandleEventInternal 	layout/base/nsPresShell.cpp:6362
12 	xul.dll 	PresShell::HandleEvent 	layout/base/nsPresShell.cpp:5977
13 	xul.dll 	PresShell::HandleEvent 	layout/base/nsPresShell.cpp:5630
14 	xul.dll 	nsViewManager::DispatchEvent 	view/src/nsViewManager.cpp:865
15 	xul.dll 	AttachedHandleEvent 	view/src/nsView.cpp:158
16 	xul.dll 	nsWindow::DispatchEvent 	widget/windows/nsWindow.cpp:3468
17 	xul.dll 	nsWindow::DispatchWindowEvent 	widget/windows/nsWindow.cpp:3494
18 	xul.dll 	nsWindow::DispatchKeyEvent 	widget/windows/nsWindow.cpp:3526
19 	xul.dll 	nsWindow::OnChar 	widget/windows/nsWindow.cpp:6647
20 	xul.dll 	nsWindow::OnKeyDown 	widget/windows/nsWindow.cpp:6366
21 	xul.dll 	nsWindow::ProcessKeyDownMessage 	widget/windows/nsWindow.cpp:5597
22 	xul.dll 	nsWindow::ProcessMessage 	widget/windows/nsWindow.cpp:4737
23 	xul.dll 	nsWindow::WindowProcInternal 	widget/windows/nsWindow.cpp:4289
24 	xul.dll 	CallWindowProcCrashProtected 	xpcom/base/nsCrashOnException.cpp:32
25 	xul.dll 	nsWindow::WindowProc 	widget/windows/nsWindow.cpp:4231
26 	user32.dll 	InternalCallWinProc 	
...

More reports at:
https://crash-stats.mozilla.com/report/list?signature=nsHTMLEditRules%3A%3AWillDoAction%28nsTypedSelection*%2C+nsRulesInfo*%2C+bool*%2C+bool*%29

Comment 1

[(no longer active)]

Hmm, Aryeh, can |range| be null here? <http://hg.mozilla.org/mozilla-central/annotate/79262a88881d/editor/libeditor/html/nsHTMLEditRules.cpp#l540>

Comment 2

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

In principle probably yes -- someone might have called removeAllRanges() or such, or the user might have just not interacted with the page.  That was stupid of me.  :(

This reminds me of an old proposal I had -- I think it would be great if we specced a Selection to always have exactly one range.  So initially it would be collapsed at (document, 0), and removing the last range would re-collapse it there.  That would save authors plenty of bugs too!

I'll write a patch ASAP.

Comment 3

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Steps to reproduce:

1) Load this page:

data:text/html,<!doctype html>
<div contenteditable>abc</div>
<script>setInterval(function() {
  getSelection().removeAllRanges()
}, 1000)</script>

2) Click in the editable region

3) Wait for the cursor to disappear

4) Hit a key

Patch in a few minutes.

Comment 4

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch v1

This fixes the crash.  It doesn't even require user interaction; document.execCommand("inserttext", false, "a") does the trick.  The testcase also caught an assertion being hit in increaseFontSize/decreaseFontSize, so I fixed that too -- nothing was checking the return code of GetStartNodeAndOffset.

I don't particularly like listing all the commands like this, since they're prone to get out of date, but I guess it works.

ExecCommand() should really check if the command is enabled, and return false without executing it if so.  That would have prevented this crash, at least the execCommand()-triggered variant.

https://tbpl.mozilla.org/?tree=Try&rev=f76bc311ecf3

Comment 5

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/550e64dd5656

All the Android XUL opt tests were red in my try push, but I'm guessing that's just Android being Android and nothing to do with my change.  I guess we'll find out!

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/550e64dd5656

Comment 7

[Paul Silaghi, QA [:pauly]]

No crash loading the STR in comment 3.
Verified fixed on FF 15b3 on Win 7/64, Ubuntu 12.04 and Mac OS X 10.6.