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

[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]

171. By Jim Hodapp on 2015-12-17

Fix formatting of 2 lines

[Alfonso Sanchez-Beato (alfonsosanchezbeato)]

Some issues, mostly related to handling this in the caller of these functions instead so errors can be returned in the DBus response. An example where this is already performed is in ServiceSkeleton::handle_set_current_player().

The only exception is probably the handler of player->on_client_disconnected(), as the trigger is not a DBus function in that case.

review: Needs Fixing

[Alfonso Sanchez-Beato (alfonsosanchezbeato)]

Ouch, I forgot to save the inline comments. Anyway, the idea is to handle this returning a DBus error where possible, see previous comment.

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

172. By Jim Hodapp on 2015-12-17

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

[Jim Hodapp (jhodapp)]

Addressed the review comments.

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

173. 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

=== 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:22:27 +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;
+            }
         });
     });
 
@@ -246,7 +255,7 @@
   return std::shared_ptr<media::Player>();
 }
 
-void media::ServiceImplementation::set_current_player(Player::PlayerKey key)
+void media::ServiceImplementation::set_current_player(Player::PlayerKey)
 {
   // no impl
 }
@@ -261,13 +270,15 @@
         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)
         d->configuration.player_store->set_current_player_for_key(key);
 
-    d->configuration.player_store->enumerate_players([current_player, key](const media::Player::PlayerKey& other_key, const std::shared_ptr<media::Player>& other_player)
+    d->configuration.player_store->enumerate_players(
+            [current_player, key](const media::Player::PlayerKey& other_key,
+                                  const std::shared_ptr<media::Player>& other_player)
     {
         // Only pause a Player if all of the following criteria are met:
         // 1) currently playing
@@ -307,7 +318,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 +344,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-11-13 15:26:35 +0000
+++ src/core/media/service_skeleton.cpp	2015-12-18 16:22:27 +0000
@@ -450,27 +450,55 @@
         msg->reader() >> key;
 
         core::dbus::Message::Ptr reply;
-        if (not configuration.player_store->has_player_for_key(key)) {
-            cerr <<  __PRETTY_FUNCTION__ << " player key not found - " << key << endl;
+        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 {
-            impl->set_current_player(key);
+        }
+        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);
+    }
+
+    void handle_pause_other_sessions(const core::dbus::Message::Ptr& msg)
+    {
+        Player::PlayerKey key;
+        msg->reader() >> key;
+        core::dbus::Message::Ptr reply;
+        try {
+            impl->pause_other_sessions(key);
             reply = dbus::Message::make_method_return(msg);
         }
-
-        impl->access_bus()->send(reply);
-    }
-
-    void handle_pause_other_sessions(const core::dbus::Message::Ptr& msg)
-    {
-        Player::PlayerKey key;
-        msg->reader() >> key;
-        impl->pause_other_sessions(key);
-
-        auto 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");
+        }
+
         impl->access_bus()->send(reply);
     }
 
@@ -864,7 +892,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);