1845904 - mozilla::PresShell::DoFlushPendingNotifications reentrancy during nsMenuPopupFrame::Init on macOS

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Yannis Juglaret [:yannis]]

While iterating over the proto signatures of the attached crash signature, I found instances of a mozilla::PresShell::DoFlushPendingNotifications reentrancy on macOS that seems to be still present in 116.0b4 at least. It produces crashes on MOZ_DIAGNOSTIC_ASSERT(!mForbiddenToFlush) (This is bad!) in nightly and early beta. I am not sure if there is currently a release crash associated to this reentrancy as it is hard to guess its consequences.

Example crash report in 116.0b4: here

Call stack:

00 XUL!mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) @ layout/base/PresShell.cpp:0
01 XUL!mozilla::PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) @ layout/base/PresShell.h:1470
02 XUL!mozilla::dom::Document::FlushPendingNotifications(mozilla::ChangesToFlush) @ dom/base/Document.cpp:10858
03 XUL!mozilla::dom::Document::FlushPendingNotifications(mozilla::FlushType) @ dom/base/Document.cpp:10790
04 XUL!nsFocusManager::FlushBeforeEventHandlingIfNeeded(nsIContent*) @ dom/base/nsFocusManager.h:159
05 XUL!mozilla::PresShell::EventHandler::HandleEventWithCurrentEventInfo(mozilla::WidgetEvent*, nsEventStatus*, bool, nsIContent*) @ layout/base/PresShell.cpp:8172
06 XUL!mozilla::PresShell::EventHandler::HandleEventAtFocusedContent(mozilla::WidgetGUIEvent*, nsEventStatus*) @ layout/base/PresShell.cpp:7944
07 XUL!mozilla::PresShell::HandleEvent(nsIFrame*, mozilla::WidgetGUIEvent*, bool, nsEventStatus*) @ layout/base/PresShell.cpp:6887
08 XUL!nsViewManager::DispatchEvent(mozilla::WidgetGUIEvent*, nsView*, nsEventStatus*) @ view/nsViewManager.cpp:678
09 XUL!nsView::HandleEvent(mozilla::WidgetGUIEvent*, bool) @ view/nsView.cpp:1149
0a XUL!nsChildView::DispatchEvent(mozilla::WidgetGUIEvent*, nsEventStatus&) @ widget/cocoa/nsChildView.mm:1272
0b XUL!nsBaseWidget::DispatchWindowEvent(mozilla::WidgetGUIEvent&) @ widget/nsBaseWidget.cpp:1319
0c XUL!nsChildView::GetEditorView() @ widget/cocoa/nsChildView.mm:1592
0d XUL!mozilla::widget::TextInputHandlerBase::GetWindowLevel() @ widget/cocoa/TextInputHandler.mm:4803
0e XUL!-[ChildView windowLevel] @ widget/cocoa/nsChildView.mm:3493
0f AppKit!-[NSTextInputContext activate]
10 AppKit!+[NSTextInputContext currentInputContext_withFirstResponderSync:]
11 AppKit!-[NSView _setWindow:]
12 AppKit!-[NSView removeFromSuperview]
13 XUL!-[BaseWindow swapOutChildViewWrapper:] @ widget/cocoa/nsCocoaWindow.mm:3321
14 XUL!-[BaseWindow setUseMenuStyle:] @ widget/cocoa/nsCocoaWindow.mm:0
15 XUL!nsCocoaWindow::SetWindowShadowStyle(mozilla::StyleWindowShadow) @ widget/cocoa/nsCocoaWindow.mm:2416
16 XUL!nsMenuPopupFrame::PropagateStyleToWidget(mozilla::EnumSet<nsMenuPopupFrame::WidgetStyle, unsigned char>) const @ layout/xul/nsMenuPopupFrame.cpp:361
17 XUL!nsMenuPopupFrame::CreateWidgetForView(nsView*) @ layout/xul/nsMenuPopupFrame.cpp:330
18 XUL!nsMenuPopupFrame::Init(nsIContent*, nsContainerFrame*, nsIFrame*) @ layout/xul/nsMenuPopupFrame.cpp:186
19 XUL!nsCSSFrameConstructor::InitAndRestoreFrame(nsFrameConstructorState const&, nsIContent*, nsContainerFrame*, nsIFrame*, bool) @ layout/base/nsCSSFrameConstructor.cpp:4616
1a XUL!nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsContainerFrame*, nsFrameList&) @ layout/base/nsCSSFrameConstructor.cpp:3794
1b XUL!nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsContainerFrame*, nsFrameList&) @ layout/base/nsCSSFrameConstructor.cpp:5574
1c XUL!nsCSSFrameConstructor::ConstructFramesFromItemList(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList&, nsContainerFrame*, bool, nsFrameList&) @ layout/base/nsCSSFrameConstructor.cpp:9508
1d XUL!nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState&, nsIContent*, mozilla::ComputedStyle*, nsContainerFrame*, bool, nsFrameList&, bool, nsIFrame*) @ layout/base/nsCSSFrameConstructor.cpp:9797
1e XUL!nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsContainerFrame*, nsFrameList&) @ layout/base/nsCSSFrameConstructor.cpp:3897
1f XUL!nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsContainerFrame*, nsFrameList&) @ layout/base/nsCSSFrameConstructor.cpp:5574
20 XUL!nsCSSFrameConstructor::ConstructFramesFromItemList(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList&, nsContainerFrame*, bool, nsFrameList&) @ layout/base/nsCSSFrameConstructor.cpp:9508
21 XUL!nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState&, nsIContent*, mozilla::ComputedStyle*, nsContainerFrame*, bool, nsFrameList&, bool, nsIFrame*) @ layout/base/nsCSSFrameConstructor.cpp:9797
22 XUL!nsCSSFrameConstructor::ConstructBlock(nsFrameConstructorState&, nsIContent*, nsContainerFrame*, nsContainerFrame*, mozilla::ComputedStyle*, nsContainerFrame**, nsFrameList&, nsIFrame*) @ layout/base/nsCSSFrameConstructor.cpp:10649
23 XUL!nsCSSFrameConstructor::ConstructNonScrollableBlock(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItem&, nsContainerFrame*, nsStyleDisplay const*, nsFrameList&) @ layout/base/nsCSSFrameConstructor.cpp:4603
24 XUL!nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsContainerFrame*, nsFrameList&) @ layout/base/nsCSSFrameConstructor.cpp:3769
25 XUL!nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsContainerFrame*, nsFrameList&) @ layout/base/nsCSSFrameConstructor.cpp:5574
26 XUL!nsCSSFrameConstructor::ConstructFramesFromItemList(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList&, nsContainerFrame*, bool, nsFrameList&) @ layout/base/nsCSSFrameConstructor.cpp:9508
27 XUL!nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState&, nsIContent*, mozilla::ComputedStyle*, nsContainerFrame*, bool, nsFrameList&, bool, nsIFrame*) @ layout/base/nsCSSFrameConstructor.cpp:9797
28 XUL!nsCSSFrameConstructor::ConstructBlock(nsFrameConstructorState&, nsIContent*, nsContainerFrame*, nsContainerFrame*, mozilla::ComputedStyle*, nsContainerFrame**, nsFrameList&, nsIFrame*) @ layout/base/nsCSSFrameConstructor.cpp:10649
29 XUL!nsCSSFrameConstructor::ConstructNonScrollableBlock(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItem&, nsContainerFrame*, nsStyleDisplay const*, nsFrameList&) @ layout/base/nsCSSFrameConstructor.cpp:4603
2a XUL!nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsContainerFrame*, nsFrameList&) @ layout/base/nsCSSFrameConstructor.cpp:3769
2b XUL!nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsContainerFrame*, nsFrameList&) @ layout/base/nsCSSFrameConstructor.cpp:5574
2c XUL!nsCSSFrameConstructor::ConstructFramesFromItemList(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList&, nsContainerFrame*, bool, nsFrameList&) @ layout/base/nsCSSFrameConstructor.cpp:9508
2d XUL!nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState&, nsIContent*, mozilla::ComputedStyle*, nsContainerFrame*, bool, nsFrameList&, bool, nsIFrame*) @ layout/base/nsCSSFrameConstructor.cpp:9797
2e XUL!nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsContainerFrame*, nsFrameList&) @ layout/base/nsCSSFrameConstructor.cpp:3897
2f XUL!nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsContainerFrame*, nsFrameList&) @ layout/base/nsCSSFrameConstructor.cpp:5574
30 XUL!nsCSSFrameConstructor::ConstructFramesFromItemList(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList&, nsContainerFrame*, bool, nsFrameList&) @ layout/base/nsCSSFrameConstructor.cpp:9508
31 XUL!nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState&, nsIContent*, mozilla::ComputedStyle*, nsContainerFrame*, bool, nsFrameList&, bool, nsIFrame*) @ layout/base/nsCSSFrameConstructor.cpp:9797
32 XUL!nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsContainerFrame*, nsFrameList&) @ layout/base/nsCSSFrameConstructor.cpp:3897
33 XUL!nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsContainerFrame*, nsFrameList&) @ layout/base/nsCSSFrameConstructor.cpp:5574
34 XUL!nsCSSFrameConstructor::ConstructFramesFromItemList(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList&, nsContainerFrame*, bool, nsFrameList&) @ layout/base/nsCSSFrameConstructor.cpp:9508
35 XUL!nsCSSFrameConstructor::ContentRangeInserted(nsIContent*, nsIContent*, nsCSSFrameConstructor::InsertionKind) @ layout/base/nsCSSFrameConstructor.cpp:7189
36 XUL!mozilla::RestyleManager::ProcessRestyledFrames(nsStyleChangeList&) @ layout/base/RestyleManager.cpp:1558
37 XUL!mozilla::RestyleManager::DoProcessPendingRestyles(mozilla::ServoTraversalFlags) @ layout/base/RestyleManager.cpp:3179
38 XUL!mozilla::RestyleManager::ProcessPendingRestyles() @ layout/base/RestyleManager.cpp:3264
39 XUL!mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) @ layout/base/PresShell.cpp:4343
3a XUL!mozilla::PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) @ layout/base/PresShell.h:1470
3b XUL!mozilla::PresShell::DoFlushPendingNotifications(mozilla::FlushType) @ layout/base/PresShell.cpp:4159
3c XUL!mozilla::PresShell::FlushPendingNotifications(mozilla::FlushType) @ layout/base/PresShell.h:1461
3d XUL!nsPresShellEventCB::HandleEvent(mozilla::EventChainPostVisitor&) @ layout/base/PresShell.cpp:495
3e XUL!mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) @ dom/events/EventDispatcher.cpp:614
3f XUL!mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) @ dom/events/EventDispatcher.cpp:1137
40 XUL!mozilla::PresShell::EventHandler::DispatchEventToDOM(mozilla::WidgetEvent*, nsEventStatus*, nsPresShellEventCB*) @ layout/base/PresShell.cpp:8694
41 XUL!mozilla::PresShell::EventHandler::DispatchEvent(mozilla::EventStateManager*, mozilla::WidgetEvent*, bool, nsEventStatus*, nsIContent*) @ layout/base/PresShell.cpp:8265
42 XUL!mozilla::PresShell::EventHandler::HandleEventWithCurrentEventInfo(mozilla::WidgetEvent*, nsEventStatus*, bool, nsIContent*) @ layout/base/PresShell.cpp:8197
43 XUL!mozilla::PresShell::EventHandler::HandleEventUsingCoordinates(nsIFrame*, mozilla::WidgetGUIEvent*, nsEventStatus*, bool) @ layout/base/PresShell.cpp:7146
44 XUL!mozilla::PresShell::HandleEvent(nsIFrame*, mozilla::WidgetGUIEvent*, bool, nsEventStatus*) @ layout/base/PresShell.cpp:6887
45 XUL!nsViewManager::DispatchEvent(mozilla::WidgetGUIEvent*, nsView*, nsEventStatus*) @ view/nsViewManager.cpp:678
46 XUL!nsView::HandleEvent(mozilla::WidgetGUIEvent*, bool) @ view/nsView.cpp:1149
47 XUL!nsChildView::DispatchEvent(mozilla::WidgetGUIEvent*, nsEventStatus&) @ widget/cocoa/nsChildView.mm:1272
48 XUL!nsBaseWidget::ProcessUntransformedAPZEvent(mozilla::WidgetInputEvent*, mozilla::layers::APZEventResult const&) @ widget/nsBaseWidget.cpp:1093
49 XUL!nsBaseWidget::DispatchInputEvent(mozilla::WidgetInputEvent*) @ widget/nsBaseWidget.cpp:1272
4a XUL!-[ChildView mouseDown:] @ widget/cocoa/nsChildView.mm:2801
4b AppKit!-[NSWindow(NSEventRouting) _handleMouseDownEvent:isDelayedEvent:]
4c AppKit!-[NSWindow(NSEventRouting) _reallySendEvent:isDelayedEvent:]
4d AppKit!-[NSWindow(NSEventRouting) sendEvent:]
4e XUL!-[ToolbarWindow sendEvent:] @ widget/cocoa/nsCocoaWindow.mm:4082
4f AppKit!-[NSApplication(NSEvent) sendEvent:]
50 XUL!-[GeckoNSApplication sendEvent:] @ widget/cocoa/nsAppShell.mm:165
51 AppKit!-[NSApplication _handleEvent:]
52 AppKit!-[NSApplication run]
53 XUL!nsAppShell::Run() @ widget/cocoa/nsAppShell.mm:803
54 XUL!nsAppStartup::Run() @ toolkit/components/startup/nsAppStartup.cpp:295
55 XUL!XREMain::XRE_mainRun() @ toolkit/xre/nsAppRunner.cpp:5642
56 XUL!XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) @ toolkit/xre/nsAppRunner.cpp:5842
57 XUL!XRE_main(int, char**, mozilla::BootstrapConfig const&) @ toolkit/xre/nsAppRunner.cpp:5898
58 firefox!do_main(int, char**, char**) @ browser/app/nsBrowserApp.cpp:227
59 firefox!main @ browser/app/nsBrowserApp.cpp:445
5a dyld!start
5b dyld!invocation function for block in dyld4::handleDyldInCache(dyld3::MachOFile const*, dyld4::KernelArgs const*, dyld3::MachOFile const*)

