Closed Bug 1179632 Opened 4 years ago Closed 4 years ago

[e10s][TSF] Remaining composition in child process causes hitting assertion aCompositionEvent->message == (2200) in child process

Categories

(Core :: User events and focus handling, defect, minor)

x86
Windows 8.1
defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
e10s + ---
firefox42 --- affected
firefox45 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

(Keywords: crash, inputmethod)

Attachments

(6 files, 6 obsolete files)

854 bytes, patch
smaug
: review+
Details | Diff | Splinter Review
7.25 KB, patch
smaug
: review+
Details | Diff | Splinter Review
5.98 KB, patch
smaug
: review+
Details | Diff | Splinter Review
21.10 KB, patch
smaug
: review+
Details | Diff | Splinter Review
31.87 KB, patch
smaug
: review+
Details | Diff | Splinter Review
39.13 KB, patch
smaug
: review+
Details | Diff | Splinter Review
STR:

1. Enable both e10s and TSF and select MS-IME.
2. Focus child process's editor.
3. Make a composition.
4. Click [X] button in the titlebar.

Then, hits the assertion.

> Assertion failure: aCompositionEvent->message == (2200), at d:/mozilla/mc-a/src/dom/events/IMEStateManager.cpp:1164
> #01: PresShell::DispatchEventToDOM (d:\mozilla\mc-a\src\layout\base\nspresshell.cpp:8011)
> #02: PresShell::HandleEventInternal (d:\mozilla\mc-a\src\layout\base\nspresshell.cpp:7913)
> #03: PresShell::HandleEvent (d:\mozilla\mc-a\src\layout\base\nspresshell.cpp:7635)
> #04: nsViewManager::DispatchEvent (d:\mozilla\mc-a\src\view\nsviewmanager.cpp:787)
> #05: nsView::HandleEvent (d:\mozilla\mc-a\src\view\nsview.cpp:1095)
> #06: mozilla::widget::PuppetWidget::DispatchEvent (d:\mozilla\mc-a\src\widget\puppetwidget.cpp:317)
> #07: mozilla::layers::APZCCallbackHelper::DispatchWidgetEvent (d:\mozilla\mc-a\src\gfx\layers\apz\util\apzccallbackhelper.cpp:443)
> #08: mozilla::dom::TabChild::RecvCompositionEvent (d:\mozilla\mc-a\src\dom\ipc\tabchild.cpp:2590)
> #09: mozilla::dom::PBrowserChild::OnMessageReceived (d:\mozilla\mc-a\fx-dbg\ipc\ipdl\pbrowserchild.cpp:3516)
> #10: mozilla::dom::PContentChild::OnMessageReceived (d:\mozilla\mc-a\fx-dbg\ipc\ipdl\pcontentchild.cpp:5435)
> #11: mozilla::ipc::MessageChannel::DispatchAsyncMessage (d:\mozilla\mc-a\src\ipc\glue\messagechannel.cpp:1281)
> #12: mozilla::ipc::MessageChannel::DispatchMessageW (d:\mozilla\mc-a\src\ipc\glue\messagechannel.cpp:1199)
> #13: mozilla::ipc::MessageChannel::OnMaybeDequeueOne (d:\mozilla\mc-a\src\ipc\glue\messagechannel.cpp:1183)
> #14: MessageLoop::RunTask (d:\mozilla\mc-a\src\ipc\chromium\src\base\message_loop.cc:365)
> #15: MessageLoop::DeferOrRunPendingTask (d:\mozilla\mc-a\src\ipc\chromium\src\base\message_loop.cc:375)
> #16: MessageLoop::DoWork (d:\mozilla\mc-a\src\ipc\chromium\src\base\message_loop.cc:459)
> #17: mozilla::ipc::DoWorkRunnable::Run (d:\mozilla\mc-a\src\ipc\glue\messagepump.cpp:221)
> #18: nsThread::ProcessNextEvent (d:\mozilla\mc-a\src\xpcom\threads\nsthread.cpp:848)
> #19: NS_ProcessNextEvent (d:\mozilla\mc-a\src\xpcom\glue\nsthreadutils.cpp:265)
> #20: mozilla::ipc::MessagePump::Run (d:\mozilla\mc-a\src\ipc\glue\messagepump.cpp:127)
> #21: mozilla::ipc::MessagePumpForChildProcess::Run (d:\mozilla\mc-a\src\ipc\glue\messagepump.cpp:289)
> #22: MessageLoop::RunInternal (d:\mozilla\mc-a\src\ipc\chromium\src\base\message_loop.cc:234)
> #23: MessageLoop::RunHandler (d:\mozilla\mc-a\src\ipc\chromium\src\base\message_loop.cc:228)
> #24: MessageLoop::Run (d:\mozilla\mc-a\src\ipc\chromium\src\base\message_loop.cc:202)
> #25: nsBaseAppShell::Run (d:\mozilla\mc-a\src\widget\nsbaseappshell.cpp:167)
> #26: nsAppShell::Run (d:\mozilla\mc-a\src\widget\windows\nsappshell.cpp:180)
> #27: XRE_RunAppShell (d:\mozilla\mc-a\src\toolkit\xre\nsembedfunctions.cpp:778)
> #28: mozilla::ipc::MessagePumpForChildProcess::Run (d:\mozilla\mc-a\src\ipc\glue\messagepump.cpp:259)
> #29: MessageLoop::RunInternal (d:\mozilla\mc-a\src\ipc\chromium\src\base\message_loop.cc:234)
> #30: MessageLoop::RunHandler (d:\mozilla\mc-a\src\ipc\chromium\src\base\message_loop.cc:228)
> #31: MessageLoop::Run (d:\mozilla\mc-a\src\ipc\chromium\src\base\message_loop.cc:202)
> #32: XRE_InitChildProcess (d:\mozilla\mc-a\src\toolkit\xre\nsembedfunctions.cpp:618)
> #33: content_process_main (d:\mozilla\mc-a\src\ipc\contentproc\plugin-container.cpp:237)
> #34: wmain (d:\mozilla\mc-a\src\toolkit\xre\nswindowswmain.cpp:138)
> #35: __tmainCRTStartup (f:\dd\vctools\crt\crtw32\startup\crt0.c:255)
> #36: BaseThreadInitThunk[KERNEL32 +0x17c04]
> #37: RtlInitializeExceptionChain[ntdll +0x5ad1f]
> #38: RtlInitializeExceptionChain[ntdll +0x5acea]

