1175392 - [e10s] IME Blur may be too late when moving focus from content to chrome due to race condition

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Makoto Kato [:m_kato]]

When moving focus from content to chrome, chrome will call remote->Deactive().  Then, IMEStateManager::OnChangeFocus on content will call NOTIFY_IME_OF_BLUR to chrome. 

When I debug on my Linux box, the following race condition seems to occur.

1. Call remote->Deactive()
2. NOTIFY_IME_OF_FOCUS on chrome's widget
3. NOTIFY_IME_OF_BLUR on content's widget
4. NOTIFY_IME_OF_BLUR on chrome's widget

So 2. should be after 4.

Comment 1

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

I think that focus/blur notification should be notified via IMEStateManager (both from IMEContentObserver and TabParent). Then, IMEStateManager in parent can manage the race. If a caller tries to send blur but IME has focus, it should ignore the call. If a caller tries send focus but IME still has focus, it should send blur first. If chrome content has focus but TabParent tries to set focus to the content, it should be ignored since chrome focus change is handled synchronously.

Comment 2

[Jim Mathies [:jimm]]

Should this block e10s rollout?

Comment 3

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

Should be. I found a lot issues around this.

FYI: There are similar issues. When child process is busy, two or more composition events are sent from parent to child. Then, IME (in parent) may try to query content. However, if content process hasn't finished to handle the received event, the cache in parent is too old. Then, IME is confused and doesn't work well. We need to emulate the content cache until TabParent receives the result from PuppetWidget.

Comment 4

[Makoto Kato [:m_kato]]

Also, IME sometimes is ever disabled on my Linux desktop when moving focus from content to chrome.  But when I  focus on same chrome, IME can be enabled.  So I think that a reason of it may be this issue. (ex. bug 1063650)

Comment 5

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

Attachment: part.1 IMEContentObserver and TabParent should use IMEStateManager::NotifyIME()

First, all notifications to IME should be sent from IMEStateManager. Then, we can manage the focus state of IME with IMEStateManager.

Comment 6

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

Attachment: part.2 IMEStateManager should manage notifications to IME especially delayed notifications from remote process

Next, IMEStateManager should guarantee that blur notification must be sent first when focus notification is sending.

Then, delayed notifications may come. IMEStateManager should not send it to widget since IME has already been linked with new focused content. So, sending notifications in old focused content may make IME confused.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8626645 [details] [diff] [review]
part.1 IMEContentObserver and TabParent should use IMEStateManager::NotifyIME()

>+IMEStateManager::NotifyIME(const IMENotification& aNotification,
>+                           nsIWidget* aWidget,
>+                           bool aOriginIsRemote)
>+{
>+  MOZ_LOG(sISMLog, LogLevel::Info,
>+    ("ISM: IMEStateManager::NotifyIME(aNotification={ mMessage=%s }, "
>+     "aWidget=0x%p, aOriginIsRemote=%s)",
>+     GetNotifyIMEMessageName(aNotification.mMessage), aWidget,
>+     GetBoolName(aOriginIsRemote)));
>+
>+  if (NS_WARN_IF(!aWidget)) {
>+    MOZ_LOG(sISMLog, LogLevel::Error,
>+      ("ISM:   IMEStateManager::NotifyIME(), FAILED due to no widget"));
>+    return NS_ERROR_INVALID_ARG;
>+  }
>+
>+  switch (aNotification.mMessage) {
>+    case NOTIFY_IME_OF_FOCUS:
>+    case NOTIFY_IME_OF_BLUR:
>+    case NOTIFY_IME_OF_SELECTION_CHANGE:
>+    case NOTIFY_IME_OF_TEXT_CHANGE:
>+    case NOTIFY_IME_OF_POSITION_CHANGE:
>+    case NOTIFY_IME_OF_MOUSE_BUTTON_EVENT:
>+      return aWidget->NotifyIME(aNotification);
>+    default:
>+      break;
>+  }
I don't understand why we special case these messages here.
Some comment at least is needed.

Comment 8

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

Yep. The reason is, these notifications are not related if there is composition. But others (handled below) depend on that. Therefore, the code becomes so.

Comment 9

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

Attachment: part.1 IMEContentObserver and TabParent should use IMEStateManager::NotifyIME()

Added the comment.

Comment 10

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

Attachment: part.2 IMEStateManager should manage notifications to IME especially delayed notifications from remote process

Just fix some nits.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8626774 [details] [diff] [review]
part.1 IMEContentObserver and TabParent should use IMEStateManager::NotifyIME()

>+      // FYI: Other notifications should be send only when there is composition.
>+      //      So, we need to handle the others below.
I'd drop FYI:
and s/be send/be sent/

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c9b9af101489
https://hg.mozilla.org/integration/mozilla-inbound/rev/333c29008d72

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c9b9af101489
https://hg.mozilla.org/mozilla-central/rev/333c29008d72