456273 - Some key events in contenteditable element crashes [@ nsNativeKeyBindings::KeyPress]

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P1)

Description

[Mats Palmgren (inactive)]

Some key events in contenteditable element crashes.

STEPS TO REPRODUCE
1. load the attached testcase
2. click on "text"
3. press CTRL+Z

ACTUAL RESULT
Crash in nightly trunk, Firefox 3 does not crash.

Patch coming up...

Comment 1

[Mats Palmgren (inactive)]

Attachment: Testcase

Comment 2

[Mats Palmgren (inactive)]

Attachment: stack

Comment 3

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

The bug is in nsXBLWindowKeyHandler::WalkHandlers() which doesn't
test the return value from nsContentUtils::DOMEventToNativeKeyEvent()
before calling the KeyPress/Up/Down methods, so 'nativeEvent' isn't
initialized properly ('nativeEvent' field has random stack value).

I took the liberty of changing the param type from nsIDOMEvent to
nsIDOMKeyEvent for DOMEventToNativeKeyEvent() since I don't think
it should be called for other types of events.

The mochitest is really a crash test but needs privileges (we can
make it a crash test when bug 448676 is fixed).

Comment 4

[Mats Palmgren (inactive)]

Hmm, it looks like Firefox 3 should have the same bug:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/content/xbl/src/nsXBLWindowKeyHandler.cpp&rev=1.47&root=/cvsroot&mark=377-379#375
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/content/base/src/nsContentUtils.cpp&rev=1.309&root=/cvsroot&mark=4020#3990

Comment 5

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

Comment on attachment 339674 [details] [diff] [review]
Patch rev. 1

>+nsTextInputListener::KeyDown(nsIDOMEvent *aDOMEvent)
> {
>+  nsCOMPtr<nsIDOMKeyEvent> keyEvent(do_QueryInterface(aDOMEvent));
>+  NS_ENSURE_TRUE(keyEvent, NS_ERROR_INVALID_ARG);

>+nsTextInputListener::KeyPress(nsIDOMEvent *aDOMEvent)
> {
>+  nsCOMPtr<nsIDOMKeyEvent> keyEvent(do_QueryInterface(aDOMEvent));
>+  NS_ENSURE_TRUE(keyEvent, NS_ERROR_INVALID_ARG);


>+nsTextInputListener::KeyUp(nsIDOMEvent *aDOMEvent)
> {
>+  nsCOMPtr<nsIDOMKeyEvent> keyEvent(do_QueryInterface(aDOMEvent));
>+  NS_ENSURE_TRUE(keyEvent, NS_ERROR_INVALID_ARG);
Because this is using specialized nsIDOMKeyListener, aDOMEvent should be always
nsIDOMKeyEvent, but I think it is still better to have those checks.

Comment 6

[Johnny Stenback (:jst)]

Not blocking on this, but we really want this patch reviewed and landed asap!

Comment 7

[Mats Palmgren (inactive)]

Jonas, ping?

Comment 8

[Mats Palmgren (inactive)]

http://hg.mozilla.org/mozilla-central/rev/0fbeaa3dd32e

-> FIXED