The log of the child process:

> 0[3011140]: ISM: IMEStateManager::DispatchCompositionEvent(aNode=0x860d260, aPresContext=0x8597c00, aCompositionEvent={ message=NS_COMPOSITION_CHANGE, mFlags={ mIsTrusted=true, mPropagationStopped=false } }, aIsSynthesized=false), tabParent=0
> 0[3011140]: ISM: IMEStateManager::NotifyIME(aNotification={ mMessage=NOTIFY_IME_OF_TEXT_CHANGE }, aWidget=0x30982b0, aOriginIsRemote=false), sFocusedIMEWidget=0x30982b0, sRemoteHasFocus=false
> 0[3011140]: ISM: IMEStateManager::NotifyIME(aNotification={ mMessage=NOTIFY_IME_OF_SELECTION_CHANGE }, aWidget=0x30982b0, aOriginIsRemote=false), sFocusedIMEWidget=0x30982b0, sRemoteHasFocus=false
> 0[3011140]: ISM: IMEStateManager::NotifyIME(aNotification={ mMessage=NOTIFY_IME_OF_POSITION_CHANGE }, aWidget=0x30982b0, aOriginIsRemote=false), sFocusedIMEWidget=0x30982b0, sRemoteHasFocus=false
> 0[3011140]: ISM: IMEStateManager::NotifyIME(aMessage=NOTIFY_IME_OF_COMPOSITION_UPDATE, aPresContext=0x8597c00, aOriginIsRemote=false)
> 0[3011140]: ISM: IMEStateManager::NotifyIME(aNotification={ mMessage=NOTIFY_IME_OF_COMPOSITION_UPDATE }, aWidget=0x30982b0, aOriginIsRemote=false), sFocusedIMEWidget=0x30982b0, sRemoteHasFocus=false
> 0[3011140]: ISM:   IMEStateManager::NotifyIME(), composition=0x306e560, composition->IsSynthesizedForTests()=false
> 0[3011140]: ISM: IMEStateManager::DispatchCompositionEvent(aNode=0x860d260, aPresContext=0x8597c00, aCompositionEvent={ message=NS_COMPOSITION_CHANGE, mFlags={ mIsTrusted=true, mPropagationStopped=false } }, aIsSynthesized=false), tabParent=0
> 0[3011140]: ISM: IMEStateManager::NotifyIME(aMessage=NOTIFY_IME_OF_COMPOSITION_UPDATE, aPresContext=0x8597c00, aOriginIsRemote=false)
> 0[3011140]: ISM: IMEStateManager::NotifyIME(aNotification={ mMessage=NOTIFY_IME_OF_COMPOSITION_UPDATE }, aWidget=0x30982b0, aOriginIsRemote=false), sFocusedIMEWidget=0x30982b0, sRemoteHasFocus=false
> 0[3011140]: ISM:   IMEStateManager::NotifyIME(), composition=0x306e560, composition->IsSynthesizedForTests()=false
> 0[3011140]: ISM: IMEStateManager::DispatchCompositionEvent(aNode=0x860d260, aPresContext=0x8597c00, aCompositionEvent={ message=NS_COMPOSITION_CHANGE, mFlags={ mIsTrusted=true, mPropagationStopped=false } }, aIsSynthesized=false), tabParent=0
> 0[3011140]: ISM:   IMEStateManager::DispatchCompositionEvent(), adding new TextComposition to the array

