636131 - iBus freezes when it retrieves surrounding text and if the caret is at end of line (IA__gtk_im_context_set_surrounding: assertion `cursor_index >= 0 && cursor_index <= len' failed)

Status: VERIFIED FIXED

(Core :: Widget: Gtk, defect)

Description

[Tommy He]

User-Agent:       Mozilla/5.0 (X11; Linux i686; rv:2.0b11) Gecko/20110211 Firefox/4.0b11
Build Identifier: Mozilla/5.0 (X11; Linux i686; rv:2.0b11) Gecko/20110211 Firefox/4.0b11

Using IBus (the default input method by almost every Linux distributions) in rich text field (Gmail composer, wordpress composer etc.) will cause the Firefox 4 Beta freeze.

Reproducible: Always

Steps to Reproduce:
1. Choose IBus as input method via system tools or explicitly set GTK_IM_MODULE=ibus
2. Open a page with rich text field such as gmail composer, wordpress blog composer(search box seems to be free from this issue). 
3. Try to type something via IBus.
Actual Results:  
The Firefox 4 Beta freezes (but not crashed). Have to be killed manually.

Expected Results:  
Firefox 4 not freeze. User can use IBus as their input method.

Since XIM input method seems to be free from this issue, this bug was initially filed into IBus bug tracker. However the dev there suggests it might have something to do with xulrunner2.

Further detail information can be found there:

http://code.google.com/p/ibus/issues/detail?id=1199

Comment 1

[Matthias Versen [:Matti]]

A regression range or a stack trace with debug symbols would help

Comment 2

[Tommy He]

For me I got this from terminal output:

(firefox-bin:16919): Gtk-CRITICAL **: IA__gtk_im_context_set_surrounding: assertion `cursor_index >= 0 && cursor_index <= len' failed

Here's what IBus dev catch:

#0  0x00007f7ed6b150af in g_utf8_offset_to_pointer (str=0x7f7edc7022d0 "",
   offset=4292507292) at gutf8.c:330
#1  0x00007f7edabcb12d in nsGtkIMModule::OnRetrieveSurroundingNative (
   this=<value optimized out>, aContext=0x7f7ebec938e0)
   at nsGtkIMModule.cpp:878
#2  0x00007f7ed5a37ad4 in _gtk_marshal_BOOLEAN__VOID (closure=0x7f7ebeb6e1f0,
   return_value=0x7fff33e822e0, n_param_values=1, param_values=
   0x7f7eb1696140, invocation_hint=0x7fff33e822a0, marshal_data=0x0)
   at gtkmarshalers.c:917
#3  0x00007f7ed6fcb03e in g_closure_invoke () from /lib64/libgobject-2.0.so.0
#4  0x00007f7ed6fdbe87 in ?? () from /lib64/libgobject-2.0.so.0
#5  0x00007f7ed6fe5555 in g_signal_emit_valist ()
  from /lib64/libgobject-2.0.so.0
#6  0x00007f7ed6fe5b6d in g_signal_emit_by_name ()
  from /lib64/libgobject-2.0.so.0
#7  0x00007f7ed5a14293 in gtk_im_multicontext_retrieve_surrounding_cb (slave=
   0x7f7eb22aacc0, multicontext=0x7f7ebec938e0) at gtkimmulticontext.c:492
#8  0x00007f7ed5a37ad4 in _gtk_marshal_BOOLEAN__VOID (closure=0x7f7eb22eef10,
   return_value=0x7fff33e82850, n_param_values=1, param_values=
   0x7f7eb166b680, invocation_hint=0x7fff33e82810, marshal_data=0x0)
   at gtkmarshalers.c:917
#9  0x00007f7ed6fcb03e in g_closure_invoke () from /lib64/libgobject-2.0.so.0
#10 0x00007f7ed6fdbe87 in ?? () from /lib64/libgobject-2.0.so.0
---Type <return> to continue, or q <return> to quit---
#11 0x00007f7ed6fe5555 in g_signal_emit_valist ()
  from /lib64/libgobject-2.0.so.0
#12 0x00007f7ed6fe5983 in g_signal_emit () from /lib64/libgobject-2.0.so.0
#13 0x00007f7ec14f857a in _request_surrounding_text (context=0x7f7eb22aacc0)
   at ibusimcontext.c:222
#14 0x00007f7ec14f902d in ibus_im_context_filter_keypress (context=
   0x7f7eb22aacc0, event=0x7f7ead482dd0) at ibusimcontext.c:516
#15 0x00007f7ed5a0fcc5 in IA__gtk_im_context_filter_keypress (context=
   0x7f7eb22aacc0, key=0x7f7ead482dd0) at gtkimcontext.c:473
#16 0x00007f7ed5a13e03 in gtk_im_multicontext_filter_keypress (context=
   0x7f7ebec938e0, event=0x7f7ead482dd0) at gtkimmulticontext.c:331
