Media Hub

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

  1. fix-1479036-trunk
  2. Merge into trunk
Proposed by Jim Hodapp
Status: Merged
Approved by: Alfonso Sanchez-Beato
Approved revision: 162
Merged at revision: 160
Proposed branch: lp:~phablet-team/media-hub/fix-1479036-trunk
Merge into: lp:media-hub
Diff against target: 170 lines (+91/-11)
3 files modified
src/core/media/mpris/service.h (+12/-0)
src/core/media/service_implementation.cpp (+34/-5)
src/core/media/service_skeleton.cpp (+45/-6)
To merge this branch: bzr merge lp:~phablet-team/media-hub/fix-1479036-trunk
Reviewer Review Type Date Requested Status
PS Jenkins bot continuous-integration Needs Fixing
Alfonso Sanchez-Beato Approve
Review via email: mp+280888@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.
lp:~phablet-team/media-hub/fix-1479036-trunk updated
161. By Jim Hodapp

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

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

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)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

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 updated
162. By Jim Hodapp

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

Revision history for this message
Alfonso Sanchez-Beato (alfonsosanchezbeato) wrote :

LGTM

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

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)

Preview Diff

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

Subscribers

People subscribed via source and target branches

to all changes: