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

Status: RESOLVED FIXED

(Core :: Widget: Win32, enhancement)

Description

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

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.

Comment 1

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=33364a9286a411768c418a798761cc5ad3130b74

Comment 2

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e055dbaa81b8e58e627c883ab3999191a878769b

Comment 3

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

Attachment: Bug 1369419 GetMessage() and PeekMessage() shouldn't be used directly as far as possible

In TSF mode, application should retrieve messages with ITfMessagePump::GetMessage() or ITfMessagePump::PeekMessage() since TSF/TIP may handle the message before or after the host application handles it.

This patch rewrites the API users with WinUtils::(Get|Peek)Message() which use ITfMessagePump if it's available.

Review commit: https://reviewboard.mozilla.org/r/145224/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/145224/

Comment 4

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

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

[Jim Mathies [:jimm]]

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.

Comment 6

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

(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

[Jim Mathies [:jimm]]

(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

[Jim Mathies [:jimm]]

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

[Jim Mathies [:jimm]]

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

Comment 10

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

(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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b404c31db6de
GetMessage() and PeekMessage() shouldn't be used directly as far as possible r=jimm

Comment 12

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

(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

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/b404c31db6de