[QDBusMenuBar] Guard m_window with a QPointer
ClosedPublic

Authored by broulik on Jun 28 2018, 6:32 AM.

Details

Summary

It can be deleted without us knowing for MDI windows as happens in QtCurve config.

BUG: 376340
BUG: 379719
FIXED-IN: 5.12.7

Test Plan
  • Can open QtCurve config dialog without crashing
  • The MDI's window becomes attached to the actual window it is contained within but it's still better than a crash

Ideally we'd listen for the destruction of the QWindow and unregisterMenuBar which should avoid the wrong menu showing but this requires more elaborate reengineering. Also, this MDI stuff is quite funky, so I would say this a Qt bug since QDBusMenuBar came from Qt mostly unaltered

Diff Detail

Repository
R135 Integration for Qt applications in Plasma
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
broulik created this revision.Jun 28 2018, 6:32 AM
Restricted Application added a project: Plasma. · View Herald TranscriptJun 28 2018, 6:32 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
broulik requested review of this revision.Jun 28 2018, 6:32 AM
broulik edited reviewers, added: davidedmundson; removed: davidbekham.
broulik edited the test plan for this revision. (Show Details)Jun 28 2018, 6:36 AM
broulik edited the summary of this revision. (Show Details)Jun 28 2018, 7:52 AM
davidedmundson accepted this revision.Jun 28 2018, 9:45 AM

In reviewing I found a quirky bit of code:

void QDBusMenuBar::handleReparent(QWindow *newParentWindow)
{
    if (newParentWindow && newParentWindow != m_window) {
        m_window = newParentWindow

if we ever got handleReparent with a nullptr, we wouldn't update our m_window. Could this be the root cause?

This revision is now accepted and ready to land.Jun 28 2018, 9:45 AM
broulik updated this revision to Diff 36819.Jun 28 2018, 9:51 AM
  • Always update m_window

QPointer still needed. Could be refactored to also unregister on destruction but that requires storing the WId separately as in QObject::destroyed the ~QWindow already ran and as such winId() cannot be accessed

This revision was automatically updated to reflect the committed changes.