[KStatusNotifierItem] Optionaly, do not take menu ownership
AbandonedPublic

Authored by kmaterka on Oct 18 2019, 11:29 AM.

Details

Summary

It is not possible to handle ownership of QMenu the Qt way, because
QMenu takes QWidget as a parent, but KSNI is a QObject. In some
situations (Qt QPA plugin) we don't want QMenu to be destroyed.

CCBUG: 365105

Diff Detail

Repository
R289 KNotifications
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 17895
Build 17913: arc lint + arc unit
kmaterka created this revision.Oct 18 2019, 11:29 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptOct 18 2019, 11:29 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
kmaterka requested review of this revision.Oct 18 2019, 11:29 AM

Continuation of D24667. I know this solution sucks, but I have no idea how this can be done better.

I have another idea, probably even crazier: set "DO_NOT_DELETE" flag as a QObject property. Then in destructor check it:
if (d->m_menu->property("DO_NOT_DELETE") == true) // ignore

Obviously that would be undocumented, ugly hack... Any other idea how to fix bug 365105?

anthonyfieroni added inline comments.
src/kstatusnotifieritemprivate_p.h
158

It should be QPointer<QMenu>, no other changes are needed.

kmaterka added inline comments.Oct 18 2019, 9:19 PM
src/kstatusnotifieritemprivate_p.h
158

Sure, but this is not my code and this is not related. What I want to do here is more like hack/workaround, so it is better to keep it as simple as possible.

anthonyfieroni added inline comments.Oct 19 2019, 6:31 AM
src/kstatusnotifieritemprivate_p.h
158

It's related and you don't need to any hacks here. QPointer takes care of ownership.

src/kstatusnotifieritemprivate_p.h
158

QPointer here won't solve the other hacks needed.

We have MenuSource, Menu and KSNI

QPointer would fix the case of MenuSource still owning the Menu and KSNI lasting longer than the Menu.

But we need things the other way round.

We have MenuSource lasting longer than KSNI we need to supply a menu to the KSNI but keep the menu after the KSNI is deleted. QPointer won't solve that.

(might be worth switching this to a QPointer anyway, now that ksni doesn't control the menu lifespan though)

I don't exactly like it, but I don't have anything better. +1

kmaterka added inline comments.Oct 19 2019, 10:18 AM
src/kstatusnotifieritemprivate_p.h
158

I will wrap m_menu in QPointer. When mane "ownership" is outside of the KSNI this is a valid case when menu is deleted before KSNI. That's a good practice anyway.

kmaterka updated this revision to Diff 68292.Oct 19 2019, 1:32 PM

Wrap menu in QPointer

kmaterka marked 5 inline comments as done.Oct 19 2019, 1:38 PM

I don't exactly like it, but I don't have anything better. +1

Me neither. Probably the best would be to use shared pointer, but it is not backward compatible.

anthonyfieroni added inline comments.Oct 19 2019, 2:30 PM
src/kstatusnotifieritem.cpp
454

Do not take ownership of the menu and delete it when it does not have a parent. takeOwnership is wrong approach, you can remove it.

kmaterka added inline comments.Oct 19 2019, 3:12 PM
src/kstatusnotifieritem.cpp
454

In theory that should be the correct approach, the "Qt way". But we have existing contract (also discussed in D24667), documentation is clear, menu ownership is always transferred and menu removed, regardless of the parent (please check my comment in line 790, parent might be null or might not be null).

790

"associatedWidget" is a KSNI parent (line 780). It might be or might not be set. If parent is not set, then "associatedWidget" is null and QMenu does not have parent. This is fine, we will delete it. But if there is parent then menu won't be deleted and we will have memory leak. Eventually this menu will be deleted, when parent is destroyed, but current contract is different.
To make things even worse, associatedWidget can change, so we can't compare the parent of the menu with associatedWidget during the destruction...
Let's say we will change it to:

new QMenu()

Then it will be removed, because there is no parent. It should not have any important side effects. So far so good.

What we want to achieve is have an ability to retake ownership after it is passed to setContextMenu. With your approach, it could be done this way:
QMenu *menu =new QMenu(); // null parent, doesn't matter here
tray->setContextMenu(menu);
menu->setParent(parent);
This way menu won't be deleted. There are two problems with this approach:

  • we don't know if no-one is doing that - most probably not and this can be ignored
  • the parent needs to be a QWidget type. This is serious issue, because there are cases when it is not possible.

The final goal is to fix KDEPlatformSystemTrayIcon and there is no QWidget to use as a parent :( Exactly:
kdeplatformsystemtrayicon.cpp:339

m_sni->setContextMenu(ourMenu->menu());

ourMenu can live longer than KStatusNotifierItem *m_sni and can be reused for another KStatusNotifierItem instance. The situation is described in BUG 365105. In other word, in QPA, menu can and should live independently to system tray icon, which is not the case in KStatusNotifierItem.

I really like your idea! Maybe I'm missing something obvious and is possible to set parent in KDEPlatformSystemTrayIcon...

anthonyfieroni added inline comments.Oct 19 2019, 3:37 PM
src/kstatusnotifieritem.cpp
790

That's easy checkable

if (!qApp->closingDown() && (!d->menu->parent() || d->menu->parent() == d->associatedWidget) {
    delete d->menu;
}

We should not own the menu, that's not tight approach at least.

kmaterka added inline comments.Oct 19 2019, 4:14 PM
src/kstatusnotifieritem.cpp
790

This check is not reliable, assosiatedWidget can change. Anyway, this doesn't matter here.
Did you read whole comment? Probably KSNI should not own the menu but it is doing that for 10 (more?) years. It is even documented in the API.
Your idea will not fix the main issue, we can't set a parent to menu in KDEPlatformSystemTrayIcon. Main purpose of this hack is to prevent deletion of menu when it is *not* possible to set parent.

anthonyfieroni added inline comments.Oct 19 2019, 6:38 PM
src/kstatusnotifieritem.cpp
790

Probably KSNI should not own the menu
Yes, widget that creates the menu should, like in bug report example. Make the changes here, then we should find a way to parent the menu, which is the right approach.


That should be fine, in QPA we have a tray icon per sni, update menu should be on same object which will not be a problem, check it.

That should be fine, in QPA we have a tray icon per sni, update menu should be on same object which will not be a problem, check it.

There are two objects in QPA that live independently:

  • KDEPlatformSystemTrayIcon (QPlatformSystemTrayIcon), with KSNI instance, KSNI and KDEPlatformSystemTrayIcon are destroyed on QSystemTrayIcon->hide() and new instance (with new KSNI) is created on QSystemTrayIcon->show()
  • SystemTrayMenu (QPlatformMenu) is not destroyed on QSystemTrayIcon->hide() and will be reused later on QSystemTrayIcon->show()

kdeplatformsystemtrayicon.cpp#L339

void KDEPlatformSystemTrayIcon::updateMenu(QPlatformMenu *menu)
{
    //...
    if (SystemTrayMenu *ourMenu = qobject_cast<SystemTrayMenu*>(menu)) {
        m_sni->setContextMenu(ourMenu->menu());
    }
}

About you patch: I understand your idea, but it changes API contract and is not backward-compatible. Current documentation says:

The KStatusNotifierItem instance takes ownership of the menu, and will delete it upon its destruction.

This is quite clear, I want to be really careful here - I don't want to be blamed for memory leaks :) I think that we need to keep:

d->menu->setParent(nullptr);

in setContextMenu.

Then, to prevent menu deletion, developer needs to explicitly retake ownership, for example:

QMenu *menu = ourMenu->menu()
QWidget *parent = menu->parent();
m_sni->setContextMenu(menu);
menu->setParent(parent);

The problem is that SystemTrayMenu->menu() has no parent and there is no QWidget to use...

That should be fine, in QPA we have a tray icon per sni, update menu should be on same object which will not be a problem, check it.

There are two objects in QPA that live independently:

  • KDEPlatformSystemTrayIcon (QPlatformSystemTrayIcon), with KSNI instance, KSNI and KDEPlatformSystemTrayIcon are destroyed on QSystemTrayIcon->hide() and new instance (with new KSNI) is created on QSystemTrayIcon->show()
  • SystemTrayMenu (QPlatformMenu) is not destroyed on QSystemTrayIcon->hide() and will be reused later on QSystemTrayIcon->show()

    kdeplatformsystemtrayicon.cpp#L339

    ` void KDEPlatformSystemTrayIcon::updateMenu(QPlatformMenu *menu) { //... if (SystemTrayMenu *ourMenu = qobject_cast<SystemTrayMenu*>(menu)) { m_sni->setContextMenu(ourMenu->menu()); } } `

    About you patch: I understand your idea, but it changes API contract and is not backward-compatible. Current documentation says:

The KStatusNotifierItem instance takes ownership of the menu, and will delete it upon its destruction.

This is quite clear, I want to be really careful here - I don't want to be blamed for memory leaks :) I think that we need to keep:

First, it will not have memory leaks, we take ownership on parentless menu, on menu that has a parent, it will destroy it by parent-child cleanups.
I want to be more clear why SystemTrayMenu is not destroyed on hide, the idea behind that code is to be created a new try menu. On updateMenu you call it by same object that's why it's not destroyed, did you have stack strace, that's not normal to me.

First, it will not have memory leaks, we take ownership on parentless menu, on menu that has a parent, it will destroy it by parent-child cleanups.

Yes, eventually menu will be deleted. I'm thinking about the situation when someone has:

if (configuration-trayIconEnabled) {
   m_sni = new KStatusNotifierItem();
   m_sni->setContextMenu(new QMenu(**this**));
} else {
  delete m_sni;
}

and user is playing with this setting and changes it 100 times during one session. It will create 100 QMenu instances.

I want to be more clear why SystemTrayMenu is not destroyed on hide, the idea behind that code is to be created a new try menu. On updateMenu you call it by same object that's why it's not destroyed, did you have stack strace, that's not normal to me.

That's how QSystemTrayIcon works, I checked the source code. QSystemTrayIcon::show() and QSystemTrayIcon::hide will create and destroy KDEPlatformSystemTrayIcon (this is a little bit more complicated, because there are also two additional methods involved: init and cleanup).
For menu, QSystemTrayIcon uses:

QPlatformMenu* KDEPlatformSystemTrayIcon::createMenu()

which is kept alive until QSystemTrayIcon instance exist.
So something like this:

QSystemTrayIcon *sysTray = new QSystemTrayIcon(this);
sysTray->setContextMenu(new QMenu()); // create QPlatformMenu (AFAIR it is postponed, but you get the idea)
sysTray->show(); // create QPlatformSystemTrayIcon
sysTray->hide(); // delete QPlatformSystemTrayIcon
sysTray->show(); // create second QPlatformSystemTrayIcon and reuse QPlatformMenu

Will create two instances of KDEPlatformSystemTrayIcon (QPlatformSystemTrayIcon) and only one of SystemTrayMenu (QPlatformMenu).

Anyway, this whole "do not take ownership" is a dead end. Even if menu is not deleted, it won't work, there is another issue in dbusmenu-qt library... I abandon this idea, sorry for taking your time. I have another solution, in:

void KDEPlatformSystemTrayIcon::updateMenu(QPlatformMenu *menu)
{
    if (SystemTrayMenu *ourMenu = qobject_cast<SystemTrayMenu*>(menu)) {
        m_sni->setContextMenu(ourMenu->menu());
    }
}

SystemTrayMenu::menu() will return new QMenu and keep track of changes.

kmaterka abandoned this revision.Oct 21 2019, 2:47 PM

This is a dead end, I will implement different solution in QPA (KDEPlatformSystemTrayIcon) plugin.