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

Proposed by Jim Hodapp on 2015-12-17
Status: Merged
Approved by: Alfonso Sanchez-Beato on 2015-12-18
Approved revision: 173
Merged at revision: 170
Proposed branch: lp:~phablet-team/media-hub/fix-1479036
Merge into: lp:media-hub/stable
Diff against target: 173 lines (+83/-23)
2 files modified
src/core/media/service_implementation.cpp (+38/-7)
src/core/media/service_skeleton.cpp (+45/-16)
To merge this branch: bzr merge lp:~phablet-team/media-hub/fix-1479036
Reviewer Review Type Date Requested Status
Alfonso Sanchez-Beato 2015-12-17 Approve on 2015-12-18
Review via email: mp+280872@code.launchpad.net

Commit message

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

Description of the change

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

To post a comment you must log in.
171. By Jim Hodapp on 2015-12-17

Fix formatting of 2 lines

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

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

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) wrote :

Addressed the review comments.

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

LGTM

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/core/media/service_implementation.cpp'
2--- src/core/media/service_implementation.cpp 2015-10-14 20:54:19 +0000
3+++ src/core/media/service_implementation.cpp 2015-12-18 16:22:27 +0000
4@@ -210,8 +210,17 @@
5 if (!d->configuration.player_store->has_player_for_key(key))
6 return;
7
8- if (d->configuration.player_store->player_for_key(key)->lifetime() == Player::Lifetime::normal)
9- d->configuration.player_store->remove_player_for_key(key);
10+ try {
11+ if (d->configuration.player_store->player_for_key(key)->lifetime() == Player::Lifetime::normal)
12+ d->configuration.player_store->remove_player_for_key(key);
13+ }
14+ catch (const std::out_of_range &e) {
15+ std::cerr << "Failed to look up Player instance for key " << key
16+ << ", no valid Player instance for that key value. Removal of Player from Player store"
17+ << " might not have completed. This most likely means that media-hub-server has"
18+ << " crashed and restarted." << std::endl;
19+ return;
20+ }
21 });
22 });
23
24@@ -246,7 +255,7 @@
25 return std::shared_ptr<media::Player>();
26 }
27
28-void media::ServiceImplementation::set_current_player(Player::PlayerKey key)
29+void media::ServiceImplementation::set_current_player(Player::PlayerKey)
30 {
31 // no impl
32 }
33@@ -261,13 +270,15 @@
34 return;
35 }
36
37- auto current_player = d->configuration.player_store->player_for_key(key);
38+ const std::shared_ptr<media::Player> current_player = d->configuration.player_store->player_for_key(key);
39
40 // We immediately make the player known as new current player.
41 if (current_player->audio_stream_role() == media::Player::multimedia)
42 d->configuration.player_store->set_current_player_for_key(key);
43
44- d->configuration.player_store->enumerate_players([current_player, key](const media::Player::PlayerKey& other_key, const std::shared_ptr<media::Player>& other_player)
45+ d->configuration.player_store->enumerate_players(
46+ [current_player, key](const media::Player::PlayerKey& other_key,
47+ const std::shared_ptr<media::Player>& other_player)
48 {
49 // Only pause a Player if all of the following criteria are met:
50 // 1) currently playing
51@@ -307,7 +318,17 @@
52 [this, resume_video_sessions](const std::pair<media::Player::PlayerKey, bool> &paused_player_pair) {
53 const media::Player::PlayerKey key = paused_player_pair.first;
54 const bool resume_play_after_phonecall = paused_player_pair.second;
55- auto player = d->configuration.player_store->player_for_key(key);
56+ std::shared_ptr<media::Player> player;
57+ try {
58+ player = d->configuration.player_store->player_for_key(key);
59+ }
60+ catch (const std::out_of_range &e) {
61+ std::cerr << "Failed to look up Player instance for key " << key
62+ << ", no valid Player instance for that key value and cannot automatically resume"
63+ << " paused players. This most likely means that media-hub-server has crashed and"
64+ << " restarted." << std::endl;
65+ return;
66+ }
67 // Only resume video playback if explicitly desired
68 if ((resume_video_sessions || player->is_audio_source()) && resume_play_after_phonecall)
69 player->play();
70@@ -323,7 +344,17 @@
71 if (not d->configuration.player_store->has_player_for_key(d->resume_key))
72 return;
73
74- auto player = d->configuration.player_store->player_for_key(d->resume_key);
75+ std::shared_ptr<media::Player> player;
76+ try {
77+ player = d->configuration.player_store->player_for_key(d->resume_key);
78+ }
79+ catch (const std::out_of_range &e) {
80+ std::cerr << "Failed to look up Player instance for key " << d->resume_key
81+ << ", no valid Player instance for that key value and cannot automatically resume"
82+ << " paused Player. This most likely means that media-hub-server has crashed and"
83+ << " restarted." << std::endl;
84+ return;
85+ }
86
87 if (player->playback_status() == Player::paused)
88 {
89
90=== modified file 'src/core/media/service_skeleton.cpp'
91--- src/core/media/service_skeleton.cpp 2015-11-13 15:26:35 +0000
92+++ src/core/media/service_skeleton.cpp 2015-12-18 16:22:27 +0000
93@@ -450,27 +450,55 @@
94 msg->reader() >> key;
95
96 core::dbus::Message::Ptr reply;
97- if (not configuration.player_store->has_player_for_key(key)) {
98- cerr << __PRETTY_FUNCTION__ << " player key not found - " << key << endl;
99+ if (not configuration.player_store->has_player_for_key(key))
100+ {
101+ std::cerr << __PRETTY_FUNCTION__ << " player key not found - " << key << std::endl;
102 reply = dbus::Message::make_error(
103 msg,
104 mpris::Service::Errors::PlayerKeyNotFound::name(),
105 "Player key not found");
106- } else {
107- impl->set_current_player(key);
108+ }
109+ else
110+ {
111+ try {
112+ impl->set_current_player(key);
113+ reply = dbus::Message::make_method_return(msg);
114+ }
115+ catch (const std::out_of_range &e) {
116+ std::cerr << "Failed to look up Player instance for key " << key
117+ << ", no valid Player instance for that key value and cannot set current player."
118+ << " This most likely means that media-hub-server has crashed and restarted."
119+ << std::endl;
120+ reply = dbus::Message::make_error(
121+ msg,
122+ mpris::Service::Errors::PlayerKeyNotFound::name(),
123+ "Player key not found");
124+ }
125+ }
126+
127+ impl->access_bus()->send(reply);
128+ }
129+
130+ void handle_pause_other_sessions(const core::dbus::Message::Ptr& msg)
131+ {
132+ Player::PlayerKey key;
133+ msg->reader() >> key;
134+ core::dbus::Message::Ptr reply;
135+ try {
136+ impl->pause_other_sessions(key);
137 reply = dbus::Message::make_method_return(msg);
138 }
139-
140- impl->access_bus()->send(reply);
141- }
142-
143- void handle_pause_other_sessions(const core::dbus::Message::Ptr& msg)
144- {
145- Player::PlayerKey key;
146- msg->reader() >> key;
147- impl->pause_other_sessions(key);
148-
149- auto reply = dbus::Message::make_method_return(msg);
150+ catch (const std::out_of_range &e) {
151+ std::cerr << "Failed to look up Player instance for key " << key
152+ << ", no valid Player instance for that key value and cannot pause other Players."
153+ << " This most likely means that media-hub-server has crashed and restarted."
154+ << std::endl;
155+ reply = dbus::Message::make_error(
156+ msg,
157+ mpris::Service::Errors::PlayerKeyNotFound::name(),
158+ "Player key not found");
159+ }
160+
161 impl->access_bus()->send(reply);
162 }
163
164@@ -864,7 +892,8 @@
165
166 void media::ServiceSkeleton::set_current_player(media::Player::PlayerKey key)
167 {
168- const auto player = d->configuration.player_store->player_for_key(key);
169+ const std::shared_ptr<media::Player> player =
170+ d->configuration.player_store->player_for_key(key);
171 // We only care to allow the MPRIS controls to apply to multimedia player (i.e. audio, video)
172 if (player->audio_stream_role() == media::Player::AudioStreamRole::multimedia)
173 d->exported.set_current_player(player);

Subscribers

People subscribed via source and target branches

to all changes: