692145 - Firefox Crash [@ nsTextFragment::CharAt(int) ]

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Marcia Knous [:marcia]]

Seen while reviewing the Firefox 7 explosive report. https://crash-stats.mozilla.com/report/list?signature=nsTextFragment%3A%3ACharAt%28int%29. It does seem to affect versions other than 7.0.1. Many of the comments are in Japanese and seem to mention text input.

https://crash-stats.mozilla.com/report/index/b7d4305b-7d2d-4b57-bd95-cacd32111004

Frame 	Module 	Signature [Expand] 	Source
0 	xul.dll 	nsTextFragment::CharAt 	
1 	xul.dll 	nsContentEventHandler::SetRangeFromFlatTextOffset 	content/events/src/nsContentEventHandler.cpp:436
2 	xul.dll 	nsContentEventHandler::OnQueryTextRect 	content/events/src/nsContentEventHandler.cpp:590
3 	xul.dll 	nsEventStateManager::PreHandleEvent 	
4 	xul.dll 	PresShell::HandleEventInternal 	layout/base/nsPresShell.cpp:7076
5 	xul.dll 	PresShell::HandleEvent 	layout/base/nsPresShell.cpp:6836
6 	xul.dll 	PresShell::HandleEvent 	
7 	xul.dll 	nsViewManager::HandleEvent 	view/src/nsViewManager.cpp:1053
8 	xul.dll 	nsViewManager::DispatchEvent 	view/src/nsViewManager.cpp:1031
9 	xul.dll 	AttachedHandleEvent 	view/src/nsView.cpp:192
10 	xul.dll 	nsWindow::DispatchEvent 	widget/src/windows/nsWindow.cpp:3550
11 	xul.dll 	nsWindow::DispatchWindowEvent 	widget/src/windows/nsWindow.cpp:3573
12 	xul.dll 	nsIMM32Handler::GetCharacterRectOfSelectedTextAt 	widget/src/windows/nsIMM32Handler.cpp:1870
13 	xul.dll 	nsIMM32Handler::SetIMERelatedWindowsPos 	widget/src/windows/nsIMM32Handler.cpp:1966
14 	xul.dll 	nsIMM32Handler::DispatchTextEvent 	widget/src/windows/nsIMM32Handler.cpp:1676
15 	xul.dll 	nsIMM32Handler::HandleComposition 	widget/src/windows/nsIMM32Handler.cpp:1271
16 	xul.dll 	nsIMM32Handler::OnIMEComposition 	widget/src/windows/nsIMM32Handler.cpp:571
17 	xul.dll 	nsIMM32Handler::ProcessMessage 	
18 	xul.dll 	nsWindow::WindowProcInternal 	widget/src/windows/nsWindow.cpp:4404
19 	xul.dll 	CallWindowProcCrashProtected 	xpcom/base/nsCrashOnException.cpp:65
20 	xul.dll 	nsWindow::WindowProc 	widget/src/windows/nsWindow.cpp:4346
21 	user32.dll 	InternalCallWinProc 	
22 	user32.dll 	UserCallWinProcCheckWow 	
23 	user32.dll 	DispatchMessageWorker 	
24 	user32.dll 	DispatchMessageW 	
25 	xul.dll 	nsAppShell::ProcessNextNativeEvent 	widget/src/windows/nsAppShell.cpp:338
26 	xul.dll 	nsBaseAppShell::OnProcessNextEvent 	widget/src/xpwidgets/nsBaseAppShell.cpp:324
27 	nspr4.dll 	PR_ExitMonitor 	nsprpub/pr/src/threads/prmon.c:132
28 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:581
29 	nspr4.dll 	_MD_CURRENT_THREAD 	nsprpub/pr/src/md/windows/w95thred.c:308
30 	xul.dll 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:134
31 	xul.dll 	MessageLoop::RunHandler 	ipc/chromium/src/base/message_loop.cc:202
32 	xul.dll 	MessageLoop::Run 	ipc/chromium/src/base/message_loop.cc:176
33 	xul.dll 	nsBaseAppShell::Run 	widget/src/xpwidgets/nsBaseAppShell.cpp:189
34 	xul.dll 	xul.dll@0xb8ab03 	
35 	xul.dll 	nsAppStartup::Run 	toolkit/components/startup/nsAppStartup.cpp:222
36 	xul.dll 	XRE_main 	toolkit/xre/nsAppRunner.cpp:3570
37 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp:107
38 	firefox.exe 	firefox.exe@0x4043 	
39 	firefox.exe 	_RTC_Initialize 	
40 	mozcrt19.dll 	_initterm 	obj-firefox/memory/jemalloc/crtsrc/crt0dat.c:852
41 	xul.dll 	_cairo_bentley_ottmann_tessellate_bo_edges 	gfx/cairo/cairo/src/cairo-bentley-ottmann.c:1603
42 	firefox.exe 	__security_init_cookie 	obj-firefox/memory/jemalloc/crtsrc/gs_support.c:139
43 	xul.dll 	_cairo_bentley_ottmann_tessellate_bo_edges 	gfx/cairo/cairo/src/cairo-bentley-ottmann.c:1603
44 	kernel32.dll 	GetCodePageFileInfo