#17 0x00007f7ed5a0fcc5 in IA__gtk_im_context_filter_keypress (context=
   0x7f7ebec938e0, key=0x7f7ead482dd0) at gtkimcontext.c:473
#18 0x00007f7edabcaab4 in nsGtkIMModule::OnKeyEvent (this=0x7f7ebecff080,
   aCaller=0x7f7ebecb1db0, aEvent=0x7f7ead482dd0, aKeyDownEventWasSent=0)
   at nsGtkIMModule.cpp:401:

Let me know if it's not enough.

Thanks.

Comment 3

[George Carstoiu]

Mozilla/5.0 (X11; Linux i686; rv:2.0b12) Gecko/20100101 Firefox/4.0b12

Using the latest beta I am unable to reproduce the issue you have described. 

Can you please retry it using beta 12:
ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/4.0b12-candidates/build1/

Also from what I understand from the link you posted this could be Fedora related. Is there any chance you can try it on other Linux distributions?

Comment 4

[Tommy He]

(In reply to comment #3)

No luck. FF4b12 still freeze after a few types with IBus in Gmail composer.

FYI simple text input field seems not be affected by this issue, such as search box and the plain text comment area here.

Please see the link in this comment by IBus dev, which pointed to a function called PRUint32 cursorPos in xulrunner2.

http://code.google.com/p/ibus/issues/detail?id=1199#c13

One of my friends confirmed Ubuntu 10.11 was also affected this issue.

Thanks.

Comment 5

[AndreiD[QA]]

Works for Me on:
Mozilla/5.0 (X11; Linux i686; rv:2.0b12) Gecko/20100101 Firefox/4.0b12

Comment 6

[(no longer active)]

Please attach gdb to the hung process, and run:

thread apply all bt

and attach the output as a text file to this bug.  Thanks!

Comment 7

[Tommy He]

Attachment: gdb attach result against freezed firefox 4 beta with ibus

Comment 8

[Tommy He]

I noticed that when attaching gdb to firefox-bin, many symbols are reported to be missing.

I downloaded the firefox-4.0b12.crashreporter-symbols.zip but haven't figured out how to use it. Appreciate any help. Thanks!

Comment 9

[(no longer active)]

(In reply to comment #8)
> I noticed that when attaching gdb to firefox-bin, many symbols are reported to
> be missing.
> 
> I downloaded the firefox-4.0b12.crashreporter-symbols.zip but haven't figured
> out how to use it. Appreciate any help. Thanks!

I don't know either.  CCing Ted and Kyle to see if they know.

Can you build Firefox locally?  I do know that the symbols could be loaded without any special effort if you do a local build... :-)

Comment 10

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

Would you attach log of ours?

export NSPR_LOG_MODULES=nsGtkIMModuleWidgets:1
export NSPR_LOG_FILE=/home/<user name>/fx.log

Then, you can get the our IM code's log to ~/fx.log. It includes native IM signal information.

Note that this logs all IM behavior since launched Fx. So, don't do anything except reproducing this bug and don't input something which you don't want to show.

Comment 11

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

Um, I'm suspicious of this code:

http://mxr.mozilla.org/mozilla-central/source/widget/src/gtk2/nsGtkIMModule.cpp#1406
> 1406     PRInt32 parStart = textContent.RFind("\n", PR_FALSE, aCursorPos, -1) + 1;
> 1407     PRInt32 parEnd = textContent.Find("\n", PR_FALSE, aCursorPos + selLength, -1);
> 1408     if (parEnd < 0) {
> 1409         parEnd = textContent.Length();
> 1410     }
> 1411     aText = nsDependentSubstring(textContent, parStart, parEnd - parStart);
> 1412     aCursorPos -= parStart;


#1412 is |(unsinged int) -= (int)|. The computed value will be used for the offset of:

> #0  0x00007f7ed6b150af in g_utf8_offset_to_pointer (str=0x7f7edc7022d0 "",
>    offset=4292507292) at gutf8.c:330

Comment 12

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

Hmm, but the parStart needs to be over 2,460,000. I have no idea...

Comment 13

[fujiwara]

To reproduce the bug, GTK im client needs to emit the signal of
"retrieve-surrounding" that it inherits the class GtkIMContextClass and
override the method filter_keypress. And I can see the problem in google gmail
with rich text format but not text format.

Hmm.., I thought it's clear to show the root cause for mozilla people.

#0  0x00007f7ed6b150af in g_utf8_offset_to_pointer (str=0x7f7edc7022d0 "",
   offset=4292507292) at gutf8.c:330
#1  0x00007f7edabcb12d in nsGtkIMModule::OnRetrieveSurroundingNative (
   this=<value optimized out>, aContext=0x7f7ebec938e0)
   at nsGtkIMModule.cpp:878

