1305355 - [e10s] Hits MOZ_ASSERT() when removing composition string of MS Pinyin on Win10

Status: RESOLVED FIXED

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

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

STR:

1. Type "ceshiceshi" in a textarea element in e10s mode with MS Pinyin
2. Commit a part of it (In my environment, "2" commits only "ceshi" as "测试"
3. Type Backspace key 5 times to remove remaining "ceshi".

Actual Result:
Hits MOZ_ASSERT in ContentEventHandler at removing "c".

> Assertion failure: startSet (The start of the range should've been set already), at a:/mozilla/src/dom/events/ContentEventHandler.cpp:1101
> #01: mozilla::ContentEventHandler::OnQueryTextRect (a:\mozilla\src\dom\events\contenteventhandler.cpp:2130)
> #02: mozilla::ContentEventHandler::OnQueryTextRectArray (a:\mozilla\src\dom\events\contenteventhandler.cpp:2092)
> #03: mozilla::ContentEventHandler::HandleQueryContentEvent (a:\mozilla\src\dom\events\contenteventhandler.cpp:1247)
> #04: mozilla::IMEContentObserver::HandleQueryContentEvent (a:\mozilla\src\dom\events\imecontentobserver.cpp:775)
> #05: mozilla::EventStateManager::HandleQueryContentEvent (a:\mozilla\src\dom\events\eventstatemanager.cpp:886)
> #06: mozilla::EventStateManager::PreHandleEvent (a:\mozilla\src\dom\events\eventstatemanager.cpp:610)
> #07: PresShell::HandleEventInternal (a:\mozilla\src\layout\base\nspresshell.cpp:8172)
> #08: PresShell::HandleEvent (a:\mozilla\src\layout\base\nspresshell.cpp:7898)
> #09: nsViewManager::DispatchEvent (a:\mozilla\src\view\nsviewmanager.cpp:816)
> #10: nsView::HandleEvent (a:\mozilla\src\view\nsview.cpp:1118)
> #11: mozilla::widget::PuppetWidget::DispatchEvent (a:\mozilla\src\widget\puppetwidget.cpp:356)
> #12: mozilla::ContentCacheInChild::QueryCharRectArray (a:\mozilla\src\widget\contentcache.cpp:310)
> #13: mozilla::ContentCacheInChild::CacheTextRects (a:\mozilla\src\widget\contentcache.cpp:351)
> #14: mozilla::ContentCacheInChild::CacheSelection (a:\mozilla\src\widget\contentcache.cpp:181)
> #15: mozilla::ContentCacheInChild::CacheText (a:\mozilla\src\widget\contentcache.cpp:271)
> #16: mozilla::widget::PuppetWidget::NotifyIMEOfTextChange (a:\mozilla\src\widget\puppetwidget.cpp:863)
> #17: mozilla::widget::PuppetWidget::NotifyIMEInternal (a:\mozilla\src\widget\puppetwidget.cpp:653)
> #18: nsBaseWidget::NotifyIME (a:\mozilla\src\widget\nsbasewidget.cpp:1806)
> #19: mozilla::IMEStateManager::NotifyIME (a:\mozilla\src\dom\events\imestatemanager.cpp:1358)
> #20: mozilla::IMEContentObserver::IMENotificationSender::SendTextChange (a:\mozilla\src\dom\events\imecontentobserver.cpp:1769)
> #21: mozilla::IMEContentObserver::IMENotificationSender::Run (a:\mozilla\src\dom\events\imecontentobserver.cpp:1561)
> #22: mozilla::IMEContentObserver::TryToFlushPendingNotifications (a:\mozilla\src\dom\events\imecontentobserver.cpp:1438)
> #23: PresShell::HandleEventInternal (a:\mozilla\src\layout\base\nspresshell.cpp:8211)
> #24: PresShell::HandleEvent (a:\mozilla\src\layout\base\nspresshell.cpp:7898)
> #25: nsViewManager::DispatchEvent (a:\mozilla\src\view\nsviewmanager.cpp:816)
> #26: nsView::HandleEvent (a:\mozilla\src\view\nsview.cpp:1118)
> #27: mozilla::widget::PuppetWidget::DispatchEvent (a:\mozilla\src\widget\puppetwidget.cpp:356)
> #28: mozilla::layers::APZCCallbackHelper::DispatchWidgetEvent (a:\mozilla\src\gfx\layers\apz\util\apzccallbackhelper.cpp:471)
> #29: mozilla::dom::TabChild::RecvCompositionEvent (a:\mozilla\src\dom\ipc\tabchild.cpp:2180)
> #30: mozilla::dom::PBrowserChild::OnMessageReceived (a:\mozilla\fx-dbg\ipc\ipdl\pbrowserchild.cpp:4199)
> #31: mozilla::dom::PContentChild::OnMessageReceived (a:\mozilla\fx-dbg\ipc\ipdl\pcontentchild.cpp:6564)
> #32: mozilla::ipc::MessageChannel::DispatchAsyncMessage (a:\mozilla\src\ipc\glue\messagechannel.cpp:1664)
> #33: mozilla::ipc::MessageChannel::DispatchMessageW (a:\mozilla\src\ipc\glue\messagechannel.cpp:1603)
> #34: mozilla::ipc::MessageChannel::OnMaybeDequeueOne (a:\mozilla\src\ipc\glue\messagechannel.cpp:1570)
> #35: mozilla::detail::RunnableMethodImpl<bool (__thiscall mozilla::ipc::MessageChannel::*)(void),0,1>::Run (a:\mozilla\fx-dbg\dist\include\nsthreadutils.h:766)
> #36: mozilla::ipc::MessageChannel::DequeueTask::Run (a:\mozilla\fx-dbg\dist\include\mozilla\ipc\messagechannel.h:561)
> #37: nsThread::ProcessNextEvent (a:\mozilla\src\xpcom\threads\nsthread.cpp:1082)
> #38: NS_ProcessNextEvent (a:\mozilla\src\xpcom\glue\nsthreadutils.cpp:290)
> #39: mozilla::ipc::MessagePump::Run (a:\mozilla\src\ipc\glue\messagepump.cpp:96)
> #40: mozilla::ipc::MessagePumpForChildProcess::Run (a:\mozilla\src\ipc\glue\messagepump.cpp:301)
> #41: MessageLoop::RunInternal (a:\mozilla\src\ipc\chromium\src\base\message_loop.cc:232)
> #42: MessageLoop::RunHandler (a:\mozilla\src\ipc\chromium\src\base\message_loop.cc:226)
> #43: MessageLoop::Run (a:\mozilla\src\ipc\chromium\src\base\message_loop.cc:206)
> #44: nsBaseAppShell::Run (a:\mozilla\src\widget\nsbaseappshell.cpp:158)
> #45: nsAppShell::Run (a:\mozilla\src\widget\windows\nsappshell.cpp:264)
> #46: XRE_RunAppShell (a:\mozilla\src\toolkit\xre\nsembedfunctions.cpp:875)
> #47: mozilla::ipc::MessagePumpForChildProcess::Run (a:\mozilla\src\ipc\glue\messagepump.cpp:269)
> #48: MessageLoop::RunInternal (a:\mozilla\src\ipc\chromium\src\base\message_loop.cc:232)
> #49: MessageLoop::RunHandler (a:\mozilla\src\ipc\chromium\src\base\message_loop.cc:226)
> #50: MessageLoop::Run (a:\mozilla\src\ipc\chromium\src\base\message_loop.cc:206)
> #51: XRE_InitChildProcess (a:\mozilla\src\toolkit\xre\nsembedfunctions.cpp:709)
> #52: content_process_main (a:\mozilla\src\ipc\contentproc\plugin-container.cpp:197)
> #53: NS_internal_main (a:\mozilla\src\browser\app\nsbrowserapp.cpp:391)
> #54: wmain (a:\mozilla\src\toolkit\xre\nswindowswmain.cpp:118)
> #55: __scrt_common_main_seh (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:253)
> #56: BaseThreadInitThunk[C:\WINDOWS\System32\KERNEL32.DLL +0x162c4]
> #57: RtlSubscribeWnfStateChangeNotification[C:\WINDOWS\SYSTEM32\ntdll.dll +0x60609]
> #58: RtlSubscribeWnfStateChangeNotification[C:\WINDOWS\SYSTEM32\ntdll.dll +0x605d4]

At removing "c", TIP generates following events:

1. eCompositionEnd for committing the "c".
2. eCompositionStart with the range which includes only "c"
3. eCompositionChange with empty string and 2 ranges.
4. eCompositionCommit

So, #2 - #4 tries to remove the "c" with composition committing with empty string.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b8cba80ce75

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce3420f64ab5

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1305355 part.1 TextComposition::GetSelectionStartOffset() should query normal selection range when composition string is empty

TextComposition::GetSelectionStartOffset() tries to query the start offset of the composition if there is a composition. Therefore, it tries to query first clause when it has some clause information of the composition string.  However, even if the composition string is empty, TextComposition::mRanges which refers CompositionEvent::mRanges of the last eCompositionChange event may have empty clause information.  In such case, this method tries to query not existing IME selection range.  Then, this returns UINT32_MAX and ContentCacheInChild will query character rects around the offset and ContentEventHandler::SetRangeFromFlatTextOffset() will meet odd case.

For avoiding this issue, TextComposition::GetSelectionStartOffset() should query normal selection when composition string in the editor is empty.  Note that TextComposition::mString will be updated after the editor completes updating its DOM contents.  Therefore, TextComposition should refer mLastData which is set before dispatching eCompositionChange in the tree.

Review commit: https://reviewboard.mozilla.org/r/81280/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/81280/

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1305355 part.2 TSFTextStore shouldn't append any ranges when composition string is empty

TSFTextStore shouldn't append any ranges to dispatch empty composition event since empty clause information may make TextComposition confused.  Additionally, it doesn't need to append caret range because CompositionTransaction always sets caret at start of composition when the composition string is empty.

Review commit: https://reviewboard.mozilla.org/r/81282/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/81282/

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1305355 part.3 IMEInputHandler shouldn't append any ranges when composition string is empty

IMEInputHandler shouldn't append any ranges to dispatch empty composition event since empty clause information may make TextComposition confused.  Additionally, it doesn't need to append caret range because CompositionTransaction always sets caret at start of composition when the composition string is empty.

Review commit: https://reviewboard.mozilla.org/r/81284/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/81284/

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

FYI: It hits the new NS_WARNING_ASSERTION() of part.1 even with following patches. However, it's not actual problem for now because it must hit after dispatching eCompositionUpdate but we don't do nothing after dispatching following eCompositionChange event. I read the code of TextComposition again, then, I think that we need to sort out the members like mString/mRanges vs. mLastData/mLastRanges. So, I should work on it in another bug for keeping the change purpose clear.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8795156 [details]
Bug 1305355 part.1 TextComposition::GetSelectionStartOffset() should query normal selection range when composition string is empty

https://reviewboard.mozilla.org/r/81280/#review79994

::: dom/events/TextComposition.cpp:445
(Diff revision 1)
>    nsCOMPtr<nsIWidget> widget = mPresContext->GetRootWidget();
>    WidgetQueryContentEvent selectedTextEvent(true, eQuerySelectedText, widget);
> -  if (mRanges && mRanges->HasClauses()) {
> +  // Due to a bug of widget, mRanges may not be nullptr even though composition
> +  // string is empty.  So, we need to check it here for avoiding to return
> +  // odd start offset.
> +  if (!mLastData.IsEmpty() && mRanges && mRanges->HasClauses()) {

What would it take to change mRanges handling so that mRanges is set to null whenever mLastData is empty?

Comment 8

[Makoto Kato [:m_kato]]

Comment on attachment 8795158 [details]
Bug 1305355 part.3 IMEInputHandler shouldn't append any ranges when composition string is empty

https://reviewboard.mozilla.org/r/81284/#review80150

Comment 9

[Makoto Kato [:m_kato]]

Comment on attachment 8795157 [details]
Bug 1305355 part.2 TSFTextStore shouldn't append any ranges when composition string is empty

https://reviewboard.mozilla.org/r/81282/#review80152

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8795156 [details]
Bug 1305355 part.1 TextComposition::GetSelectionStartOffset() should query normal selection range when composition string is empty

https://reviewboard.mozilla.org/r/81280/#review79994

> What would it take to change mRanges handling so that mRanges is set to null whenever mLastData is empty?

mLastData is not empty means either:
* there is a non-empty composition string with clause information
* there is commit string without clause information (or only with caret information, but it won't be set with following patches)

So, in other words, there is composition string only when mLastData is not empty and there are some clause information.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f4cedc85d48d
part.1 TextComposition::GetSelectionStartOffset() should query normal selection range when composition string is empty r=smaug
https://hg.mozilla.org/integration/autoland/rev/fa9da7230277
part.2 TSFTextStore shouldn't append any ranges when composition string is empty r=m_kato
https://hg.mozilla.org/integration/autoland/rev/fb4fd0ff5215
part.3 IMEInputHandler shouldn't append any ranges when composition string is empty r=m_kato

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f4cedc85d48d
https://hg.mozilla.org/mozilla-central/rev/fa9da7230277
https://hg.mozilla.org/mozilla-central/rev/fb4fd0ff5215