Closed Bug 971393 Opened 6 years ago Closed 5 years ago

IME composition string becomes invalid if parent of DOM tree is changed

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: m_kato, Assigned: masayuki)

References

Details

(Keywords: inputmethod)

Attachments

(2 files, 1 obsolete file)

Attached file test case
When changing parent of DOM tree of textarea with IME composition string, string is committed.  But this string keeps composting state (still render underline).

- Step
1. Open test case
2. Input string via IME.  String must keep composing state.
3. Wait 10s until change parent DOM tree. (margin and padding are changed)

- Result
If candidate window is opened, this window is closed. And, string is rendered as composing state. Then, when I focus textarea again, rendering composing state of string is invalid.

- Expected Result
If string is committed, string should remove composing state.
Its result is using IMM32.  If using TSF, we cannot input string forever after that.
REQUEST_TO_CANCEL_COMPOSITION doesn't work well on windows widget (imm32 and tsf)?
Attachment #8374562 - Attachment description: text case → test case
(In reply to Makoto Kato (:m_kato) from comment #2)
> REQUEST_TO_CANCEL_COMPOSITION doesn't work well on windows widget (imm32 and
> tsf)?

It should work. I guess that reframing or something causes text event and compositionend event not listened by editor.
The patches of bug 960866 improves the symptom. However, it's not perfect. In TSF mode, composition string never appears on any editors after that.
Depends on: 960866
Maybe, PresShell seems to disallow dispatching text event because nsContentUtils::IsSafeToRunScript() is false.

0:000> k
ChildEBP RetAddr
00a0d188 108487fb xul!PresShell::HandleEvent+0xd2
00a0d1a8 1043a04c xul!nsView::HandleEvent+0x8d
00a0d1c0 1043ca41 xul!nsWindow::DispatchEvent+0x2f
00a0d1d4 103ed07a xul!nsWindow::DispatchWindowEvent+0x13
00a0d340 103ed3fb xul!nsIMM32Handler::DispatchTextEvent+0x18f
00a0d3dc 103ee35f xul!nsIMM32Handler::OnIMEEndComposition+0x130
00a0d3f8 1040fff7 xul!nsIMM32Handler::ProcessMessage+0xb9
00a0d418 10446b01 xul!mozilla::widget::IMEHandler::ProcessMessage+0x57
00a0d478 10446bd5 xul!nsWindow::ExternalHandlerProcessMessage+0x42
00a0d5d8 10445ccd xul!nsWindow::ProcessMessage+0x3f
00a0d604 0f6ac04b xul!nsWindow::WindowProcInternal+0x11c
00a0d644 10445e46 xul!CallWindowProcCrashProtected+0x1f
00a0d668 76b67694 xul!nsWindow::WindowProc+0x63
00a0d694 76b68baa USER32!_InternalCallWinProc+0x23
00a0d724 76b99f49 USER32!UserCallWinProcCheckWow+0x184
00a0d788 76b6f71a USER32!SendMessageWorker+0x2aa7f
00a0d7b8 762dfc99 USER32!SendMessageW+0x12b
00a0d7f0 7601ea0d IMM32!CtfImmGenerateMessage+0xd8
 :
 :
00a0db28 762d6628 MSCTF!CtfNotifyIME+0x4f
00a0db48 103ed21c IMM32!ImmNotifyIME+0x49
00a0db74 10415904 xul!nsIMM32Handler::CancelComposition+0xec
00a0dbe8 1043a735 xul!mozilla::widget::IMEHandler::NotifyIME+0xcd
00a0dbf8 1067b764 xul!nsWindow::NotifyIME+0xe
00a0de24 1067b8c9 xul!nsIMEStateManager::NotifyIME+0x2e4
00a0de34 1064fb83 xul!nsIMEStateManager::NotifyIME+0x43
00a0de44 1067dced xul!mozilla::TextComposition::NotifyIME+0x30
00a0de60 1064babf xul!nsIMEStateManager::OnRemoveContent+0x82
00a0de80 10d74429 xul!nsEventStateManager::ContentRemoved+0x6e
00a0dea0 10931a24 xul!PresShell::ContentRemoved+0x7f
00a0ded8 10917460 xul!nsNodeUtils::ContentRemoved+0x10e
00a0df04 108a85ec xul!nsINode::doRemoveChildAt+0x9e
00a0df28 10923f1b xul!mozilla::dom::FragmentOrElement::RemoveChildAt+0x62
00a0e11c 101b7a70 xul!nsINode::ReplaceOrInsertBefore+0x2a4
00a0e164 103699f9 xul!mozilla::dom::NodeBinding::appendChild+0xb6
00a0e1b4 509fd292 xul!mozilla::dom::GenericBindingMethod+0x113
00a0e1dc 50a2ebc6 mozjs!js::CallJSNative+0x92
00a0e3c8 50a4fa71 mozjs!js::Invoke+0x196
00a0e824 50a02d4b mozjs!Interpret+0x4241
00a0e854 50a1b531 mozjs!js::RunScript+0x1bb
00a0e924 50a1b7dc mozjs!js::ExecuteKernel+0x251
00a0e98c 508f3e63 mozjs!js::Execute+0x1dc
00a0ea60 10609df6 mozjs!JS::Evaluate+0x1f3
 :
 :

SynthesizeCommit cannot commit/cancel well?
Oh, I see. nsIMEStateManager::OnRemoveContent() should use runnable class for calling NotifyIME(). I'll take this in my queue.

# I have some patches changing around nsIMEStateManager.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Component: DOM: Events → Event Handling
OS: Windows 8.1 → All
Hardware: x86 → All
Attached patch Patch (obsolete) — Splinter Review
According to the logs, nsTextStore turns errors when TSF accesses it. The reason is that fails to query contents while script blocker is set. However, query content events don't cause DOM events. Therefore, I think that PresShell::HandleEvent() shouldn't discard any events which won't cause DOM events.
Attachment #8452964 - Flags: review?(bugs)
FYI: Even with the patch, compositionupdate, text and compositionend events are discarded. So, the patch doesn't fix this bug strictly. However, it's enough for now. This is very serious bug if occurs since IME won't work after reproducing this bug.
If we would fix this bug strictly, OnRemoveContent() and OnDestroyPresContext() should call NotifyIME() asynchronously. However, then, perhaps we need to retarget QueryContentEvents, SetSelectionEvents, CompositionEvents and TextEvents. I guess that it needs a lot of work for this edge case...
Asynchronously could mean a script runner, not a runnable, so at least event loop wouldn't
need to spin.
Comment on attachment 8452964 [details] [diff] [review]
Patch

>-  if (!nsContentUtils::IsSafeToRunScript())
>+  if (!nsContentUtils::IsSafeToRunScript() &&
>+      aEvent->IsAllowedToDispatchDOMEvent()) {
>+#ifdef DEBUG
>+    if (aEvent->IsIMERelatedEvent()) {
>+      nsPrintfCString warning("%d event is discarded", aEvent->message);
>+      NS_WARNING(warning.get());
>+    }
>+#endif
>     return NS_OK;
>+  }
Ok, so only events which may create a DOM event return early, fine.


>@@ -7715,17 +7723,22 @@ PresShell::HandleEventInternal(WidgetEve
> 
>     // 2. Give event to the DOM for third party and JS use.
>     if (NS_SUCCEEDED(rv)) {
>       bool wasHandlingKeyBoardEvent =
>         nsContentUtils::IsHandlingKeyBoardEvent();
>       if (aEvent->eventStructType == NS_KEY_EVENT) {
>         nsContentUtils::SetIsHandlingKeyBoardEvent(true);
>       }
>-      if (aEvent->IsAllowedToDispatchDOMEvent()) {
>+      // Some members of aEvent may be modified by ESM or something.  Then,
>+      // the result of IsAllowedToDispatchDOMEvent() might be changed to true.
>+      // Therefore, we need to confirm if nsContentUtils::IsSafeToRunScript() is
>+      // true.
>+      if (nsContentUtils::IsSafeToRunScript() &&
>+          aEvent->IsAllowedToDispatchDOMEvent()) {
But I don't understand this.
What properties change so that IsAllowedToDispatchDOMEvent might suddenly start returning true?
That sounds like a bug to me.
Attachment #8452964 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #10)
> Asynchronously could mean a script runner, not a runnable, so at least event
> loop wouldn't
> need to spin.

Ah, I meant runnable. In TSF mode, Windows' TSF asks us committed string and/or caret position *after* compositionend. It's difficult to retarget query content events (and/or selection set events) to the last focused editor because composition has been already finished. Therefore, I want to fix with this hacky way.

(In reply to Olli Pettay [:smaug] from comment #11)
> >@@ -7715,17 +7723,22 @@ PresShell::HandleEventInternal(WidgetEve
> > 
> >     // 2. Give event to the DOM for third party and JS use.
> >     if (NS_SUCCEEDED(rv)) {
> >       bool wasHandlingKeyBoardEvent =
> >         nsContentUtils::IsHandlingKeyBoardEvent();
> >       if (aEvent->eventStructType == NS_KEY_EVENT) {
> >         nsContentUtils::SetIsHandlingKeyBoardEvent(true);
> >       }
> >-      if (aEvent->IsAllowedToDispatchDOMEvent()) {
> >+      // Some members of aEvent may be modified by ESM or something.  Then,
> >+      // the result of IsAllowedToDispatchDOMEvent() might be changed to true.
> >+      // Therefore, we need to confirm if nsContentUtils::IsSafeToRunScript() is
> >+      // true.
> >+      if (nsContentUtils::IsSafeToRunScript() &&
> >+          aEvent->IsAllowedToDispatchDOMEvent()) {
> But I don't understand this.
> What properties change so that IsAllowedToDispatchDOMEvent might suddenly
> start returning true?
> That sounds like a bug to me.

Um, I worry about wheel event:
http://mxr.mozilla.org/mozilla-central/source/widget/shared/WidgetEventImpl.cpp#228

If all delta values are 0, wheel event doesn't cause a DOM event.

Then, ESM may change delta value at PreHandleEvent():
http://mxr.mozilla.org/mozilla-central/source/dom/events/EventStateManager.cpp#689

However, indeed, it never becomes true from false at least for now because currently, not replacing delta values directly. I mean that the delta values may multiplied by double values specified by prefs. I.e., by the early return style, all delta values must be 0 at script blocked. Then, delta value (0) * n is always 0.

So, yes. we don't need the double check for now, indeed.
Comment on attachment 8453323 [details] [diff] [review]
Patch

"Somebody changed aEvent to cause a DOM event!" or something like that
Attachment #8453323 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/24e6a1f85897
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.