1513145 - [TSF] Hits MOZ_ASSERT if TSFTextStore cannot initialize selection while enabling a TSFTextStore instance

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

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

[Parent 13572, Main Thread] WARNING: '!IsSelectionValid()', file m:/src/widget/ContentCache.cpp, line 658
[Parent 13572, Main Thread] WARNING: '!aEvent.mSucceeded', file m:/src/dom/ipc/TabParent.cpp, line 2105
[Parent 13572, Main Thread] WARNING: '!querySelection.mSucceeded', file m:/src/widget/windows/TSFTextStore.cpp, line 2977
Assertion failure: !mDirty, at m:/src/widget/windows/TSFTextStore.h:603
#01: CtfNotifyIME[C:\WINDOWS\System32\MSCTF.dll +0x2ca47]
#02: HasDeferredInputForCoreDispatcher[C:\WINDOWS\System32\MSCTF.dll +0x238bb]
#03: CtfNotifyIME[C:\WINDOWS\System32\MSCTF.dll +0x2a423]
#04: CtfNotifyIME[C:\WINDOWS\System32\MSCTF.dll +0x2a63a]
#05: TF_GetAppCompatFlags[C:\WINDOWS\System32\MSCTF.dll +0x19acf]
#06: TF_GetAppCompatFlags[C:\WINDOWS\System32\MSCTF.dll +0x1947c]
#07: TF_InitSystem[C:\WINDOWS\System32\MSCTF.dll +0x3a6ea]
#08: mozilla::widget::TSFTextStore::CreateAndSetFocus (m:\src\widget\windows\TSFTextStore.cpp:5885)
#09: mozilla::widget::TSFTextStore::OnFocusChange (m:\src\widget\windows\TSFTextStore.cpp:5830)
#10: mozilla::widget::TSFTextStore::SetInputContext (m:\src\widget\windows\TSFTextStore.cpp:6728)
#11: mozilla::widget::IMEHandler::SetInputContext (m:\src\widget\windows\WinIMEHandler.cpp:491)
#12: nsWindow::SetInputContext (m:\src\widget\windows\nsWindow.cpp:7074)
#13: mozilla::IMEStateManager::SetInputContext (m:\src\dom\events\IMEStateManager.cpp:1361)
#14: mozilla::IMEStateManager::SetInputContextForChildProcess (m:\src\dom\events\IMEStateManager.cpp:1208)
#15: mozilla::dom::TabParent::RecvSetInputContext (m:\src\dom\ipc\TabParent.cpp:2295)
#16: mozilla::dom::PBrowserParent::OnMessageReceived (m:\fx64-dbg\ipc\ipdl\PBrowserParent.cpp:2994)
#17: mozilla::dom::PContentParent::OnMessageReceived (m:\fx64-dbg\ipc\ipdl\PContentParent.cpp:3612)
#18: mozilla::ipc::MessageChannel::DispatchAsyncMessage (m:\src\ipc\glue\MessageChannel.cpp:2160)
#19: mozilla::ipc::MessageChannel::DispatchMessage (m:\src\ipc\glue\MessageChannel.cpp:2086)
#20: mozilla::ipc::MessageChannel::RunMessage (m:\src\ipc\glue\MessageChannel.cpp:1936)
#21: mozilla::ipc::MessageChannel::MessageTask::Run (m:\src\ipc\glue\MessageChannel.cpp:1968)
#22: nsThread::ProcessNextEvent (m:\src\xpcom\threads\nsThread.cpp:1144)
#23: NS_ProcessNextEvent (m:\src\xpcom\threads\nsThreadUtils.cpp:468)
#24: mozilla::ipc::MessagePump::Run (m:\src\ipc\glue\MessagePump.cpp:88)
#25: MessageLoop::RunHandler (m:\src\ipc\chromium\src\base\message_loop.cc:308)
#26: MessageLoop::Run (m:\src\ipc\chromium\src\base\message_loop.cc:290)
#27: nsBaseAppShell::Run (m:\src\widget\nsBaseAppShell.cpp:139)
#28: nsAppShell::Run (m:\src\widget\windows\nsAppShell.cpp:409)
#29: nsAppStartup::Run (m:\src\toolkit\components\startup\nsAppStartup.cpp:272)
#30: XREMain::XRE_mainRun (m:\src\toolkit\xre\nsAppRunner.cpp:4622)
#31: XREMain::XRE_main (m:\src\toolkit\xre\nsAppRunner.cpp:4760)
#32: XRE_main (m:\src\toolkit\xre\nsAppRunner.cpp:4845)
#33: NS_internal_main (m:\src\browser\app\nsBrowserApp.cpp:293)
#34: wmain (m:\src\toolkit\xre\nsWindowsWMain.cpp:129)
#35: __scrt_common_main_seh (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:283)
#36: BaseThreadInitThunk[C:\WINDOWS\System32\KERNEL32.DLL +0x17e94]
#37: RtlUserThreadStart[C:\WINDOWS\SYSTEM32\ntdll.dll +0x67ad1]

