Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Alt-F Crash fix #11328

Merged
merged 2 commits into from
Mar 7, 2023
Merged

Alt-F Crash fix #11328

merged 2 commits into from
Mar 7, 2023

Conversation

daschuer
Copy link
Member

@daschuer daschuer commented Mar 5, 2023

This PR does not use the probably broken broken QMenuBar::setNativeMenuBar(false) call.

Even though I still cannot reproduce the issue. It is obvious from the backtrace in #11320 that there is an issue that happens after
deleting the platformMenuBar here: https://github.com/qt/qtbase/blob/8c47a86eecd286dbe66ca55ed4deaaf0f901d13b/src/widgets/widgets/qmenubar.cpp#L1882

I assume that the final crash happens than here:
https://github.com/qt/qtbase/blob/8c47a86eecd286dbe66ca55ed4deaaf0f901d13b/src/widgets/widgets/qmenubar.cpp#L1767
because of a dangling pointer in the sub-menu actions.

It is still a puzzling why the crash does not happen here. At least all is still working tested on Linux Mint Mate, Ubuntu Unity and Ubuntu Desktop.

I will now single step through the code to discover the issue.

@ronso0 I hope this PR gives us the freedom to consider the best solution in your pending work without the fear of a crash.
Can you confirm the crash is fixed?

@daschuer
Copy link
Member Author

daschuer commented Mar 5, 2023

Debugging does not reveal anything. It turns out hat the stored pointer in the actions is a QPointer that cannot become dangling.
https://github.com/qt/qtbase/blob/de8538966f51cb1589bfc4032be99b266707e26c/src/widgets/widgets/qmenu_p.h#L462
Also Valgrind is not complaining...
I am really curios if this PR fixes the crash.

@@ -1438,14 +1440,13 @@ void MixxxMainWindow::slotViewFullScreen(bool toggle) {
// Fix for "No menu bar with ubuntu unity in full screen mode" Bug
// #885890 and Bug #1076789. Before touching anything here, please read
// those bugs.
QApplication::setAttribute(Qt::AA_DontUseNativeMenuBar, true);
Copy link
Member

@ronso0 ronso0 Mar 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my tests I did exactly that (just at the beginning of createMenuBar() depending on isFullScreen(), not directly before the call) and I did set it in main.cpp, not in mixxx.cpp.

So I wonder how that makes the difference...
Anyway, crash fixed!

@ronso0
Copy link
Member

ronso0 commented Mar 5, 2023

Great, that does the trick 🎉

Crash fixed on Ubuntu 20.04.5 with xfce + vala-appmenu-plugin
and VM Ubuntu 20.04 with Unity desktop + global menu

@daschuer
Copy link
Member Author

daschuer commented Mar 5, 2023

Cool, so we can merge this?
There remains just the issue that we have no explanation for this.
Maybe a race condition with QPointer?

@ronso0
Copy link
Member

ronso0 commented Mar 5, 2023

@ronso0 I hope this PR gives us the freedom to consider the best solution in your pending work without the fear of a crash.

This can easily be combined with #11313 (we can remove the hotkey popup) which fixes the issue with identical fullscreen shortcuts in OS and Mixxx.

Thank you for looking into this!

@ronso0
Copy link
Member

ronso0 commented Mar 5, 2023

FWIW still hitting the debug assert on shutdown because of leaked controls.

@daschuer
Copy link
Member Author

daschuer commented Mar 5, 2023

Is this related to this PR? Which one is leaked?

@ronso0
Copy link
Member

ronso0 commented Mar 5, 2023

Unrelated, this happens every time after recreating the menubar.

QApplication::setAttribute(
Qt::AA_DontUseNativeMenuBar,
args.getStartInFullscreen() || fullscreenPref);
#endif // __LINUX__
Copy link
Member

@ronso0 ronso0 Mar 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for future improvements:
it looks like this attribute is the only way to figure whether a platform uses native menus, so let's note the initial state before setting it. That way we could restrict recreating and reconnecting the menu to platforms that actually require/support it.

@ronso0
Copy link
Member

ronso0 commented Mar 6, 2023

Which one is leaked?

All VisibilityControlConnections in the View menu.

Anyway, that's no regression.
Gonna merge this now.

Thank you very much for digging!

@ronso0
Copy link
Member

ronso0 commented Mar 6, 2023

Gonna merge this now.

I'm about to add/refine the related comments.

@ronso0
Copy link
Member

ronso0 commented Mar 6, 2023

I'm about to add/refine the related comments.

634cb44

@daschuer
Copy link
Member Author

daschuer commented Mar 7, 2023

Thank you. I have cherry picked the linked commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants