Closed Bug 1175392 Opened 5 years ago Closed 5 years ago

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


(Core :: DOM: Events, defect)

Not set



Tracking Status
firefox41 --- fixed


(Reporter: m_kato, Assigned: masayuki)


(Blocks 1 open bug)


(Keywords: inputmethod)


(2 files, 2 obsolete files)

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.
Blocks: e10s-ime
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.
Should this block e10s rollout?
Flags: needinfo?(masayuki)
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.
Flags: needinfo?(masayuki)
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)
Assignee: nobody → masayuki
First, all notifications to IME should be sent from IMEStateManager. Then, we can manage the focus state of IME with IMEStateManager.
Attachment #8626645 - Flags: review?(bugs)
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.
Attachment #8626646 - Flags: review?(bugs)
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_BLUR:
>+      return aWidget->NotifyIME(aNotification);
>+    default:
>+      break;
>+  }
I don't understand why we special case these messages here.
Some comment at least is needed.
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.
Flags: needinfo?(bugs)
Added the comment.
Attachment #8626645 - Attachment is obsolete: true
Attachment #8626645 - Flags: review?(bugs)
Flags: needinfo?(bugs)
Attachment #8626774 - Flags: review?(bugs)
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/
Attachment #8626774 - Flags: review?(bugs) → review+
Attachment #8626776 - Flags: review?(bugs) → review+
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Depends on: 1178652
Depends on: 1179661
Blocks: 1063650
You need to log in before you can comment on or make changes to this bug.