208095 - Crash when I input space with kinput [@ nsEditor::InsertTextIntoTextNodeImpl]

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect, P1)

Description

[Michitomo Nakahara]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4) Gecko/20030529
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4) Gecko/20030529

Crashes Mozilla when I input space follows English text with kinput.

Reproducible: Always

Steps to Reproduce:
1.Open composer or webpage with text form.
2.Input something in English. (eg. "a")
3.Press "Shift + space" to start kinput Japanese input mode.
4.Press "Space" to input space

Actual Results:  
Crash and start "Quality feedback agent".


This problem was not cause in Windows or input Japanese with "jmode" instead
kinput (on Linux).

Comment 1

[Olivier Cahagne]

can you post Talkback ID for this crash "mozilla/bin/components/talkback/talkback" ?

Comment 2

[Michitomo Nakahara]

Thanks.
ID is "TB20695646".

And I think it happen in all PCs that uses "kinput".
I've tested my 4 PCs.(SuSE8.1,SuSE8.2,Mandrake9.1,Plamo3.1)

Comment 3

[Olivier Cahagne]

Marking NEW to get the stack.

Comment 4

[Simon Montagu :smontagu]

Attachment: Stack from TB20695646

Comment 5

[Olivier Cahagne]

Didn't find dupes, should this be moved to Editor:Core ?

Comment 6

[Simon Montagu :smontagu]

There are some similar stacks in Talkback which start with Windows IME calls,
and some which start in Javascript. Neil on IRC mentioned that he has
encountered crashes when inserting text in text nodes from script.

Comment 7

[Kathleen :Brade]

Do we know when this regression first started?  Has it ever worked?

Comment 8

[Simon Montagu :smontagu]

The oldest build in talkback with this stack is 2003021008 MozillaTrunk

Comment 9

[kinmoz]

The line numbers in the stack seem to be off a bit, even in the source from
around that time. Looking at the stack calls etc, it looks like it may be
crashing when dereferencing |mIMETextRangeList| ... did SetCompositionString()
get called prior to |InsertTextIntoTextNodeImpl()| getting called?

It looks like |SetCompositionString()| is only called when
|nsTextEditorTextListener::HandleText()| gets called ... did a handle text event
callback ever happen?

Also, another thing I noticed is that we probably leak |mIMETextRangeList| since
the original pointer gets AddRef'd in |SetCompositionString()| and there is no
corresponding Release call ... also the |mIMETextRangeList| member is a raw
pointer which never gets addref'd or released.

Comment 10

[Masaki Katakai]

Attachment: proposed patch

Even in IME mode, the event comes from widget through OnKey(), not
through IMECommitEvent(), which means there is no text range, I understand.

In this case, kinput2 tries to insert an ascii space.
mIMETextRangeList is null so mIMETextRangeList->GetLength() causes crash.

kin, what do you think?

Comment 11

[kinmoz]

Comment on attachment 125400 [details] [diff] [review]
proposed patch

Both jfrancis and I were thinking that there's still a problem here in that
kinput2 isn't triggering the IME calls we expect ... that is, we thought that
only IME events/callbacks were allowed between BeginComposition and
EndComposition events, but in this case we're getting normal OnKey event
callbacks.

I'm not exactly sure if things will work properly when mixing input from normal
means (non IME callbacks) with input from IME callbacks.

Along those lines, the null check will prevent the IME code at the top of
InsertTextIntoTextNodeImpl() from executing, but what about the IME code
towards the bottom of the method which makes assumptions that |txn| is an
IMETxn?

It looks like the IME code in the editor relies on mIMETextnode and
mIMETextOffset being set when inserting text, but that won't account for any
text added via the OnKey() callback.

Comment 12

[Kevin McCluskey (gone)]

editorbase+
-> smontagu. Simon can you collaborate with Joe and Kin on this one?

Comment 13

[Simon Montagu :smontagu]

Can anyone else confirm that the crash does not happen with
user_pref("xim.input_style", "over-the-spot");
in prefs.js?

Comment 14

[Masaki Katakai]

I got a report that over-the-spot does not have this problem.

Comment 15