If TSFTextStore fails to get selection, e.g., the content is really odd like fuzzing tests, its mSelectionForTSF is marked as dirty.  However, Windows may try to retrieve writing mode while we're creating new TSFTextStore. Then, we may hit MOZ_ASSERT(!mDirty) in TSFTextStore::Selection::GetWritingMode() in debug build.

So that we need to make some callers of GetWritingMode() check whether selection is marked as dirty.

Comment 1

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

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

Comment 2

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

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

Comment 3

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

Attachment: Bug 1513145 - Make some callers of TSFTextStore::Selection::GetWritingMode() check whether the selection has already been initialized

If TSFTextStore fails to get selection, e.g., the content is really odd like
fuzzing tests, its mSelectionForTSF is marked as dirty.  However, Windows may
try to retrieve writing mode while we're creating new TSFTextStore. Then, we
may hit MOZ_ASSERT(!mDirty) in TSFTextStore::Selection::GetWritingMode() in
debug build.

So, we need to make some callers of GetWritingMode() check whether selection
is marked as dirty.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/62a30f449215
Make some callers of TSFTextStore::Selection::GetWritingMode() check whether the selection has already been initialized r=m_kato

Comment 5

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/62a30f449215

Comment 6

[Ryan VanderMeulen [:RyanVM]]

This is hitting on mozilla-release and ESR60 since the Win10 workers were updated from 1703 to 1803.
https://treeherder.mozilla.org/logviewer.html#?job_id=227619187&repo=mozilla-release
https://treeherder.mozilla.org/logviewer.html#?job_id=227619156&repo=mozilla-release

etc...

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Because it's after midnight in Japan and this blocks gtb for today's dot releases, I've pushed this to m-r. I checked with smaug and we felt that this was likely safe enough to take, but will also follow-up to ensure that's the case.

https://hg.mozilla.org/releases/mozilla-release/rev/55b9be64fd88

Comment 8

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/f55f68d6f5676a8c73d84105af6a8fb92763898e

Comment 9

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

Comment on attachment 9030415 [details]
Bug 1513145 - Make some callers of TSFTextStore::Selection::GetWritingMode() check whether the selection has already been initialized

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

None

User impact if declined

Cannot run debug builds on Windows 10 build 1803. I.e., blocking to upgrade the workers.

Is this code covered by automated tests?

No

Has the fix been verified in Nightly?

Yes

Needs manual test from QE?

No

If yes, steps to reproduce

List of other uplifts needed

None

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

This makes IME handler (TSFTextStore) check whether widget in chrome process receives layout information as expected when OS requests to retrieve it. In unusual cases like fuzzing tests, this could occur. In other words, the new patch won't be used in usual cases so that this change shouldn't affect to actual users.

String changes made/needed

None.

ESR Uplift Approval Request

If this is not a sec:{high,crit} bug, please state case for ESR consideration

Cannot run debug builds on Windows 10 build 1803. I.e., blocking to upgrade the workers.

User impact if declined

Fix Landed on Version

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

This makes IME handler (TSFTextStore) check whether widget in chrome process receives layout information as expected when OS requests to retrieve it. In unusual cases like fuzzing tests, this could occur. In other words, the new patch won't be used in usual cases so that this change shouldn't affect to actual users.

String or UUID changes made by this patch

None.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9030415 [details]
Bug 1513145 - Make some callers of TSFTextStore::Selection::GetWritingMode() check whether the selection has already been initialized

[Triage Comment]
Approving this retroactively. Thanks for the risk analysis.