Closed Bug 1234422 Opened 9 years ago Closed 8 years ago

[TSF] Google Japanese Input sometimes fails to update its candidate window at converting the composition string

Categories

(Core :: Widget: Win32, defect)

Product:
Core
Component:
Widget: Win32
Platform:
x86
Windows 8
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla46
Tracking Flags:
Tracking Status
firefox42 --- unaffected
firefox43 --- wontfix
firefox44 + fixed
firefox45 + fixed
firefox46 + fixed
firefox-esr38 --- unaffected
firefox-esr45 - fixed
b2g-v2.5 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: inputmethod, regression)

Attachments

(2 files, 1 obsolete file)

TSFTextStore should retry to notify TSF/TIP of layout change if synchronous calls of OnLayoutChange() don't cause retrieving layout information (r=m_kato)
17.19 KB, patch
masayuki
: review+
ritu
: approval-mozilla-aurora+
ritu
: approval-mozilla-beta+
Details | Diff | Splinter Review
The patch for beta
17.18 KB, patch
Details | Diff | Splinter Review 
As far as I tested, this is a regression of bug 1204519. In bug 1204519, we removed TS_E_NOLAYOUT hack for Google Japanese Input. Then, we met this "hidden" bug, perhaps in TSFTextStore.

STR:
1. Type "ばーじょん" in the search bar
2. Press spacebar * n

ER:
The composition string in the searchbar should be modified and proper item in the candidate window of Google Japanese Input should be highlighted.

AR:
The composition string in the searchbar is modified but sometimes not modified in the candidate window.


[Tracking Requested - why for this release]:

I see some tweets which claim that this is very inconvenient and they want to change their default browser. I'm still investigating the cause of this bug, but even if the fix becomes risky, we should back out the patches of bug 1204519 from the branches.
I'm still investigating and testing this issue, though.

Google Japanese Input does or can not retrieve new layout information after returning TS_E_NOLAYOUT error from GetTextExt() even though we call ITextStoreACPSink::OnLayoutChange() and ITfContextOwnerServices::OnLayoutChange(). If we retry to call them after ITextStoreACP::RequestLock() completely, we can avoid this issue. So, let's use PostMessage() for flushing the pending layout change notification only when it's not handled correctly by TIP at synchronous calls.

Before landing this patch, I'll investigate and test this patch more tomorrow.
Attachment #8701785 - Flags: review?(m_kato)
When I tested with the patch, the call stacks of TSFTextStore::RequestLock() at synchronous calls of OnLayoutChange() are:

ITextStoreACPSink::OnLayoutChange():

> #01: mozilla::widget::TSFTextStore::NotifyTSFOfLayoutChange (a:\mozilla\mc-b\src\widget\windows\tsftextstore.cpp:4823)
> #02: mozilla::widget::TSFTextStore::MaybeFlushPendingNotifications (a:\mozilla\mc-b\src\widget\windows\tsftextstore.cpp:1912)
> #03: mozilla::widget::TSFTextStore::RequestLock (a:\mozilla\mc-b\src\widget\windows\tsftextstore.cpp:1632)
> #04: CtfImeProcessCicHotkey[MSCTF +0x2e2ef]
> #05: CtfImeProcessCicHotkey[MSCTF +0x2663a]
> #06: CtfImeProcessCicHotkey[MSCTF +0x274f1]
> #07: DllRegisterServer[GoogleIMEJaTIP64 +0x12475]
> #08: DllRegisterServer[GoogleIMEJaTIP64 +0xd77a]
> #09: DllRegisterServer[GoogleIMEJaTIP64 +0x4263]
> #10: TF_GetInitSystemFlags[MSCTF +0x6ccff]
> #11: TF_GetInitSystemFlags[MSCTF +0x6d0f9]
> #12: CtfImeInquire[MSCTF +0x53d84]
> #13: TF_GetInitSystemFlags[MSCTF +0x6cbed]
> #14: mozilla::widget::TSFTextStore::ProcessRawKeyMessage (a:\mozilla\mc-b\src\widget\windows\tsftextstore.cpp:5495)
> #15: nsAppShell::ProcessNextNativeEvent (a:\mozilla\mc-b\src\widget\windows\nsappshell.cpp:367)
> #16: nsBaseAppShell::DoProcessNextNativeEvent (a:\mozilla\mc-b\src\widget\nsbaseappshell.cpp:139)
> #17: nsBaseAppShell::OnProcessNextEvent (a:\mozilla\mc-b\src\widget\nsbaseappshell.cpp:289)
> #18: nsThread::ProcessNextEvent (a:\mozilla\mc-b\src\xpcom\threads\nsthread.cpp:961)
> #19: NS_ProcessNextEvent (a:\mozilla\mc-b\src\xpcom\glue\nsthreadutils.cpp:297)
> #20: mozilla::ipc::MessagePump::Run (a:\mozilla\mc-b\src\ipc\glue\messagepump.cpp:128)
> #21: MessageLoop::RunHandler (a:\mozilla\mc-b\src\ipc\chromium\src\base\message_loop.cc:228)
> #22: MessageLoop::Run (a:\mozilla\mc-b\src\ipc\chromium\src\base\message_loop.cc:202)
> #23: nsBaseAppShell::Run (a:\mozilla\mc-b\src\widget\nsbaseappshell.cpp:158)
> #24: nsAppShell::Run (a:\mozilla\mc-b\src\widget\windows\nsappshell.cpp:259)
> #25: nsAppStartup::Run (a:\mozilla\mc-b\src\toolkit\components\startup\nsappstartup.cpp:282)
> #26: XREMain::XRE_mainRun (a:\mozilla\mc-b\src\toolkit\xre\nsapprunner.cpp:4285)
> #27: XREMain::XRE_main (a:\mozilla\mc-b\src\toolkit\xre\nsapprunner.cpp:4382)
> #28: XRE_main (a:\mozilla\mc-b\src\toolkit\xre\nsapprunner.cpp:4484)
> #29: do_main (a:\mozilla\mc-b\src\browser\app\nsbrowserapp.cpp:212)
> #30: NS_internal_main (a:\mozilla\mc-b\src\browser\app\nsbrowserapp.cpp:352)
> #31: wmain (a:\mozilla\mc-b\src\toolkit\xre\nswindowswmain.cpp:138)
> #32: __tmainCRTStartup (f:\dd\vctools\crt\crtw32\startup\crt0.c:255)
> #33: BaseThreadInitThunk[KERNEL32 +0x18102]
> #34: RtlUserThreadStart[ntdll +0x5c2e4]

ITfContextOwnerServices::OnLayoutChange():

> #01: CtfImeProcessCicHotkey[MSCTF +0x27a54]
> #02: mozilla::widget::TSFTextStore::NotifyTSFOfLayoutChange (a:\mozilla\mc-b\src\widget\windows\tsftextstore.cpp:4839)
> #03: mozilla::widget::TSFTextStore::MaybeFlushPendingNotifications (a:\mozilla\mc-b\src\widget\windows\tsftextstore.cpp:1912)
> #04: mozilla::widget::TSFTextStore::RequestLock (a:\mozilla\mc-b\src\widget\windows\tsftextstore.cpp:1632)
> #05: CtfImeProcessCicHotkey[MSCTF +0x2e2ef]
> #06: CtfImeProcessCicHotkey[MSCTF +0x2663a]
> #07: CtfImeProcessCicHotkey[MSCTF +0x274f1]
> #08: DllRegisterServer[GoogleIMEJaTIP64 +0x12475]
> #09: DllRegisterServer[GoogleIMEJaTIP64 +0xd77a]
> #10: DllRegisterServer[GoogleIMEJaTIP64 +0x4263]
> #11: TF_GetInitSystemFlags[MSCTF +0x6ccff]
> #12: TF_GetInitSystemFlags[MSCTF +0x6d0f9]
> #13: CtfImeInquire[MSCTF +0x53d84]
> #14: TF_GetInitSystemFlags[MSCTF +0x6cbed]
> #15: mozilla::widget::TSFTextStore::ProcessRawKeyMessage (a:\mozilla\mc-b\src\widget\windows\tsftextstore.cpp:5495)
> #16: nsAppShell::ProcessNextNativeEvent (a:\mozilla\mc-b\src\widget\windows\nsappshell.cpp:367)
> #17: nsBaseAppShell::DoProcessNextNativeEvent (a:\mozilla\mc-b\src\widget\nsbaseappshell.cpp:139)
> #18: nsBaseAppShell::OnProcessNextEvent (a:\mozilla\mc-b\src\widget\nsbaseappshell.cpp:289)
> #19: nsThread::ProcessNextEvent (a:\mozilla\mc-b\src\xpcom\threads\nsthread.cpp:961)
> #20: NS_ProcessNextEvent (a:\mozilla\mc-b\src\xpcom\glue\nsthreadutils.cpp:297)
> #21: mozilla::ipc::MessagePump::Run (a:\mozilla\mc-b\src\ipc\glue\messagepump.cpp:128)
> #22: MessageLoop::RunHandler (a:\mozilla\mc-b\src\ipc\chromium\src\base\message_loop.cc:228)
> #23: MessageLoop::Run (a:\mozilla\mc-b\src\ipc\chromium\src\base\message_loop.cc:202)
> #24: nsBaseAppShell::Run (a:\mozilla\mc-b\src\widget\nsbaseappshell.cpp:158)
> #25: nsAppShell::Run (a:\mozilla\mc-b\src\widget\windows\nsappshell.cpp:259)
> #26: nsAppStartup::Run (a:\mozilla\mc-b\src\toolkit\components\startup\nsappstartup.cpp:282)
> #27: XREMain::XRE_mainRun (a:\mozilla\mc-b\src\toolkit\xre\nsapprunner.cpp:4285)
> #28: XREMain::XRE_main (a:\mozilla\mc-b\src\toolkit\xre\nsapprunner.cpp:4382)
> #29: XRE_main (a:\mozilla\mc-b\src\toolkit\xre\nsapprunner.cpp:4484)
> #30: do_main (a:\mozilla\mc-b\src\browser\app\nsbrowserapp.cpp:212)
> #31: NS_internal_main (a:\mozilla\mc-b\src\browser\app\nsbrowserapp.cpp:352)
> #32: wmain (a:\mozilla\mc-b\src\toolkit\xre\nswindowswmain.cpp:138)
> #33: __tmainCRTStartup (f:\dd\vctools\crt\crtw32\startup\crt0.c:255)
> #34: BaseThreadInitThunk[KERNEL32 +0x18102]
> #35: RtlUserThreadStart[ntdll +0x5c2e4]

During both cases, no methods of TSFTextStore are not called.

Log:

> 0[19269c0e800]: IMECO: 0x1920012f0d0 IMEContentObserver::IMENotificationSender::SendPositionChange(), sending NOTIFY_IME_OF_POSITION_CHANGE...
> 0[19269c0e800]: TSF: 0x19200264300   TSFTextStore::OnLayoutChangeInternal(), calling NotifyTSFOfLayoutChange()...
> 0[19269c0e800]: TSF: 0x19200264300   TSFTextStore::NotifyTSFOfLayoutChange(), calling ITextStoreACPSink::OnLayoutChange()...
> 0[19269c0e800]: TSF: 0x19200264300 TSFTextStore::RequestLock(dwLockFlags=TS_LF_READ, phrSession=0x10425fdab0), mLock=TS_LF_READWRITE, mDestroyed=false
> 0[19269c0e800]: TSF: 0x19200264300   TSFTextStore::RequestLock() didn't allow to lock, *phrSession=TS_E_SYNCHRONOUS
> 0[19269c0e800]: TSF: 0x19200264300   TSFTextStore::NotifyTSFOfLayoutChange(), called ITextStoreACPSink::OnLayoutChange()
> 0[19269c0e800]: TSF: 0x19200264300   TSFTextStore::NotifyTSFOfLayoutChange(), calling ITfContextOwnerServices::OnLayoutChange()...
> 0[19269c0e800]: TSF: 0x19200264300 TSFTextStore::RequestLock(dwLockFlags=TS_LF_READ, phrSession=0x10425fda80), mLock=TS_LF_READWRITE, mDestroyed=false
> 0[19269c0e800]: TSF: 0x19200264300   TSFTextStore::RequestLock() didn't allow to lock, *phrSession=TS_E_SYNCHRONOUS
> 0[19269c0e800]: TSF: 0x19200264300   TSFTextStore::NotifyTSFOfLayoutChange(), called ITfContextOwnerServices::OnLayoutChange()
> 0[19269c0e800]: TSF: 0x19200264300   TSFTextStore::OnLayoutChangeInternal(), calling MaybeFlushPendingNotifications()...
> 0[19269c0e800]: TSF: 0x19200264300   TSFTextStore::MaybeFlushPendingNotifications(), putting off flushing pending notifications due to being the document locked...
> 0[19269c0e800]: IMECO: 0x1920012f0d0 IMEContentObserver::IMENotificationSender::SendPositionChange(), sent NOTIFY_IME_OF_POSITION_CHANGE
> 0[19269c0e800]: TSF: 0x19200264300   TSFTextStore::MaybeFlushPendingNotifications(), mLockedContent is cleared
> 0[19269c0e800]: TSF: 0x19200264300   TSFTextStore::MaybeFlushPendingNotifications(), calling TSFTextStore::NotifyTSFOfLayoutChange()...
> 0[19269c0e800]: TSF: 0x19200264300   TSFTextStore::NotifyTSFOfLayoutChange(), calling ITextStoreACPSink::OnLayoutChange()...
> 0[19269c0e800]: TSF: 0x19200264300 TSFTextStore::RequestLock(dwLockFlags=TS_LF_READ, phrSession=0x10425fe930), mLock=not-specified, mDestroyed=false
> 0[19269c0e800]: TSF: 0x19200264300   Locking (TS_LF_READ) >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
> 0[19269c0e800]: TSF: 0x19200264300   Unlocked (TS_LF_READ) <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
> 0[19269c0e800]: TSF: 0x19200264300   TSFTextStore::RequestLock() succeeded: *phrSession=S_OK
> 0[19269c0e800]: TSF: 0x19200264300   TSFTextStore::NotifyTSFOfLayoutChange(), called ITextStoreACPSink::OnLayoutChange()
> 0[19269c0e800]: TSF: 0x19200264300   TSFTextStore::NotifyTSFOfLayoutChange(), calling ITfContextOwnerServices::OnLayoutChange()...
> 0[19269c0e800]: TSF: 0x19200264300 TSFTextStore::RequestLock(dwLockFlags=TS_LF_READ, phrSession=0x10425fe900), mLock=not-specified, mDestroyed=false
> 0[19269c0e800]: TSF: 0x19200264300   Locking (TS_LF_READ) >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
> 0[19269c0e800]: TSF: 0x19200264300   Unlocked (TS_LF_READ) <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
> 0[19269c0e800]: TSF: 0x19200264300   TSFTextStore::RequestLock() succeeded: *phrSession=S_OK
> 0[19269c0e800]: TSF: 0x19200264300   TSFTextStore::NotifyTSFOfLayoutChange(), called ITfContextOwnerServices::OnLayoutChange()
> 0[19269c0e800]: TSF: 0x19200264300   TSFTextStore::RequestLock() succeeded: *phrSession=S_OK
Oh, I realized that MS-IME and ATOK 2015 on Windows 10, we won't return TS_E_NOLAYOUT with same STR... So, I'm not sure how they work immediately after OnLayoutChange()s after TS_E_NOLAYOUT. I'll check this tomorrow with other environments.
As far as I've tested, looks like this is a bug of TSF.

