Merge lp:~thomas-voss/qtubuntu-media/fix-races-for-access-to-destroyed-controls into lp:qtubuntu-media

[Thomas Voß]

Make sure that invocations on probably deleted objects in async callbacks are guarded.
Condense creation/deletion of controls in one place.

Make sure that invocations on probably deleted objects in async callbacks are guarded.
Condense creation/deletion of controls in one place.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:47
http://jenkins.qa.ubuntu.com/job/qtubuntu-media-ci/60/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtubuntu-media-utopic-amd64-ci/11/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtubuntu-media-utopic-armhf-ci/11/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtubuntu-media-utopic-i386-ci/11/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtubuntu-media-ci/60/rebuild

review: Needs Fixing (continuous-integration)

[Michał Sawicz (saviq)]

Code makes sense! Will try it out from a silo, trying to reproduce the crash.

review: Approve

[lp:~thomas-voss/qtubuntu-media/fix-races-for-access-to-destroyed-controls]

48. By Thomas Voß on 2014-10-15

Adjust test setup for stricter checks in ctor.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:48
http://jenkins.qa.ubuntu.com/job/qtubuntu-media-ci/61/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtubuntu-media-utopic-amd64-ci/12
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtubuntu-media-utopic-armhf-ci/12
        deb: http://jenkins.qa.ubuntu.com/job/qtubuntu-media-utopic-armhf-ci/12/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtubuntu-media-utopic-i386-ci/12

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtubuntu-media-ci/61/rebuild

review: Approve (continuous-integration)

[Antti Kaijanmäki (kaijanmaki)]

OK. Looks better to me :)

review: Approve

[Antti Kaijanmäki (kaijanmaki)]

Does not fix bug #1376467.

Seems to fix bug #1380736.

tested on krillin, rtm-proposed image #105

[Michał Sawicz (saviq)]

Confirmed, steps in bug #1376467 are still broken.

[Alberto Aguirre (albaguirre)]

Does not seem to fix #1378311 with the example.qml attached in that bug description.

=== modified file 'src/aal/aalmediaplayerservice.cpp'
--- src/aal/aalmediaplayerservice.cpp	2014-09-22 16:11:46 +0000
+++ src/aal/aalmediaplayerservice.cpp	2014-10-15 10:24:28 +0000
@@ -38,13 +38,22 @@
 
 using namespace std::placeholders;
 
+namespace
+{
 enum {
     OK          = 0,
     NO_ERROR    = 0,
     BAD_VALUE   = -EINVAL,
 };
 
-AalMediaPlayerService *AalMediaPlayerService::m_service = 0;
+core::ubuntu::media::Player::FrameAvailableCb empty_frame_available_cb = [](void*)
+{
+};
+
+core::ubuntu::media::Player::PlaybackCompleteCb empty_playback_complete_cb = [](void*)
+{
+};
+}
 
 AalMediaPlayerService::AalMediaPlayerService(QObject *parent):
     QMediaService(parent),
@@ -61,25 +70,16 @@
      , m_frameDecodeAvg(0)
 #endif
 {
-    m_service = this;
-
     m_hubService = media::Service::Client::instance();
 
     if (!newMediaPlayer())
         qWarning() << "Failed to create a new media player backend. Video playback will not function." << endl;
 
-    m_videoOutput = new AalVideoRendererControl(this);
-    m_mediaPlayerControl = new AalMediaPlayerControl(this);
-
     if (m_hubPlayerSession == NULL)
         return;
 
-    m_hubPlayerSession->set_playback_complete_callback([](void *context)
-    {
-        auto control = static_cast<AalMediaPlayerControl*>(context);
-        control->playbackComplete();
-    },
-    static_cast<void*>(m_mediaPlayerControl));
+    createMediaPlayerControl();
+    createVideoRendererControl();
 
     m_hubPlayerSession->playback_status_changed().connect(
             std::bind(&AalMediaPlayerService::onPlaybackStatusChanged, this, _1));
@@ -87,10 +87,8 @@
 
 AalMediaPlayerService::~AalMediaPlayerService()
 {
-    if (m_mediaPlayerControl != NULL)
-        delete m_mediaPlayerControl;
-    if (m_videoOutput != NULL)
-        delete m_videoOutput;
+    deleteMediaPlayerControl();
+    deleteVideoRendererControl();
 }
 
 QMediaControl *AalMediaPlayerService::requestControl(const char *name)
@@ -98,7 +96,7 @@
     if (qstrcmp(name, QMediaPlayerControl_iid) == 0)
     {
         if (m_mediaPlayerControlRef == 0 && m_mediaPlayerControl == NULL)
-            m_mediaPlayerControl = new AalMediaPlayerControl(this);
+            createMediaPlayerControl();
 
         ++m_mediaPlayerControlRef;
         return m_mediaPlayerControl;
@@ -107,7 +105,7 @@
     if (qstrcmp(name, QVideoRendererControl_iid) == 0)
     {
         if (m_videoOutputRef == 0 && m_videoOutput == NULL)
-            m_videoOutput = new AalVideoRendererControl(this);
+            createVideoRendererControl();
 
         ++m_videoOutputRef;
         return m_videoOutput;
@@ -124,14 +122,7 @@
             --m_mediaPlayerControlRef;
 
         if (m_mediaPlayerControlRef == 0)
-        {
-            if (m_mediaPlayerControl != NULL)
-            {
-                delete m_mediaPlayerControl;
-                m_mediaPlayerControl = NULL;
-                control = NULL;
-            }
-        }
+            deleteMediaPlayerControl();
     }
     else if (control == m_videoOutput)
     {
@@ -139,14 +130,7 @@
             --m_videoOutputRef;
 
         if (m_videoOutputRef == 0)
-        {
-            if (m_videoOutput != NULL)
-            {
-                delete m_videoOutput;
-                m_videoOutput = NULL;
-                control = NULL;
-            }
-        }
+            deleteVideoRendererControl();
     }
 }
 
@@ -202,6 +186,12 @@
             if (context != NULL)
             {
                 auto s = static_cast<AalMediaPlayerService*>(context);
+
+                // We might receive this callback after the control has been released.
+                // In that case, we return early.
+                if (not s->videoOutputControl())
+                    return;
+
 #ifdef MEASURE_PERFORMANCE
                 s->measurePerformance();
 #endif
@@ -453,8 +443,49 @@
     }
 }
 
+void AalMediaPlayerService::createMediaPlayerControl()
+{
+    m_mediaPlayerControl = new AalMediaPlayerControl(this);
+    m_hubPlayerSession->set_playback_complete_callback([](void *context)
+    {
+        auto control = static_cast<AalMediaPlayerControl*>(context);
+        control->playbackComplete();
+    },
+    m_mediaPlayerControl);
+}
+
+void AalMediaPlayerService::createVideoRendererControl()
+{
+    m_videoOutput = new AalVideoRendererControl(this);
+}
+
+void AalMediaPlayerService::deleteMediaPlayerControl()
+{
+    m_hubPlayerSession->set_playback_complete_callback(
+                empty_playback_complete_cb,
+                nullptr);
+
+    delete m_mediaPlayerControl;
+    m_mediaPlayerControl = NULL;
+}
+
+void AalMediaPlayerService::deleteVideoRendererControl()
+{
+    m_hubPlayerSession->set_frame_available_callback(
+                empty_frame_available_cb,
+                nullptr);
+
+    delete m_videoOutput;
+    m_videoOutput = NULL;
+}
+
 void AalMediaPlayerService::onPlaybackStatusChanged(const media::Player::PlaybackStatus &status)
 {
+    // The media player control might have been released prior to this call. For that, we check for
+    // null and return early in that case.
+    if (m_mediaPlayerControl == nullptr)
+        return;
+
     // If the playback status changes from underneath (e.g. GStreamer or media-hub), make sure
     // the app is notified about this so it can change it's status
     switch (status)
@@ -497,6 +528,12 @@
 void AalMediaPlayerService::setPlayer(const std::shared_ptr<core::ubuntu::media::Player> &player)
 {
     m_hubPlayerSession = player;
+
+    createMediaPlayerControl();
+    createVideoRendererControl();
+
+    m_hubPlayerSession->playback_status_changed().connect(
+            std::bind(&AalMediaPlayerService::onPlaybackStatusChanged, this, _1));
 }
 
 void AalMediaPlayerService::setService(const std::shared_ptr<core::ubuntu::media::Service> &service)

=== modified file 'src/aal/aalmediaplayerservice.h'
--- src/aal/aalmediaplayerservice.h	2014-09-22 16:11:46 +0000
+++ src/aal/aalmediaplayerservice.h	2014-10-15 10:24:28 +0000
@@ -83,8 +83,6 @@
 
     void pushPlaylist();
 
-    static AalMediaPlayerService *instance() { return m_service; }
-
     /* This is for unittest purposes to be able to set a mock-object version of a
      * player object */
     void setPlayer(const std::shared_ptr<core::ubuntu::media::Player> &player);
@@ -101,9 +99,14 @@
 #endif
 
 private:
+    void createMediaPlayerControl();
+    void createVideoRendererControl();
+
+    void deleteMediaPlayerControl();
+    void deleteVideoRendererControl();
+
     void onPlaybackStatusChanged(const core::ubuntu::media::Player::PlaybackStatus &status);
 
-    static AalMediaPlayerService *m_service;
     std::shared_ptr<core::ubuntu::media::Service> m_hubService;
     std::shared_ptr<core::ubuntu::media::Player> m_hubPlayerSession;
 

=== modified file 'src/aal/aalmediaplayerserviceplugin.cpp'
--- src/aal/aalmediaplayerserviceplugin.cpp	2014-03-26 16:08:35 +0000
+++ src/aal/aalmediaplayerserviceplugin.cpp	2014-10-15 10:24:28 +0000
@@ -27,7 +27,7 @@
 QMediaService* AalServicePlugin::create(QString const& key)
 {
     if (key == QLatin1String(Q_MEDIASERVICE_MEDIAPLAYER))
-        return new AalMediaPlayerService;
+        return new AalMediaPlayerService();
     else
         qWarning() << "Key not supported:" << key;
 

=== modified file 'unittests/tst_mediaplayerplugin.cpp'
--- unittests/tst_mediaplayerplugin.cpp	2014-04-25 21:12:56 +0000
+++ unittests/tst_mediaplayerplugin.cpp	2014-10-15 10:24:28 +0000
@@ -67,10 +67,10 @@
 {
     m_hubService.reset(new TestService());
     m_service.reset(new AalMediaPlayerService(this));
-    m_service->setService(m_hubService);
-    m_mediaPlayerControl = m_service->mediaPlayerControl();
+    m_service->setService(m_hubService);    
     m_player.reset(new TestPlayer());
     m_service->setPlayer(m_player);
+    m_mediaPlayerControl = m_service->mediaPlayerControl();
     m_playerControl = m_service->requestControl(QMediaPlayerControl_iid);
     QVERIFY(m_playerControl != NULL);
     m_rendererControl = m_service->requestControl(QVideoRendererControl_iid);