Closed
Bug 1369419
Opened 6 years ago
Closed 6 years ago
[TSF] Anybody shouldn't use GetMessage() API and PeekMessage() API directly (except WinUtils)
Categories
(Core :: Widget: Win32, enhancement)
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: masayuki, Assigned: masayuki)
References
Details
(Keywords: inputmethod)
Attachments
(1 file)
Could be a cause of bug 1361132. TSF-aware application should retrieve message with ITfMessagePump. However, some our code and other libraries use GetMessage() and PeekMessage() directly. We should fix the former case ASAP.
Assignee | ||
Comment 1•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=33364a9286a411768c418a798761cc5ad3130b74
Assignee | ||
Comment 2•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e055dbaa81b8e58e627c883ab3999191a878769b
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•6 years ago
|
||
I have no idea how to refer ITfMessagePump instance from mozglue/misc/StackWalk.cpp because it's QI'ed from ITfThreadMgr which is created by TSFTextStore. And I'm not sure how should we do for following modules: https://searchfox.org/mozilla-central/source/ipc/chromium/src/base/message_pump_win.cc https://searchfox.org/mozilla-central/source/media/webrtc/trunk/webrtc/base/win32socketserver.cc https://searchfox.org/mozilla-central/source/gfx/skia/skia/src/views/win/skia_win.cpp And I don't know about third party's dll which is loaded in our process since they can retrieve message with the APIs directly.
![]() |
||
Comment 5•6 years ago
|
||
I was looking over chromium code and noticed they don't interact with ITfMessagePump at all, do they not support TSF? How serious is the non-use of the WinUtils routines because not using them in message_pump_win.cc seems like it may be serious, but I don't understand why all of our peek/get message calls need to go through ITfMessagePump.
Assignee | ||
Comment 6•6 years ago
|
||
(In reply to Jim Mathies [:jimm] from comment #5) > I was looking over chromium code and noticed they don't interact with > ITfMessagePump at all, do they not support TSF? Yes, they tried to support TSF when they try to implement Metoro ver. However, they dropped TSF support when they discontinue to support Metro ver. > How serious is the non-use of the WinUtils routines Honestly, I'm not sure since the crash bug which this blocks is a bug of TSF or TIP. This patch is just a "try" to avoid the crash. > because not using them > in message_pump_win.cc seems like it may be serious, but I don't understand > why all of our peek/get message calls need to go through ITfMessagePump. It depends on what TSF/TIP does. If they watch window closing or something, using ITfMessagePump must be important for them. If they assume that every message can be handled before application because of ITfMessagePump rule, they may have stateful cache. However, if not using ITfMessagePump causes breaking such assumption, they may hit a crash like bug 1361132. I mean that if we can avoid the crash with this patch, the number of crash reports should be decreased. (Anyway, the crash should be fixed by Microsoft since somebody may use (Get|Peek)Message API directly, e.g., security apps DLL or something.)
![]() |
||
Comment 7•6 years ago
|
||
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #6) > (In reply to Jim Mathies [:jimm] from comment #5) > > I was looking over chromium code and noticed they don't interact with > > ITfMessagePump at all, do they not support TSF? > > Yes, they tried to support TSF when they try to implement Metoro ver. > However, they dropped TSF support when they discontinue to support Metro ver. Does this negatively impact their browser in certain circumstances? For example, I'm assuming this means they don't support context sensitive keyboard hints? Other than the built-in soft keyboard I don't know of any TSF apps that act as text services, do you?
![]() |
||
Comment 8•6 years ago
|
||
From the ITfMessagePump docs - "This interface enables the TSF manager to perform any necessary pre-message or post-message processing." This doesn't sound bad assuming the wrappers make the call they claim to. If they don't these changes could break things in odd ways.. particularly the WindowsMessageLoop calls.
![]() |
||
Comment 9•6 years ago
|
||
mozreview-review |
Comment on attachment 8873797 [details] Bug 1369419 GetMessage() and PeekMessage() shouldn't be used directly as far as possible https://reviewboard.mozilla.org/r/145224/#review149298
Attachment #8873797 -
Flags: review?(jmathies) → review+
Assignee | ||
Comment 10•6 years ago
|
||
(In reply to Jim Mathies [:jimm] from comment #7) > (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #6) > > (In reply to Jim Mathies [:jimm] from comment #5) > > > I was looking over chromium code and noticed they don't interact with > > > ITfMessagePump at all, do they not support TSF? > > > > Yes, they tried to support TSF when they try to implement Metoro ver. > > However, they dropped TSF support when they discontinue to support Metro ver. > > Does this negatively impact their browser in certain circumstances? For > example, I'm assuming this means they don't support context sensitive > keyboard hints? Yeah, could be. Even when they implemented TSF, it was limited. They didn't want TSF to access editor contents fully. Perhaps, the reason was difficult to implement (especially in multi process model) and/or the performance. On the other hand, IMM has simple feature to access around caret: https://msdn.microsoft.com/ja-jp/library/windows/desktop/dd318634(v=vs.85).aspx https://msdn.microsoft.com/ja-jp/library/windows/desktop/dd318632(v=vs.85).aspx TSF might provide contents with them via IMM emulator, CUAS to TIP. > Other than the built-in soft keyboard I don't know of any TSF apps that act > as text services, do you? Voice input and screen reader could be implemented as TIP. But I don't know actual of them. And our TSF module care only keyboard and IME, though.
Comment 11•6 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/b404c31db6de GetMessage() and PeekMessage() shouldn't be used directly as far as possible r=jimm
Assignee | ||
Comment 12•6 years ago
|
||
(In reply to Jim Mathies [:jimm] from comment #8) > From the ITfMessagePump docs - > > "This interface enables the TSF manager to perform any necessary pre-message > or post-message processing." > > This doesn't sound bad assuming the wrappers make the call they claim to. If > they don't these changes could break things in odd ways.. particularly the > WindowsMessageLoop calls. Even if the patch would break something, should be easy to back it out.
![]() |
||
Comment 13•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b404c31db6de
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•