Closed
Bug 1175392
Opened 9 years ago
Closed 9 years ago
[e10s] IME Blur may be too late when moving focus from content to chrome due to race condition
Categories
(Core :: DOM: Events, defect)
Core
DOM: Events
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: m_kato, Assigned: masayuki)
References
Details
(Keywords: inputmethod)
Attachments
(2 files, 2 obsolete files)
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
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.
Assignee | ||
Comment 3•9 years ago
|
||
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)
Reporter | ||
Comment 4•9 years ago
|
||
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 | ||
Updated•9 years ago
|
Assignee: nobody → masayuki
Assignee | ||
Updated•9 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•9 years ago
|
||
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)
Assignee | ||
Comment 6•9 years ago
|
||
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 7•9 years ago
|
||
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.
Assignee | ||
Comment 8•9 years ago
|
||
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)
Assignee | ||
Comment 9•9 years ago
|
||
Added the comment.
Attachment #8626645 -
Attachment is obsolete: true
Attachment #8626645 -
Flags: review?(bugs)
Flags: needinfo?(bugs)
Attachment #8626774 -
Flags: review?(bugs)
Assignee | ||
Comment 10•9 years ago
|
||
Just fix some nits.
Attachment #8626646 -
Attachment is obsolete: true
Attachment #8626646 -
Flags: review?(bugs)
Attachment #8626776 -
Flags: review?(bugs)
Comment 11•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8626776 -
Flags: review?(bugs) → review+
Comment 12•9 years ago
|
||
Comment 13•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c9b9af101489
https://hg.mozilla.org/mozilla-central/rev/333c29008d72
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in
before you can comment on or make changes to this bug.
Description
•