[Katsuhiko Momoi]

I can confirm that the over-the-spot method does not cause a crash with
the latest branch build.

Comment 16

[Katsuhiko Momoi]

We may have to consider changing the default pref for the 
next release unless this problem gets fixed. (Maybe for L10n builds
only?)

Comment 17

[Simon Montagu :smontagu]

This seems to be an event targetting issue. Could it be a result of bug 52416?

Comment 18

[Katsuhiko Momoi]

I played with this a problem a little bit and found the following:

1. **Once** you input Japanese string successfully, **thereafter** you will not
   see this crash even if the condition is met.
2. If you input an ASCII space before you turn on the Japanese IME, you will
   not have this crash.

How likely is it that average users of JA IME will get into the condition
described by the reporter? Hard to say but it may not be that often because
of natural workarounds in 1 and 2.

Comment 19

[Simon Montagu :smontagu]

Yes, that was my experience too. Maybe a wallpaperish patch, either attachment
125400 [details] [diff] [review] or something similar in nsPlainTextEditor::InsertText, will be sufficient.

Comment 20

[Simon Montagu :smontagu]

Attachment: Another possibility

This prevents the crash by not calling IMEComposeStart() immediately on IME
activation. It is still called as soon as a non-space character is entered.

Comment 21

[Simon Montagu :smontagu]

Attachment: Both patches combined, for review

Comment 22

[Choe Hwanjin]

I have same problem on XFree86(4.3.1) Debian GNU/Linux(2.4.20)

I'm making hangul xim program based on IMdkit. 
(http://nabi.kldp.net/)
When it forward keypress event that is not accepted by xim to the application,
mozilla crashes.
This case, it use code like:

    IMForwardEvent(xim, event);

But when it commit string by calling

    IMCommitString(xim, commit_data);

Mozilla does not make any error.

GTK+ app, Qt app and many other programs don't have such a problem.

I think XIM has to forward key event that is not needed.
So this bug should be fixed.

The attachment(id=126508) make mozilla not crash.

Comment 23

[Koike Kazuhiko]

2004013007-trunk/Linux still crashes.

Who is reviewer? Please review and check in the patch.

Comment 24

[Jungshik Shin]

Simon, can you ask for review so that we can move forward this? 

Comment 25

[Jungshik Shin]

(In reply to comment #22)

> When it forward keypress event that is not accepted by xim to the application,
> mozilla crashes.

Does Mozilla always crash if your forward keypress events not handled by your
XIM server? Or, doesn't it crash once you enter Korean characters successfully
(see comment #18)? 

Comment 26

[Choe Hwanjin]

(From comment #25)
> Does Mozilla always crash if your forward keypress events not handled by your
> XIM server? Or, doesn't it crash once you enter Korean characters successfully

Once I enter korean successfully, mozilla does not crash. Mozilla crashes only
when I input keys which my xim does not treat just after changing mode to korean.

Comment 27

[Jungshik Shin]

Comment on attachment 126508 [details] [diff] [review]
Both patches combined, for review

asking for r/sr.

Comment 28

[Jungshik Shin]

Per comment #26, I updated 'international known issues' document for 1.6 to
mention that this is not only about Kinput2 but also about other XIMs. 

Comment 29

[Jungshik Shin]

Comment on attachment 126508 [details] [diff] [review]
Both patches combined, for review

asking sfraser for sr.

let's fix this for 1.7alpha

Comment 30

[Simon Fraser [no longer active]]

Comment on attachment 126508 [details] [diff] [review]
Both patches combined, for review

> void
> nsWindow::ime_preedit_start() {
>-  IMEComposeStart(nsnull);
> }

I'm not qualified to sr this, so rs=sfraser here.

>   // suppressIME s used when editor must insert text, yet this text is not
>   // part of current ime operation.  example: adjusting whitespace around an ime insertion.
>-  if (mInIMEMode && !suppressIME)
>+  if (mIMETextRangeList && mInIMEMode && !suppressIME)
>   {
>     if (!mIMETextNode)
>     {

sr=sfraser on this part.

Comment 31

[Jungshik Shin]

thanks for r/sr and a. fix checked in.