1872154 - Reentrancy crash in PresShell::DoFlushPendingNotifications on Android Nightly and Beta, through nsFocusManager

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Yannis Juglaret [:yannis]]

Example crash: here.

Crash reason: MOZ_DIAGNOSTIC_ASSERT(!mForbiddenToFlush) (This is bad!)

Top frames:

0 	mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) @ layout/base/PresShell.cpp:4182
1 	mozilla::dom::Document::FlushPendingNotifications(mozilla::ChangesToFlush) @ dom/base/Document.cpp:10899
2 	mozilla::dom::Document::FlushPendingNotifications(mozilla::FlushType) @ dom/base/Document.cpp:10831
2 	nsFocusManager::FlushAndCheckIfFocusable(mozilla::dom::Element*, unsigned int) @ dom/base/nsFocusManager.cpp:2116
3 	nsFocusManager::Focus(nsPIDOMWindowOuter*, mozilla::dom::Element*, unsigned int, bool, bool, bool, bool, unsigned long, mozilla::Maybe<nsFocusManager::BlurredElementInfo> const&) @ dom/base/nsFocusManager.cpp:2629
4 	nsFocusManager::WindowShown(mozIDOMWindowProxy*, bool) @ dom/base/nsFocusManager.cpp:1006
5 	nsGlobalWindowInner::SetReadyForFocus() @ dom/base/nsGlobalWindowInner.cpp:4471
6 	mozilla::PresShell::UnsuppressAndInvalidate() @ layout/base/PresShell.cpp:3993
7 	mozilla::PresShell::ProcessReflowCommands(bool) @ layout/base/PresShell.cpp:9915
8 	nsPresContext::UpdateContainerQueryStyles() @ layout/base/nsPresContext.cpp:1078
9 	mozilla::RestyleManager::DoProcessPendingRestyles(mozilla::ServoTraversalFlags) @ layout/base/RestyleManager.cpp:3297
10 	mozilla::RestyleManager::ProcessPendingRestyles() @ layout/base/RestyleManager.cpp:3340
10 	mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) @ layout/base/PresShell.cpp:4326
11 	nsRefreshDriver::TickObserverArray(unsigned int, mozilla::TimeStamp) @ layout/base/nsRefreshDriver.cpp:2502
12 	nsRefreshDriver::Tick(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp, nsRefreshDriver::IsExtraTick) @ layout/base/nsRefreshDriver.cpp:2736
13 	mozilla::RefreshDriverTimer::TickDriver(nsRefreshDriver*, mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) @ layout/base/nsRefreshDriver.cpp:367
13 	mozilla::RefreshDriverTimer::TickRefreshDrivers(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) @ layout/base/nsRefreshDriver.cpp:345
14 	mozilla::RefreshDriverTimer::Tick(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) @ layout/base/nsRefreshDriver.cpp:361
15 	mozilla::VsyncRefreshDriverTimer::RunRefreshDrivers(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) @ layout/base/nsRefreshDriver.cpp:951
15 	mozilla::VsyncRefreshDriverTimer::TickRefreshDriver(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) @ layout/base/nsRefreshDriver.cpp:861
16 	mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsyncTimerOnMainThread() @ layout/base/nsRefreshDriver.cpp:592
16 	mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::VsyncEvent const&) @ layout/base/nsRefreshDriver.cpp:549

Reentrancy through this path only seems to affect Android builds. The crash signature is shared with other bugs (bug 1851430, bug 1851533) so the crash volume can be misleading unless filtered by platform. This seems to have been around for a while (119, maybe even 115) but has been gaining volume in 122, here are the top versions currently when asking for the Android volume hitting the MOZ_ASSERT:

1 	122.0a1 	27 	31.76 %
2 	123.0a1 	22 	25.88 %
3 	122.0b3 	12 	14.12 %
4 	120.0a1 	9 	10.59 %
5 	121.0b6 	6 	7.06 %

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

For more information, please visit BugBot documentation.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1872154 - Move paint unsuppression and similar code to DidDoReflow. r=dholbert

This can run script, and as such shouldn't run from a container query
update. Instead, let's move the check to DidDoReflow, which deals with
that appropriately already.

I don't know of an easy / reliable way to reproduce this, since paint
unsuppression is very timing-dependent.

While at it, remove the long reflow telemetry check for
GetRootElement(). This comes from a time where we used the root
element's namespace to determine which telemetry counter to use.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/165a83ec3c01
Move paint unsuppression and similar code to DidDoReflow. r=dholbert

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b9371a18f1d5
Remove android-specific expectations for a test.

Comment 5

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/165a83ec3c01
https://hg.mozilla.org/mozilla-central/rev/b9371a18f1d5