Closed Bug 1388647 Opened 7 years ago Closed 7 years ago

[e10s] Crash in mozilla::ContentCacheInParent::OnEventNeedingAckHandled on macOS

Categories

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

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- wontfix
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: crash, inputmethod)

Crash Data

Attachments

(2 files)

This bug was filed from the Socorro interface and is 
report bp-79fe859f-d8b3-4f50-bd5a-a9b8c0170809.
=============================================================
> mozilla::ContentCacheInParent::OnEventNeedingAckHandled(nsIWidget*, mozilla::EventMessage)	widget/ContentCache.cpp:1222
> mozilla::dom::TabParent::RecvOnEventNeedingAckHandled(mozilla::EventMessage const&)	dom/ipc/TabParent.cpp:1862
> mozilla::dom::PBrowserParent::OnMessageReceived(IPC::Message const&)	obj-firefox/ipc/ipdl/PBrowserParent.cpp:2251
> mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&)	ipc/glue/MessageChannel.cpp:2092
> mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&)	ipc/glue/MessageChannel.cpp:2018
> mozilla::ipc::MessageChannel::MessageTask::Run()	ipc/glue/MessageChannel.cpp:1920
> nsThread::ProcessNextEvent(bool, bool*)	xpcom/threads/nsThread.cpp:1446
> NS_ProcessPendingEvents(nsIThread*, unsigned int)	xpcom/threads/nsThreadUtils.cpp:422
> nsBaseAppShell::NativeEventCallback()	widget/nsBaseAppShell.cpp:97
> nsAppShell::ProcessGeckoEvents(void*)	widget/cocoa/nsAppShell.mm:409 

STR:
1. Open two windows.
2. Start composition with IME in a tab (do not commit the composition).
3. Activate another application, e.g., click on desktop.
4. Click the other window's title bar.

I cannot reproduce this crash with this STR on the other platforms.  However, this is similar to bug 1387357.
I checked the native IME behavior on OS X 10.9 (oldest one which we support) and macOS 10.12 (current).
Comment on attachment 8895747 [details]
Bug 1388647 - part1: IMEStateManager::OnChangeFocusInternal() shouldn't request to commit composition with sFocusedIMETabParent

https://reviewboard.mozilla.org/r/166506/#review173206
Attachment #8895747 - Flags: review?(m_kato) → review+
Comment on attachment 8895748 [details]
Bug 1388647 - part2: Make IMEInputHandler of Cocoa widget handle request to commit/cancel composition synchronously

https://reviewboard.mozilla.org/r/166508/#review173208

::: commit-message-c06c1:3
(Diff revision 1)
> +Bug 1388647 - part2: Make IMEInputHandler of Cocoa widget handle request to commit/cancel composition synchronously r?m_kato
> +
> +When Gecko starts to support Cocoa widget, we needed to use NSInputManager.

When Gecko started ...
Attachment #8895748 - Flags: review?(m_kato) → review+
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/8f09ad78575d
part1: IMEStateManager::OnChangeFocusInternal() shouldn't request to commit composition with sFocusedIMETabParent r=m_kato
https://hg.mozilla.org/integration/autoland/rev/91b531826640
part2: Make IMEInputHandler of Cocoa widget handle request to commit/cancel composition synchronously r=m_kato
Looks like this affects 56 as well. Should we request uplift to Beta?
Flags: needinfo?(masayuki)
Comment on attachment 8895747 [details]
Bug 1388647 - part1: IMEStateManager::OnChangeFocusInternal() shouldn't request to commit composition with sFocusedIMETabParent

Approval Request Comment
[Feature/Bug causing the regression]:
Regression of e10s mode (no occurs without e10s mode).

[User impact if declined]:
If user switches windows of Firefox when there is IME composition, composition events are handled in wrong TabParent and crashes with MOZ_RELEASE_ASSERT.

[Is this code covered by automated tests?]:
No.

[Has the fix been verified in Nightly?]:
Yes.

[Needs manual test from QE? If yes, steps to reproduce]: 
No.

[List of other uplifts needed for the feature/fix]:
No.

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
Making IMEStateManager::OnChangeFocusInternal() request IME to commit composition only when there is composition and:
* focus is moving to another element or
* all windows are being inactivated and active IME doesn't want us to keep composition during deactive.

The old behavior is, in the latter case, it puts off to request commit composition until a window is being activated later. The reason is ancient Cocoa API limitation.

[String changes made/needed]:
No.
Flags: needinfo?(masayuki)
Attachment #8895747 - Flags: approval-mozilla-beta?
Comment on attachment 8895748 [details]
Bug 1388647 - part2: Make IMEInputHandler of Cocoa widget handle request to commit/cancel composition synchronously

Approval Request Comment
[Is this code covered by automated tests?]:
No.

[Has the fix been verified in Nightly?]:
Yes.

[Needs manual test from QE? If yes, steps to reproduce]: 
No.

[List of other uplifts needed for the feature/fix]:
The "part1".

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
This makes the behavior of REQUEST_IME_TO_COMMIT_COMPOSITION on macOS same as the other platforms. Due to the ancient Cocoa API limitation, we couldn't commit composition when window is being inactivated.  However, currently, it's possible. Therefore, this patch removes the focus check and async commit request handlers which are for the cases if the request is posted during deactive.

So, even though this patch changes a lot of lines, but the changes are very simple.

[String changes made/needed]:
No.
Attachment #8895748 - Flags: approval-mozilla-beta?
Comment on attachment 8895747 [details]
Bug 1388647 - part1: IMEStateManager::OnChangeFocusInternal() shouldn't request to commit composition with sFocusedIMETabParent

Crash fix, let's take this for beta 5.
Attachment #8895747 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8895748 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 1396302
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.