The log in parent process:

> 0[2811140]: ISM: IMEStateManager::DispatchCompositionEvent(aNode=0x19eaace0, aPresContext=0xb3fb800, aCompositionEvent={ message=NS_COMPOSITION_CHANGE, mFlags={ mIsTrusted=true, mPropagationStopped=false } }, aIsSynthesized=false), tabParent=19cc9f00
> 0[2811140]: ISM: IMEStateManager::NotifyIME(aNotification={ mMessage=NOTIFY_IME_OF_COMPOSITION_UPDATE }, aWidget=0xb368000, aOriginIsRemote=true), sFocusedIMEWidget=0xb368000, sRemoteHasFocus=true
> 0[2811140]: ISM:   IMEStateManager::NotifyIME(), composition=0x286f060, composition->IsSynthesizedForTests()=false
> 0[2811140]: ISM: IMEStateManager::DispatchCompositionEvent(aNode=0x19eaace0, aPresContext=0xb3fb800, aCompositionEvent={ message=NS_COMPOSITION_CHANGE, mFlags={ mIsTrusted=true, mPropagationStopped=false } }, aIsSynthesized=false), tabParent=19cc9f00
> 0[2811140]: ISM: IMEStateManager::DispatchCompositionEvent(aNode=0x19eaace0, aPresContext=0xb3fb800, aCompositionEvent={ message=NS_COMPOSITION_COMMIT, mFlags={ mIsTrusted=true, mPropagationStopped=false } }, aIsSynthesized=false), tabParent=19cc9f00
> 0[2811140]: ISM:   IMEStateManager::DispatchCompositionEvent(), removing TextComposition from the array since NS_COMPOSTION_END was dispatched
> 0[2811140]: ISM: IMEStateManager::OnChangeFocus(aPresContext=0x0, aContent=0x0, aCause=CAUSE_UNKNOWN)
> 0[2811140]: ISM: IMEStateManager::OnChangeFocusInternal(aPresContext=0x0, aContent=0x0 (TabParent=0x0), aAction={ mCause=CAUSE_UNKNOWN, mFocusChange=FOCUS_NOT_CHANGED }), sPresContext=0xb3fb800, sContent=0x19eaace0, sActiveTabParent=0x19cc9f00, sActiveIMEContentObserver=0x0, sInstalledMenuKeyboardListener=false
> 0[2811140]: ISM:   IMEStateManager::OnChangeFocusInternal(), no nsPresContext is being activated
> 0[2811140]: ISM: IMEStateManager::OnDestroyPresContext(aPresContext=0xb3fb800), sPresContext=0xb3fb800, sContent=0x19eaace0, sTextCompositions=0x3317dc0
> 0[2811140]: ISM: IMEStateManager::DestroyIMEContentObserver(), sActiveIMEContentObserver=0x0
> 0[2811140]: ISM:   IMEStateManager::DestroyIMEContentObserver() does nothing
> 0[2811140]: ISM: IMEStateManager::GetNewIMEState(aPresContext=0xb3fb800, aContent=0x0), sInstalledMenuKeyboardListener=false
> 0[2811140]: ISM:   IMEStateManager::GetNewIMEState() returns DISABLED because no content has focus
> 0[2811140]: ISM: IMEStateManager::SetIMEState(aState={ mEnabled=DISABLED, mOpen=DONT_CHANGE_OPEN_STATE }, aContent=0x0 (TabParent=0x0), aWidget=0xb368000, aAction={ mCause=CAUSE_UNKNOWN, mFocusChange=LOST_FOCUS })
> 0[2811140]: ISM: IMEStateManager::SetInputContext(aWidget=0xb368000, aInputContext={ mIMEState={ mEnabled=DISABLED, mOpen=DONT_CHANGE_OPEN_STATE }, mHTMLInputType="", mHTMLInputInputmode="", mActionHint="" }, aAction={ mCause=CAUSE_UNKNOWN_CHROME, mAction=LOST_FOCUS }), sActiveTabParent=0x19cc9f00
> 0[2811140]: ISM: IMEEnabledStateChangedEvent::Run(), notifies observers of "ime-enabled-state-changed"
> 0[2811140]: ISM: IMEStateManager::Shutdown(), sTextCompositions=0x3317dc0, sTextCompositions->Length()=0

I'm not sure why this isn't reproduced with Google Japanese Input nor ATOK.
At least there are two bugs:

1. When child process dispatches a composition event coming from its parent process, PuppetWidget::GetInputContext().mNativeIMEContext may be nullptr. At this time, IMEStateManager::DispatchCompositionEvent() cannot retrieve proper TextComposition instance.
2. When a window is being destroyed, child process tries to commit composition via PBrowser.EndIMEComposition(). However, TabParent::RecvEndIMEComposition() cannot retrieve widget for itself already. Therefore, it does nothing, i.e, not initialize aNoCompositionEvent. Then, PuppetWidget dispatches composition commit event accidentally when aNoCompositionEvent returns true even in such case. This causes destroying TextComposition instance in the child process, and commit events will be sent from the parent process because parent process detects focus change. Finally, the delayed commit event causes crash since there is already no TextComposition instance.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
This bug is now reproducible with any IME after the fix of bug 1224454.
Summary: [e10s][TSF] Remaining composition in child process causes hitting assertion aCompositionEvent->message == (2200) in child process only with MS-IME on Win8.1 → [e10s][TSF] Remaining composition in child process causes hitting assertion aCompositionEvent->message == (2200) in child process
See comment 1 for checking what the cause of this bug is.

For fixing the former bug, we need to make remote process stop accessing native input context via IPC. First of fix this issue, we should remove native IME context member from InputContext since we should reduce the number of calls of GetInputContext() via IPC as far as possible.

This patch is a preparation, and roc might be better reviewer due to changes under widget/, though. If you think so, feel free to redirect the review request to him.

This patch doesn't change current logic except changing the special value meaning change. Currently, if only one IME context is available in a process, the widget on such platform uses nullptr. But this does not make sense because cannot distinguish if the member is initialized correctly or not. This patch defines NS_ONLY_ONE_NATIVE_IME_CONTEXT as reinterpret_cast<void*>(static_cast<intptr_t>(-1)). And make nullptr uninitialized value.
Attachment #8693493 - Flags: review?(bugs)
For reducing to retrieve native IME context via IPC, WidgetCompositionEvent should tell the native IME context which caused the composition. Then, PuppetWidget can cache active native IME context for it. Note that native IME context is necessary only for looking for TextComposition instance for specific widget. Therefore, if PuppetWidget hasn't received WidgetCompositionEvent, there must not be no TextComposition instance for it. Therefore, even if PuppetWidget::mNativeIMEContext is nullptr, related code (IMEStateManager, etc) works fine.

However, we need a hack for automated tests and IME on Firefox OS which use TextInputProcessor (and TextEventDispatcher). They dispatch composition events from a remote process. In this time, the widget is always PuppetWidget. However, PuppetWidget may not know actual native IME context. Therefore, PuppetWidget::GetNativeData(NS_NATIVE_IME_CONTEXT) may return nullptr. So, only in such case, TextEventDispatcher should set the widget's pointer as native IME context instead. TextEventDispatcher is now created per widget and TextInputProcessor can create input transaction per widget. So, this hack does make sense. I'll make this behavior used even in parent process in bug 1137572. Until then, this hack is enough.
Attachment #8693494 - Flags: review?(bugs)
This is an existing bug. (#2 of comment 1). This causes unexpected eCompositionCommit event in the remote process. That causes hits similar assertion.
Attachment #8693501 - Flags: review?(bugs)
The name "EndIMEComposition" isn't good name for our IME code. We should rename it to "RequestIMEToCommitComposition".

And we should remove mCompositionEventsDuringRequest and mRequestedToCommitOrCancelComposition if we make mCommitStringByRequest a pointer to nsString. Then, the code becomes simpler.

And at writing this patch, I realized that PuppetWidget doesn't set mOnDestroyCalled properly. Therefore, nsIWidget::Destroyed() won't work...
Attachment #8693503 - Flags: review?(bugs)
At looking for a TextComposition instance with WidgetCompositionEvent, it should use WidgetCompositionEvent::mNativeIMEContext instead of calling nsIWidget::GetNativeData(NS_NATIVE_IME_CONTEXT) of WidgetGUIEvent::widget.
Attachment #8693504 - Flags: review?(bugs)
This fix isn't related to this bug, but we should fix this here.

Currently, ESM initializes WidgetKeyboardEvent::mIsComposing with native IME context. However, this way can expose IME state on different document. So, it should initialize the value with nsPresContext. Then, it won't expose other document's IME state accidentally. But please note that I believe that this bug won't occur actually because native keyboard event won't be fired on different document which shares native IME context since composition is always committed at changing focus in same native IME context. This is the reason why I don't want to this is fixed on different bug. And we can get rid of IMEStateManager::GetTextCompositionFor(const WidgetKeyboardEvent*).
Attachment #8693506 - Flags: review?(bugs)
I believe that part.1 and part.2 reduce a lot of IPC during composition. So, the response of UI must be improves on slow machines.

And I forgot to say, part.2 moves some bool members of PuppetWidget. This reduces the size of PuppetWidget instance even adding new member, mNativeIMEContext. A lot of developers don't mind memory alignment, sigh...
Comment on attachment 8693493 [details] [diff] [review]
part.1 native IME context should not be stored in InputContext but should be able to retrieve with nsIWidget::GetNativeData()


>+  const IMEContext& DefaultIMC() const { return mDefaultIMC; }
...
>   HBRUSH                mBrush;
>+  IMEContext            mDefaultIMC;
I don't understand the name mDefaultIMC.
Perhaps DefaultIMEContext() and mDefaultIMEContext?

But I'll need to go through this patch still once more.
Attachment #8693501 - Flags: review?(bugs) → review+
Attachment #8693506 - Flags: review?(bugs) → review+
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #15)
> And I forgot to say, part.2 moves some bool members of PuppetWidget. This
> reduces the size of PuppetWidget instance even adding new member,
> mNativeIMEContext. A lot of developers don't mind memory alignment, sigh...
Yeah, though in case of PuppetWidget we don't have too many instances of them.
Attachment #8693504 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #16)
> Comment on attachment 8693493 [details] [diff] [review]
> part.1 native IME context should not be stored in InputContext but should be
> able to retrieve with nsIWidget::GetNativeData()
> 
> 
> >+  const IMEContext& DefaultIMC() const { return mDefaultIMC; }
> ...
> >   HBRUSH                mBrush;
> >+  IMEContext            mDefaultIMC;
> I don't understand the name mDefaultIMC.
> Perhaps DefaultIMEContext() and mDefaultIMEContext?

Ah, IMC is enough name for developers who know IMM.
https://msdn.microsoft.com/en-us/dynamics/dd318558%28v=vs.80%29

But if you like mDefaultIMEContext better, I'd change the name.
Comment on attachment 8693493 [details] [diff] [review]
part.1 native IME context should not be stored in InputContext but should be able to retrieve with nsIWidget::GetNativeData()

>+    // If there is no default IMC, we should return the pointer to the window.
>+    // This is possible if no IME is installed.
>+    return aWindow;
I don't understand what in the old code gives this behavior.
Comment on attachment 8693503 [details] [diff] [review]
part.4 Clean up the code to request IME to commit composition across process boundary

Note to myself
I don't yet understand the changes to ContentCacheInParent::OnCompositionEvent
nor ContentCacheInParent::RequestIMEToCommitComposition
Comment on attachment 8693494 [details] [diff] [review]
part.2 WidgetCompositionEvent should store native IME context which caused the event and PuppetWidget should store it for GetNativeData(NS_NATIVE_IME_CONTEXT)

>+++ b/widget/nsGUIEventIPC.h
>@@ -539,33 +539,37 @@ template<>
> struct ParamTraits<mozilla::WidgetCompositionEvent>
> {
>   typedef mozilla::WidgetCompositionEvent paramType;
> 
>   static void Write(Message* aMsg, const paramType& aParam)
>   {
>     WriteParam(aMsg, static_cast<mozilla::WidgetGUIEvent>(aParam));
>     WriteParam(aMsg, aParam.mData);
>+    WriteParam(aMsg, reinterpret_cast<intptr_t>(aParam.mNativeIMEContext));
>     bool hasRanges = !!aParam.mRanges;
>     WriteParam(aMsg, hasRanges);
>     if (hasRanges) {
>       WriteParam(aMsg, *aParam.mRanges.get());
>     }
>   }
> 
>   static bool Read(const Message* aMsg, void** aIter, paramType* aResult)
>   {
>     bool hasRanges;
>+    intptr_t nativeIMEContext;
>     if (!ReadParam(aMsg, aIter,
>                    static_cast<mozilla::WidgetGUIEvent*>(aResult)) ||
>         !ReadParam(aMsg, aIter, &aResult->mData) ||
>+        !ReadParam(aMsg, aIter, &nativeIMEContext) ||
>         !ReadParam(aMsg, aIter, &hasRanges)) {
>       return false;
>     }
> 
>+    aResult->mNativeIMEContext = reinterpret_cast<void*>(nativeIMEContext);

I don't understand how this setup can work. We use mNativeIMEContext in comparisons and it can be
be either a pointer to something in other process's virtual space, or some nsIWidget object in current process.
Those both may have same numerical value.
Attachment #8693494 - Flags: review?(bugs) → review-
Comment on attachment 8693493 [details] [diff] [review]
part.1 native IME context should not be stored in InputContext but should be able to retrieve with nsIWidget::GetNativeData()

I think I need some answer to comment 19 and part 2 needs to be fixed too, and those might affect to this patch.
Attachment #8693493 - Flags: review?(bugs)
Comment on attachment 8693503 [details] [diff] [review]
part.4 Clean up the code to request IME to commit composition across process boundary

Somehow super hard to read patch. (perhaps time 2:18am doesn't help :) )

+  // When the composition wasn't committed synchronously.  The remote process's
s/wasn't/isn't/
s/. The/, the/


+  // When the composition is committed synchronously.  Then, return the commit
s/. Then,/,

+  // to the remote process. Then, PuppetWidget will dispatch eCompositionCommit
+  // event with this.
'this'? I don't know what 'this' refers to here.


  Then, TextComposition in the remote process will be
+  // destroyed by the event (in IMEStateManager::DispatchCompositionEvent())
+  return composition->Destroyed();
And what if the composition isn't destroyed? In which case does the method return false here?


I'd like to see that comment cleaned up quite a bit.
Attachment #8693503 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #21)
> Comment on attachment 8693494 [details] [diff] [review]
> part.2 WidgetCompositionEvent should store native IME context which caused
> the event and PuppetWidget should store it for
> GetNativeData(NS_NATIVE_IME_CONTEXT)
> 
> >+++ b/widget/nsGUIEventIPC.h
> >@@ -539,33 +539,37 @@ template<>
> > struct ParamTraits<mozilla::WidgetCompositionEvent>
> > {
> >   typedef mozilla::WidgetCompositionEvent paramType;
> > 
> >   static void Write(Message* aMsg, const paramType& aParam)
> >   {
> >     WriteParam(aMsg, static_cast<mozilla::WidgetGUIEvent>(aParam));
> >     WriteParam(aMsg, aParam.mData);
> >+    WriteParam(aMsg, reinterpret_cast<intptr_t>(aParam.mNativeIMEContext));
> >     bool hasRanges = !!aParam.mRanges;
> >     WriteParam(aMsg, hasRanges);
> >     if (hasRanges) {
> >       WriteParam(aMsg, *aParam.mRanges.get());
> >     }
> >   }
> > 
> >   static bool Read(const Message* aMsg, void** aIter, paramType* aResult)
> >   {
> >     bool hasRanges;
> >+    intptr_t nativeIMEContext;
> >     if (!ReadParam(aMsg, aIter,
> >                    static_cast<mozilla::WidgetGUIEvent*>(aResult)) ||
> >         !ReadParam(aMsg, aIter, &aResult->mData) ||
> >+        !ReadParam(aMsg, aIter, &nativeIMEContext) ||
> >         !ReadParam(aMsg, aIter, &hasRanges)) {
> >       return false;
> >     }
> > 
> >+    aResult->mNativeIMEContext = reinterpret_cast<void*>(nativeIMEContext);
> 
> I don't understand how this setup can work. We use mNativeIMEContext in
> comparisons and it can be
> be either a pointer to something in other process's virtual space, or some
> nsIWidget object in current process.
> Those both may have same numerical value.

They should be used as identifier of native IME context. E.g., looking for TextComposition instance which is caused by specific native IME context. So, WidgetCompositionEvent::mNativeIMEContext can be intptr_t. (Note that on Windows, all windows should have same IME context unless third party application associates its own IME context with our window. On the other hand, on Linux and Mac, IME context is independent per window. I.e., widgets in a window can share same IME context, though.)
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #19)
> Comment on attachment 8693493 [details] [diff] [review]
> part.1 native IME context should not be stored in InputContext but should be
> able to retrieve with nsIWidget::GetNativeData()
> 
> >+    // If there is no default IMC, we should return the pointer to the window.
> >+    // This is possible if no IME is installed.
> >+    return aWindow;
> I don't understand what in the old code gives this behavior.

return nullptr means that it fails to retrieve native IME context. For preventing this failure, we should return non-nullptr value. The pointer to the widget is enough if there is no IME context since when active window is changed to another our window, existing composition should be committed. (FYI: even if there is no IME, dead key should cause composition in the future, see https://w3c.github.io/uievents/#keys-dead)
Updated the comment and use |if| block for making each explanation clearer.
Attachment #8693503 - Attachment is obsolete: true
Attachment #8695267 - Flags: review?(bugs)
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #25)
> return nullptr means that it fails to retrieve native IME context. For
> preventing this failure, we should return non-nullptr value. The pointer to
> the widget is enough if there is no IME context since when active window is
> changed to another our window, existing composition should be committed.
Well it sure needs a comment why aWindow is used there.
Flags: needinfo?(bugs)
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #24)
> They should be used as identifier of native IME context.
Yes, and what guarantees they are unique on child side? Do operating systems map some memory location in a process space always to a IME? I doubt. What guarantees the pointer in one process can be used as a unique id in the other
process?
(In reply to Olli Pettay [:smaug] from comment #28)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #24)
> > They should be used as identifier of native IME context.
> Yes, and what guarantees they are unique on child side?

Unfortunately, not guaranteed. However, in usual cases, this is enough safe because:

* On Windows, basically, all windows should be associated with default IME context which is created by Windows per process. And when it's in TSF mode, it always same pointer since we can guarantee that third party's something never creates new IME context.
* On Linux and Mac, native IME context is allocated when a window is created. When the window is destroyed, TextComposition is always destroyed immediately before that. So, even if new IME context is same as old instance, there must not be TextComposition instance created by the old context.
* On other platforms, we use NS_ONLY_ONE_NATIVE_IME_CONTEXT.

I believe that this is enough safe. If you don't think so, please suggest some bad scenarios, I'll try to fix that on other bugs.
Oh, but the TextEventDispatcher.cpp's code in child process may conflict with parent process's native IME context.

Perhaps, we should manage native IME context with its origin process ID??
Hmm, but looks like "process ID" isn't guaranteed that it's available on all platforms (I checked NSPR). Let me look for new approach. (But even if we have this risk, that must not cause any problem because TextInputProcessor is used by automated tests and VKB on B2G. In use cases of both of them, mNativeIMEContext is never conflict since they are used *without* actual native IME context.)
could be:

struct NativeIMEContextID
{
  void* mNativeIMEContext;
  // mProcessGeneration is incremented when it's copied to remote process
  uint8_t mProcessGeneration;

  explicit NativeIMEContextID(nsIWidget* aWidget)
    : mNativeIMEContext(aWidget->GetNativeData(NS_NATIVE_IME_CONTEXT))
    , mProcessGeneration(0)
  {
  }
};

And WriteParam increments mProcessGeneration. Then, TextComposition should compare NativeIMEContextID instances.
Hmm, I'm pretty sure there is Gecko level process ID available on all the platforms, since for
DOMWindow IDs we use process ID + process specific counter.
See NextWindowID() in ContentChild.cpp. nsGlobalWindow.cpp uses that.
(In reply to Olli Pettay [:smaug] from comment #34)
> See NextWindowID() in ContentChild.cpp. nsGlobalWindow.cpp uses that.

Oh, I got it. Thanks!
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentChild.cpp?rev=f04beafaf860#3092

So, I think that part.1 doesn't have problem for now. Could you review it? I'll rewrite part.2 tomorrow.
This patches defines NativeIMEContext struct in IMEData.h.

The struct initializes the process ID automatically. I've not yet tested the value actually because I couldn't get IMEStateManager's log when I run test_bug1026397.html with --e10s... I'm not sure the reason (only IMEStateManager::Shutdown() was there...).

Now, "raw" native IME context can be retrieved with nsIWidget::GetNativeData(NS_RAW_NATIVE_IME_CONTEXT). But XP code should use nsIWidget::GetNativeIMEContext() which returns NativeIMEContext (i.e., a pair of raw native IME context and its origin process ID).
Attachment #8695809 - Attachment is obsolete: true
Attachment #8695898 - Flags: review?(bugs)
Comment on attachment 8695807 [details] [diff] [review]
part.1 native IME context should not be stored in InputContext but should be able to retrieve with nsIWidget::GetNativeData()

Updated the comments before returning a pointer of nsWindow (or similar) instead of native IME context.
Attachment #8695807 - Flags: review?(bugs)
Sorry, I posted a patch for different bug.
Attachment #8695898 - Attachment is obsolete: true
Attachment #8695898 - Flags: review?(bugs)
Attachment #8695901 - Flags: superreview?(bugs)
Attachment #8695901 - Flags: review?(bugs)
Sorry for the spam. Fixing some nits of the logging code in IMEStateManager.
Attachment #8695901 - Attachment is obsolete: true
Attachment #8695901 - Flags: superreview?(bugs)
Attachment #8695901 - Flags: review?(bugs)
Attachment #8695915 - Flags: superreview?(bugs)
Attachment #8695915 - Flags: review?(bugs)
Attachment #8695915 - Flags: superreview?(bugs)
Attachment #8695915 - Flags: superreview+
Attachment #8695915 - Flags: review?(bugs)
Attachment #8695915 - Flags: review+
Attachment #8695267 - Flags: review?(bugs) → review+
I'm surprised because you reviewed the patches in this timing. Thank you very much.

Just a reminder to you, part.1 is still r?. But if you don't have much time, it's okay to land next week since most users won't close window during composition.
Sorry, the dinner last night got in my way to review part1 :) Going to review it today.
Attachment #8695807 - Flags: review?(bugs) → review+
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.