[Bug 43239] New: Tp::SharedPtr tries to be thread-safe, but secretly isn't

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=43239

             Bug #: 43239
           Summary: Tp::SharedPtr tries to be thread-safe, but secretly
                    isn't
    Classification: Unclassified
           Product: Telepathy
           Version: git master
          Platform: Other
        OS/Version: All
            Status: NEW
          Severity: blocker
          Priority: high
         Component: tp-qt4
        AssignedTo: andrunko at gmail.com
        ReportedBy: ollisal at gmail.com
         QAContact: telepathy-bugs at lists.freedesktop.org


As discovered when porting TpQt4 to Qt5,

    explicit inline SharedPtr(const QWeakPointer<T> &o)
    {
        if (o.data() && o.data()->strongref.fetchAndAddOrdered(0) > 0) {
            d = static_cast<T*>(o.data());
            d->ref();
        }

Used like that, the atomic integer used to represent the reference count of
Tp::RefCounted objects is completely useless. First, after the
o.data() check, an another thread can cause data() to go NULL, and then after
the
refcount check, an another thread can cause the refcount to drop to zero and
the object
be deleted.

The second issue is more severe. The same (by-value) WeakPtr instance doesn't
really need to be accessed safely from multiple threads. But the reference
count part of the RefCounted object pointed to by o.data() needs to, because
multiple QWeakPointer and Tp::SharedPtr value instances can point to it
(exactly one instance).

This is perhaps also why they removed the implicit integer conversion in qt5 -
the whole point of the atomic integer is to be able to implement
inspection/test AND modify in one atomic operation to guard against an another
thread ruining our day in between them. And that removal of API helped us to
notice the pathological usage here.

The resolution can't simply be "even this part of tp-qt4 doesn't need to be
thread-safe". Why then
would we even be using an atomic integer? It'd serve no purpose other than
to slow things down in solely single-threaded usage...

However, passing object refs across threads safely (even if not using the
objects pointed to from multiple threads at a time) IS useful. So we must fix
this before committing this inline implementation to tp-qt >= 0.9 release
headers (that results in an ABI commitment, because all clients will contain
this code, and try to access RefCounted in the exact way presented here so we
can't change it afterwards, in addition to suffering from no thread safety).

I suggest taking a look at other Qt shared pointer impls (webkit qt's ?
qsharedpointers? qexplicitlyshared...'s ?) for ideas on getting the thread
safety right. I expect that to turn out as either a certain pattern of atomic
integer operations, or an explicit locking of the target object for the weak
pointer to strong pointer promotion.

Could that actually be as simple as doing fetchAndAddOrdered(1)? And if that
only yields <= 1 (not < 1, note), consider the object dead and initialize the
SharedPtr as null. If it yields > 1, make the SharedPtr point to the object and
keep the ref.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA Contact for the bug.