Closed Bug 1369419 Opened 3 years ago Closed 3 years ago

[TSF] Anybody shouldn't use GetMessage() API and PeekMessage() API directly (except WinUtils)

Categories

(Core :: Widget: Win32, enhancement)

Product:
Core
Component:
Widget: Win32
Platform:
All
Windows
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: inputmethod)

Attachments

(1 file)

Bug 1369419 GetMessage() and PeekMessage() shouldn't be used directly as far as possible
59 bytes, text/x-review-board-request
jimm
: review+
Details 
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.
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.
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.
(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.)
(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?
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 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+
(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.
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
(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.
https://hg.mozilla.org/mozilla-central/rev/b404c31db6de
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1370198
You need to log in before you can comment on or make changes to this bug.