477911 - [TSF] crash after committing long IME transaction string at losing focus [@ msctf.dll@0x3931e1 - ... - nsTextStore::Destroy]

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Masahiro YAMADA]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090210
Firefox/3.1b2pre
Built from http://hg.mozilla.org/mozilla-central/rev/2e550e7b8f3c

If TSF is enabled, Firefox crashes after commiting long IME transaction string at losing focus.

Step to reproduce:
1.Enable TSF
2.Enabled automatic key repeat
3.Go to http://www.mozilla.org/projects/minefield/ を開く
4.Move focus to <TEXTAREA> or <INPUT TYPE="TEXT> or any other textbox.
5.enable IME, ant change mode to roma-ji hiragana.
6.keep pushing "A" about 30 seconds.
7.un-committed string is not shown , but its length will automatically be longer and longer.
8.move focus to other application

Crashed address is always same:
-----------------------
AppName: firefox.exe AppVer: 1.9.1.3157 ModName: msctf.dll
ModVer: 5.1.2600.5512 Offset: 0003931e
-----------------------
Output to console;

WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file
f:/mozbuild/src/content/events/src/nsContentEventHandler.cpp, line 451
WARNING: NS_ENSURE_TRUE(event.mSucceeded) failed: file
f:/mozbuild/src/widget/src/windows/nsTextStore.cpp, line 424

repeat of both many times.

And, when inputting to <INPUT TYPE="TEXT">, last line of console output is

WARNING: NS_ENSURE_TRUE(mIsIMEComposing) failed: file
f:/mozbuild/src/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp, line 368

Comment 1

[Masahiro YAMADA]

My environment:
Windows XP Home Japanese Edition (SP3) + Office XP (SP3) + MS-IME 2002
 + all important updates before 2009-2-10.

Comment 2

[Masahiro YAMADA]

When using Built from http://hg.mozilla.org/mozilla-central/rev/561dc7d5c746+,
ASSERTION is outputed before crash:
-- console output --
WARNING: NS_ENSURE_TRUE(event.mSucceeded) failed: file f:/mozbuild/src/widget/src/windows/nsTextStore.cpp, line 424
WARNING: NS_ENSURE_TRUE(-1 == acpEnd || event.mReply.mString.Length() == length) failed: file f:/mozbuild/src/widget/src/windows/nsTextStore.cpp, line 436
###!!! ASSERTION: Cannot Collapse: 'NS_SUCCEEDED(result)', file f:/mozbuild/src/editor/libeditor/base/IMETextTxn.cpp, line 320
WARNING: NS_ENSURE_TRUE(mIsIMEComposing) failed: file f:/mozbuild/src/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp, line 368

Comment 3

[Masahiro YAMADA]

Crashed code is here:
mscft.dll + 0x931E
CComposition::_Uninit:
74699319  mov         edi,edi 
7469931B  push        esi  
7469931C  mov         esi,ecx 
7469931E  mov         eax,dword ptr [esi+20h] <--  esi = 0x00000000
-----------------------
Stack trace

msctf.dll!CComposition::_Uninit() 	
msctf.dll!CInputContext::_AbortCompositions()  + 0x30 bytes	
msctf.dll!CInputContext::_UnadviseSinks()  + 0xf bytes	
msctf.dll!CDocumentInputManager::_Pop()  + 0x25 bytes	
msctf.dll!CDocumentInputManager::Pop()  + 0x6a bytes	
gkwidget.dll!nsTextStore::Destroy()  line 126	C++
gkwidget.dll!nsTextStore::OnFocusChange(int aFocus=0x00000000, nsWindow * aWindow=0x0266f010, unsigned int aIMEEnabled=0x00000001)  line 1060	C++
gkwidget.dll!nsWindow::OnIMEFocusChange(int aFocus=0x00000000)  line 7867 + 0x1a bytes	C++
gklayout.dll!nsIMEStateManager::OnTextStateBlur(nsPresContext * aPresContext=0x00000000, nsIContent * aContent=0x00000000)  line 529	C++
gklayout.dll!nsEventStateManager::PreHandleEvent(nsPresContext * aPresContext=0x02665638, nsEvent * aEvent=0x0012f044, nsIFrame * aTargetFrame=0x02741598, nsEventStatus * aStatus=0x0012ee38, nsIView * aView=0x026652f0)  line 1330 + 0x9 bytes	C++
gklayout.dll!PresShell::HandleEventInternal(nsEvent * aEvent=0x0012f044, nsIView * aView=0x026652f0, nsEventStatus * aStatus=0x0012ee38)  line 5917 + 0x36 bytes	C++
gklayout.dll!PresShell::HandleEvent(nsIView * aView=0x026652f0, nsGUIEvent * aEvent=0x0012f044, nsEventStatus * aEventStatus=0x0012ee38)  line 5721 + 0x1a bytes	C++
gklayout.dll!nsViewManager::HandleEvent(nsView * aView=0x026652f0, nsPoint aPoint={...}, nsGUIEvent * aEvent=0x0012f044, int aCaptured=0x00000000)  line 1362	C++
gklayout.dll!nsViewManager::DispatchEvent(nsGUIEvent * aEvent=0x0012f044, nsEventStatus * aStatus=0x0012ef88)  line 1338 + 0x22 bytes	C++
gklayout.dll!HandleEvent(nsGUIEvent * aEvent=0x0012f044)  line 170	C++
gkwidget.dll!nsWindow::DispatchEvent(nsGUIEvent * event=0x0012f044, nsEventStatus & aStatus=nsEventStatus_eIgnore)  line 1019 + 0xc bytes	C++
gkwidget.dll!nsWindow::DispatchWindowEvent(nsGUIEvent * event=0x0012f044)  line 1040	C++
gkwidget.dll!nsWindow::DispatchFocus(unsigned int aEventType=0x0000006c, int isMozWindowTakingFocus=0x00000000)  line 6498 + 0x11 bytes	C++
gkwidget.dll!nsWindow::ProcessMessage(unsigned int msg=0x00000008, unsigned int wParam=0x00000000, long lParam=0x00000000, long * aRetValue=0x0012f560)  line 4851 + 0x19 bytes	C++
gkwidget.dll!nsWindow::WindowProc(HWND__ * hWnd=0x000b02c6, unsigned int msg=0x00000008, unsigned int wParam=0x00000000, long lParam=0x00000000)  line 1235 + 0x1d bytes	C++

Comment 4

[Masahiro YAMADA]

Crashed position in nsTextStore:Destroy is here
 
123   mContext = NULL;
124   if (mDocumentMgr) {
125     mDocumentMgr->Pop(TF_POPF_ALL);
126     mDocumentMgr = NULL; // <-- ******** Crashed at here!! *****
127   }
128   mSink = NULL;
129   PR_LOG(sTextStoreLog, PR_LOG_ALWAYS,
130          ("TSF: Destroyed, window=%08x\n", mWindow));
131   mWindow = NULL;
132   return PR_TRUE;
133 }

Comment 5

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

I cannot reproduce this bug on WinVista now. However, I reproduced this bug at debugging with VS2008...

Comment 6

[David Gaskill]

TSF is required to enable the speech recognition system built into Vista to dictate into Firefox, (and presumably also Thunderbird).  So far as I'm aware it would serve no purpose in any other Windows operating system.  Third party speech recognition systems on other Windows operating systems do not require TSF.

Would it be possible to enable TSF only in Vista either automatically or manually?

Comment 7

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

(In reply to comment #6)
> TSF is required to enable the speech recognition system built into Vista to
> dictate into Firefox, (and presumably also Thunderbird).  So far as I'm aware
> it would serve no purpose in any other Windows operating system.  Third party
> speech recognition systems on other Windows operating systems do not require
> TSF.

Hey, please don't comment such non-valuable comments for *fixing* the bug. Maybe, we need to fix many bugs before enabling the TSF support in default settings.

> Would it be possible to enable TSF only in Vista either automatically or
> manually?

change "intl.enable_tsf_support" to true from about:config. but the current TSF support code has very many bugs, probably. be carefully for changing the pref.

Comment 8

[Masahiro YAMADA]

crashreport id: cb7e7bec-0e4d-4a15-b2b0-50d882090211

Comment 9

[Masahiro YAMADA]

Accrding to search result of crash-report,
this problem can be reproduced on both of XP SP2 and SP3.
http://crash-stats.mozilla.com/?do_query=1&product=Firefox&branch=1.9.2&platform=windows&query_search=signature&query_type=contains&query=msctf.dll&date=&range_value=1&range_unit=weeks

Comment 10

[Masahiro YAMADA]

changed to NEW, because came crash log other than me is found on crash report search.

Comment 11

[timeless]

UUID cb7e7bec-0e4d-4a15-b2b0-50d882090211
msctf.dll msctf.dll@0x3931e1
msctf.dll msctf.dll@0x90ee2
msctf.dll msctf.dll@0x92153
xul.dll nsTextStore::Destroy widget/src/windows/nsTextStore.cpp:1254
xul.dll nsTextStore::OnFocusChange widget/src/windows/nsTextStore.cpp:1058
xul.dll nsWindow::OnIMEFocusChange widget/src/windows/nsWindow.cpp:78676

Comment 12

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

maybe, I found the cause. taking.

Comment 13

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

Attachment: Patch v1.0 (work in progress)

Comment 14

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

The cause of this bug is |sTsfThreadMgr->SetFocus(NULL);| in |nsTextStore::Blur|. MSDN said that the argument cannot be NULL. But this method return S_OK :-(

This patch fixes this bug. However, this patch creates a new bug. We need to save the root content in nsTestStore. And nsIMEStateManager should notify the root content removed event to nsTextStore via nsIWidget. After that, nsTextStore must not dispatch content events. But focus handling is being changed now in bug 178324. It may be better that we wait the bug fix.

Comment 15

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

Attachment: Patch v1.1

I think this is ok for now.

We keep the nsTextStore instance after lost focus. But the instance always returns E_FAIL for TSF/TIPs if they access to it.

However, this patch means our TSF support code works only when the window is active. This is bad. Because some software keyboard like UIs of TIPs might be need focus. Then, our window is deactivated and the TIPs cannot access to us. So, we should not destroy nsTextStateManager until another content gets focus.

Comment 16

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

Comment on attachment 364072 [details] [diff] [review]
Patch v1.1

this cannot pass the test wait a moment.

Comment 17

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

Attachment: Patch v2.0

This is simpler fix.

And this patch changes the test to real TIP/TSF implementation. I separated TSFImpl to 3 classes:

1 is TSFMgrImpl which inherits ITfThreadMgr, ITfDisplayAttributeMgr and ITfCategoryMgr. This is needed just one instance per thread.

2 is TSFDocumentMgrImpl which inherits ITfDocumentMgr. This is container of context. This is referenced both TSFMgrImpl::mFocusedDocument and TSFContextImpl. When this ref count becomes 1 by release and the document is focused, this changes the focused document to NULL and release itself automatically.

3 is TSFContextImpl which inherits ITfContext, ITfCompositionView and ITextStoreACPSink. This is actual context.

I think this is last big change for the test.

Comment 18

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

Jim Chenn:

Would you review this bug and bug 480111? If you are busy, we will find another reviewers. Thanks.

Comment 19

[(inactive) Jim Chen [:jchen] [:darchons]]

Sorry I have been busy these days.
If you can find another reviewer that will be great.
Thanks.

Comment 20

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

Comment on attachment 364389 [details] [diff] [review]
Patch v2.0

Thank you, Jim.

I think I and Kimura-san can review TSF code.

Kimura-san, can you review this patch?

Comment 21

[Masatoshi Kimura [:emk]]

Comment on attachment 364389 [details] [diff] [review]
Patch v2.0

> +  STDMETHODIMP SetFocus(ITfDocumentMgr *pdimFocus)
> +  {
> +    if (!pdimFocus) {
> +      NS_NOTREACHED("ITfThreadMgr::SetFocus must not be called with NULL");
> +      return E_FAIL;
> +    }
> +    mFocusedDocument = static_cast<TSFDocumentMgrImpl*>(pdimFocus);
> +    mFocusedDocument->AddRef();
If SetFocus() is called multiple times without destroying document mgr, reference count will be added multiple times and mFocusedDocument will never be deleted. Is it valid?

> -  mImpl->mTextChanged = PR_FALSE;
> +  mMgr->GetFocusedContext()->mTextChanged = PR_FALSE;
Add null check for mMgr->GetFocusedContext() or test may crash.

> -  mImpl->mSelChanged = PR_FALSE;
> +  mMgr->GetFocusedContext()->mSelChanged = PR_FALSE;
Ditto.

Comment 22

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

Attachment: Patch v2.1

Comment 23

[Masatoshi Kimura [:emk]]

Comment on attachment 366059 [details] [diff] [review]
Patch v2.1

> +    if (mFocusedDocument == pdimFocus) {
> +      return S_OK;
> +    }
> +    mFocusedDocument = static_cast<TSFDocumentMgrImpl*>(pdimFocus);
> +    mFocusedDocument->AddRef();
> +    mFocusCount++;
mFocusCount is not a reference count. It's just a counter only to check how many times SetFocus() was called. It should be always added, no?
Otherwise looks good.

Comment 24

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

Attachment: Patch v2.2

Thank you Kimura-san.

Comment 25

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

http://hg.mozilla.org/mozilla-central/rev/7a6071857512

-> FIXED