Fix crashes and freezes when Bonjour contact comes on/offLine
Needs ReviewPublic

Authored by jpalecek on Jan 22 2019, 2:16 AM.

Details

Reviewers
None
Group Reviewers
Kopete
Summary

The code of the BonjourAccount class in Kopete uses synchronous
(blocking) forms of the KDNSSD functions, which is a bad thing. It
would be bad enough in itself, however, in case of these functions it
is even worse because these execute an event loop inside them, which
can itself invoke slots of bonjourAccount, possibly invoking
something non-reentrant, like the disconnect code that deletes KDNSSD
objects (whose functions may still be running up in the call
chain). This can be seen in the backtraces of bugs 216021 265440
301743 304338. You can see the functions
(ie. ServiceBrowserPrivate::gotRemoveService) several times in
them.

Moreover these nested calls can freeze the GUI.

I considered making KDNSSD more robust, since it's the KDNSSD code
that usually crashes, but that would still mean the (possibly)
non-reentrant code of Kopete could run nested so this couldn't help
much. This is describe in
comment in Bugzilla. Still, it is a possibility.

The easiest approach is to change BonjourAccount code to use
asynchronous versions of those calls. That way confers several
advantages:

  • no nested event loops
  • no GUI freezes
  • safe against deletion of the objects involved. The signal-slot connections cease to exist when the objects in question are deleted.
  • the async service resolver continues to work after the service is resolved, listening to potential changes (ie. in user name or status)

FIXES: 216021
FIXES: 265440
FIXES: 301743
FIXES: 304338

Diff Detail

Repository
R434 Kopete
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 7396
Build 7414: arc lint + arc unit
jpalecek created this revision.Jan 22 2019, 2:16 AM
Restricted Application added a project: Kopete. · View Herald TranscriptJan 22 2019, 2:16 AM
Restricted Application added a subscriber: kopete-devel. · View Herald Transcript
jpalecek requested review of this revision.Jan 22 2019, 2:16 AM
jpalecek added inline comments.
protocols/bonjour/bonjouraccount.cpp
241

We use a raw pointer value here instead of RemoteService::Ptr, because that is a shared pointer which would create an ownership loop RemoteService -> (via signal-slot connection) lambda -> RemoteService (via captured variable).