File 0001-Fix-MoodbarPipeline-crash-on-gstreamer-err... of Package clementine

Overview Repositories Revisions Requests Users Attributes Meta

File 0001-Fix-MoodbarPipeline-crash-on-gstreamer-error.patch of Package clementine (Revision c19ba2309df02f3a9bd5563275d43d5d)

Currently displaying revision c19ba2309df02f3a9bd5563275d43d5d , Show latest

From 55edcf5321051e44281f067a7e3ee44871982c12 Mon Sep 17 00:00:00 2001
From: Jim Broadus <jbroadus@gmail.com>
Date: Sun, 10 Mar 2019 23:34:11 -0700
Subject: [PATCH] Fix MoodbarPipeline crash on gstreamer error.

As reported in issue 6302, playing a stream that causes gstreamer to error at
start can cause a crash. The problem occurs when the MoodbarPipeline receives a
pad-added signal after it has handled an error callback. In the error callback,
the builder_ is freed. In the pad-added handler (NewPadCallback), this object
is accessed.

This change adds a running_ flag that is set when the pipeline is started and
cleared on an error, end of stream, or object destruction. We check this flag at
the beginning of NewPadCallback. For sanity sake, we also check the builder_
pointer before dereferencing. Note that checking the state of the pipeline
wasn't an option since the pipeline is in the process of changing states during
the pad-added callback and gst_element_get_state wants to block during a state
change.

This solution is not complete as there are still some syncronization issues.
With this specific situation, the error and new pad callbacks appear to always
occur on the same thread, but that's probably not true for all error conditions.
The object is also destroyed by a different thread, so it may be possible that a
callback can occur at the wrong time during or after the deletion of the object.

See https://github.com/clementine-player/Clementine/issues/6302
---
 src/moodbar/moodbarpipeline.cpp | 17 +++++++++++++++--
 src/moodbar/moodbarpipeline.h   |  1 +
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/src/moodbar/moodbarpipeline.cpp b/src/moodbar/moodbarpipeline.cpp
index 7163efda1..bddbf678c 100644
--- a/src/moodbar/moodbarpipeline.cpp
+++ b/src/moodbar/moodbarpipeline.cpp
@@ -37,7 +37,8 @@ MoodbarPipeline::MoodbarPipeline(const QUrl& local_filename)
       local_filename_(local_filename),
       pipeline_(nullptr),
       convert_element_(nullptr),
-      success_(false) {}
+      success_(false),
+      running_(false) {}
 
 MoodbarPipeline::~MoodbarPipeline() { Cleanup(); }
 
@@ -117,6 +118,7 @@ void MoodbarPipeline::Start() {
   gst_object_unref(bus);
 
   // Start playing
+  running_ = true;
   gst_element_set_state(pipeline_, GST_STATE_PLAYING);
 }
 
@@ -135,6 +137,12 @@ void MoodbarPipeline::ReportError(GstMessage* msg) {
 
 void MoodbarPipeline::NewPadCallback(GstElement*, GstPad* pad, gpointer data) {
   MoodbarPipeline* self = reinterpret_cast<MoodbarPipeline*>(data);
+
+  if (!self->running_) {
+    qLog(Warning) << "Received gstreamer callback after pipeline has stopped.";
+    return;
+  }
+
   GstPad* const audiopad =
       gst_element_get_static_pad(self->convert_element_, "sink");
 
@@ -152,7 +160,10 @@ void MoodbarPipeline::NewPadCallback(GstElement*, GstPad* pad, gpointer data) {
   gst_structure_get_int(structure, "rate", &rate);
   gst_caps_unref(caps);
 
-  self->builder_->Init(kBands, rate);
+  if (self->builder_ != nullptr)
+    self->builder_->Init(kBands, rate);
+  else
+    qLog(Error) << "Builder does not exist";
 }
 
 GstBusSyncReply MoodbarPipeline::BusCallbackSync(GstBus*, GstMessage* msg,
@@ -177,6 +188,7 @@ GstBusSyncReply MoodbarPipeline::BusCallbackSync(GstBus*, GstMessage* msg,
 
 void MoodbarPipeline::Stop(bool success) {
   success_ = success;
+  running_ = false;
   if (builder_ != nullptr) {
     data_ = builder_->Finish(1000);
     builder_.reset();
@@ -189,6 +201,7 @@ void MoodbarPipeline::Cleanup() {
   Q_ASSERT(QThread::currentThread() == thread());
   Q_ASSERT(QThread::currentThread() != qApp->thread());
 
+  running_ = false;
   if (pipeline_) {
     GstBus* bus = gst_pipeline_get_bus(GST_PIPELINE(pipeline_));
     gst_bus_set_sync_handler(bus, nullptr, nullptr, nullptr);
diff --git a/src/moodbar/moodbarpipeline.h b/src/moodbar/moodbarpipeline.h
index c7acad8e5..629781d64 100644
--- a/src/moodbar/moodbarpipeline.h
+++ b/src/moodbar/moodbarpipeline.h
@@ -71,6 +71,7 @@ class MoodbarPipeline : public QObject {
   std::unique_ptr<MoodbarBuilder> builder_;
 
   bool success_;
+  bool running_;
   QByteArray data_;
 };
 
-- 
2.16.4

Places

File 0001-Fix-MoodbarPipeline-crash-on-gstreamer-error.patch of Package clementine (Revision c19ba2309df02f3a9bd5563275d43d5d)

Places