Closed Bug 1513145 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Widget: Win32, defect)

Product:
Core
Component:
Widget: Win32
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla66
Tracking Flags:
Tracking Status
firefox-esr60 65+ fixed
firefox65 + fixed
firefox66 + fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: assertion, crash, inputmethod)

Attachments

(1 file)

Bug 1513145 - Make some callers of TSFTextStore::Selection::GetWritingMode() check whether the selection has already been initialized
47 bytes, text/x-phabricator-request
RyanVM
: approval-mozilla-release+
RyanVM
: approval-mozilla-esr60+
Details | Review 
[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.
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.
Pushed by masayuki@d-toybox.com:
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
https://hg.mozilla.org/mozilla-central/rev/62a30f449215
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox66: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66

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

status-firefox65: affected → fixed

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.

Attachment #9030415 - Flags: approval-mozilla-release?
Attachment #9030415 - Flags: approval-mozilla-esr60?

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.

Attachment #9030415 - Flags: approval-mozilla-release?
Attachment #9030415 - Flags: approval-mozilla-release+
Attachment #9030415 - Flags: approval-mozilla-esr60?
Attachment #9030415 - Flags: approval-mozilla-esr60+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: