[SNI] Fix race condition in item registration
ClosedPublic

Authored by kmaterka on Feb 3 2020, 1:09 PM.

Details

Summary

If StatusNotifierItem is registered and then immediately destroyed, it is possible that QDBusServiceWatcher will not emit the serviceUnregistered signal.
Add an additional check to avoid such situations.

BUG: 416652
FIXED-IN: 5.18.0

Test Plan

Telegram should not add empty items.

Diff Detail

Repository
R120 Plasma Workspace
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 22006
Build 22024: arc lint + arc unit
kmaterka requested review of this revision.Feb 3 2020, 1:09 PM
kmaterka created this revision.

It is hard to reproduce, but with enough tries you should get empty items in "hidden" area of system tray. I used this script:

for i in `seq 1 100`; do
        echo $i
        telegram-desktop &
        sleep 2
        killall telegram-desktop
done

Well identified, it is racey.

But the solution seems off

on line 68 we check
on line 75 we implicitly check again
now we're checking a 3rd time!

I think we can instead solve it with some re-arranging

 m_serviceWatcher->addWatchedService(service);
 if (QDBusConnection::sessionBus().interface()->isServiceRegistered(service).value())  {
  // as before except for the moved line
} else {
  m_serviceWatcher->removeWatchedService(service);
}
kmaterka updated this revision to Diff 74929.Feb 3 2020, 3:48 PM

Updated the code. This time I was not event able to reproduce race condition. Is something like this OK?

kmaterka added inline comments.Feb 3 2020, 3:49 PM
statusnotifierwatcher/statusnotifierwatcher.cpp
72

Is this OK? Are mid-method returns allowed?

davidedmundson accepted this revision.Feb 3 2020, 3:53 PM
davidedmundson added inline comments.
statusnotifierwatcher/statusnotifierwatcher.cpp
72

They are

This revision is now accepted and ready to land.Feb 3 2020, 3:53 PM
This revision was automatically updated to reflect the committed changes.