Fix SoundEffect(pulseaudio) crash in qfeedbackmmk auto test

Task-Number: QTBUG-22779

Some pulseaudio callback may happen after SoundEffect was deleted,
thus the userdata(SoundEffect point) we passed previously may
result in potential crash with QMetaObject::invokeMethod to queue some event.

To solve this problem, the release mehtod is added to SoundEffectPrivate,
and instead of calling d->deleteLater in SoundEffect::dtor,
 d->release is called. So SoundEffectPrivate will no when it is going to
be deleted soon rather than handle everything in SoundEffectPrivate::dtor
which may be too late.

class RefObject is also added to be able to track the SoundEffectPrivate
status by pulseaduio callbacks. I thought this could be avoided by checking
the connection state of pulse stream. However, that doesn't work as expected,
stream state remains Ready when checked in callbacks even after disconnect
stream has been called. So RefObject is used instead and its lifecycle is
managed by an internal reference count.

When release is invoked,
m_ref->onDeleted is called first, this will mark SoundEffectPrivate as dead.
and then unloadPulseStream is called.
After those two invocations, we can be asured that:
1. if some pulse callbacks has been called without knowing
 SoundEffectPrivate dead, the queued invocation on SoundEffectPrivate
would be safe, since SoundEffectPrivate::deleteLater would
 be called after them.

2. Since on pulse callbacks would be executed when unloadPulseStream is called,
then at this moment if some pulse callbacks is called again, it would certainly
knows that SoundEffectPrivate is marked as dead and would not queue and
event on SoundEffectPrivate.

Now, the deleteLater can be safely called.

Change-Id: I807f29cddb677d1f4bc078fd306ed0d83d6f7dc4
Reviewed-by: Ling Hu <ling.hu@nokia.com>

diff --git a/src/multimedia/effects/qsoundeffect.cpp b/src/multimedia/effects/qsoundeffect.cpp
index 7cfcdce709a626d3cad4c732d3c3f77fb0d64586..19021fd0cea20f2a02ce38e773cc85be2058303b 100644 (file)
--- a/src/multimedia/effects/qsoundeffect.cpp
+++ b/src/multimedia/effects/qsoundeffect.cpp
@@ -173,7 +173,7 @@ QSoundEffect::QSoundEffect(QObject *parent) :
 
 QSoundEffect::~QSoundEffect()
 {
-    d->deleteLater();
+    d->release();
 }
 
 QStringList QSoundEffect::supportedMimeTypes()

diff --git a/src/multimedia/effects/qsoundeffect_pulse_p.cpp b/src/multimedia/effects/qsoundeffect_pulse_p.cpp
index da617287a0ce553e549e0fd367ee42ea94ee7651..aa88f4c4b9e3fd9ea4346964ba2e7627046dae23 100644 (file)
--- a/src/multimedia/effects/qsoundeffect_pulse_p.cpp
+++ b/src/multimedia/effects/qsoundeffect_pulse_p.cpp
@@ -276,6 +276,67 @@ public:
 };
 }
 
+class QSoundEffectRef
+{
+public:
+    QSoundEffectRef(QSoundEffectPrivate *target)
+        : m_ref(1)
+        , m_target(target)
+    {
+#ifdef QT_PA_DEBUG
+        qDebug() << "QSoundEffectRef(" << this << ") ctor";
+#endif
+    }
+
+    QSoundEffectRef *getRef()
+    {
+#ifdef QT_PA_DEBUG
+        qDebug() << "QSoundEffectRef(" << this << ") getRef";
+#endif
+        QMutexLocker locker(&m_mutex);
+        m_ref++;
+        return this;
+    }
+
+    void release()
+    {
+#ifdef QT_PA_DEBUG
+        qDebug() << "QSoundEffectRef(" << this << ") Release";
+#endif
+        m_mutex.lock();
+        --m_ref;
+        if (m_ref == 0) {
+            m_mutex.unlock();
+#ifdef QT_PA_DEBUG
+            qDebug() << "QSoundEffectRef(" << this << ") deleted";
+#endif
+            delete this;
+            return;
+        }
+        m_mutex.unlock();
+    }
+
+    QSoundEffectPrivate* soundEffect() const
+    {
+        QMutexLocker locker(&m_mutex);
+        return m_target;
+    }
+
+    void notifyDeleted()
+    {
+#ifdef QT_PA_DEBUG
+        qDebug() << "QSoundEffectRef(" << this << ") notifyDeleted";
+#endif
+        QMutexLocker locker(&m_mutex);
+        m_target = NULL;
+    }
+
+private:
+    int m_ref;
+    mutable QMutex m_mutex;
+    QSoundEffectPrivate *m_target;
+};
+
 QSoundEffectPrivate::QSoundEffectPrivate(QObject* parent):
     QObject(parent),
     m_pulseStream(0),
@@ -293,15 +354,28 @@ QSoundEffectPrivate::QSoundEffectPrivate(QObject* parent):
     m_sample(0) ,
     m_position(0)
 {
+    m_ref = new QSoundEffectRef(this);
     pa_sample_spec_init(&m_pulseSpec);
 }
 
-QSoundEffectPrivate::~QSoundEffectPrivate()
+void QSoundEffectPrivate::release()
 {
+#ifdef QT_PA_DEBUG
+    qDebug() << this << "release";
+#endif
+    m_ref->notifyDeleted();
     unloadPulseStream();
-
-    if (m_sample)
+    if (m_sample) {
         m_sample->release();
+        m_sample = 0;
+    }
+
+    this->deleteLater();
+}
+
+QSoundEffectPrivate::~QSoundEffectPrivate()
+{
+    m_ref->release();
 }
 
 QStringList QSoundEffectPrivate::supportedMimeTypes()
@@ -396,7 +470,7 @@ void QSoundEffectPrivate::updateVolume()
     PulseDaemonLocker locker;
     pa_cvolume volume;
     volume.channels = m_pulseSpec.channels;
-    pa_operation_unref(pa_context_set_sink_input_volume(daemon()->context(), m_sinkInputId, daemon()->calcVolume(&volume, m_volume), setvolume_callback, this));
+    pa_operation_unref(pa_context_set_sink_input_volume(daemon()->context(), m_sinkInputId, daemon()->calcVolume(&volume, m_volume), setvolume_callback, m_ref->getRef()));
     Q_ASSERT(pa_cvolume_valid(&volume));
 #ifdef QT_PA_DEBUG
     qDebug() << this << "updateVolume =" << pa_cvolume_max(&volume);
@@ -420,9 +494,9 @@ void QSoundEffectPrivate::updateMuted()
     if (m_sinkInputId < 0)
         return;
     PulseDaemonLocker locker;
-    pa_operation_unref(pa_context_set_sink_input_mute(daemon()->context(), m_sinkInputId, m_muted, setmuted_callback, this));
+    pa_operation_unref(pa_context_set_sink_input_mute(daemon()->context(), m_sinkInputId, m_muted, setmuted_callback, m_ref->getRef()));
 #ifdef QT_PA_DEBUG
-    qDebug() << this << "updateMuted = " << daemon()->calcMuted(m_muted);
+    qDebug() << this << "updateMuted = " << m_muted;
 #endif
 }
 
@@ -502,17 +576,20 @@ void QSoundEffectPrivate::play()
 
 void QSoundEffectPrivate::emptyStream()
 {
+#ifdef QT_PA_DEBUG
+    qDebug() << this << "emptyStream";
+#endif
     m_emptying = true;
-    pa_stream_set_write_callback(m_pulseStream, 0, this);
-    pa_stream_set_underflow_callback(m_pulseStream, 0, this);
-    pa_operation_unref(pa_stream_flush(m_pulseStream, stream_flush_callback, this));
+    pa_stream_set_write_callback(m_pulseStream, 0, 0);
+    pa_stream_set_underflow_callback(m_pulseStream, 0, 0);
+    pa_operation_unref(pa_stream_flush(m_pulseStream, stream_flush_callback, m_ref->getRef()));
 }
 
 void QSoundEffectPrivate::emptyComplete()
 {
     PulseDaemonLocker locker;
     m_emptying = false;
-    pa_operation_unref(pa_stream_cork(m_pulseStream, 1, stream_cork_callback, this));
+    pa_operation_unref(pa_stream_cork(m_pulseStream, 1, stream_cork_callback, m_ref->getRef()));
 }
 
 void QSoundEffectPrivate::sampleReady()
@@ -546,7 +623,7 @@ void QSoundEffectPrivate::sampleReady()
             pa_buffer_attr newBufferAttr;
             newBufferAttr = *bufferAttr;
             newBufferAttr.prebuf = m_sample->data().size();
-            pa_stream_set_buffer_attr(m_pulseStream, &newBufferAttr, stream_adjust_prebuffer_callback, this);
+            pa_operation_unref(pa_stream_set_buffer_attr(m_pulseStream, &newBufferAttr, stream_adjust_prebuffer_callback, m_ref->getRef()));
         } else {
             streamReady();
         }
@@ -559,12 +636,12 @@ void QSoundEffectPrivate::sampleReady()
             newBufferAttr.minreq = bufferAttr->tlength / 2;
             newBufferAttr.prebuf = -1;
             newBufferAttr.fragsize = -1;
-            pa_stream_set_buffer_attr(m_pulseStream, &newBufferAttr, stream_reset_buffer_callback, this);
+            pa_operation_unref(pa_stream_set_buffer_attr(m_pulseStream, &newBufferAttr, stream_reset_buffer_callback, m_ref->getRef()));
         } else if (bufferAttr->prebuf > uint32_t(m_sample->data().size())) {
             pa_buffer_attr newBufferAttr;
             newBufferAttr = *bufferAttr;
             newBufferAttr.prebuf = m_sample->data().size();
-            pa_stream_set_buffer_attr(m_pulseStream, &newBufferAttr, stream_adjust_prebuffer_callback, this);
+            pa_operation_unref(pa_stream_set_buffer_attr(m_pulseStream, &newBufferAttr, stream_adjust_prebuffer_callback, m_ref->getRef()));
         } else {
             streamReady();
         }
@@ -801,7 +878,7 @@ void QSoundEffectPrivate::contextReady()
 void QSoundEffectPrivate::stream_write_callback(pa_stream *s, size_t length, void *userdata)
 {
     Q_UNUSED(length);
-    Q_UNUSED(s)
+    Q_UNUSED(s);
 
     QSoundEffectPrivate *self = reinterpret_cast<QSoundEffectPrivate*>(userdata);
 #ifdef QT_PA_DEBUG
@@ -825,7 +902,7 @@ void QSoundEffectPrivate::stream_state_callback(pa_stream *s, void *userdata)
                 pa_buffer_attr newBufferAttr;
                 newBufferAttr = *bufferAttr;
                 newBufferAttr.prebuf = self->m_sample->data().size();
-                pa_stream_set_buffer_attr(self->m_pulseStream, &newBufferAttr, stream_adjust_prebuffer_callback, userdata);
+                pa_stream_set_buffer_attr(self->m_pulseStream, &newBufferAttr, stream_adjust_prebuffer_callback, self->m_ref->getRef());
             } else {
                 QMetaObject::invokeMethod(self, "streamReady", Qt::QueuedConnection);
             }
@@ -851,10 +928,18 @@ void QSoundEffectPrivate::stream_state_callback(pa_stream *s, void *userdata)
 
 void QSoundEffectPrivate::stream_reset_buffer_callback(pa_stream *s, int success, void *userdata)
 {
+#ifdef QT_PA_DEBUG
+    qDebug() << "stream_reset_buffer_callback";
+#endif
     Q_UNUSED(s);
+    QSoundEffectRef *ref = reinterpret_cast<QSoundEffectRef*>(userdata);
+    QSoundEffectPrivate *self = ref->soundEffect();
+    ref->release();
+    if (!self)
+        return;
+
     if (!success)
         qWarning("QSoundEffect(pulseaudio): faild to reset buffer attribute");
-    QSoundEffectPrivate *self = reinterpret_cast<QSoundEffectPrivate*>(userdata);
 #ifdef QT_PA_DEBUG
     qDebug() << self << "stream_reset_buffer_callback";
 #endif
@@ -872,10 +957,18 @@ void QSoundEffectPrivate::stream_reset_buffer_callback(pa_stream *s, int success
 
 void QSoundEffectPrivate::stream_adjust_prebuffer_callback(pa_stream *s, int success, void *userdata)
 {
+#ifdef QT_PA_DEBUG
+    qDebug() << "stream_adjust_prebuffer_callback";
+#endif
     Q_UNUSED(s);
+    QSoundEffectRef *ref = reinterpret_cast<QSoundEffectRef*>(userdata);
+    QSoundEffectPrivate *self = ref->soundEffect();
+    ref->release();
+    if (!self)
+        return;
+
     if (!success)
         qWarning("QSoundEffect(pulseaudio): faild to adjust pre-buffer attribute");
-    QSoundEffectPrivate *self = reinterpret_cast<QSoundEffectPrivate*>(userdata);
 #ifdef QT_PA_DEBUG
     qDebug() << self << "stream_adjust_prebuffer_callback";
 #endif
@@ -884,10 +977,18 @@ void QSoundEffectPrivate::stream_adjust_prebuffer_callback(pa_stream *s, int suc
 
 void QSoundEffectPrivate::setvolume_callback(pa_context *c, int success, void *userdata)
 {
+#ifdef QT_PA_DEBUG
+    qDebug() << "setvolume_callback";
+#endif
     Q_UNUSED(c);
     Q_UNUSED(userdata);
+    QSoundEffectRef *ref = reinterpret_cast<QSoundEffectRef*>(userdata);
+    QSoundEffectPrivate *self = ref->soundEffect();
+    ref->release();
+    if (!self)
+        return;
 #ifdef QT_PA_DEBUG
-    qDebug() << reinterpret_cast<QSoundEffectPrivate*>(userdata) << "setvolume_callback";
+    qDebug() << self << "setvolume_callback";
 #endif
     if (!success) {
         qWarning("QSoundEffect(pulseaudio): faild to set volume");
@@ -896,10 +997,18 @@ void QSoundEffectPrivate::setvolume_callback(pa_context *c, int success, void *u
 
 void QSoundEffectPrivate::setmuted_callback(pa_context *c, int success, void *userdata)
 {
+#ifdef QT_PA_DEBUG
+    qDebug() << "setmuted_callback";
+#endif
     Q_UNUSED(c);
     Q_UNUSED(userdata);
+    QSoundEffectRef *ref = reinterpret_cast<QSoundEffectRef*>(userdata);
+    QSoundEffectPrivate *self = ref->soundEffect();
+    ref->release();
+    if (!self)
+        return;
 #ifdef QT_PA_DEBUG
-    qDebug() << reinterpret_cast<QSoundEffectPrivate*>(userdata) << "setmuted_callback";
+    qDebug() << self << "setmuted_callback";
 #endif
     if (!success) {
         qWarning("QSoundEffect(pulseaudio): faild to set muted");
@@ -923,10 +1032,18 @@ void QSoundEffectPrivate::stream_underrun_callback(pa_stream *s, void *userdata)
 
 void QSoundEffectPrivate::stream_cork_callback(pa_stream *s, int success, void *userdata)
 {
+#ifdef QT_PA_DEBUG
+    qDebug() << "stream_cork_callback";
+#endif
     Q_UNUSED(s);
+    QSoundEffectRef *ref = reinterpret_cast<QSoundEffectRef*>(userdata);
+    QSoundEffectPrivate *self = ref->soundEffect();
+    ref->release();
+    if (!self)
+        return;
+
     if (!success)
         qWarning("QSoundEffect(pulseaudio): faild to stop");
-    QSoundEffectPrivate *self = reinterpret_cast<QSoundEffectPrivate*>(userdata);
 #ifdef QT_PA_DEBUG
     qDebug() << self << "stream_cork_callback";
 #endif
@@ -935,10 +1052,18 @@ void QSoundEffectPrivate::stream_cork_callback(pa_stream *s, int success, void *
 
 void QSoundEffectPrivate::stream_flush_callback(pa_stream *s, int success, void *userdata)
 {
+#ifdef QT_PA_DEBUG
+    qDebug() << "stream_flush_callback";
+#endif
     Q_UNUSED(s);
+    QSoundEffectRef *ref = reinterpret_cast<QSoundEffectRef*>(userdata);
+    QSoundEffectPrivate *self = ref->soundEffect();
+    ref->release();
+    if (!self)
+        return;
+
     if (!success)
         qWarning("QSoundEffect(pulseaudio): faild to drain");
-    QSoundEffectPrivate *self = reinterpret_cast<QSoundEffectPrivate*>(userdata);
 #ifdef QT_PA_DEBUG
     qDebug() << self << "stream_flush_callback";
 #endif

diff --git a/src/multimedia/effects/qsoundeffect_pulse_p.h b/src/multimedia/effects/qsoundeffect_pulse_p.h
index eaf78e8dd5efa0034de5096aa1acae162dfc272e..3992617ba75ab5e63cb1c81fcb7d8782acd9fb5e 100644 (file)
--- a/src/multimedia/effects/qsoundeffect_pulse_p.h
+++ b/src/multimedia/effects/qsoundeffect_pulse_p.h
@@ -68,6 +68,7 @@ QT_BEGIN_NAMESPACE
 
 QT_MODULE(Multimedia)
 
+class QSoundEffectRef;
 
 class QSoundEffectPrivate : public QObject
 {
@@ -90,6 +91,8 @@ public:
     bool isPlaying() const;
     QSoundEffect::Status status() const;
 
+    void release();
+
 public Q_SLOTS:
     void play();
     void stop();
@@ -154,6 +157,7 @@ private:
 
     QSample *m_sample;
     int m_position;
+    QSoundEffectRef *m_ref;
 };
 
 QT_END_NAMESPACE

diff --git a/src/multimedia/effects/qsoundeffect_qmedia_p.cpp b/src/multimedia/effects/qsoundeffect_qmedia_p.cpp
index dfd56b09c71f39613e3c05d110b021fb93ab06e3..64a1754a919e22774f84c8b6a3b8ce117cef3fad 100644 (file)
--- a/src/multimedia/effects/qsoundeffect_qmedia_p.cpp
+++ b/src/multimedia/effects/qsoundeffect_qmedia_p.cpp
@@ -76,6 +76,11 @@ QSoundEffectPrivate::QSoundEffectPrivate(QObject* parent):
     connect(m_player, SIGNAL(volumeChanged(int)), SIGNAL(volumeChanged()));
 }
 
+void QSoundEffectPrivate::release()
+{
+    this->deleteLater();
+}
+
 QSoundEffectPrivate::~QSoundEffectPrivate()
 {
 }

diff --git a/src/multimedia/effects/qsoundeffect_qmedia_p.h b/src/multimedia/effects/qsoundeffect_qmedia_p.h
index 3e448f718b853044bb3d4f84cd069269f6d42d62..b93f47ce3d50226cc89ef5c2b336da74fc096590 100644 (file)
--- a/src/multimedia/effects/qsoundeffect_qmedia_p.h
+++ b/src/multimedia/effects/qsoundeffect_qmedia_p.h
@@ -88,6 +88,8 @@ public:
     bool isPlaying() const;
     QSoundEffect::Status status() const;
 
+    void release();
+
 public Q_SLOTS:
     void play();
     void stop();