Merge lp:~albaguirre/media-hub/fix-using-dead-objects into lp:media-hub

Proposed by Alberto Aguirre
Status: Merged
Approved by: Alberto Aguirre
Approved revision: 75
Merged at revision: 71
Proposed branch: lp:~albaguirre/media-hub/fix-using-dead-objects
Merge into: lp:media-hub
Diff against target: 93 lines (+45/-2)
3 files modified
src/core/media/gstreamer/engine.cpp (+5/-1)
src/core/media/player_implementation.cpp (+26/-0)
src/core/media/player_skeleton.cpp (+14/-1)
To merge this branch: bzr merge lp:~albaguirre/media-hub/fix-using-dead-objects
Reviewer Review Type Date Requested Status
Thomas Voß (community) Approve
PS Jenkins bot continuous-integration Needs Fixing
Jim Hodapp Pending
Review via email: mp+237516@code.launchpad.net

Commit message

Fix potential access to dead objects.

Description of the change

Fix potential access to dead objects.

This fixes crashes seen while stress testing lifecycle by creating/destroying media-player app instances.

It also may fix the linked bugs, but not 100% sure due to the vague stack traces from https://errors.ubuntu.com

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
73. By Alberto Aguirre

Specify functor type for null getter handlers

74. By Alberto Aguirre

avoid state changes after client has been disconnected.

There is a potential race between getting a notification, resetting the pipeline and another instance starting, which will attempt to pause the other player instance for which its client has died.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
75. By Alberto Aguirre

Remove previous workaround - gstreamer sometimes hangs when resetting the pipeline.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Thomas Voß (thomas-voss) wrote :

LGTM.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'src/core/media/gstreamer/engine.cpp'
--- src/core/media/gstreamer/engine.cpp 2014-09-08 10:16:38 +0000
+++ src/core/media/gstreamer/engine.cpp 2014-10-10 16:04:29 +0000
@@ -146,6 +146,10 @@
146 {146 {
147 }147 }
148148
149 // Ensure the playbin is the last item destroyed
150 // otherwise properties could try to access a dead playbin object
151 gstreamer::Playbin playbin;
152
149 std::shared_ptr<Engine::MetaDataExtractor> meta_data_extractor;153 std::shared_ptr<Engine::MetaDataExtractor> meta_data_extractor;
150 core::Property<Engine::State> state;154 core::Property<Engine::State> state;
151 core::Property<std::tuple<media::Track::UriType, media::Track::MetaData>> track_meta_data;155 core::Property<std::tuple<media::Track::UriType, media::Track::MetaData>> track_meta_data;
@@ -155,7 +159,7 @@
155 core::Property<media::Player::AudioStreamRole> audio_role;159 core::Property<media::Player::AudioStreamRole> audio_role;
156 core::Property<bool> is_video_source;160 core::Property<bool> is_video_source;
157 core::Property<bool> is_audio_source;161 core::Property<bool> is_audio_source;
158 gstreamer::Playbin playbin;162
159 core::ScopedConnection about_to_finish_connection;163 core::ScopedConnection about_to_finish_connection;
160 core::ScopedConnection on_state_changed_connection;164 core::ScopedConnection on_state_changed_connection;
161 core::ScopedConnection on_tag_available_connection;165 core::ScopedConnection on_tag_available_connection;
162166
=== modified file 'src/core/media/player_implementation.cpp'
--- src/core/media/player_implementation.cpp 2014-09-25 16:26:13 +0000
+++ src/core/media/player_implementation.cpp 2014-10-10 16:04:29 +0000
@@ -386,6 +386,32 @@
386386
387media::PlayerImplementation::~PlayerImplementation()387media::PlayerImplementation::~PlayerImplementation()
388{388{
389 // Install null getters as these properties may be destroyed
390 // after the engine has been destroyed since they are owned by the
391 // base class.
392 std::function<uint64_t()> position_getter = [this]()
393 {
394 return static_cast<uint64_t>(0);
395 };
396 position().install(position_getter);
397
398 std::function<uint64_t()> duration_getter = [this]()
399 {
400 return static_cast<uint64_t>(0);
401 };
402 duration().install(duration_getter);
403
404 std::function<bool()> video_type_getter = [this]()
405 {
406 return false;
407 };
408 is_video_source().install(video_type_getter);
409
410 std::function<bool()> audio_type_getter = [this]()
411 {
412 return false;
413 };
414 is_audio_source().install(audio_type_getter);
389}415}
390416
391std::shared_ptr<media::TrackList> media::PlayerImplementation::track_list()417std::shared_ptr<media::TrackList> media::PlayerImplementation::track_list()
392418
=== modified file 'src/core/media/player_skeleton.cpp'
--- src/core/media/player_skeleton.cpp 2014-09-10 21:05:34 +0000
+++ src/core/media/player_skeleton.cpp 2014-10-10 16:04:29 +0000
@@ -40,7 +40,7 @@
40namespace dbus = core::dbus;40namespace dbus = core::dbus;
41namespace media = core::ubuntu::media;41namespace media = core::ubuntu::media;
4242
43struct media::PlayerSkeleton::Private : public std::enable_shared_from_this<media::PlayerSkeleton::Private>43struct media::PlayerSkeleton::Private
44{44{
45 Private(media::PlayerSkeleton* player,45 Private(media::PlayerSkeleton* player,
46 const std::string& identity,46 const std::string& identity,
@@ -348,6 +348,19 @@
348348
349media::PlayerSkeleton::~PlayerSkeleton()349media::PlayerSkeleton::~PlayerSkeleton()
350{350{
351 // The session object may outlive the private instance
352 // so uninstall all method handlers.
353 d->object->uninstall_method_handler<mpris::Player::Next>();
354 d->object->uninstall_method_handler<mpris::Player::Previous>();
355 d->object->uninstall_method_handler<mpris::Player::Pause>();
356 d->object->uninstall_method_handler<mpris::Player::Stop>();
357 d->object->uninstall_method_handler<mpris::Player::Play>();
358 d->object->uninstall_method_handler<mpris::Player::PlayPause>();
359 d->object->uninstall_method_handler<mpris::Player::Seek>();
360 d->object->uninstall_method_handler<mpris::Player::SetPosition>();
361 d->object->uninstall_method_handler<mpris::Player::OpenUri>();
362 d->object->uninstall_method_handler<mpris::Player::CreateVideoSink>();
363 d->object->uninstall_method_handler<mpris::Player::Key>();
351}364}
352365
353const core::Property<bool>& media::PlayerSkeleton::can_play() const366const core::Property<bool>& media::PlayerSkeleton::can_play() const

Subscribers

People subscribed via source and target branches