The xulrunner2's nsGtkIMModule.cpp:nsGtkIMModule assigns the wrong offset ==
4292507292.
It's a bug for xulrunner2
http://hg.mozilla.org/mozilla-central/file/42e7f9088975/widget/src/gtk2/nsGtkIMModule.cpp#l877
PRUint32 cursorPos doesn't assign the right value.

If it's not clear, I will start the furthermore investigation.
Currently Fedora ibus applies an internal patch to provide a feature of
surrounding-text which is still under the discussion in upstream.
So if you try ibus with other platforms, the feature is not available.

Comment 14

[(not currently active) Ted Mielczarek]

(In reply to comment #8)
> I noticed that when attaching gdb to firefox-bin, many symbols are reported to
> be missing.
> 
> I downloaded the firefox-4.0b12.crashreporter-symbols.zip but haven't figured
> out how to use it. Appreciate any help. Thanks!

You can't, it's not real debug symbols. We do upload the actual debug symbols, and there's a script you can use to fetch them:
https://developer.mozilla.org/en/Using_the_Mozilla_symbol_server#Downloading_symbols_on_Linux_.2f_Mac_OS_X

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Attachment: fix

The problem is that parStart gets bigger than aCursorPos when LF code is located at the cursor position.

Comment 16

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

This is a new regression of Fx4. We must fix this in final.

I'll post a patch soon. Thank you, Zoe-san.

Comment 17

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

Attachment: Patch v1.0

This is singed vs. unsigned problem and computing paragraph start position is wrong.

On Linux, LF means line-breaking, not CRLF. The code is ported from Windows' code. Therefore, the difference between Windows and Linux causes this bug.

> PRInt32 parStart = textContent.RFind("\n", PR_FALSE, aCursorPos, -1) + 1;

E.g., if the editor has "a|LFb" (| is caret position), parStart should be 0 because the caret is at the end of the first paragraph.  However, aCursorPos is 1, this code sets 2 for parStart. So, we should search \n from aCursorPos - 1.

# On Windows, the text is "a|CRLFb", therefore, even if the param is aCursorPos, it works.

This patch also fixes other possible similar bugs in GetCurrentParagraph().

Comment 18

[Tommy He]

I guess there's no need for debug symbols now. You guys are awesome! :)

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 515008 [details] [diff] [review]
Patch v1.0

>-    aCursorPos = querySelectedTextEvent.mReply.mOffset;
>-    PRUint32 selLength = querySelectedTextEvent.mReply.mString.Length();
>+    PRInt32 targetOffset = PRInt32(querySelectedTextEvent.mReply.mOffset);
>+    PRInt32 targetLength =
>+      PRInt32(querySelectedTextEvent.mReply.mString.Length());
>+
>+    // XXX nsString::Find and nsString::RFind take PRInt32 for offset, so,
>+    //     we cannot support this request when the current offset is larger
>+    //     than PR_INT32_MAX.
>+    if (targetOffset < 0 || targetLength < 0 ||
>+        targetOffset + targetLength < 0) {
>+        PR_LOG(gGtkIMLog, PR_LOG_ALWAYS,
>+            ("    FAILED (The selection is out of range)"));
>+        return NS_ERROR_FAILURE;
>+    }

I think we should handle partial string as surrounding text in such cases.

Comment 20

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

How? and Why?

Such situation means the text content of the focused editor is larger than 2GB in plain text. And the text will be duplicated in heap by each query content event. I.e., Gecko needs to allocate 4GB at least and it must cause OOM (or slow down by disk swapping) on typical environment. Therefore, I think there are no such web application in the world. And also such web applications don't work on current web browsers.

Furthermore, gtk_im_context_set_surrounding() takes gint rather than guint.

Comment 21

[Hiroyuki Ikezoe (:hiro)]

(In reply to comment #20)
> How? and Why?
> 
> Such situation means the text content of the focused editor is larger than 2GB
> in plain text. And the text will be duplicated in heap by each query content
> event. I.e., Gecko needs to allocate 4GB at least and it must cause OOM (or
> slow down by disk swapping) on typical environment. Therefore, I think there
> are no such web application in the world. And also such web applications don't
> work on current web browsers.

I mean "partial" is reasonable length. I do not think that IM needs such long text.

If you are worrying about memory usage, we should also consider the limit of the length at:

>+    PRInt32 parStart = (targetOffset == 0) ? 0 :
>+      textContent.RFind("\n", PR_FALSE, targetOffset - 1, -1) + 1;
>+    PRInt32 parEnd = textContent.Find("\n", PR_FALSE, targetOffset + targetLength, -1);

Moreover, PR_INT32_MAX is still too large, since utf8 string needs about 3 times memory of the character length if the string is written in Japanese.

Comment 22

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

(In reply to comment #21)
> (In reply to comment #20)
> > How? and Why?
> > 
> > Such situation means the text content of the focused editor is larger than 2GB
> > in plain text. And the text will be duplicated in heap by each query content
> > event. I.e., Gecko needs to allocate 4GB at least and it must cause OOM (or
> > slow down by disk swapping) on typical environment. Therefore, I think there
> > are no such web application in the world. And also such web applications don't
> > work on current web browsers.
> 
> I mean "partial" is reasonable length. I do not think that IM needs such long
> text.

No, I mean that the query event handler use such huge memory. See nsContentEventHandler. And also I still don't think that there is such huge content on current web.

> If you are worrying about memory usage, we should also consider the limit of
> the length at:
> 
> >+    PRInt32 parStart = (targetOffset == 0) ? 0 :
> >+      textContent.RFind("\n", PR_FALSE, targetOffset - 1, -1) + 1;
> >+    PRInt32 parEnd = textContent.Find("\n", PR_FALSE, targetOffset + targetLength, -1);
> 
> Moreover, PR_INT32_MAX is still too large, since utf8 string needs about 3
> times memory of the character length if the string is written in Japanese.

The targetOffset is negative if the offset is larger than PR_INT32_MAX. So, the RFind() and Find() don't work (textContent has over 2GB UTF-16 text in such situation).

Comment 23

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

(In reply to comment #22)
> (In reply to comment #21)
> > (In reply to comment #20)
> > >+    PRInt32 parStart = (targetOffset == 0) ? 0 :
> > >+      textContent.RFind("\n", PR_FALSE, targetOffset - 1, -1) + 1;
> > >+    PRInt32 parEnd = textContent.Find("\n", PR_FALSE, targetOffset + targetLength, -1);
> > 
> > Moreover, PR_INT32_MAX is still too large, since utf8 string needs about 3
> > times memory of the character length if the string is written in Japanese.
> 
> The targetOffset is negative if the offset is larger than PR_INT32_MAX. So, the
> RFind() and Find() don't work (textContent has over 2GB UTF-16 text in such
> situation).

And g_utf16_to_utf8() should fail if the UTF-16 string is too large for UTF-8 string whose length is indicated with glong.

Comment 24

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

We should focus on fixing the hang-up bug because we don't have much time.

The if block is ported from nsIMM32Handler which is for Windows. If you still think that we should care such huge content handling, please file a new bug and fix it on both platforms in next cycle.

Comment 25

[Karl Tomlinson (:karlt)]

Comment on attachment 515008 [details] [diff] [review]
Patch v1.0

>+    PRInt32 targetOffset = PRInt32(querySelectedTextEvent.mReply.mOffset);
>+    PRInt32 targetLength =
>+      PRInt32(querySelectedTextEvent.mReply.mString.Length());
>+
>+    // XXX nsString::Find and nsString::RFind take PRInt32 for offset, so,
>+    //     we cannot support this request when the current offset is larger
>+    //     than PR_INT32_MAX.
>+    if (targetOffset < 0 || targetLength < 0 ||
>+        targetOffset + targetLength < 0) {
>+        PR_LOG(gGtkIMLog, PR_LOG_ALWAYS,
>+            ("    FAILED (The selection is out of range)"));
>+        return NS_ERROR_FAILURE;
>+    }

Technically it is more correct to check the unsigned values against
PR_INT32_MAX.  The conversion from unsigned to signed is undefined if the
unsigned value cannot be represented by the signed type.  Similarly the result
after overflow of signed arithmetic is undefined.

I suggest making targetOffset and targetLength PRUint32.  Then the explicit
casts to PRInt32 and back to PRUint32 will also be unnecessary.

>+    if (PRUint32(targetOffset) > textContent.Length() ||
>+        PRUint32(targetOffset) + targetLength > textContent.Length()) {

Here both targetOffset and targetLength are less than or equal to half the
maximum representable value in a PRUint32 so the first check here is now
redundant.

Comment 26

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

Attachment: Patch v1.1

Thank you, Karl. I'll land this ASAP.

Comment 27

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

Attachment: Patch v1.2

fix some nits:

* %d -> %u in PR_LOG() for PRUint32
* new variables target* are renamed to sel*.
* logging more information for failure case

not changed any logic and no warnings at build time.

Comment 28

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

http://hg.mozilla.org/mozilla-central/rev/fc44e3ee2608

Comment 29

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

Tommy, would you check whether this bug is fixed by the patch completely with today's build?

The first build with the patch:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux/1298873087/

Comment 30

[Tommy He]

Just tested with FF4b13pre. Unable to reproduce the issue.

Problem solved. Thanks for your effort. :)

Comment 31

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

Thank you for your report and testing.

-> v. per comment 30.