Currently, in non-e10s mode, we call OnLayoutChange() before unlocking the document lock which caused composition string change. With this behavior, TSF tries to read-lock, but of course it's failed due to still locking the document.

If we call OnLayoucChange() after unlocking the document but from RequestLock(), TSF tries to read-lock the document but does nothing or just retrieve selection range. According to the log and stack-trace at these calls of RequestLock(), the layout change notification is also not reached to TIP.

Finally, like the patch, we call OnLayoutChange() completely after (from outside) RequestLock(), TIP can take some actions.

In conclusion, I believe that this is a mistake of the design of TSF or just a bug of TSF. From TIP's view point, OnLayoutChange() isn't available if the layout change is caused by a change of composition string. OnLayoutChange() is available only when the layout change occurs without a call of RequestLock().


Kato-san, could you review the patch? The patch renames mPendingOnLayoutChange to mHasReturned_TS_E_NOLAYOUT. The new name represents its meaning clearer. When mHasReturned_TS_E_NOLAYOUT is true, calls of OnLayoutChange() should cause calling GetTextExt() or GetACPFromPoint() because TSF/TIP should retrieve the newest layout information. I.e., in this time, we expect some calls of them. Therefore, the patch checks if they are called with mWaitingQueryLayout. If mWaitingQueryLayout is false at the end of OnLayoutChangeInternal(), we should post our internal message for trying to notify TIP of layout change later.
FYI: The reason why this bug won't be reproduced with other TIPs is, other TIPs try to retrieve the latest layout information after a call of RequestLock(), i.e., they try it by themselves, don't depend on calls of OnLayoutChange().
Comment on attachment 8701785 [details] [diff] [review]
TSFTextStore should retry to notify TSF/TIP of layout change if synchronous calls of OnLayoutChange() don't cause retrieving layout information

Review of attachment 8701785 [details] [diff] [review]:
-----------------------------------------------------------------

sorry for delay

::: widget/windows/TSFTextStore.h
@@ +830,5 @@
>    bool                         mPendingOnSelectionChange;
>    // If GetTextExt() or GetACPFromPoint() is called and the layout hasn't been
> +  // calculated yet, these methods return TS_E_NOLAYOUT.  At that time,
> +  // mHasReturned_TS_E_NOLAYOUT is set to true.
> +  bool                         mHasReturned_TS_E_NOLAYOUT;

I think that CamelCase such as mHasReturnedNoLayout is better than mHasReturned_TS_EN_NOLAYOUT.
Attachment #8701785 - Flags: review?(m_kato) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/9104abe421a8311a199fdb3be7a60b3ab73a83b5
Bug 1234422 TSFTextStore should retry to notify TSF/TIP of layout change if synchronous calls of OnLayoutChange() don't cause retrieving layout information r=m_kato
I wonder, it might be better to call PeekMessage() immediately after a call of sKeystrokeMgr->KeyDown(). Then, we could make Google Japanese Input update its UI (pseudo) synchronously. I'll try to check this.
# With current patch, Google Japanese Input sometimes doesn't show candidate window if the process is busy.
https://hg.mozilla.org/mozilla-central/rev/9104abe421a8
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox46: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Tracked for 44 bcuz of negative impact to Japanese users. I hope we can either backout fixes or land a proper fix soon as we are already halfway through Beta44 cycle.
tracking-firefox44: ? → +
Just like Ritu said!
tracking-firefox45: ? → +
tracking-firefox46: ? → +
Comment on attachment 8702602 [details] [diff] [review]
TSFTextStore should retry to notify TSF/TIP of layout change if synchronous calls of OnLayoutChange() don't cause retrieving layout information (r=m_kato)

Approval Request Comment
[Feature/regressing bug #]: bug 1204519
[User impact if declined]: Looks like some keyboard events to convert composition string isn't handled quickly due to not updating UI of Google Japanese Input. (and mismatches composition string on Gecko and selected word in the UI of Google Japanese Input)
[Describe test coverage new/current, TreeHerder]: Landed on m-c a week ago.
[Risks and why]: Very low because the hack works only when IME doesn't try to retrieve layout information as expected. And even if the hack works unexpectedly, IME just retries to refresh its UI.
[String/UUID change made/needed]: Nothing.
Attachment #8702602 - Flags: approval-mozilla-beta?
Attachment #8702602 - Flags: approval-mozilla-aurora?
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) (offline until 1/6) from comment #12)
> I wonder, it might be better to call PeekMessage() immediately after a call
> of sKeystrokeMgr->KeyDown(). Then, we could make Google Japanese Input
> update its UI (pseudo) synchronously. I'll try to check this.
> # With current patch, Google Japanese Input sometimes doesn't show candidate
> window if the process is busy.

I've tried to test with latest Nightly builds. However, I don't meet such situation. I guess in most cases, we don't need additional hack. If we need it, we should do it in another bug.
Comment on attachment 8702602 [details] [diff] [review]
TSFTextStore should retry to notify TSF/TIP of layout change if synchronous calls of OnLayoutChange() don't cause retrieving layout information (r=m_kato)

Taking this because of the significant negative impact this has on Japanese IME users. beta44+, aurora45+
Attachment #8702602 - Flags: approval-mozilla-beta?
Attachment #8702602 - Flags: approval-mozilla-beta+
Attachment #8702602 - Flags: approval-mozilla-aurora?
Attachment #8702602 - Flags: approval-mozilla-aurora+
Oops, I forgot to attach the patch for Beta. Please use this to fix this bug on Beta. (Other change in TSFTextStore::GetACPFromPoint() causes failing to apply the patch for m-c/Aurora)

Approval Request Comment
See above.
Attachment #8704908 - Flags: approval-mozilla-beta?
Comment on attachment 8704908 [details] [diff] [review]
The patch for beta

Thank you for landing, this is now not necessary.
Attachment #8704908 - Flags: approval-mozilla-beta?
Depends on: 1237582
Depends on: 1255627
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: