1851533 - [TSF] mozilla::PresShell::DoFlushPendingNotifications reentrancy during TSF gets focus

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P3)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

bp-f233debb-ba2b-4437-aa06-e05c10230831

0 	xul.dll	mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush)	layout/base/PresShell.cpp:4181
1 	xul.dll	mozilla::PresShell::FlushPendingNotifications(mozilla::ChangesToFlush)	layout/base/PresShell.h:1472
1 	xul.dll	mozilla::dom::Document::FlushPendingNotifications(mozilla::ChangesToFlush)	dom/base/Document.cpp:10934
2 	xul.dll	mozilla::dom::Document::FlushPendingNotifications(mozilla::FlushType)	dom/base/Document.cpp:10866
2 	xul.dll	mozilla::ContentEventHandler::InitBasic(bool)	dom/events/ContentEventHandler.cpp:348
2 	xul.dll	mozilla::ContentEventHandler::InitCommon(mozilla::EventMessage, mozilla::SelectionType, bool)	dom/events/ContentEventHandler.cpp:421
3 	xul.dll	mozilla::ContentEventHandler::Init(mozilla::WidgetQueryContentEvent*)	dom/events/ContentEventHandler.cpp:493
4 	xul.dll	mozilla::ContentEventHandler::OnQueryEditorRect(mozilla::WidgetQueryContentEvent*)	dom/events/ContentEventHandler.cpp:2803
4 	xul.dll	mozilla::ContentEventHandler::HandleQueryContentEvent(mozilla::WidgetQueryContentEvent*)	dom/events/ContentEventHandler.cpp:1433
5 	xul.dll	mozilla::IMEContentObserver::HandleQueryContentEvent(mozilla::WidgetQueryContentEvent*)	dom/events/IMEContentObserver.cpp:657
6 	xul.dll	mozilla::EventStateManager::HandleQueryContentEvent(mozilla::WidgetQueryContentEvent*)	dom/events/EventStateManager.cpp:1160
7 	xul.dll	mozilla::EventStateManager::PreHandleEvent(nsPresContext*, mozilla::WidgetEvent*, nsIFrame*, nsIContent*, nsEventStatus*, nsIContent*)	dom/events/EventStateManager.cpp:698
8 	xul.dll	mozilla::PresShell::EventHandler::DispatchEvent(mozilla::EventStateManager*, mozilla::WidgetEvent*, bool, nsEventStatus*, nsIContent*)	layout/base/PresShell.cpp:8226
9 	xul.dll	mozilla::PresShell::EventHandler::HandleEventWithCurrentEventInfo(mozilla::WidgetEvent*, nsEventStatus*, bool, nsIContent*)	layout/base/PresShell.cpp:8195
10 	xul.dll	mozilla::PresShell::EventHandler::HandleEventAtFocusedContent(mozilla::WidgetGUIEvent*, nsEventStatus*)	layout/base/PresShell.cpp:7940
11 	xul.dll	mozilla::PresShell::HandleEvent(nsIFrame*, mozilla::WidgetGUIEvent*, bool, nsEventStatus*)	layout/base/PresShell.cpp:6884
12 	xul.dll	nsViewManager::DispatchEvent(mozilla::WidgetGUIEvent*, nsView*, nsEventStatus*)	view/nsViewManager.cpp:653
13 	xul.dll	nsView::HandleEvent(mozilla::WidgetGUIEvent*, bool)	view/nsView.cpp:1149
14 	xul.dll	nsWindow::DispatchEvent(mozilla::WidgetGUIEvent*, nsEventStatus&)	widget/windows/nsWindow.cpp:4188
15 	xul.dll	nsBaseWidget::DispatchWindowEvent(mozilla::WidgetGUIEvent&)	widget/nsBaseWidget.cpp:1319
15 	xul.dll	mozilla::widget::TSFTextStore::DispatchEvent(mozilla::WidgetGUIEvent&)	widget/windows/TSFTextStore.cpp:2205
15 	xul.dll	mozilla::widget::TSFTextStore::GetScreenExtInternal(tagRECT&)	widget/windows/TSFTextStore.cpp:4955
16 	xul.dll	mozilla::widget::TSFTextStore::GetScreenExt(unsigned long, tagRECT*)	widget/windows/TSFTextStore.cpp:4931
17 	textinputframework.dll	CContextView::GetScreenExt(tagRECT*)	
18 	textinputframework.dll	CInputContext::_OnLayoutChangeInternal	
19 	textinputframework.dll	CInputContext::_NotifyEndEdit(void)	
20 	textinputframework.dll	CInputContext::OnLockGranted(unsigned long)	
21 	textinputframework.dll	CACPWrap::OnLockGranted(unsigned long)	
22 	xul.dll	mozilla::widget::TSFTextStore::RequestLock(unsigned long, HRESULT*)	widget/windows/TSFTextStore.cpp:2119
23 	textinputframework.dll	CInputContext::OnLayoutChange(__MIDL___MIDL_itf_textstor_0000_0000_0002, unsigned long)	
24 	xul.dll	mozilla::widget::TSFTextStore::CreateAndSetFocus(nsWindow*, mozilla::widget::InputContext const&)	widget/windows/TSFTextStore.cpp:5904
24 	xul.dll	mozilla::widget::TSFTextStore::OnFocusChange(bool, nsWindow*, mozilla::widget::InputContext const&)	widget/windows/TSFTextStore.cpp:5790
25 	xul.dll	mozilla::widget::TSFTextStore::SetInputContext(nsWindow*, mozilla::widget::InputContext const&, mozilla::widget::InputContextAction const&)	widget/windows/TSFTextStore.cpp:6709
26 	xul.dll	mozilla::widget::IMEHandler::SetInputContext(nsWindow*, mozilla::widget::InputContext&, mozilla::widget::InputContextAction const&)	widget/windows/WinIMEHandler.cpp:434
26 	xul.dll	nsWindow::SetInputContext(mozilla::widget::InputContext const&, mozilla::widget::InputContextAction const&)	widget/windows/nsWindow.cpp:7397
27 	xul.dll	mozilla::IMEStateManager::SetInputContext(nsIWidget&, mozilla::widget::InputContext const&, mozilla::widget::InputContextAction const&)	dom/events/IMEStateManager.cpp:1781
28 	xul.dll	mozilla::IMEStateManager::SetIMEState(mozilla::widget::IMEState const&, nsPresContext const*, mozilla::dom::Element*, nsIWidget&, mozilla::widget::InputContextAction, mozilla::widget::InputContext::Origin)	dom/events/IMEStateManager.cpp:1765
29 	xul.dll	mozilla::IMEStateManager::OnChangeFocusInternal(nsPresContext*, mozilla::dom::Element*, mozilla::widget::InputContextAction)	dom/events/IMEStateManager.cpp:715
30 	xul.dll	mozilla::IMEStateManager::OnInstalledMenuKeyboardListener(bool)	dom/events/IMEStateManager.cpp:753
31 	xul.dll	nsContentUtils::NotifyInstalledMenuKeyboardListener(bool)	dom/base/nsContentUtils.cpp:5814
31 	xul.dll	nsXULPopupManager::UpdateKeyboardListeners()	layout/xul/nsXULPopupManager.cpp:2050
32 	xul.dll	nsXULPopupManager::HidePopupsInList(nsTArray<nsMenuPopupFrame*> const&)	layout/xul/nsXULPopupManager.cpp:1459
33 	xul.dll	nsXULPopupManager::PopupDestroyed(nsMenuPopupFrame*)	layout/xul/nsXULPopupManager.cpp:1995
34 	xul.dll	nsMenuPopupFrame::DestroyFrom(nsIFrame*, mozilla::PostFrameDestroyData&)	layout/xul/nsMenuPopupFrame.cpp:2147
35 	xul.dll	nsIFrame::Destroy()	layout/generic/nsIFrame.h:656
35 	xul.dll	nsFrameList::DestroyFrame(nsIFrame*)	layout/generic/nsFrameList.cpp:111
36 	xul.dll	nsPlaceholderFrame::DestroyFrom(nsIFrame*, mozilla::PostFrameDestroyData&)	layout/generic/nsPlaceholderFrame.cpp:164
<snip>
50 	xul.dll	nsContainerFrame::DestroyFrom(nsIFrame*, mozilla::PostFrameDestroyData&)	layout/generic/nsContainerFrame.cpp:232
51 	xul.dll	nsLineBox::DeleteLineList(nsPresContext*, nsLineList&, nsIFrame*, nsFrameList*, mozilla::PostFrameDestroyData&)	layout/generic/nsLineBox.cpp:369
51 	xul.dll	nsBlockFrame::DestroyFrom(nsIFrame*, mozilla::PostFrameDestroyData&)	layout/generic/nsBlockFrame.cpp:482
52 	xul.dll	nsFrameList::DestroyFramesFrom(nsIFrame*, mozilla::PostFrameDestroyData&)	layout/generic/nsFrameList.cpp:50
52 	xul.dll	nsContainerFrame::DestroyFrom(nsIFrame*, mozilla::PostFrameDestroyData&)	layout/generic/nsContainerFrame.cpp:232
53 	xul.dll	nsIFrame::Destroy()	layout/generic/nsIFrame.h:656
53 	xul.dll	nsContainerFrame::RemoveFrame(mozilla::FrameChildListID, nsIFrame*)	layout/generic/nsContainerFrame.cpp:186
54 	xul.dll	nsFrameManager::RemoveFrame(mozilla::FrameChildListID, nsIFrame*)	layout/base/nsFrameManager.cpp:119
54 	xul.dll	nsCSSFrameConstructor::ContentRemoved(nsIContent*, nsIContent*, nsCSSFrameConstructor::RemoveFlags)	layout/base/nsCSSFrameConstructor.cpp:7535
55 	xul.dll	nsCSSFrameConstructor::RecreateFramesForContent(nsIContent*, nsCSSFrameConstructor::InsertionKind)	layout/base/nsCSSFrameConstructor.cpp:8517
56 	xul.dll	mozilla::RestyleManager::ProcessRestyledFrames(nsStyleChangeList&)	layout/base/RestyleManager.cpp:1612
56 	xul.dll	mozilla::RestyleManager::DoProcessPendingRestyles(mozilla::ServoTraversalFlags)	layout/base/RestyleManager.cpp:3194
57 	xul.dll	mozilla::RestyleManager::ProcessPendingRestyles()	layout/base/RestyleManager.cpp:3279
57 	xul.dll	mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush)	layout/base/PresShell.cpp:4328
58 	xul.dll	mozilla::PresShell::FlushPendingNotifications(mozilla::ChangesToFlush)	layout/base/PresShell.h:1472
58 	xul.dll	mozilla::PresShell::DoFlushPendingNotifications(mozilla::FlushType)	layout/base/PresShell.cpp:4144
58 	xul.dll	mozilla::PresShell::FlushPendingNotifications(mozilla::FlushType)	layout/base/PresShell.h:1463
58 	xul.dll	mozilla::EventStateManager::FlushLayout(nsPresContext*)	dom/events/EventStateManager.cpp:6034
58 	xul.dll	mozilla::EventStateManager::PreHandleEvent(nsPresContext*, mozilla::WidgetEvent*, nsIFrame*, nsIContent*, nsEventStatus*, nsIContent*)	dom/events/EventStateManager.cpp:849 

So, when menu losts pseudo-focus, it's notified synchronously (and immediately) by IMEStateManager. Then, TSFTextStore handles focus immediately. Finally, TSF tries to cache "new" focused content and that causes flushing layout.

It seems that we should notify widget of that asynchrnously.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The bug is linked to a topcrash signature, which matches the following criterion:

• Top 10 AArch64 and ARM crashes on nightly

:edgar, could you consider increasing the severity of this top-crash bug?

For more information, please visit BugBot documentation.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

This is a bug about a Windows specific path. So, the crash rate on Android is not handled by here.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Sorry for the spam. I'm not sure how to include TSFTextStore method calls into the stack search query.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The bug is linked to a topcrash signature, which matches the following criterion:

• Top 10 AArch64 and ARM crashes on nightly

For more information, please visit BugBot documentation.

Comment 5

[Gabriele Svelto [:gsvelto]]

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #3)

Sorry for the spam. I'm not sure how to include TSFTextStore method calls into the stack search query.

There's no way to do it here on Bugzilla, but on crash-stats you can use the proto-signature field to specify that a method should be on the stack. See this query.

Comment 6

[Emilio Cobos Álvarez [:emilio]]

Masayuki, can we just use a script runner in NotifyInstalledMenuKeyboardListener? We shouldn't run random Windows code from frame destruction.

