771983 - crash in nsHTMLEditor::DoInsertHTMLWithContext @ nsEditor::GetNodeLocation

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Scoobidiver (away)]

It first appeared in 16.0a1/20120706. The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=675f55c4310c&tochange=4b1249ae1906

Here are some comments:
"Inserting a text in google (CTRL+DEL, CTRL+INS)"
"It crashed when pasting "The House of Wisdom (Arabic: بيت الحكمة‎; Bayt Ul-Hikma)" (and the rest of the paragraph from that Wikipedia page[1], but probably choked on the arabic characters) into the Tumblr bookmarklet blog submission form. Firefox crashes repeatably. Can't paste bookmarklet code in crash reporter... [1] https://en.wikipedia.org/wiki/House_of_Wisdom"

Signature 	nsEditor::GetNodeLocation(nsIDOMNode*, nsCOMPtr<nsIDOMNode>*, int*) More Reports Search
UUID	6ec7c04f-8867-498b-8105-4606a2120708
Date Processed	2012-07-08 18:59:11
Uptime	133039
Last Crash	2.1 days before submission
Install Age	1.5 days since version was first installed.
Install Time	2012-07-07 06:01:29
Product	Firefox
Version	16.0a1
Build ID	20120706075126
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 5
Crash Reason	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address	0x0
App Notes 	
AdapterVendorID: 0x10de, AdapterDeviceID: 0x0a7a, AdapterSubsysID: 00000000, AdapterDriverVersion: 8.17.12.6754
D2D? D2D+ DWrite? DWrite+ D3D10 Layers? D3D10 Layers+ 
EMCheckCompatibility	True
Adapter Vendor ID	0x10de
Adapter Device ID	0x0a7a
Total Virtual Memory	4294836224
Available Virtual Memory	3334799360
System Memory Use Percentage	74
Available Page File	4439797760
Available Physical Memory	1067667456

Frame 	Module 	Signature 	Source
0 	xul.dll 	nsEditor::GetNodeLocation 	editor/libeditor/base/nsEditor.cpp:3133
1 	xul.dll 	nsHTMLEditor::DoInsertHTMLWithContext 	editor/libeditor/html/nsHTMLDataTransfer.cpp:716
2 	xul.dll 	nsHTMLEditor::InsertFromTransferable 	editor/libeditor/html/nsHTMLDataTransfer.cpp:1379
3 	xul.dll 	nsHTMLEditor::Paste 	editor/libeditor/html/nsHTMLDataTransfer.cpp:1598
4 	xul.dll 	nsPasteCommand::DoCommand 	editor/libeditor/base/nsEditorCommands.cpp:399
5 	xul.dll 	nsControllerCommandTable::DoCommand 	embedding/components/commandhandler/src/nsControllerCommandTable.cpp:158
6 	xul.dll 	nsBaseCommandController::DoCommand 	embedding/components/commandhandler/src/nsBaseCommandController.cpp:137
7 	xul.dll 	nsXBLPrototypeHandler::DispatchXBLCommand 	content/xbl/src/nsXBLPrototypeHandler.cpp:472
8 	xul.dll 	nsXBLPrototypeHandler::ExecuteHandler 	content/xbl/src/nsXBLPrototypeHandler.cpp:221
9 	xul.dll 	nsXBLWindowKeyHandler::WalkHandlersAndExecute 	content/xbl/src/nsXBLWindowKeyHandler.cpp:526
10 	xul.dll 	nsXBLWindowKeyHandler::WalkHandlersInternal 	content/xbl/src/nsXBLWindowKeyHandler.cpp:445
11 	xul.dll 	nsXBLWindowKeyHandler::WalkHandlers 	content/xbl/src/nsXBLWindowKeyHandler.cpp:319
12 	xul.dll 	nsXBLWindowKeyHandler::HandleEvent 	content/xbl/src/nsXBLWindowKeyHandler.cpp:365
13 	xul.dll 	nsEventListenerManager::HandleEventInternal 	content/events/src/nsEventListenerManager.cpp:893
14 	xul.dll 	nsEventTargetChainItem::HandleEventTargetChain 	content/events/src/nsEventDispatcher.cpp:339
15 	xul.dll 	nsEventTargetChainItem::HandleEventTargetChain 	content/events/src/nsEventDispatcher.cpp:371
16 	xul.dll 	nsEventDispatcher::Dispatch 	content/events/src/nsEventDispatcher.cpp:639
17 	xul.dll 	PresShell::HandleEventInternal 	layout/base/nsPresShell.cpp:6408
18 	xul.dll 	PresShell::HandleEvent 	layout/base/nsPresShell.cpp:6023
19 	xul.dll 	PresShell::HandleEvent 	layout/base/nsPresShell.cpp:5676
20 	xul.dll 	nsViewManager::DispatchEvent 	view/src/nsViewManager.cpp:863
21 	xul.dll 	AttachedHandleEvent 	view/src/nsView.cpp:159
22 	xul.dll 	nsWindow::DispatchEvent 	widget/windows/nsWindow.cpp:3504
...

More reports at:
https://crash-stats.mozilla.com/report/list?signature=nsEditor%3A%3AGetNodeLocation%28nsIDOMNode*%2C+nsCOMPtr%3CnsIDOMNode%3E*%2C+int*%29
https://crash-stats.mozilla.com/report/list?signature=nsEditor%3A%3AGetNodeLocation

Comment 1

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

That'll be bug 769967 part 2, which changed GetNodeLocation to MOZ_ASSERT if a null pointer was passed instead of NS_ENSURE_TRUE.  Perhaps the NS_ENSURE_TRUE should have been kept as well to be on the safe side.

Comment 2

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

Attachment: Patch, re-adding a null check

There's no test here, because I couldn't reproduce the crash.  It's not obvious from code inspection what would cause it.  The null node passed to GetNodeLocation was obtained from nsEditor::SplitNodeDeep, which is too complicated for me to figure out what code path might be causing it without a reproducible test-case.

This should stop any callers from crashing if they previously didn't crash, as long as they null-check the returned parent.  I checked the nsHTMLEditor::DoInsertHTMLWithContext call that was triggering this crash, and all it does with the result is pass it to Selection::Collapse, which null-checks with NS_ENSURE_*, so it should stop crashing (although it's probably still broken in some other way).

There could conceivably be callers that passed a null child sometimes and did NS_ENSURE_SUCCESS, then dereferenced the parent without null-checking it, such that when they passed a non-null child its parent was always non-null.  Any such callers will still crash after this patch, and will have to be fixed individually.

I didn't bother with a try run, because anything that hits the added line will have already crashed in debug builds.

Comment 3

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b8bea26cdfb

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6b8bea26cdfb