Crash in nsIWidget::IMENotificationRequestsRef

RESOLVED FIXED in Firefox 56

Status

()

defect
--
critical
RESOLVED FIXED
2 years ago
3 months ago

People

(Reporter: philipp, Assigned: masayuki)

Tracking

({crash, regression})

56 Branch
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 fixed, firefox57 fixed)

Details

(crash signature)

Attachments

(1 attachment)

Reporter

Description

2 years ago
This bug was filed from the Socorro interface and is 
report bp-ebb8a6be-17fb-4c9a-952d-d18140170902.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	nsIWidget::IMENotificationRequestsRef() 	widget/nsBaseWidget.cpp:2361
1 	xul.dll 	mozilla::IMEStateManager::OnChangeFocusInternal(nsPresContext*, nsIContent*, mozilla::widget::InputContextAction) 	dom/events/IMEStateManager.cpp:489
2 	xul.dll 	nsFocusManager::Blur(nsPIDOMWindowOuter*, nsPIDOMWindowOuter*, bool, bool, nsIContent*) 	dom/base/nsFocusManager.cpp:1673
3 	xul.dll 	nsFocusManager::WindowLowered(mozIDOMWindowProxy*) 	dom/base/nsFocusManager.cpp:810
4 	xul.dll 	nsWebShellWindow::WindowDeactivated() 	xpfe/appshell/nsWebShellWindow.cpp:500
5 	xul.dll 	nsWindow::DispatchFocusToTopLevelWindow(bool) 	widget/windows/nsWindow.cpp:4641
6 	xul.dll 	nsWindow::ProcessMessage(unsigned int, unsigned __int64&, __int64&, __int64*) 	widget/windows/nsWindow.cpp:5950
7 	xul.dll 	nsWindow::WindowProcInternal(HWND__*, unsigned int, unsigned __int64, __int64) 	widget/windows/nsWindow.cpp:4953
8 	xul.dll 	CallWindowProcCrashProtected 	xpcom/base/nsCrashOnException.cpp:35
9 	xul.dll 	nsWindow::WindowProc(HWND__*, unsigned int, unsigned __int64, __int64) 	widget/windows/nsWindow.cpp:4905
10 	user32.dll 	UserCallWinProcCheckWow 	
11 	user32.dll 	DispatchClientMessage 	
12 	user32.dll 	_fnDWORD 	
13 	ntdll.dll 	KiUserCallbackDispatch 	
14 	user32.dll 	ZwUserPeekMessage 	
15 	user32.dll 	PeekMessageW 	
16 	msctf.dll 	CThreadInputMgr::PeekMessageW(tagMSG*, HWND__*, unsigned int, unsigned int, unsigned int, int*) 	
17 	msctf.dll 	CThreadInputMgr::PeekMessageW(tagMSG*, HWND__*, unsigned int, unsigned int, unsigned int, int*) 	
18 	xul.dll 	mozilla::widget::WinUtils::PeekMessageW(tagMSG*, HWND__*, unsigned int, unsigned int, unsigned int) 	widget/windows/WinUtils.cpp:713
19 	xul.dll 	nsAppShell::ProcessNextNativeEvent(bool) 	widget/windows/nsAppShell.cpp:319
20 	xul.dll 	nsBaseAppShell::DoProcessNextNativeEvent(bool) 	widget/nsBaseAppShell.cpp:140
21 	xul.dll 	nsBaseAppShell::OnProcessNextEvent(nsIThreadInternal*, bool) 	widget/nsBaseAppShell.cpp:291
22 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp:952
23 	xul.dll 	NS_ProcessNextEvent(nsIThread*, bool) 	xpcom/threads/nsThreadUtils.cpp:521
24 	xul.dll 	mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:125
25 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc:319
26 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc:299
27 	xul.dll 	nsBaseAppShell::Run() 	widget/nsBaseAppShell.cpp:158
28 	xul.dll 	nsAppShell::Run() 	widget/windows/nsAppShell.cpp:230
29 	xul.dll 	nsAppStartup::Run() 	toolkit/components/startup/nsAppStartup.cpp:288
30 	xul.dll 	XREMain::XRE_mainRun() 	toolkit/xre/nsAppRunner.cpp:4635
31 	xul.dll 	XREMain::XRE_main(int, char** const, mozilla::BootstrapConfig const&) 	toolkit/xre/nsAppRunner.cpp:4799
32 	xul.dll 	XRE_main(int, char** const, mozilla::BootstrapConfig const&) 	toolkit/xre/nsAppRunner.cpp:4894
33 	firefox.exe 	NS_internal_main(int, char**, char**) 	browser/app/nsBrowserApp.cpp:309
34 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp:115
35 	firefox.exe 	__scrt_common_main_seh 	f:/dd/vctools/crt/vcstartup/src/startup/exe_common.inl:253
36 	kernel32.dll 	BaseThreadInitThunk 	
37 	ntdll.dll 	RtlUserThreadStart

this crash signature is newly appearing on 57.0a1 and 56.0b5 after the patch for bug 1388647 has landed there.
Masayuki might know the best about this.
Flags: needinfo?(masayuki)
Yeah, looks like that this is simple mistake:

https://hg.mozilla.org/mozilla-central/annotate/73e8f351b28f/dom/events/IMEStateManager.cpp#l490

>   nsCOMPtr<nsIWidget> oldWidget = sWidget;
>   nsCOMPtr<nsIWidget> newWidget =
>     aPresContext ? aPresContext->GetRootWidget() : nullptr;
>   bool focusActuallyChanging =
>     (sContent != aContent || sPresContext != aPresContext ||
>      oldWidget != newWidget || sActiveTabParent != newTabParent);
> 
>   // If old widget has composition, we may need to commit composition since
>   // a native IME context is shared on all editors on some widgets or all
>   // widgets (it depends on platforms).
>   if (oldWidget && focusActuallyChanging && sTextCompositions) {
>     RefPtr<TextComposition> composition =
>       sTextCompositions->GetCompositionFor(oldWidget);
>     if (composition) {
>       // However, don't commit the composition if we're being inactivated
>       // but the composition should be kept even during deactive.
>       if (aPresContext ||
>           !sFocusedIMEWidget->IMENotificationRequestsRef().
>            WantDuringDeactive()) {

sFocusedIMEWidget may be nullptr here.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Flags: needinfo?(masayuki)

Updated

2 years ago
See Also: → 1387357
Comment hidden (mozreview-request)

Comment 4

2 years ago
mozreview-review
Comment on attachment 8904170 [details]
Bug 1396302 - IMEStateManager::OnChangeFocusInternal() should check oldWidget's IME notification requests rather than sFocusedIMEWidget

https://reviewboard.mozilla.org/r/175944/#review180968
Attachment #8904170 - Flags: review?(m_kato) → review+

Comment 5

2 years ago
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/3dbc8ba3dc09
IMEStateManager::OnChangeFocusInternal() should check oldWidget's IME notification requests rather than sFocusedIMEWidget r=m_kato
https://hg.mozilla.org/mozilla-central/rev/3dbc8ba3dc09
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Please request Beta approval on this when you get a chance. It grafts cleanly from m-c.
Flags: needinfo?(masayuki)
Comment on attachment 8904170 [details]
Bug 1396302 - IMEStateManager::OnChangeFocusInternal() should check oldWidget's IME notification requests rather than sFocusedIMEWidget

Approval Request Comment
[Feature/Bug causing the regression]:
Regression of bug 1388647.

[User impact if declined]:
I'm still not sure of the STR, but a lot of users meet this crash everyday.

[Is this code covered by automated tests?]:
No, since not sure the STR.

[Has the fix been verified in Nightly?]:
The latest build ID is 9/4's.
https://crash-stats.mozilla.com/signature/?product=Firefox&signature=nsIWidget%3A%3AIMENotificationRequestsRef&date=%3E%3D2017-08-30T05%3A01%3A00.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-build_id&_sort=-date&page=1

[Needs manual test from QE? If yes, steps to reproduce]:
No.

[List of other uplifts needed for the feature/fix]:
No.

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
Just checking right pointer.

[String changes made/needed]:
No.
Flags: needinfo?(masayuki)
Attachment #8904170 - Flags: approval-mozilla-beta?
Comment on attachment 8904170 [details]
Bug 1396302 - IMEStateManager::OnChangeFocusInternal() should check oldWidget's IME notification requests rather than sFocusedIMEWidget

Fix for new crash in 56, let's uplift for beta 10.
Attachment #8904170 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.