Alternatively, we should at least change this to call nsContentUtils::IsSafeToRunScript() or so, and not flush if it's not safe.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Yeah, it's possible. And its deadline is the next event loop. What's the proper runner to run it at last of current loop?

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

(Ah, but for mochitests, script runner might be better for next line of JS.)

Comment 9

[Emilio Cobos Álvarez [:emilio]]

Not sure, but script runner is probably less risky/breaking?

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Attachment: Bug 1851533 - Make `IMEStateManager::OnInstalledMenuKeyboardListener` call `OnChangeFocusInternal` when safe r=m_kato!,smaug!,#dom-core

Notifying IME of the pseudo focus change may cause query content with flushing
pending things. Therefore, it should be run when safe.

Ideally, all notification should be async, but it requires too big change
because all things handled in IMEStateManager needs to be async. E.g.,
UpdateIMEState etc. Therefore, this patch just fixes the known issue.

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

Based on the topcrash criteria, the crash signature linked to this bug is not a topcrash signature anymore.

For more information, please visit BugBot documentation.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0fd828998c56
Make `IMEStateManager::OnInstalledMenuKeyboardListener` call `OnChangeFocusInternal` when safe r=m_kato,smaug

Comment 13

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/0fd828998c56

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:masayuki, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox123 to wontfix.

For more information, please visit BugBot documentation.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

It's a little bit risky for uplift.