Closed Bug 1368554 Opened 7 years ago Closed 7 years ago

Crash in mozilla::ContentCacheInParent::OnCompositionEvent

Categories

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

Product:
Core
Component:
Widget: Win32
Version:
52 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox-esr52 --- wontfix
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: philipp, Assigned: masayuki)

References

Details

(Keywords: crash, inputmethod, regression, Whiteboard: tpi:+)

Crash Data

Attachments

(2 files)

Bug 1368554 ContentCacheInParent::mPendingCompositionCount should be decreased when TextCompositin which has dispatched composition events to corresponding remote process
59 bytes, text/x-review-board-request
m_kato
: review+
Details
Patch for ESR52
12.70 KB, patch
Details | Diff | Splinter Review 
This bug was filed from the Socorro interface and is 
report bp-da39123c-cfce-48b9-a95c-b79650170521.
=============================================================


Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::ContentCacheInParent::OnCompositionEvent(mozilla::WidgetCompositionEvent const&) 	widget/ContentCache.cpp:1106
1 	xul.dll 	mozilla::dom::TabParent::SendCompositionEvent(mozilla::WidgetCompositionEvent&) 	dom/ipc/TabParent.cpp:2051
2 	xul.dll 	mozilla::TextComposition::DispatchCompositionEvent(mozilla::WidgetCompositionEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, bool) 	dom/events/TextComposition.cpp:252
3 	xul.dll 	mozilla::IMEStateManager::DispatchCompositionEvent(nsINode*, nsPresContext*, mozilla::WidgetCompositionEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, bool) 	dom/events/IMEStateManager.cpp:1171
4 	xul.dll 	mozilla::PresShell::DispatchEventToDOM(mozilla::WidgetEvent*, nsEventStatus*, nsPresShellEventCB*) 	layout/base/PresShell.cpp:8179
5 	xul.dll 	mozilla::PresShell::HandleEventInternal(mozilla::WidgetEvent*, nsEventStatus*, bool) 	layout/base/PresShell.cpp:8056
6 	xul.dll 	mozilla::PresShell::HandleEvent(nsIFrame*, mozilla::WidgetGUIEvent*, bool, nsEventStatus*, nsIContent**) 	layout/base/PresShell.cpp:7758
7 	xul.dll 	nsViewManager::DispatchEvent(mozilla::WidgetGUIEvent*, nsView*, nsEventStatus*) 	view/nsViewManager.cpp:814
8 	xul.dll 	nsView::HandleEvent(mozilla::WidgetGUIEvent*, bool) 	view/nsView.cpp:1125
9 	xul.dll 	nsWindow::DispatchEvent(mozilla::WidgetGUIEvent*, nsEventStatus&) 	widget/windows/nsWindow.cpp:4102
10 	xul.dll 	mozilla::widget::TextEventDispatcher::DispatchEvent(nsIWidget*, mozilla::WidgetGUIEvent&, nsEventStatus&) 	widget/TextEventDispatcher.cpp:186
11 	xul.dll 	mozilla::widget::TextEventDispatcher::StartComposition(nsEventStatus&, mozilla::WidgetEventTime const*) 	widget/TextEventDispatcher.cpp:237
12 	xul.dll 	mozilla::widget::TSFTextStore::FlushPendingActions() 	widget/windows/TSFTextStore.cpp:1775
13 	xul.dll 	mozilla::widget::TSFTextStore::DidLockGranted() 	widget/windows/TSFTextStore.cpp:1685
14 	xul.dll 	mozilla::widget::TSFTextStore::RequestLock(unsigned long, long*) 	widget/windows/TSFTextStore.cpp:1639
15 	msctf.dll 	CInputContext::_QueueItem(_TS_QUEUE_ITEM*, int, long*) 	
16 	msctf.dll 	CInputContext::RequestEditSession(unsigned long, ITfEditSession*, unsigned long, long*) 	
17 	imetip.dll 	CTipFnEditSessionHandler::Invoke(__MIDL___MIDL_itf_tipfunc_0000_0009_0001, __MIDL___MIDL_itf_tipfunc_0000_0009_0002, ITipEditSessionProcedure*, IUnknown*, long*) 	
18 	imetip.dll 	CTipFnCompose::OnApplyComposition(CTipComposition*, int) 	
19 	imetip.dll 	CTipComposition::ApplyToContext(int) 	
20 	imetip.dll 	CTipFnProductStringHandler::UpdateComposition(int) 	
21 	imetip.dll 	CTipFnProductStringHandler::_OnProductObjectChanged(unsigned long, void*) 	
22 	imetip.dll 	Imeapiutil::CImeReconversionEndNotify::OnReconversionEnded(__MIDL___MIDL_itf_imeapi_0000_0043_0001) 	
23 	imjkapi.dll 	CImeProductObject_JK::EndUpdateProductObject(int) 	
24 	imjpapi.dll 	CImeIPointCallBack::GenerateMessage() 	
25 	imjpapi.dll 	CIImeIPoint::UpdateContext(int) 	
26 	imjpapi.dll 	CCmdUpdateContext::ExecuteWithParam(IImeEMManager*, __int64) 	
27 	imjpapi.dll 	CImeEMManager::Execute(_GUID const*) 	
28 	imjpapi.dll 	CImeCommonAPI_JPN_Desktop_V1::EndBulkChange(int) 	
29 	imjpapi.dll 	CImeKeyEventHandler_JPN_Desktop_V1::ProcessKey(__MIDL___MIDL_itf_imeapi_0000_0001_0001 const*, unsigned short) 	
30 	imjpapi.dll 	CImeKeyEventHandler_JPN_Desktop_V1::OnKeyDown(__MIDL___MIDL_itf_imeapi_0000_0001_0001 const*, int*) 	
31 	imetip.dll 	CTipFnKeyEventHandler::OnKeyDown(unsigned __int64, __int64, int*) 	
32 	imetip.dll 	CTipContextEditorMgr::OnKeyboardEvent(unsigned int, ITfContext*, unsigned __int64, __int64, int*) 	
33 	imetip.dll 	CTipContextEditorMgr::_OnKeyboardEvent(unsigned int, ITfContext*, unsigned __int64, __int64, int*, void*) 	
34 	imetip.dll 	Tsfutil::CTfKeyEventSink::OnKeyDown(ITfContext*, unsigned __int64, __int64, int*) 	
35 	msctf.dll 	CThreadInputMgr::_CallKeyEventSink(unsigned long, CInputContext*, KSEnum, unsigned __int64, __int64, int*) 	
36 	msctf.dll 	CheckKnownModuleOnWinlogonDesktop() 	
37 	msctf.dll 	CThreadInputMgr::KeyDown(unsigned __int64, __int64, int*) 	
38 	xul.dll 	mozilla::widget::TSFTextStore::ProcessRawKeyMessage(tagMSG const&) 	widget/windows/TSFTextStore.cpp:5878
39 	xul.dll 	nsAppShell::ProcessNextNativeEvent(bool) 	widget/windows/nsAppShell.cpp:369
40 	xul.dll 	nsBaseAppShell::DoProcessNextNativeEvent(bool) 	widget/nsBaseAppShell.cpp:138
41 	xul.dll 	nsBaseAppShell::OnProcessNextEvent(nsIThreadInternal*, bool) 	widget/nsBaseAppShell.cpp:289
42 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp:1213
43 	xul.dll 	NS_ProcessNextEvent(nsIThread*, bool) 	xpcom/glue/nsThreadUtils.cpp:390
44 	xul.dll 	mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:124
45 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc:231
46 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc:211
47 	xul.dll 	nsBaseAppShell::Run() 	widget/nsBaseAppShell.cpp:156
48 	xul.dll 	nsAppShell::Run() 	widget/windows/nsAppShell.cpp:262
49 	xul.dll 	nsAppStartup::Run() 	toolkit/components/startup/nsAppStartup.cpp:283
50 	xul.dll 	XREMain::XRE_mainRun() 	toolkit/xre/nsAppRunner.cpp:4477
51 	xul.dll 	XREMain::XRE_main(int, char** const, mozilla::BootstrapConfig const&) 	toolkit/xre/nsAppRunner.cpp:4654
52 	xul.dll 	XRE_main(int, char** const, mozilla::BootstrapConfig const&) 	toolkit/xre/nsAppRunner.cpp:4745
53 	firefox.exe 	NS_internal_main(int, char**, char**) 	browser/app/nsBrowserApp.cpp:305
54 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp:115
55 	firefox.exe 	__scrt_common_main_seh 	f:/dd/vctools/crt/vcstartup/src/startup/exe_common.inl:253
Ø 56 	kernel32.dll 	kernel32.dll@0x159cc 	
Ø 57 	ntdll.dll 	ntdll.dll@0x2a560 	
Ø 58 	kernel32.dll 	kernel32.dll@0x9baaf 	
Ø 59 	kernel32.dll 	kernel32.dll@0x9baaf

this cross-platform crash is appearing in firefox 52 and subsequent versions (next to en-us mostly in ja, zh & ko locales) with MOZ_RELEASE_ASSERT(mPendingCompositionCount < 0xffui8) that got added in bug 1304620.
not bad crash volume, something todo with text composition. Masayuki, care to take a look?
Flags: needinfo?(masayuki)
Priority: -- → P2
Whiteboard: tpi:+
Oh, mPendingCompositionCount reaches 255... it means that the main process dispatched 255 composition events were sent to a remote process but they are not set back yet. If it's true, the child process is really stopped.

However, I bet that this is caused by other bug. Perhaps, mPendingCompositionCount isn't reset in some cases or decreased from 0?
Flags: needinfo?(masayuki)
Keywords: inputmethod
Ah, I see that when composition is committed forcibly in a remote process, it sends a request to commit to the parent process. However, the parent process won't send composition events to the remote process. Therefore, mContentCacheInParent in TabParent doesn't have a chance to reset mPendingCompositionCount.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Comment on attachment 8876327 [details]
Bug 1368554 ContentCacheInParent::mPendingCompositionCount should be decreased when TextCompositin which has dispatched composition events to corresponding remote process

https://reviewboard.mozilla.org/r/147660/#review152222
Attachment #8876327 - Flags: review?(m_kato) → review+
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/401127e8ba3b
ContentCacheInParent::mPendingCompositionCount should be decreased when TextCompositin which has dispatched composition events to corresponding remote process r=m_kato
https://hg.mozilla.org/mozilla-central/rev/401127e8ba3b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Is this worth nominating for ESR52 uplift or should we let it ride the trains?
status-firefox-esr52: --- → affected
status-thunderbird_esr52: affected → ---
Flags: needinfo?(masayuki)
I think that the crash volume isn't so high but this might be reproduced a lot for some specific users. If you keep open a tab which supports autocomplete during composition during a browser session and you often use such input field with IME, you will meat this crash after autocomplete commits composition forcibly 255 times.

I don't think we need to fix this bug urgently due to the rate, but I think that we should take this to 52ESR:
https://crash-stats.mozilla.com/signature/?product=Firefox&version=52.0esr&version=52.0.1esr&version=52.0.2esr&version=52.1.0esr&version=52.1.1esr&version=52.1.2esr&version=52.2.0esr&signature=mozilla%3A%3AContentCacheInParent%3A%3AOnCompositionEvent&date=%3E%3D2016-12-14T05%3A13%3A16.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-date&page=1#reports

I'll check if they can be uplifted cleanly.
Flags: needinfo?(masayuki)
Attached patch Patch for ESR52 — — Splinter Review 
# Unfortunately, the patch conflicts a line in logging code. So, actual code isn't touched in this patch.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:

Fixing a crash bug of e10s.

User impact if declined:

If user or web page commits IME composition forcibly (e.g., clicking somewhere during composition or web page commits composition for autocomplete), TabParent's counter of number of composition won't be decreased. The counter is uint8_t. Therefore, when the counter reaches 254 and user tries to use IME, then, the main process crashes itself.

So, this can be problem if a tab is opened a long time. E.g., some users may meet this bug in Gmail.

Fix Landed on Version:

55

Risk to taking this patch (and alternatives if risky):

Not so risky. The counter was managed with composition events (when TabParent sends compositionstart, incremented and when composiitonend, decremented). This patch makes that TextComposition class which creates for every composition notify TabParent. Therefore, TabParent can decrement the counter in any cases.

String or UUID changes made by this patch:

No.
Attachment #8880010 - Flags: approval-mozilla-esr52?
Comment on attachment 8880010 [details] [diff] [review]
Patch for ESR52

Oops, I found a regression of this. This may cause setting current TextComposition instance to old composition's start offset. So, this patch is really buggy only when the child process is really slow.
Attachment #8880010 - Flags: approval-mozilla-esr52?
Is this something you still were hoping to land on ESR52 or should we wontfix?
Flags: needinfo?(masayuki)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #15)
> Is this something you still were hoping to land on ESR52 or should we
> wontfix?

I think that we should take only part1 of bug 1376424 instead.
Flags: needinfo?(masayuki)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: