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

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Core
Widget: Win32
--
critical
RESOLVED FIXED
10 years ago
7 years ago

People

(Reporter: Masahiro YAMADA, Assigned: masayuki)

Tracking

({crash, inputmethod, intl})

Trunk
mozilla1.9.2a1
x86
Windows XP
crash, inputmethod, intl
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

10 years ago
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
(Reporter)

Comment 1

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

Comment 2

10 years ago
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
(Reporter)

Comment 3

10 years ago
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++
(Reporter)

Comment 4

10 years ago
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 }
(Assignee)

Updated

10 years ago
Blocks: 478029
I cannot reproduce this bug on WinVista now. However, I reproduced this bug at debugging with VS2008...
Summary: [TSF]crash after commiting long IME transaction string at losing focus → [TSF] crash after commiting long IME transaction string at losing focus

Comment 6

10 years ago
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?
(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.
(Reporter)

Comment 8

10 years ago
crashreport id: cb7e7bec-0e4d-4a15-b2b0-50d882090211
(Reporter)

Comment 10

10 years ago
changed to NEW, because came crash log other than me is found on crash report search.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 11

10 years ago
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
Summary: [TSF] crash after commiting long IME transaction string at losing focus → [TSF] crash after commiting long IME transaction string at losing focus [@ msctf.dll@0x3931e1 - ... - nsTextStore::Destroy]
(Assignee)

Updated

10 years ago
Summary: [TSF] crash after commiting long IME transaction string at losing focus [@ msctf.dll@0x3931e1 - ... - nsTextStore::Destroy] → [TSF] crash after committing long IME transaction string at losing focus [@ msctf.dll@0x3931e1 - ... - nsTextStore::Destroy]
maybe, I found the cause. taking.
Assignee: nobody → masayuki
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
Created attachment 363840 [details] [diff] [review]
Patch v1.0 (work in progress)
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.
Created attachment 364072 [details] [diff] [review]
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.
Attachment #363840 - Attachment is obsolete: true
Attachment #364072 - Flags: review?(chenn)
Comment on attachment 364072 [details] [diff] [review]
Patch v1.1

this cannot pass the test wait a moment.
Attachment #364072 - Flags: review?(chenn) → review-
Created attachment 364389 [details] [diff] [review]
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.
Attachment #364072 - Attachment is obsolete: true
Attachment #364389 - Flags: review?(chenn)
Jim Chenn:

Would you review this bug and bug 480111? If you are busy, we will find another reviewers. Thanks.
Sorry I have been busy these days.
If you can find another reviewer that will be great.
Thanks.
Attachment #364389 - Flags: review?(chenn) → review?(VYV03354)
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 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.
Created attachment 366059 [details] [diff] [review]
Patch v2.1
Attachment #364389 - Attachment is obsolete: true
Attachment #364389 - Flags: review?(VYV03354)
Attachment #366059 - Flags: review?(VYV03354)
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.
Attachment #366059 - Flags: review?(VYV03354) → review+
Created attachment 366092 [details] [diff] [review]
Patch v2.2

Thank you Kimura-san.
Attachment #366059 - Attachment is obsolete: true
Attachment #366092 - Flags: superreview?(roc)
Attachment #366092 - Flags: superreview?(roc) → superreview+
http://hg.mozilla.org/mozilla-central/rev/7a6071857512

-> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Keywords: inputmethod
Crash Signature: [@ msctf.dll@0x3931e1 - ... - nsTextStore::Destroy]
You need to log in before you can comment on or make changes to this bug.