Closed Bug 1377672 Opened 7 years ago Closed 7 years ago

Intermittent toolkit/content/tests/chrome/test_bug360437.xul | Assertion count 1 is greater than expected range 0-0 assertions on Windows (ASSERTION: input context of focused widget should be set from a remote process)

Categories

(Core :: DOM: UI Events & Focus Handling, defect)

Product:
Core
Component:
DOM: UI Events & Focus Handling
Platform:
All
Windows
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- unaffected
firefox56 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: inputmethod, intermittent-failure, regression, Whiteboard: [stockwell fixed:product])

Attachments

(5 files)

Bug 1377672 - part1: IMEStateManager in the main process should destroy active IME content observer when a tab parent takes focus
59 bytes, text/x-review-board-request
m_kato
: review+
Details
Bug 1377672 - part2: IMEStateManager::SetIMEState() should set input context with proper origin information
59 bytes, text/x-review-board-request
m_kato
: review+
Details
Bug 1377672 - part3: IMEStateManager::NotifyIME() should ignore notifications and requests which comes from unexpected process
59 bytes, text/x-review-board-request
smaug
: review+
m_kato
: review+
Details
Bug 1377672 - part4: ContentCacheInParent::RequestIMEToCommitComposition() should ignore too late requests
59 bytes, text/x-review-board-request
m_kato
: review+
Details
Bug 1377672 - part5: IMEStateManager::OnChangeFocusInternal() should notify IME of blur when focus is moving from a remote process to another process
59 bytes, text/x-review-board-request
m_kato
: review+
Details 
This is new regression orange and different from bug 1321173. Perhaps, caused by the fix of bug 1375491 and some other fix in https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=6e0dd1705c75d8aeedc26e7f5c9ea466f92aec96 because this orange appeared high frequently after the bug fix met some of changes in the latter.

The bug 1375491 changes the timing of notifying focus. I guess that it could cause some race between setting IME focus and destroying the window. Anyway, bug 1349255 will make it async. So, this would appear anyway.

I'll investigate this in next Monday.
The cause of this bug is, IMEStateManager in the main process doesn't handle delayed notifications and requests to IME properly. For example, IMEStateManager doesn't discard some delayed notifications which came from previously focused remote process. Additionally, IMEStateManager doesn't set the origin of InputContext properly in some cases. So, we need to update IMEStateManager. Especially for bug 1349255.

I'd like smaug to review around process checking and non-IMEStateManager part and kato-san to review IMEStateManager's changes.

# Patches coming soon.
Comment on attachment 8883584 [details]
Bug 1377672 - part3: IMEStateManager::NotifyIME() should ignore notifications and requests which comes from unexpected process

https://reviewboard.mozilla.org/r/154466/#review159594

::: commit-message-71b7f:1
(Diff revision 1)
> +Bug 1377672 - part3: IMEStateManager::NotifyIME() should ignore notifications nor requests which comes from unexpected process r?smaug, m_kato

I don't understand this sentence "nor"?

Should this be
IMEStateManager::NotifyIME() should not ignore notifications nor requests which comes from unexpected process.

Or should it be
IMEStateManager::NotifyIME() should ignore notifications and requests which comes from unexpected process

or
IMEStateManager::NotifyIME() should ignore notifications, not requests, which comes from unexpected process


Based on the explanation, the second one with 'and'

::: dom/events/IMEStateManager.h:284
(Diff revision 1)
>    // sWidget is cache for the root widget of sPresContext.  Even afer
>    // sPresContext has gone, we need to clean up some IME state on the widget
>    // if the widget is available.
>    static nsIWidget* sWidget;
> -  // sFocusedIMEWidget is, the widget which was sent to "focus" notification
> -  // from IMEContentObserver and not yet sent "blur" notification.
> +  // sFocusedIMEWidget and sFocusedIMETabParent are, the tab parent sent
> +  // "focus" notification to the widget (and not yet send "blur" notification).

Rather weird sentence.

Perhaps

sFocusedIMETabParent is the tab parent, which sent "focus" notification to sFocusedIMEWidget (and didn't yet sent "blur" notification).

::: dom/events/IMEStateManager.cpp:145
(Diff revision 1)
> +IsSameProcess(const TabParent* aTabParent1, const TabParent* aTabParent2)
> +{
> +  if (aTabParent1 == aTabParent2) {
> +    return true;
> +  }
> +  if (!aTabParent1 ^ !aTabParent2) {

Why ^
Wouldn't !aTabParent1 != !aTabParent2 work.
Attachment #8883584 - Flags: review?(bugs) → review+
Comment on attachment 8883584 [details]
Bug 1377672 - part3: IMEStateManager::NotifyIME() should ignore notifications and requests which comes from unexpected process

https://reviewboard.mozilla.org/r/154466/#review159594

> I don't understand this sentence "nor"?
> 
> Should this be
> IMEStateManager::NotifyIME() should not ignore notifications nor requests which comes from unexpected process.
> 
> Or should it be
> IMEStateManager::NotifyIME() should ignore notifications and requests which comes from unexpected process
> 
> or
> IMEStateManager::NotifyIME() should ignore notifications, not requests, which comes from unexpected process
> 
> 
> Based on the explanation, the second one with 'and'

Ah, the second is I tried to say.

> Rather weird sentence.
> 
> Perhaps
> 
> sFocusedIMETabParent is the tab parent, which sent "focus" notification to sFocusedIMEWidget (and didn't yet sent "blur" notification).

Thanks!

> Why ^
> Wouldn't !aTabParent1 != !aTabParent2 work.

Wow, you're right!
Whiteboard: [stockwell needswork]
Comment on attachment 8883582 [details]
Bug 1377672 - part1: IMEStateManager in the main process should destroy active IME content observer when a tab parent takes focus

https://reviewboard.mozilla.org/r/154462/#review159822
Attachment #8883582 - Flags: review?(m_kato) → review+
Comment on attachment 8883583 [details]
Bug 1377672 - part2: IMEStateManager::SetIMEState() should set input context with proper origin information

https://reviewboard.mozilla.org/r/154464/#review159836
Attachment #8883583 - Flags: review?(m_kato) → review+
Comment on attachment 8883585 [details]
Bug 1377672 - part4: ContentCacheInParent::RequestIMEToCommitComposition() should ignore too late requests

https://reviewboard.mozilla.org/r/154468/#review159846
Attachment #8883585 - Flags: review?(m_kato) → review+
Comment on attachment 8883586 [details]
Bug 1377672 - part5: IMEStateManager::OnChangeFocusInternal() should notify IME of blur when focus is moving from a remote process to another process

https://reviewboard.mozilla.org/r/154470/#review160114
Attachment #8883586 - Flags: review?(m_kato) → review+
Comment on attachment 8883584 [details]
Bug 1377672 - part3: IMEStateManager::NotifyIME() should ignore notifications and requests which comes from unexpected process

https://reviewboard.mozilla.org/r/154466/#review160116
Attachment #8883584 - Flags: review?(m_kato) → review+
Wow, I realized that I requested the review with older patches. But the difference is only minor fixes. So, I don't request review again. If you have some disagreement about the different, please file a followup bug since this bug causes a lot of oranges which waste the time of a lot of developers.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 02b18f9cf6f4 -d 52d810352774: rebasing 405647:02b18f9cf6f4 "Bug 1377672 - part1: IMEStateManager in the main process should destroy active IME content observer when a tab parent takes focus r=m_kato"
merging dom/events/IMEStateManager.cpp
rebasing 405648:2f507d297f8e "Bug 1377672 - part2: IMEStateManager::SetIMEState() should set input context with proper origin information r=m_kato"
merging dom/events/IMEStateManager.cpp
merging widget/nsBaseWidget.cpp
warning: conflicts while merging dom/events/IMEStateManager.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Wow... The patches were rebased with the latest m-c...
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/dde8300a4831
part1: IMEStateManager in the main process should destroy active IME content observer when a tab parent takes focus r=m_kato
https://hg.mozilla.org/integration/autoland/rev/cd648a42778f
part2: IMEStateManager::SetIMEState() should set input context with proper origin information r=m_kato
https://hg.mozilla.org/integration/autoland/rev/14a41922c96a
part3: IMEStateManager::NotifyIME() should ignore notifications and requests which comes from unexpected process r=m_kato,smaug
https://hg.mozilla.org/integration/autoland/rev/feb3d54b7cca
part4: ContentCacheInParent::RequestIMEToCommitComposition() should ignore too late requests r=m_kato
https://hg.mozilla.org/integration/autoland/rev/3bdc87d4e538
part5: IMEStateManager::OnChangeFocusInternal() should notify IME of blur when focus is moving from a remote process to another process r=m_kato
Removing trailing whitespace of unrelated line was landed before this and that caused the rebasing error.
Whiteboard: [stockwell needswork] → [stockwell fixed:product]
Depends on: 1381732
Depends on: 1387357
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: