Differential D4301 Diff 10713 dataengines/statusnotifieritem/statusnotifieritemsource.cpp

Changeset View

Standalone View

View Options

dataengines/statusnotifieritem/statusnotifieritemsource.cpp

Show First 20 Lines • Show All 463 Lines • ▼ Show 20 Line(s)422{
464 // additionally added pixmaps". 464​ // additionally added pixmaps".
465 *icon = tmp; 465​ *icon = tmp;
466 //hopefully huge and enormous not necessary right now, since it's quite costly 466​ //hopefully huge and enormous not necessary right now, since it's quite costly
467} 467​}
468 468​
469void StatusNotifierItemSource::activate(int x, int y) 469​void StatusNotifierItemSource::activate(int x, int y)
470{ 470​{
471 if (m_statusNotifierItemInterface && m_statusNotifierItemInterface->isValid()) { 471​ if (m_statusNotifierItemInterface && m_statusNotifierItemInterface->isValid()) {
472 m_statusNotifierItemInterface->call(QDBus::NoBlock, QStringLiteral("Activate"), x, y); 472​ QDBusMessage message = QDBusMessage::createMethodCall(m_statusNotifierItemInterface->service(),
473​ m_statusNotifierItemInterface->path(), m_statusNotifierItemInterface->interface(), QStringLiteral("Activate"));
broulikUnsubmitted
Done

I think that method should return true for success, not failure.

broulik: I think that method should return true for success, not failure.
474​
475​ message << x << y;
476​ QDBusPendingCall call = m_statusNotifierItemInterface->connection().asyncCall(message);
477​ QDBusPendingCallWatcher *watcher = new QDBusPendingCallWatcher(call, this);
478​ connect(watcher, &QDBusPendingCallWatcher::finished, this, &StatusNotifierItemSource::activateCallback);
broulikUnsubmitted
Not Done

This spins its own event loop which is dangerous when dealing with QML

broulik: This spins its own event loop which is dangerous when dealing with QML
romanggAuthorUnsubmitted
Not Done

This spins its own event loop which is dangerous when dealing with QML

Since the event loop is part of the service, isn't the QML shielded from it anyway? The QML code connects only to the job finished signal in the end, which is emitted again by the job's/service's main thread.

romangg: > This spins its own event loop which is dangerous when dealing with QML Since the event loop…
davidedmundsonUnsubmitted
Not Done

Since the event loop is part of the service, isn't the QML shielded from it anyway? The QML code connects only to the job finished signal in the end, which is emitted again by the job's/service's main thread.

If only :)

We spawn a new event loop here, from inside this event loop we continue processing animations, mouse, DBus whatever.
During this, we might end up deleting the SNI and implicitly this datasource.

We then finish this method, potentially 30 seconds after we started, and come out of this line.

"this" is now a danging pointer and the m_statusNotifierItemInterface->interface on the next line seg faults.

This is a nice simple explanation of a simpler version of the same problem:
https://blogs.kde.org/2009/03/26/how-crash-almost-every-qtkde-application-and-how-fix-it-0

only in QML it's even worse! Even if you did guard against it, QMLEngine then returns with the nicely wrapped JS value to a javascript context that doesn't exist - and there's no way to guard that.

davidedmundson: >Since the event loop is part of the service, isn't the QML shielded from it anyway? The QML…
davidedmundsonUnsubmitted
Not Done

we could just move the the calling ContextMenu on failure logic into the C++

It saves this blocking, fixes the bug for all SNI datasource users, and allows us to act differently depending on what the DBus error actually is

davidedmundson: we could just move the the calling ContextMenu on failure logic into the C++ It saves this…
473 } 479​ }
474} 480​}
broulikUnsubmitted
Not Done

Can this lead to false positives? Maybe we need a tri-state here, ie. activate succeeded, activate failed (fall back to menu), nothing to activate (do nothing).

broulik: Can this lead to false positives? Maybe we need a tri-state here, ie. activate succeeded…
romanggAuthorUnsubmitted
Not Done

Imo if "nothing activates", there should be still always the attempt to open the context menu. The user expects that atleast "something" happens on left click.

But in reality if we're at this point, this won't work either, since there is a general problem with m_statusNotifierItemInterface, which means the context menu won't work either.

romangg: Imo if "nothing activates", there should be still always the attempt to open the context menu.
475 481​
482​void StatusNotifierItemSource::activateCallback(QDBusPendingCallWatcher *call)
483​{
484​ QDBusPendingReply<void> reply = *call;
485​ emit activateResult(!reply.isError());
486​ call->deleteLater();
487​}
488​
476void StatusNotifierItemSource::secondaryActivate(int x, int y) 489​void StatusNotifierItemSource::secondaryActivate(int x, int y)
477{ 490​{
478 if (m_statusNotifierItemInterface && m_statusNotifierItemInterface->isValid()) { 491​ if (m_statusNotifierItemInterface && m_statusNotifierItemInterface->isValid()) {
479 m_statusNotifierItemInterface->call(QDBus::NoBlock, QStringLiteral("SecondaryActivate"), x, y); 492​ m_statusNotifierItemInterface->call(QDBus::NoBlock, QStringLiteral("SecondaryActivate"), x, y);
480 } 493​ }
481} 494​}
482 495​
483void StatusNotifierItemSource::scroll(int delta, const QString &direction) 496​void StatusNotifierItemSource::scroll(int delta, const QString &direction)
Show All 19 Lines