Comment 1

[Marcia Knous [:marcia]]

Add on correlations:

23% (16/69) vs.  14% (12389/89832) {20a82645-c095-46ed-80e3-08825760534b} (Microsoft .NET Framework Assistant, http://www.windowsclient.net/)
      6% (4/69) vs.   0% (62/89832) [e-mail]
      6% (4/69) vs.   0% (135/89832) {22181a4d-af90-4ca3-a569-faed9118d6bc} (Trend Micro Toolbar, http://us.trendmicro.com/us/products/personal/internet-security-pro/index.html)
      6% (4/69) vs.   0% (174/89832) {5C655500-E712-41e7-9349-CE462F844B19}
      6% (4/69) vs.   0% (350/89832) [e-mail]
      9% (6/69) vs.   4% (3276/89832) {e4a8a97b-f2ed-450b-b12d-ee082ba24781} (Greasemonkey, https://addons.mozilla.org/addon/748)

Comment 2

[Toshihiro Yamada]

I got almost same signature while typing with Japanese IME.
(Probably, crash happens only typing with IME since no problem while writing this...)

My signature:
https://crash-stats.mozilla.com/report/index/ec868bdb-a51d-45cf-a225-4e1f42111030

As far as I see, this bug seems very critical for Japanese users
because not a small number of crashes were reported (currently 196th position in the query for 7.0.1)
and many reports have Japanese IME in module section...

I'll investigate the reproducible way or some useful data for this.

Comment 3

[Mats Palmgren (inactive)]

Attachment: wallpaper

I think it would be safe to just wallpaper the crash for now.
(and add an assertion to catch it later)

The real bug probably has something to do with converting back and
forth between "native" text offsets and normal text offsets.
See GetNativeTextLength() / CountNewlinesIn() in this file.
(some weird special handling of \r\n only on Windows)

Comment 4

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

Hmm, I don't understand this bug yet. Let me check this bug today...

Comment 5

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

Yamada-san, do you find the steps to reproduce this?

Comment 6

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

Comment on attachment 592437 [details] [diff] [review]
wallpaper

Okay, we should meet the deadline of Fx12.

Comment 7

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

Attachment: Patch

I see the actual cause. CountNewlinesIn()'s aMaxOffset is assumed as XP string's length. However, ConvertToXPOffset() passes native offset to it. If there are a lot of \r\n in the text, the loop in CountNewlinesIn() will access the out of range of the array. This patch creates another CountNewlinesIn() for ConvertToXPOffset().

I think that this patch should be reviewed by ehsan, however, he isn't on IRC...

Comment 8

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

I landed the wallpaper on inbound.
https://hg.mozilla.org/integration/mozilla-inbound/rev/69fbba57d30e

But the regression causes that IME candidate window appears at wrong position in some cases. So, I'd like to fix this bug completely tomorrow and on Fx12.

Comment 9

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

Comment on attachment 592676 [details] [diff] [review]
Patch

This patch needs r+ by ehsan or roc.

Comment 10

[(no longer active)]

Comment on attachment 592676 [details] [diff] [review]
Patch

Looks good, r=me.

Comment 11

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

The wallpapaer hasn't been merged to m-c now. philor said, it's not problem to land same patch to m-c. Therefore, I landed both patches on m-c directly.

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/481d57edc9b1
https://hg.mozilla.org/mozilla-central/rev/7597b7feee73

Comment 13

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/69fbba57d30e

Comment 14

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

This crash happens when users types text with IME. We need to tell position of composition string because IME needs to show its candidate window which shows the possible results for composing string. The crash may happens during computing the character position for that.

Additionally, when user just sets focus to our editor during IME is turned ON. Whether this happens depends on IME. IME might ask us the caret position for shows something of navigation tooltip. This is additional function of some IMEs. But if IME did it, it may have crashed.

This crash happens randomly, even though accessing out of bounds happens every time if user types text at the end of editor and there are many lines. However, even if it doesn't crash, we compute wrong position and tell it to IME. So, the candidate window shows wrong position. It annoys users when it overlaps composing string (i.e., hiding it).

I think the count of crash isn't too high (but not low), however, the most important thing is, users get frustration by this crash. User types text more and more, the possibility of this crash becomes higher and higher. It means that when this crash happens, user may lost large amount of inputting text, like blog.

Therefore, I'd like to fix this bug on the branches. Automated tests were made in bug 722639, if it should be landed together, let me know.

Comment 15

[Alex Keybl [:akeybl]]

This does not meet the criteria for our ESR as outlined in https://wiki.mozilla.org/Release_Management/ESR_Landing_Process (not a top crasher), and doesn't need to be tracked for any specific release given it isn't a recent regression.

Comment 16

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

(In reply to Alex Keybl [:akeybl] from comment #15)
> This does not meet the criteria for our ESR as outlined in
> https://wiki.mozilla.org/Release_Management/ESR_Landing_Process (not a top
> crasher), and doesn't need to be tracked for any specific release given it
> isn't a recent regression.

Alex:

I think that this bug *cannot* be a top crash even if this is actually serious because this bug is only reproduced while composing with IME only on Windows. And most of crash reports which have comments are written in Japanese. So, if the most victims were Japanese Windows users, this bug couldn't win other crash bugs which can be reproduced by all platform users and all language users.

Cannot we count the crash bug reports for every language? If it could, and the raking were not high even only for Japanese users, I agree with your decision. If not, I think that it's unfair rule for this bug.

And please be aware. When this bug occurs, very long inputting text must be lost. It's pointed by the comments of crash reports.

Comment 17

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

Kairo and Smoony:

I'd like you to show us the crash bug ranking on in Japanese Windows. Is it possible? See comment 16 for the reason. If this bug occurs frequently for them, it means Japanese users often meet this dataloss. So, if so, it's serious.

Comment 18

[Robert Kaiser]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #17)
> I'd like you to show us the crash bug ranking on in Japanese Windows. Is it
> possible?

I don't know of any way to look at Japanese only with crash stats.

Comment 19

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

(In reply to Robert Kaiser (:[e-mail]) from comment #18)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #17)
> > I'd like you to show us the crash bug ranking on in Japanese Windows. Is it
> > possible?
> 
> I don't know of any way to look at Japanese only with crash stats.

Hmm, cannot we assume that including reports which include imm32.dll in the Modules are only from Windows for CJK?

Comment 20

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #19)

> Hmm, cannot we assume that including reports which include imm32.dll in the
> Modules are only from Windows for CJK?

imm32.dll is always loaded on all locale.  msctfime.ime will be loaded to input text by IME on XP.  But after Vista, this is no way to detect using IME.

We can detect by Winsock LSP string whether users uses specific language version Windows.  LSP string has Japanese, users will use Japanese version.

Comment 21

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

Thank you, Kato-san.

So, if we could count them only from reports which loaded msctfime.ime on NT5.1, we would know the crash ranking on CJK WinXP.

And also, if we could count them only from reports which include "サービス" in Winsock LSP string, we would know the crash ranking on Japanese Vista and later.

kario: Can we do it? I don't know the structure of the DB, though.

Comment 22

[Robert Kaiser]

You can try filing a bug in Socorro / Data request and list there what you need and how to find it.

Comment 23

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

(In reply to Robert Kaiser (:[e-mail]) from comment #22)
> You can try filing a bug in Socorro / Data request and list there what you
> need and how to find it.

Thanks, I filed bug 731493.

Comment 24

[u279076]

Masayuki, can you please verify this is fixed?

Comment 25

[Scoobidiver (away)]

It's #10 top browser crasher in 10.0.5 ESR over the last four weeks.

Comment 26

[(no longer active)]

Masayuki, if you believe this fix is safe for ESR, can you please nominate the patch for approval?  Thanks!

Comment 27

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

Comment on attachment 592676 [details] [diff] [review]
Patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:

This causes a lot of crash reports and displaying composition candidate window in wrong position. This bug is only reproduced on Windows.

User impact if declined:

Users could lose the writing text data. There were some such reports before fixing on trunk.
 
Fix Landed on Version:

mozilla 12.

Risk to taking this patch (and alternatives if risky): 

There is no regression report as far as I know.

String or UUID changes made by this patch: 

No.

Comment 28

[Lukas Blakk [:lsblakk] use ?needinfo]

This still doesn't meet ESR criteria, and it's fairly low crash volume so I'm not inclined to take it unless there's a clear way to verify this is fixed after landing, otherwise it can ride the trains.

Comment 29

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

(In reply to Lukas Blakk [:lsblakk] from comment #28)
> This still doesn't meet ESR criteria, and it's fairly low crash volume so
> I'm not inclined to take it unless there's a clear way to verify this is
> fixed after landing, otherwise it can ride the trains.

As I said that we cannot know crash ranking for each language. (I filed bug 731493 at comment 23, but nobody has responded.) And only 3 languages which use IME at input and we have a lot of users. Additionally, Japanese IME needs the query more times than other languages. So, it's unfair to decide frequency of crash from all languages' crash ranking.

And the crashed point is very clear because we have the stacktrace. And the most crash reports of same point are reported from Fx11 or earlier.
https://crash-stats.mozilla.com/report/list?product=Firefox&version=Firefox%3A13.0.1&version=Firefox%3A13.0&version=Firefox%3A12.0&version=Firefox%3A11.0&platform=windows&query_search=signature&query_type=exact&query=nsTextFragment%3A%3ACharAt%28int%29&reason_type=contains&date=07%2F11%2F2012%2023%3A26%3A55&range_value=30&range_unit=days&hang_type=any&process_type=any&do_query=1&signature=nsTextFragment%3A%3ACharAt%28int%29

And as I said, even if the crash doesn't happen, this bug caused the IME's candidate list window overlaps inputting text. It's also fixed by my patch.

Note that these users may meet the dataloss by this crash...

Comment 30

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

Comment on attachment 592437 [details] [diff] [review]
wallpaper

I forgot to request the approval for this patch. See above for the request reasons.

This patch ensures that the loop doesn't access out of bounds of the array. My patch depends on this patch.

Comment 31

[(no longer active)]

FWIW, even though the patch doesn't look safe, it's pretty well understood and seems very rational.  This is the sort of thing that I would be a little worried to take on ESR if it had not baked, but it has been shipped to our users in a couple of releases now, so I strongly suggest that we should take it, based on the patch itself and comment 29.

Comment 32

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 592437 [details] [diff] [review]
wallpaper

Thanks Ehsan for that evaluation, that helps. In light of that information I will take the risk of landing this on ESR then.  Please go ahead and land as per https://wiki.mozilla.org/Release_Management/ESR_Landing_Process

Comment 33

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

(In reply to Lukas Blakk [:lsblakk] from comment #32)
> Comment on attachment 592437 [details] [diff] [review]
> wallpaper
> 
> Thanks Ehsan for that evaluation, that helps. In light of that information I
> will take the risk of landing this on ESR then.  Please go ahead and land as
> per https://wiki.mozilla.org/Release_Management/ESR_Landing_Process

Thank you. But I'm still not sure the landing rules for ESR. So, I'd like to confirm it.

The current status of this bug seems "Needs to land now". Then, can I land the patches on https://hg.mozilla.org/releases/mozilla-esr10/ by myself? I'm not sure the difference between "Needs to land now" and "Needs to land very soon". And also what "checking-pending" status means and how to use it.

Comment 34

[(no longer active)]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #33)
> (In reply to Lukas Blakk [:lsblakk] from comment #32)
> > Comment on attachment 592437 [details] [diff] [review]
> > wallpaper
> > 
> > Thanks Ehsan for that evaluation, that helps. In light of that information I
> > will take the risk of landing this on ESR then.  Please go ahead and land as
> > per https://wiki.mozilla.org/Release_Management/ESR_Landing_Process
> 
> Thank you. But I'm still not sure the landing rules for ESR. So, I'd like to
> confirm it.
> 
> The current status of this bug seems "Needs to land now". Then, can I land
> the patches on https://hg.mozilla.org/releases/mozilla-esr10/ by myself? I'm
> not sure the difference between "Needs to land now" and "Needs to land very
> soon". And also what "checking-pending" status means and how to use it.

Yes, you should be good to land the patches on mozilla-esr10 now.

Comment 35

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-esr10/rev/5b67a8a2bfc2
https://hg.mozilla.org/releases/mozilla-esr10/rev/5be573d1665f