1715292 - Crash in [@ nsWindow::NativeMoveResizeWaylandPopup]

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect, P1)

Description

[Gabriele Svelto [:gsvelto]]

Crash report: https://crash-stats.mozilla.org/report/index/cef85e28-653a-46c3-9dd1-9b2f70210604

MOZ_CRASH Reason: MOZ_RELEASE_ASSERT(IsInPopupHierarchy())

Top 10 frames of crashing thread:

0 libxul.so nsWindow::NativeMoveResizeWaylandPopup widget/gtk/nsWindow.cpp:2061
1 libxul.so nsWindow::NativeMove widget/gtk/nsWindow.cpp:2277
2 libxul.so nsWindow::Move widget/gtk/nsWindow.cpp:1260
3 libxul.so nsBaseWidget::MoveClient widget/nsBaseWidget.cpp:1499
4 libxul.so nsViewManager::ProcessPendingUpdatesForView view/nsViewManager.cpp:380
5 libxul.so nsViewManager::ProcessPendingUpdates view/nsViewManager.cpp:972
6 libxul.so nsRefreshDriver::Tick layout/base/nsRefreshDriver.cpp:2476
7 libxul.so mozilla::RefreshDriverTimer::TickRefreshDrivers layout/base/nsRefreshDriver.cpp:326
8 libxul.so mozilla::RefreshDriverTimer::Tick layout/base/nsRefreshDriver.cpp:342
9 libxul.so mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver layout/base/nsRefreshDriver.cpp:704

The failing assertion was added in bug 1710436. Martin can you have a look please?

Comment 2

[Martin Stránský [:stransky] (ni? me)]

Hm, the assertion here tests:

mPopupTrackInHierarchy && mWaylandToplevel && mWaylandPopupPrev

which means when popup is visible it should be in hierarchy and must have assigned parent window. I expect mPopupTrackInHierarchy is always true here so the popup was removed although is still visible.

Comment 3

[Martin Stránský [:stransky] (ni? me)]

I'd need more context or reproduction steps here. I wonder which popup causes that - if some extension panel or so. Would be great to run Firefox with

MOZ_LOG="WidgetPopup:5"

env variable and get the log.

Comment 4

[Martin Stránský [:stransky] (ni? me)]

The patch here depends on Bug 1661516.

Comment 5

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1715292 [Wayland] Update popup visibility and hierarchy handling, r?jhorak

• Don't remove popup from widget hierarchy on nsWindow::Destroy() but leave nsWindow::Show(false) do the job.
• Don't check popup widget visibility from Gtk as it's not reliable.

Comment 6

[Martin Stránský [:stransky] (ni? me)]

We'd need to backport this one to 90 to avoid crashes.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b2f81bedfe4d
[Wayland] Update popup visibility and hierarchy handling, r=jhorak

Comment 8

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/b2f81bedfe4d

Comment 9

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

The patch landed in nightly and beta is affected.
:stransky, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 10

[Martin Stránský [:stransky] (ni? me)]

We can apply it as-is on beta.

Comment 11

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 9226722 [details]
Bug 1715292 [Wayland] Update popup visibility and hierarchy handling, r?jhorak

Beta/Release Uplift Approval Request

• User impact if declined: Firefox crashes on Wayland when popup menus and tooltip are displayed.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): - Baked on nighly.

• Minor functionality changes (don't release tracked popups early but leave it to destructor)
• Don't assert when popups are not visible from Gtk perspective (that info is useless for us anyway).

• String changes made/needed:

Comment 12

[Julien Cristau [:jcristau]]

To confirm, is this ok to uplift on its own despite the dependency mentioned in comment 4?

Comment 13

[Martin Stránský [:stransky] (ni? me)]

(In reply to Julien Cristau [:jcristau] from comment #12)

To confirm, is this ok to uplift on its own despite the dependency mentioned in comment 4?

Yes, I checked it and it works without Bug 1661516. I expected there might be some overlap but it can be applied without it.

Comment 14

[Julien Cristau [:jcristau]]

Comment on attachment 9226722 [details]
Bug 1715292 [Wayland] Update popup visibility and hierarchy handling, r?jhorak

approved for 90 rc1

Comment 15

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-release/rev/47c82d2ce122