Merge lp:~phablet-team/media-hub/fix-1479036-trunk into lp:media-hub

[Jim Hodapp]

Fix bug #1479036 which prevents the out_of_range exception from causing media-hub-server from crashing when a player key is not found

Fix bug #1479036 which prevents the out_of_range exception from causing media-hub-server from crashing when a player key is not found

[lp:~phablet-team/media-hub/fix-1479036-trunk]

161. By Jim Hodapp on 2015-12-17

Make sure to pass on the failed Player key lookup to the client when possible.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:160
http://jenkins.qa.ubuntu.com/job/media-hub-ci/381/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-amd64-ci/221/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-armhf-ci/221/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-i386-ci/221/console

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:161
http://jenkins.qa.ubuntu.com/job/media-hub-ci/382/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-amd64-ci/222/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-armhf-ci/222/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-i386-ci/222/console

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

review: Needs Fixing (continuous-integration)

[lp:~phablet-team/media-hub/fix-1479036-trunk]

162. By Jim Hodapp on 2015-12-18

No need to catch out_of_range in set_current_player since it's handled in handle_set_current_player

[Alfonso Sanchez-Beato (alfonsosanchezbeato)]

LGTM

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:162
http://jenkins.qa.ubuntu.com/job/media-hub-ci/383/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-amd64-ci/223/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-armhf-ci/223/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-i386-ci/223/console

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

review: Needs Fixing (continuous-integration)

=== modified file 'src/core/media/mpris/service.h'
--- src/core/media/mpris/service.h	2015-09-08 20:03:56 +0000
+++ src/core/media/mpris/service.h	2015-12-18 16:24:17 +0000
@@ -107,6 +107,18 @@
                 return s;
             }
         };
+
+        struct PlayerKeyNotFound
+        {
+            static const std::string& name()
+            {
+                static const std::string s
+                {
+                    "core.ubuntu.media.Service.Error.PlayerKeyNotFound"
+                };
+                return s;
+            }
+        };
     };
 
     DBUS_CPP_METHOD_WITH_TIMEOUT_DEF(CreateSession, Service, 1000)

=== modified file 'src/core/media/service_implementation.cpp'
--- src/core/media/service_implementation.cpp	2015-10-14 20:54:19 +0000
+++ src/core/media/service_implementation.cpp	2015-12-18 16:24:17 +0000
@@ -210,8 +210,17 @@
             if (!d->configuration.player_store->has_player_for_key(key))
                 return;
 
-            if (d->configuration.player_store->player_for_key(key)->lifetime() == Player::Lifetime::normal)
-                d->configuration.player_store->remove_player_for_key(key);
+            try {
+                if (d->configuration.player_store->player_for_key(key)->lifetime() == Player::Lifetime::normal)
+                    d->configuration.player_store->remove_player_for_key(key);
+            }
+            catch (const std::out_of_range &e) {
+                std::cerr << "Failed to look up Player instance for key " << key
+                << ", no valid Player instance for that key value. Removal of Player from Player store"
+                << " might not have completed. This most likely means that media-hub-server has"
+                << " crashed and restarted." << std::endl;
+                return;
+            }
         });
     });
 
@@ -261,7 +270,7 @@
         return;
     }
 
-    auto current_player = d->configuration.player_store->player_for_key(key);
+    const std::shared_ptr<media::Player> current_player = d->configuration.player_store->player_for_key(key);
 
     // We immediately make the player known as new current player.
     if (current_player->audio_stream_role() == media::Player::multimedia)
@@ -307,7 +316,17 @@
         [this, resume_video_sessions](const std::pair<media::Player::PlayerKey, bool> &paused_player_pair) {
             const media::Player::PlayerKey key = paused_player_pair.first;
             const bool resume_play_after_phonecall = paused_player_pair.second;
-            auto player = d->configuration.player_store->player_for_key(key);
+            std::shared_ptr<media::Player> player;
+            try {
+                player = d->configuration.player_store->player_for_key(key);
+            }
+            catch (const std::out_of_range &e) {
+                std::cerr << "Failed to look up Player instance for key " << key
+                    << ", no valid Player instance for that key value and cannot automatically resume"
+                    << " paused players. This most likely means that media-hub-server has crashed and"
+                    << " restarted." << std::endl;
+                return;
+            }
             // Only resume video playback if explicitly desired
             if ((resume_video_sessions || player->is_audio_source()) && resume_play_after_phonecall)
                 player->play();
@@ -323,7 +342,17 @@
     if (not d->configuration.player_store->has_player_for_key(d->resume_key))
         return;
 
-    auto player = d->configuration.player_store->player_for_key(d->resume_key);
+    std::shared_ptr<media::Player> player;
+    try {
+        player = d->configuration.player_store->player_for_key(d->resume_key);
+    }
+    catch (const std::out_of_range &e) {
+        std::cerr << "Failed to look up Player instance for key " << d->resume_key
+            << ", no valid Player instance for that key value and cannot automatically resume"
+            << " paused Player. This most likely means that media-hub-server has crashed and"
+            << " restarted." << std::endl;
+        return;
+    }
 
     if (player->playback_status() == Player::paused)
     {

=== modified file 'src/core/media/service_skeleton.cpp'
--- src/core/media/service_skeleton.cpp	2015-10-14 20:54:19 +0000
+++ src/core/media/service_skeleton.cpp	2015-12-18 16:24:17 +0000
@@ -442,9 +442,34 @@
     {
         Player::PlayerKey key;
         msg->reader() >> key;
-        impl->set_current_player(key);
-
-        auto reply = dbus::Message::make_method_return(msg);
+
+        core::dbus::Message::Ptr reply;
+        if (not configuration.player_store->has_player_for_key(key))
+        {
+            std::cerr <<  __PRETTY_FUNCTION__ << " player key not found - " << key << std::endl;
+            reply = dbus::Message::make_error(
+                            msg,
+                            mpris::Service::Errors::PlayerKeyNotFound::name(),
+                            "Player key not found");
+        }
+        else
+        {
+            try {
+                impl->set_current_player(key);
+                reply = dbus::Message::make_method_return(msg);
+            }
+            catch (const std::out_of_range &e) {
+                std::cerr << "Failed to look up Player instance for key " << key
+                    << ", no valid Player instance for that key value and cannot set current player."
+                    << " This most likely means that media-hub-server has crashed and restarted."
+                    << std::endl;
+                reply = dbus::Message::make_error(
+                            msg,
+                            mpris::Service::Errors::PlayerKeyNotFound::name(),
+                            "Player key not found");
+            }
+        }
+
         impl->access_bus()->send(reply);
     }
 
@@ -452,9 +477,22 @@
     {
         Player::PlayerKey key;
         msg->reader() >> key;
-        impl->pause_other_sessions(key);
+        core::dbus::Message::Ptr reply;
+        try {
+            impl->pause_other_sessions(key);
+            reply = dbus::Message::make_method_return(msg);
+        }
+        catch (const std::out_of_range &e) {
+            std::cerr << "Failed to look up Player instance for key " << key
+                << ", no valid Player instance for that key value and cannot pause other Players."
+                << " This most likely means that media-hub-server has crashed and restarted."
+                << std::endl;
+            reply = dbus::Message::make_error(
+                    msg,
+                    mpris::Service::Errors::PlayerKeyNotFound::name(),
+                    "Player key not found");
+        }
 
-        auto reply = dbus::Message::make_method_return(msg);
         impl->access_bus()->send(reply);
     }
 
@@ -848,7 +886,8 @@
 
 void media::ServiceSkeleton::set_current_player(media::Player::PlayerKey key)
 {
-    const auto player = d->configuration.player_store->player_for_key(key);
+    const std::shared_ptr<media::Player> player =
+        d->configuration.player_store->player_for_key(key);
     // We only care to allow the MPRIS controls to apply to multimedia player (i.e. audio, video)
     if (player->audio_stream_role() == media::Player::AudioStreamRole::multimedia)
         d->exported.set_current_player(player);