Closed
Bug 636131
Opened 14 years ago
Closed 14 years ago
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)
Categories
(Core :: Widget: Gtk, defect)
Tracking
()
VERIFIED
FIXED
mozilla2.0
Tracking | Status | |
---|---|---|
blocking2.0 | --- | final+ |
status1.9.2 | --- | unaffected |
status1.9.1 | --- | unaffected |
People
(Reporter: tommy.he, Assigned: masayuki)
References
Details
(Keywords: hang, inputmethod, regression, Whiteboard: [hardblocker] [has patch])
Attachments
(2 files, 3 obsolete files)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
masayuki
:
review+
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
A regression range or a stack trace with debug symbols would help
Component: General → Editor
Keywords: hang
Product: Firefox → Core
QA Contact: general → editor
Version: unspecified → Trunk
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•14 years ago
|
||
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?
(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•14 years ago
|
||
Works for Me on:
Mozilla/5.0 (X11; Linux i686; rv:2.0b12) Gecko/20100101 Firefox/4.0b12
Comment 6•14 years ago
|
||
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!
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•14 years ago
|
||
(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... :-)
Assignee | ||
Comment 10•14 years ago
|
||
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.
Assignee | ||
Comment 11•14 years ago
|
||
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
Assignee | ||
Comment 12•14 years ago
|
||
Hmm, but the parStart needs to be over 2,460,000. I have no idea...
Comment 13•14 years ago
|
||
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•14 years ago
|
||
(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•14 years ago
|
||
The problem is that parStart gets bigger than aCursorPos when LF code is located at the cursor position.
Attachment #514999 -
Flags: review?(masayuki)
Assignee | ||
Comment 16•14 years ago
|
||
This is a new regression of Fx4. We must fix this in final.
I'll post a patch soon. Thank you, Zoe-san.
Assignee: nobody → masayuki
Status: UNCONFIRMED → ASSIGNED
blocking2.0: --- → ?
status1.9.1:
--- → unaffected
status1.9.2:
--- → unaffected
Ever confirmed: true
Keywords: inputmethod,
regression
Assignee | ||
Comment 17•14 years ago
|
||
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().
Attachment #514999 -
Attachment is obsolete: true
Attachment #514999 -
Flags: review?(masayuki)
Attachment #515008 -
Flags: review?(karlt)
Reporter | ||
Comment 18•14 years ago
|
||
I guess there's no need for debug symbols now. You guys are awesome! :)
Updated•14 years ago
|
blocking2.0: ? → final+
Whiteboard: [hardblocker]
Updated•14 years ago
|
Whiteboard: [hardblocker] → [hardblocker] [hsa patch]
Updated•14 years ago
|
Whiteboard: [hardblocker] [hsa patch] → [hardblocker] [has patch]
Assignee | ||
Updated•14 years ago
|
Component: Editor → Widget: Gtk
QA Contact: editor → gtk
Summary: IBus in rich text field results a Freeze Firefox 4 Beta → 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)
Comment 19•14 years ago
|
||
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.
Assignee | ||
Comment 20•14 years ago
|
||
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•14 years ago
|
||
(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.
Assignee | ||
Comment 22•14 years ago
|
||
(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).
Assignee | ||
Comment 23•14 years ago
|
||
(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.
Assignee | ||
Comment 24•14 years ago
|
||
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•14 years ago
|
||
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.
Attachment #515008 -
Flags: review?(karlt) → review+
Assignee | ||
Comment 26•14 years ago
|
||
Thank you, Karl. I'll land this ASAP.
Attachment #515008 -
Attachment is obsolete: true
Attachment #515553 -
Flags: review+
Assignee | ||
Comment 27•14 years ago
|
||
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.
Attachment #515553 -
Attachment is obsolete: true
Attachment #515563 -
Flags: review+
Assignee | ||
Comment 28•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0
Assignee | ||
Comment 29•14 years ago
|
||
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/
Reporter | ||
Comment 30•14 years ago
|
||
Just tested with FF4b13pre. Unable to reproduce the issue.
Problem solved. Thanks for your effort. :)
Assignee | ||
Comment 31•14 years ago
|
||
Thank you for your report and testing.
-> v. per comment 30.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•