The earliest instance of this proto signature we have is in 113.0b2. It is a bit hidden among all the Windows volume for the signature, but this is the proto signature for 3 of the 7 crashes we received on the attached signature during the 116 early beta cycle, so ~43% of the macOS volume (the rest of the proto signatures in the macOS volume are the ones we think could be causing bug 1809492 in release).

Comment 1

[Emilio Cobos Álvarez (:emilio)]

The fact that nsCocoaWindow::SetWindowShadowStyle can run script seems like a bug to me. Masayuki, is it expected / can we avoid the re-entrant flush here?

Comment 2

[Timothy Nikkel (:tnikkel)]

If we can't avoid that, then I believe we've handled similar problems in the past by moving work to be async by calling nsViewManager::UpdateWidgetGeometry.

Comment 3

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

Well, the view is "fixed" when focused element is changed, so perhaps, we can stop getting the view dynamically.

However, we've already do this:
https://searchfox.org/mozilla-central/rev/2bf90dc51ce7e8274ce208fbb9d68b3ff535185e/widget/cocoa/nsChildView.mm#1587-1591

So I think that here should check whether the query content event requires a flush.

Comment 4

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

Attachment: Bug 1845904 - Make `PresShell::EventHandler::HandleEventWithCurrentEventInfo` stop flushing pending notifications if a query content event does not require that r=emilio!,#dom-core

We've already known that WidgetQueryContentEvent is dispatched at initializing
a popup menu on macOS (bug 1530188). Therefore, I made the dispatcher under
widget/cocoa set WidgetQueryContentEvent::mNeedLayoutFlush to false to
avoid to flush pending things in ContentEventHandler, but
PresShell::EventHandler does it.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/06273ebf279a
Make `PresShell::EventHandler::HandleEventWithCurrentEventInfo` stop flushing pending notifications if a query content event does not require that r=emilio

Comment 7

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/06273ebf279a

Comment 8

[Gabriele Svelto [:gsvelto]]

There's still some crash volume here on nightly and beta, Masayuki do you know what might be going on? Should I open a separate bug given the crashes seem to be mostly on Windows and this was macOS-specific?

Comment 9

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

Most of them are not related to this bug because their stack is the cases running only in the layout module. Looks like bp-f233debb-ba2b-4437-aa06-e05c10230831 and bp-cfe2c213-4c60-4ab6-804d-7d8b50230831 are similar bug of this.

And yes, could you file a new bug?

Comment 10

[Gabriele Svelto [:gsvelto]]

On it. If there's anything useful that could be gleaned by inspecting the contents of the minidumps just let me know.