1312302 - [e10s] Crash in textinputframework.dll@0x337c6

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect, P2)

Description

[Mats Palmgren (inactive)]

This bug was filed from the Socorro interface and is 
report bp-df77ad87-462c-4ec4-978a-b93f82161023.
=============================================================

The trend for this signature is a bit worrying:

Product 	Version Count 	Percentage 	Installations
Firefox 	52.0a1 	37 	59.7% 	27
Firefox 	50.0b9 	10 	16.1% 	5
Firefox 	50.0b8 	8 	12.9% 	3
Firefox 	49.0.2 	3 	4.8% 	3
Firefox 	51.0a2 	3 	4.8% 	3
Firefox 	49.0.1 	1 	1.6% 	1
(crashes in the past 7 days)

All crashes are on Windows 10.

Stack:
textinputframework.dll	textinputframework.dll@0x337c6
	@0x7fffffff
textinputframework.dll	textinputframework.dll@0x5fb5f
ntdll.dll	ntdll.dll@0x26867
msctf.dll	msctf.dll@0x390fa
xul.dll	mozilla::widget::TSFTextStore::GetWnd(unsigned long, HWND__**)
msvcrt.dll	msvcrt.dll@0x1bb78
msvcrt.dll	msvcrt.dll@0x1bb78
textinputframework.dll	textinputframework.dll@0x235f5
textinputframework.dll	textinputframework.dll@0x149dd

Comment 1

[Mats Palmgren (inactive)]

Masayuki-san, maybe this is related to IME?

Comment 2

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

Yeah, TSF does something. However, I don't understand what occurs because TSFTextStore::GetWnd() only calls mWidget::GetWindowHandle() only when mWidget is not nullptr, mWidget::GetWindowHandle() just returns nsWindow::mWnd via a call of nsWindowBase::GetNativeData(NS_NATIVE_WINDOW). So, I don't understand what's the msctf.dll@0x390fa means.

Comment 3

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

FYI: TSFTextStore::mWidget is RefPtr<nsWindowBase>.

Comment 4

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

There are 3 comments:

> 4a3406b4-bc65-49d5-9448-9b5ad2161021 	Opening LastPass add-on.
> 1d8b5739-a23f-4d3a-8880-585b42161023 	Opened font selection on Gravit.io - attempt 3.
> 5e6c4e4b-b66f-428d-89f0-60bad2161023 	Clicked 'Font' selection on Gravit.io, browser instantly exited. 

I tried the 3rd comment's STR. However, I cannot reproduce this crash.

Comment 5

[Makoto Kato [:m_kato]]

This crashes on insider preview (10.0.14393), so we might not be able to reproduce on Windows 10 release version.

Comment 6

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

Ah, good point. I have build 14391 but I cannot reproduce this. Do you have Fast ring environment? If not, I'll create the environment in VM.

Comment 7

[Makoto Kato [:m_kato]]

I have one, so I try it.

Comment 8

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

Thanks in advance!

Comment 9

[Makoto Kato [:m_kato]]

I can reproduce with Tablet mode.   TextInputFramework.dll will be for OSK, we cannot reproduce normal IME.

Comment 10

[Makoto Kato [:m_kato]]

On my environment.

0:000> k
Child-SP          RetAddr           Call Site
0000009b`6f9fbcf0 00007ff8`faf98ea8 TextInputFramework!`Microsoft::WRL::Module<1,Microsoft::WRL::Details::DefaultModule<1> >::Create'::`2'::`dynamic atexit destructor for 'moduleSingleton''+0x98e6
0000009b`6f9fbd20 00007ff8`faf9107f TextInputFramework!CGetSelectionAsync::RunContinuation+0x78
0000009b`6f9fbd60 00007ff8`fafa35f6 TextInputFramework!TextInputClient::EditControlRegister+0x46f
0000009b`6f9fc020 00007ff8`faf949de TextInputFramework!std::_Func_impl<std::_Callable_obj<<lambda_fd5862b078e28acefb4844d5d2fbcbbb>,0>,std::allocator<std::_Func_class<long,std::_Nil,std::_Nil,std::_Nil,std::_Nil,std::_Nil,std::_Nil,std::_Nil> >,long,std::_Nil,std::_Nil,std::_Nil,std::_Nil,std::_Nil,std::_Nil,std::_Nil>::_Do_call+0x46
*** ERROR: Symbol file could not be found.  Defaulted to export symbols for C:\WINDOWS\System32\MSCTF.dll -
0000009b`6f9fc050 00007ff9`05d885a3 TextInputFramework!CTextInputClientFreeThread::EditControlRegister+0x88e
0000009b`6f9fc270 00007ff9`05d63768 MSCTF!TF_SetDefaultRemoteKeyboardLayout+0x4d23
0000009b`6f9fc2a0 00007ff9`05d864a6 MSCTF!CtfImeProcessKey+0xaa88
0000009b`6f9fc2d0 00007ff9`05d4c9d5 MSCTF!TF_SetDefaultRemoteKeyboardLayout+0x2c26
0000009b`6f9fc330 00007ff9`05d487ae MSCTF!TF_GetThreadMgr+0x7dd5
0000009b`6f9fc450 00007ff8`d31587c3 MSCTF!TF_GetThreadMgr+0x3bae
0000009b`6f9fc480 00007ff8`d3160d8d xul!mozilla::widget::TSFTextStore::CreateAndSetFocus+0x197
0000009b`6f9fc4e0 00007ff8`d1760487 xul!mozilla::widget::TSFTextStore::OnFocusChange+0x19d
0000009b`6f9fc5f0 00007ff8`d17600fb xul!mozilla::widget::TSFTextStore::SetInputContext+0x183
0000009b`6f9fc650 00007ff8`d175e8e2 xul!mozilla::widget::IMEHandler::SetInputContext+0xab
0000009b`6f9fc710 00007ff8`d175e88f xul!nsWindow::SetInputContext+0x2a
0000009b`6f9fc780 00007ff8`d2d94369 xul!mozilla::IMEStateManager::SetInputContext+0x1e7
0000009b`6f9fc950 00007ff8`d3068bb6 xul!mozilla::IMEStateManager::SetInputContextForChildProcess+0x245
0000009b`6f9fcb60 00007ff8`d255c1b7 xul!mozilla::dom::TabParent::RecvSetInputContext+0xae
0000009b`6f9fcbd0 00007ff8`d25a3ffc xul!mozilla::dom::PBrowserParent::OnMessageReceived+0x1db7
0000009b`6f9fd980 00007ff8`d19ec19f xul!mozilla::dom::PContentParent::OnMessageReceived+0x6c
0000009b`6f9feab0 00007ff8`d19ebeb5 xul!mozilla::ipc::MessageChannel::DispatchAsyncMessage+0x6f
0000009b`6f9feaf0 00007ff8`d19ea938 xul!mozilla::ipc::MessageChannel::DispatchMessageW+0x215
0000009b`6f9febb0 00007ff8`d19ea8bf xul!mozilla::ipc::MessageChannel::OnMaybeDequeueOne+0x70
(Inline Function) --------`-------- xul!mozilla::detail::RunnableMethodArguments<>::applyImpl+0x3
(Inline Function) --------`-------- xul!mozilla::detail::RunnableMethodArguments<>::apply+0x3
0000009b`6f9fec70 00007ff8`d19ea867 xul!mozilla::detail::RunnableMethodImpl<bool (__cdecl mozilla::ipc::MessageChannel::*)(void) __ptr64,0,1>::Run+0x13
(Inline Function) --------`-------- xul!mozilla::ipc::MessageChannel::RefCountedTask::Run+0xa
0000009b`6f9feca0 00007ff8`d17a7e53 xul!mozilla::ipc::MessageChannel::DequeueTask::Run+0x17
0000009b`6f9fecd0 00007ff8`d19c4f2e xul!nsThread::ProcessNextEvent+0x477
0000009b`6f9fedc0 00007ff8`d19eea66 xul!NS_ProcessNextEvent+0x22
0000009b`6f9fedf0 00007ff8`d19c4ceb xul!mozilla::ipc::MessagePump::Run+0x102
0000009b`6f9fee70 00007ff8`d19c4cae xul!MessageLoop::RunHandler+0x1b
0000009b`6f9feea0 00007ff8`d19ffc54 xul!MessageLoop::Run+0x3e
0000009b`6f9feef0 00007ff8`d19ff91c xul!nsBaseAppShell::Run+0x3c
0000009b`6f9fef20 00007ff8`d19ff8c7 xul!nsAppShell::Run+0x2c
0000009b`6f9fef50 00007ff8`d1cc285f xul!nsAppStartup::Run+0x27
0000009b`6f9fef80 00007ff8`d1cc40fb xul!XREMain::XRE_mainRun+0x62b
0000009b`6f9ff230 00007ff8`d1e8acf1 xul!XREMain::XRE_main+0x1eb
0000009b`6f9ff2b0 00007ff7`61c8198a xul!XRE_main+0x55
0000009b`6f9ff4b0 00007ff7`61c83579 firefox!do_main+0x38a
(Inline Function) --------`-------- firefox!NS_internal_main+0x2a3
0000009b`6f9ff7f0 00007ff7`61c86615 firefox!wmain+0x409
*** ERROR: Symbol file could not be found.  Defaulted to export symbols for C:\WINDOWS\System32\KERNEL32.DLL -
(Inline Function) --------`-------- firefox!invoke_main+0x22
0000009b`6f9ffbf0 00007ff9`05464224 firefox!__scrt_common_main_seh+0x11d
0000009b`6f9ffc30 00007ff9`06784501 KERNEL32!BaseThreadInitThunk+0x14
0000009b`6f9ffc60 00000000`00000000 ntdll!RtlUserThreadStart+0x21

Comment 11

[Makoto Kato [:m_kato]]

It can reproduce on stable and insider preview.

Comment 12

[Jacob W. Klein]

I've been bit by this bug, too, and I have some more info, with steps to try to reproduce it. Here goes.

First of all, this problem only started happening, for me, after I installed Windows 10 Insider Preview Build 14951, on 10/19/2016. Since then, my Firefox crashes seemingly randomly, but more often when clicking into a textbox. I don't believe this problem was happening on any prior Windows 10 Insider build. I haven't tested my Windows 10 Release partition yet, for this bug.

Secondly, for me, I use 32-bit Firefox on Windows 10 x64, and this crash is happening. So, it's not just a 64-bit-browser thing - happening on 32-bit for me.

When I used about:crashes and looked at my reports, for me, the signature's address is not textinputframework.dll@0x337c6 -- instead, mine is: textinputframework.dll@0x337d6 (Notice "d" instead of "c").

Now, some steps to reproduce:
- Browse for a while, in various tabs. Might need to hit a site that has flash video, not sure.
- Now go to: www.neowin.net
- Let it load
- Now hit the Search icon (magnifying glass) on that site
- What's supposed to happen is that a searchbox slides into view
- What happens about 70% of the time, for me, is that Firefox immediately crashes, with this bug's signature.
- If it didn't crash right away, try browsing more, before hitting that search icon.

That's all the info I know about this. It's very frustrating. I'd LOVE to get feedback in this bugzilla bug, to at least tell me (just a user) whether this is something Mozilla will fix, or whether this is something Microsoft needs to fix. I'm going to submit the bug to their Feedback app as well.

Looking forward to someone resolving this one!
- Jacob Klein

Comment 13

[Makoto Kato [:m_kato]]

sThreadMgr->SetFocus(textStore->mDocumentMgr) cause this crash...

Comment 14

[Jacob W. Klein]

If anyone has a workaround to prevent the crashing ... I'd be VERY grateful to know it.

Comment 15

[Makoto Kato [:m_kato]]

And this occurs on e10s only.  I cannot reproduce on non-e10s mode.  humm....

Comment 16

[Jacob W. Klein]

I **think** I'm noticing additional behavior with this bug.

- If you stay within the tab group that Firefox loaded with, then you can click into any textbox on any webpage within that tab group without it crashing.
- If you navigate to a different new tab group, then you can expect Firefox to crash when you click into a (any?) textbox
- After you crash, and restore the tabs, then you can start clicking into textboxes within that new tab group you selected before the crash.

What a painful experience!

Note/disclaimer: I use 3 addons for my tabbed browsing experience: "Tab Mix Plus", "Tab Groups", and "TabGroups Menu".

Comment 17

[Makoto Kato [:m_kato]]

(In reply to Jacob W. Klein from comment #14)
> If anyone has a workaround to prevent the crashing ... I'd be VERY grateful
> to know it.

Disable e10s.  I can reproduce this on e10s only.

Comment 18

[Jacob W. Klein]

Can e10s be disabled on Firefox 49.0.2?
If so, how?

Comment 19

[Jacob W. Klein]

THANK YOU FOR THIS WORKAROUND.

about:support showed that Multiprocess Windows was set to 1 (Enabled by user). I clicked the Add-ons button, to access the "Add-ons Manager". On the Extensions tab, it showed that "Multiprocess is enabled", with a link to "More information". I clicked that, which took me to:
https://wiki.mozilla.org/Electrolysis
Then I basically went into about:config, and filtered for browser.tabs.remote ... and then right-clicked each bold entry and clicked Reset.
Then I restarted firefox, and now about:support shows 0 (Disabled by add-ons). And now Firefox isn't crashing.

Sorry for cluttering up the bug, just trying to help normal users find a way around this frustrating experience. When a fix lands for this bug, I'll be sure to turn e10s back on and re-test.

Thanks,
Jacob

Comment 20

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

(In reply to Makoto Kato [:m_kato] from comment #15)
> And this occurs on e10s only.  I cannot reproduce on non-e10s mode.  humm....

It's really mysterious... Even in e10s mode, focus change for setting input context is managed by IMEStateManager in the main process. So, TSFTextStore cannot distinguish if a remote process has focus without the flag in InputContext. I mean I have no idea what's different for TSFTextStore at focus change.

Comment 21

[Makoto Kato [:m_kato]]

When TSFTextStore::GetSelection returns E_FAIL, this crash occurs. You know, it might return error with e10s... :-<

Although I believe that this issue is Microsoft's bug, we need workaround code such as we set dummy range.

Comment 22

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

Wow, that sounds bad... Anyway, we should report it to Microsoft from Feedback Hub.

Does it depends on TIP? If so, I'd be happy if we could restrict to run the hack only with buggy TIPs.

Otherwise, we should be careful to notify TSF of selection change after we return dummy range.

Comment 23

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #22)
> Does it depends on TIP? If so, I'd be happy if we could restrict to run the
> hack only with buggy TIPs.

No, when using English TIP or ATOK, this occurs unfortunately.
 
> Otherwise, we should be careful to notify TSF of selection change after we
> return dummy range.

As long as I know, it seems to be SetFocus() only.  But I need more investigation for it.

Comment 24

[Jacob W. Klein]

For anyone that wants to Upvote the Windows Feedback item I sent to Microsoft, or add info to it, it is located here: (copy/paste into any browser or File Explorer):
feedback-hub:?contextid=156&feedbackid=10414c2b-0e5e-48ec-970e-f0014346ab0f&form=1&src=2

Regards,
Jacob Klein

Comment 25

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

(In reply to Jacob W. Klein from comment #24)
> For anyone that wants to Upvote the Windows Feedback item I sent to
> Microsoft, or add info to it, it is located here: (copy/paste into any
> browser or File Explorer):
> feedback-hub:?contextid=156&feedbackid=10414c2b-0e5e-48ec-970e-
> f0014346ab0f&form=1&src=2
> 
> Regards,
> Jacob Klein

Thanks. I added a comment to the feedback and voted it.

Comment 26

[Makoto Kato [:m_kato]]

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

Comment 27

[Jacob W. Klein]

Not fixed in Windows 10 Insider Build 14955, released today.

Comment 28

[Makoto Kato [:m_kato]]

Attachment: Bug 1312302 - Set dummy Selection during initializing TextStore.

When PC supports table mode, TextInputFramme.dll is loaded and it can be used for TIP.

When creating new TextStore object, selection might be nothing yet on e10s.  At this time, GetSelection will return E_FAIL since selection data isn't received yet.  If GetSelection returns error during SetFocus, TextInputFrame.dll will crash.

So we set temporary selection to avoid crash.

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

Comment 29

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

Comment on attachment 8804535 [details]
Bug 1312302 - Set dummy Selection during initializing TextStore.

https://reviewboard.mozilla.org/r/88468/#review87546

> When PC supports table mode, TextInputFramme.dll is loaded and it can be used for TIP.

"tablet mode" and "TextInptFramework.dll"?

::: widget/windows/TSFTextStore.h:597
(Diff revision 1)
>     * actually using it.
>     * Note that this is also called by ContentForTSFRef().
>     */
>    Selection& SelectionForTSFRef();
>  
> +  class MOZ_STACK_CLASS AutoSetTemporarySelection final

I like "AutoTemporarySelectionSetter" better.

::: widget/windows/TSFTextStore.h:600
(Diff revision 1)
>    Selection& SelectionForTSFRef();
>  
> +  class MOZ_STACK_CLASS AutoSetTemporarySelection final
> +  {
> +  public:
> +    AutoSetTemporarySelection(Selection& aSelection) : mSelection(aSelection)

Please put ": mSelection(aSelection)" to the next line with indent level++.

And don't we need |explicit|?

::: widget/windows/TSFTextStore.cpp:4804
(Diff revision 1)
> +    // Windows 10's softwware keyboard requires that SetSelection must be
> +    // always successful into SetFocus.  If returning error, it might crash
> +    // into TextInputFramework.dll.
> +    AutoSetTemporarySelection setSelection(textStore->SelectionForTSFRef());
> +
> +    hr = sThreadMgr->SetFocus(textStore->mDocumentMgr);

Okay, did you check if TSFTextStore notifies selection change when it got valid selection and the range is not [0-0]?

If it notifies TSF of the change, r=masayuki.  Otherwise, please request review to me again with new patch.

Comment 30

[Makoto Kato [:m_kato]]

Humm, selection has another bug that select() method of <input type="text"> doesn't send selection to parent process notify correctly....

Comment 31

[Makoto Kato [:m_kato]]

Attachment: test case

Comment 32

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

(In reply to Makoto Kato [:m_kato] from comment #30)
> Humm, selection has another bug that select() method of <input type="text">
> doesn't send selection to parent process notify correctly....

Should be fixed in another bug... (I should create automated tests of IMEContentObserver..., bug 1217700)

Comment 33

[Jim Mathies [:jimm]]

[Tracking Requested - why for this release]:
recent spike in crashes in 52.

Comment 34

[Marcia Knous [:marcia]]

Tracking 52+ for the reasons in Comment 33.

Comment 35

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #29)
> Okay, did you check if TSFTextStore notifies selection change when it got
> valid selection and the range is not [0-0]?

Except to test case (and this crash site), we can get selection and selection will updated from content process via IME Notify.

Comment 36

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

(In reply to Makoto Kato [:m_kato] from comment #35)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #29)
> > Okay, did you check if TSFTextStore notifies selection change when it got
> > valid selection and the range is not [0-0]?
> 
> Except to test case (and this crash site), we can get selection and
> selection will updated from content process via IME Notify.

Thanks, then, the patch is fine.

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/70c4b5cde515
Set dummy Selection during initializing TextStore. r=masayuki

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/70c4b5cde515

Comment 39

[[:philipp]]

would this patch be upliftable to beta/aurora in terms of scope and risk too?

Comment 40

[Makoto Kato [:m_kato]]

Comment on attachment 8804535 [details]
Bug 1312302 - Set dummy Selection during initializing TextStore.

Approval Request Comment
[Feature/regressing bug #]:
No
[User impact if declined]:
Firefox crashes on gravit.io on Windows 10 that PC supports tablet mode.
Also, this issue is on e10s only and Microsoft's bug.

[Describe test coverage new/current, TreeHerder]:
Landed in m-c

[Risks and why]: 
Low.  This depends on attached test case.  When error situation, we set dummy data.

[String/UUID change made/needed]:
None

Comment 41

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Jim, I'd like a second opinion on whether we should uplift this to 50 or not. The data from Nightly seems to indicate the fix worked on 52. We entered RC mode today and I'd like to uplift this one if the risk is indeed very low. Thanks!

Comment 42

[Jim Mathies [:jimm]]

(In reply to Ritu Kothari (:ritu) from comment #41)
> Hi Jim, I'd like a second opinion on whether we should uplift this to 50 or
> not. The data from Nightly seems to indicate the fix worked on 52. We
> entered RC mode today and I'd like to uplift this one if the risk is indeed
> very low. Thanks!

Looking over the patch, doesn't appear to involve risky or significant change. Makoto says the risk is low and he knows this are of our code base. LGTM.

Comment 43

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8804535 [details]
Bug 1312302 - Set dummy Selection during initializing TextStore.

Crash fix, taking it, Aurora51+, Beta50+

Comment 44

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b44e1445660d

Comment 45

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/9a31f71f8be676c184954ee9872473cbb75908a0
https://hg.mozilla.org/releases/mozilla-release/rev/9a31f71f8be676c184954ee9872473cbb75908a0

Comment 46

[Jacob W. Klein]

I confirm the fix, using Firefox 50, with e10s enabled, on Windows 10 Insider Preview 14971. Thank you for fixing this promptly!