Closed Bug 208095 Opened 21 years ago Closed 20 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.7alpha

People

(Reporter: master, Assigned: smontagu)

References

Details

(Keywords: crash, intl, Whiteboard: editorbase+)

Crash Data

Attachments

(2 files, 2 obsolete files)

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).
can you post Talkback ID for this crash "mozilla/bin/components/talkback/talkback" ?
Assignee: general → smontagu
Component: Browser-General → Internationalization
Keywords: crash, stackwanted
QA Contact: general → ylong
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)
Marking NEW to get the stack.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: TB20695646
Attached file Stack from TB20695646
Keywords: stackwanted
Didn't find dupes, should this be moved to Editor:Core ?
Summary: Crash when I input space with kinput → Crash when I input space with kinput [@ nsEditor::InsertTextIntoTextNodeImpl]
Whiteboard: TB20695646
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.
Assignee: smontagu → jfrancis
Component: Internationalization → Editor: Core
OS: Linux → All
QA Contact: ylong → sairuh
Hardware: PC → All
Do we know when this regression first started?  Has it ever worked?
Whiteboard: editorbase
The oldest build in talkback with this stack is 2003021008 MozillaTrunk
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.
Attached patch proposed patch (obsolete) — Splinter Review
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 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.
editorbase+
-> smontagu. Simon can you collaborate with Joe and Kin on this one?
Assignee: jfrancis → smontagu
Whiteboard: editorbase → editorbase+
Can anyone else confirm that the crash does not happen with
user_pref("xim.input_style", "over-the-spot");
in prefs.js?
I got a report that over-the-spot does not have this problem.
I can confirm that the over-the-spot method does not cause a crash with
the latest branch build.
We may have to consider changing the default pref for the 
next release unless this problem gets fixed. (Maybe for L10n builds
only?)
This seems to be an event targetting issue. Could it be a result of bug 52416?
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.
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.
Attached patch Another possibility (obsolete) — Splinter Review
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.
Depends on: 206550
Attachment #125400 - Attachment is obsolete: true
Attachment #126430 - Attachment is obsolete: true
Priority: -- → P1
Target Milestone: --- → Future
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.
2004013007-trunk/Linux still crashes.

Who is reviewer? Please review and check in the patch.
Simon, can you ask for review so that we can move forward this? 
Keywords: intl
(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)? 
Flags: blocking1.7a?
Target Milestone: Future → mozilla1.7alpha
(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 on attachment 126508 [details] [diff] [review]
Both patches combined, for review

asking for r/sr.
Attachment #126508 - Flags: superreview?(kinmoz)
Attachment #126508 - Flags: review?(katakai)
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. 
Attachment #126508 - Flags: review?(katakai) → review+
Comment on attachment 126508 [details] [diff] [review]
Both patches combined, for review

asking sfraser for sr.

let's fix this for 1.7alpha
Attachment #126508 - Flags: superreview?(kinmoz) → superreview?(sfraser)
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.
Attachment #126508 - Flags: superreview?(sfraser) → superreview+
Attachment #126508 - Flags: approval1.7a+
Flags: blocking1.7a? → blocking1.7a+
thanks for r/sr and a. fix checked in.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Crash Signature: [@ nsEditor::InsertTextIntoTextNodeImpl]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: