569023 - IME composition is committed unexpectedly when the focused window is hanging up on Vista and later.

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

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

On Vista and later, when a window is hanging, Windows paints the contents of the window by lighter colors. Then, (maybe) Windows sends WM_IME_SETCONTEXT to the window and tries to hide the IME related window.

Currently, we commit the composition when we receive the message. Probably the reason is that all composition events for a composition should be handled on a window. That will be guaranteed by bug 519913. So, after that, we can stop committing a composition at WM_IME_SETCONTEXT.

Comment 1

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

Attachment: Patch v1.0

Comment 2

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

Attachment: Patch v2.0

This should work on windowless plug-ins too. I'll test this patch on slow machines.

Comment 3

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

Attachment: Patch v2.1

Comment 4

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

Attachment: Patch v2.1.1

This patch works for me.

This patch has many changes by the WM_IME_SETCONTEXT behavior change.

The main issue is that we shouldn't commit forcedly when we receive WM_IME_SETCONTEXT with true wParam.

1. On nsIMM32Handler::OnIMESetContext(), we should ignore the message if the received window is a top level window. When a window is activating, the IME context is activated on the top level window first. Next, the context is deactivated on the top level window. Finally, the context is activated on the new focused window. So, if we handle the message on top level window, we commit composition unexpectedly.

2. If there is composition and the IME context is activated on non-composing window, we should dispatch text event and end composition event on the composing window. Note that the focus is already moved to the new window which the context is activated on. So, we cannot commit the composition via native events.

3. If we commit the composition on the old window, we should cancel the composition on IME too. Note that after #2, we don't have composition but IME still has it.

4. Some windows which are ancestors of the window which the IME context was activated shouldn't receive WM_IME_SETCONTEXT for being simple code. Therefore, the patch passes the message to DefWindowProc() directly and consumes the message.

However, only these changes, there are some issues on windowless plug-in. Therefore, I moved IME handling code for windowless plug-in from nsWindow::ProcessMessageForPlugin() to nsIMM32Handler. Some new methods which are named OnIME*OnPlugin() are the handler on nsIMM32Handler.

By this change, we can check the state easily.

Finally, I found a bug at testing. When I move focus from windowless plug-in to our editor by a click during composing, the committed text is inputted on the editor. However, it shouldn't be so by some reasons, e.g., the editor might be different web site's.

The cause is that WM_CHAR messages which are caused by WM_IME_CHAR messages was passed to DefWindowProc are received *after* the focus has been moved to us. So, when windowless plug-in has focus, we need to record the WM_IME_CHAR messages which are passed to DefWindowProc. And when we receive the same WM_CHAR messages, compare the messages with recorded WM_IME_CHAR message parameters. If they are matched, we should ignore the WM_CHAR message.

By this patch, the composition isn't going to be committed when Fx is completely deactivated. This is same behavior with Linux and Mac. E.g., even if another process's window steal focus from us accidentally, the user can continue to compose after he/she activates the composing window.

NOTE: case of switch statement indent level was changed on latest coding style rules.
https://developer.mozilla.org/En/Developer_Guide/Coding_Style#Control_Structures
I'll file a follow up bug for other switch statements in nsIMM32Handler.cpp.

Comment 5

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

The patch works fine without the patch of bug 519913.

Comment 6

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

FYI: The switch-case indentation is reverted now, I'll update the patch in next version, so, you don't mind this issue.

Comment 7

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

Attachment: Patch v2.1.2

updating for the latest trunk.

Comment 8

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

Kimura-san, if you don't have much time, I'll request the review to jimm. Do you have much time for it?

Comment 9

[Masatoshi Kimura [:emk]]

Comment on attachment 448688 [details] [diff] [review]
Patch v2.1.2

Sorry, I have no enough time to review the patch for a while.

Comment 10

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

Comment on attachment 448688 [details] [diff] [review]
Patch v2.1.2

O.K., thank you, Kimura-san.

Jimm, would you review this patch?

Comment 11

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

Jimm?

Comment 12

[Jim Mathies [:jimm]]

(In reply to comment #11)
> Jimm?

Sorry, was hoping this would be a smallish patch, but it's not. I really really do not have time to review this right now, as I'm busy with theming. Either find another peer if this is pressing or give me a week or two to get back to it.

Comment 13

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

Jimm:

Okay, the patch doesn't conflict/block other my work. So, I can wait your review because I want the patch on Fx4.0 final.

Comment 14

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

Attachment: Patch v2.1.3

updated for latest trunk.

Comment 15

[Jim Mathies [:jimm]]

Sorry for the delay on reviews. Good news is I should be able to get to it this week.

Comment 16

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

jimm?

Comment 17

[Jim Mathies [:jimm]]

Comment on attachment 457476 [details] [diff] [review]
Patch v2.1.3

>+  return gIMM32Handler && gIMM32Handler->mIsComposingOnPlugin;

What's the lifetime of gIMM32Handler? Could these be statics?

>+  // We can release the instance here, because the instance may be nerver

nit - 'never'

>+  delete gIMM32Handler;
>+  gIMM32Handler = nsnull;

Maybe use Terminate() in place of this.

>+  PRBool handled =
>+    aWindow->DispatchPluginEvent(WM_IME_COMPOSITION, wParam, lParam);
>+  aWindow->DispatchPendingEvents();
>+  return handled;

You've done this in a number of places after processing IME related events for plugins. Processing all events plus paint is pretty heavy work that should happen during normal event processing. Are these calls to DispatchPendingEvents() really needed? If they really aren't needed we should remove these calls.

If they are needed, maybe pass a param to DispatchPluginEvent indicating DispatchPendingEvents should be called, and call it in the body of DispatchPluginEvent. Then this code can be cleaned up:

return aWindow->DispatchPluginEvent(WM_IME_COMPOSITION, wParam, lParam, PR_TRUE/PR_FALSE);

This doesn't work in all cases, but looks like it would for most of them.

>+  static PRBool IsComposing()
>+  {

nit - move brace up.

>+  nsTArray<WPARAM> mPassedIMECharWParam;
>+  nsTArray<LPARAM> mPassedIMECharLParam;
>+    return mPassedIMECharWParam.IsEmpty();
>+    mPassedIMECharWParam.Clear();
>+    mPassedIMECharLParam.Clear();
>+    wParam = mPassedIMECharWParam.ElementAt(0);
>+    lParam = mPassedIMECharLParam.ElementAt(0);

Could we use a single array and store the values in a struct?

Comment 18

[Jim Mathies [:jimm]]

Comment on attachment 457476 [details] [diff] [review]
Patch v2.1.3

Mostly minor nits, nothing serious. Sorry it took so long to get to this. I'll revisit as soon as you post a new rev.

Comment 19

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

(In reply to comment #17)
> >+  return gIMM32Handler && gIMM32Handler->mIsComposingOnPlugin;
> 
> What's the lifetime of gIMM32Handler? Could these be statics?

gIMM32handler is created when current IME sends a first composition message or some other messages. So, if current keyboard layout isn't IME, gIMM32Handler is never created.

When keyboard layout (or IME) changed, gIMM32Handler is destroyed. And when the new keyboard layout (IME) sends a first composition message, this is recreated for the new IME.

First time I know, IME stuff was member variable of nsWindow. Next, I made them static. However, for footprint of non-IME users, I made them that it's created dynamically when it's needed.

> Maybe use Terminate() in place of this.

Oh, thanks.

> >+  PRBool handled =
> >+    aWindow->DispatchPluginEvent(WM_IME_COMPOSITION, wParam, lParam);
> >+  aWindow->DispatchPendingEvents();
> >+  return handled;
> 
> You've done this in a number of places after processing IME related events for
> plugins. Processing all events plus paint is pretty heavy work that should
> happen during normal event processing. Are these calls to
> DispatchPendingEvents() really needed? If they really aren't needed we should
> remove these calls.
> 
> If they are needed, maybe pass a param to DispatchPluginEvent indicating
> DispatchPendingEvents should be called, and call it in the body of
> DispatchPluginEvent. Then this code can be cleaned up:
> 
> return aWindow->DispatchPluginEvent(WM_IME_COMPOSITION, wParam, lParam,
> PR_TRUE/PR_FALSE);
> 
> This doesn't work in all cases, but looks like it would for most of them.

Ugh, I'm not sure whether DispatchPendingEvents(). I guess that from current code, _input events_ should dispatch them for quick feedback. So, I _think_ that WM_KEY*, WM_CHAR and WM_IME_COMPOSITION should dispatch it at least.

Your idea sounds good, I'll do it.

> >+  static PRBool IsComposing()
> >+  {
> 
> nit - move brace up.

Why? Our coding style document said:
> Class and function definitions are not control structures; left brace goes by itself on the second line and without extra indentation, in general.

and https://developer.mozilla.org/En/Developer_Guide/Coding_Style#Methods_and_Functions

> >+  nsTArray<WPARAM> mPassedIMECharWParam;
> >+  nsTArray<LPARAM> mPassedIMECharLParam;
> >+    return mPassedIMECharWParam.IsEmpty();
> >+    mPassedIMECharWParam.Clear();
> >+    mPassedIMECharLParam.Clear();
> >+    wParam = mPassedIMECharWParam.ElementAt(0);
> >+    lParam = mPassedIMECharLParam.ElementAt(0);
> 
> Could we use a single array and store the values in a struct?

O.K.

Comment 20

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

Attachment: Patch v2.2

fix except IsComposing()'s brace.

Comment 21

[Jim Mathies [:jimm]]

Comment on attachment 461093 [details] [diff] [review]
Patch v2.2

Masayuki, could we move the body of DispatchPluginEvent into the nsWindow cpp around the other plugin methods? Seems like a lot of code to put in the header.

Comment 22

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

Attachment: Patch v2.2.1

Thank you, jimm.

This is needed for every IME user on Win Vista and Win 7. We shouldn't commit composition forcibly even if our process becomes busy. If we commit composition at that situation, dictionary of IME may learn the result (committed string) which the user doesn't want to choose. And the learning can cause the dictionary non-useful and that will be shared on all of the system. So, this bug can be very serious (may be breaking user's dictionary of IME).

Comment 23

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

http://hg.mozilla.org/mozilla-central/rev/4926d6b601f7