1034103 - crash in nsEditor::BeginIMEComposition(mozilla::WidgetCompositionEvent*)

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[Martijn Wargers (dead)]

Attachment: crash4.xhtml

See testcase, you need to have the specialpowers extension to see the crash:
http://people.mozilla.org/~mwargers/extensions/specialpowers/specialpowers_20140612.xpi
Then load the testcase and click inside the document.
Result->crash

This bug was filed from the Socorro interface and is 
report bp-2d8a9f86-4b6c-4547-89c4-b189d2140703.
=============================================================
0 	XUL 	nsEditor::BeginIMEComposition(mozilla::WidgetCompositionEvent*) 	editor/libeditor/base/nsEditor.cpp
1 	XUL 	nsEditorEventListener::HandleEvent(nsIDOMEvent*) 	editor/libeditor/base/nsEditorEventListener.cpp
2 	XUL 	mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) 	dom/events/EventListenerManager.cpp
3 	XUL 	mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) 	dom/events/EventListenerManager.cpp
4 	XUL 	mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) 	dom/events/EventDispatcher.cpp
5 	XUL 	mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) 	dom/events/EventDispatcher.cpp
6 	XUL 	mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsCOMArray<mozilla::dom::EventTarget>*) 	dom/events/EventDispatcher.cpp
7 	XUL 	mozilla::EventDispatcher::DispatchDOMEvent(nsISupports*, mozilla::WidgetEvent*, nsIDOMEvent*, nsPresContext*, nsEventStatus*) 	dom/events/EventDispatcher.cpp
8 	XUL 	nsINode::DispatchEvent(nsIDOMEvent*, bool*) 	content/base/src/nsINode.cpp
9 	XUL 	mozilla::dom::EventTarget::DispatchEvent(mozilla::dom::Event&, mozilla::ErrorResult&) 	content/base/src/nsINode.cpp
10 	XUL 	mozilla::dom::EventTargetBinding::dispatchEvent 	obj-firefox/x86_64/dom/bindings/EventTargetBinding.cpp

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oh, good catch. I wonder why did you find such hacky case, by reading the code of nsEditor?

Comment 2

[Martijn Wargers (dead)]

No, by fuzzing.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1 Create a test for checking if editor ignores created events in chrome script

This patch checks that composition events which are created by JS are not handled by editor. (It's impossible to create mozilla::TextComposition for such case since such composition events don't have WidgetGUIEvent::widget)

And also I confirmed that this patch can reproduce this bug.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.2 editor shouldn't accept all events which are created with incorrect event interface

This patch makes IsAcceptableInputEvent() use Widget*Event this allows following new check and might improve performance because of avoiding some virtual calls, QI and comparing string of event type.

nsEditor should not accept all events whose message is incorrect for the event instance. At this time, the message value is always NS_USER_DEFINED_EVENT as far as I researched. So, this check should be performed for all events.

This patch also changes QI to nsIDOMMouseEvent since it equals to use WidgetEvent::IsUsingCorrdinates() and its meaning is more explicitly.

Currently, ns*Editor handles "mousedown", "mouseup", "click", "dragenter", "dragover", "dragexit" and "drop".
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/base/nsEditorEventListener.cpp#136

So, the DOM event instance must be DragEvent or MouseEvent. DragEvent inherits MouseEvent. MouseEvent inherits nsIDOMMouseEvent. Therefore, IsUsingCoordinates() always same as QI'ing nsIDOMMouseEvent in this method.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oh, and also, QI'ing nsIDOMMouseEvent is always same as widgetEvent->AsMouseEventBase(). MouseEvent, DragEvent, WheelEvent, MouseScrollEvent, PointerEvent and SimpleGestureEvent inherit nsIDOMMouseEvent. WidgetMouseEvent, WidgetDragEvent, WidgetWheelEvent, WidgetMouseScrollEvent, WidgetPointerEvent and WidgetSimpleGestureEvent inherit WidgetMouseEventBase.

http://mxr.mozilla.org/mozilla-central/ident?i=nsIDOMMouseEvent&tree=mozilla-central&filter=Event.h
http://mxr.mozilla.org/mozilla-central/ident?i=MouseEvent&tree=mozilla-central&filter=Event.h

http://mxr.mozilla.org/mozilla-central/ident?i=WidgetMouseEventBase&tree=mozilla-central&filter=Events.h
http://mxr.mozilla.org/mozilla-central/ident?i=WidgetMouseEvent&tree=mozilla-central&filter=Events.h

Comment 6

[(no longer active)]

Comment on attachment 8451818 [details] [diff] [review]
part.2 editor shouldn't accept all events which are created with incorrect event interface

Review of attachment 8451818 [details] [diff] [review]:
-----------------------------------------------------------------

I'm fine with Olli's review here.

Comment 7

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

Comment on attachment 8451818 [details] [diff] [review]
part.2 editor shouldn't accept all events which are created with incorrect event interface

A bit ugly, but fine.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cab8bac9117a
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d3889868608

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cab8bac9117a
https://hg.mozilla.org/mozilla-central/rev/0d3889868608

Comment 10

[vasilicamihasca]

I was able to reproduce this bug on Nightly 33.0a1 (2014-07-03), using Mac OSX 10.9.5.

Verified fixed on Mac OSX 10.9.5 using Nightly 35.0a1 (2014-10-12).

This fix can be marked as verified.

[bugday-20141015]

Comment 11

[Martijn Wargers (dead)]

Thanks!