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
1=== modified file 'src/core/media/gstreamer/engine.cpp'
2--- src/core/media/gstreamer/engine.cpp 2014-09-08 10:16:38 +0000
3+++ src/core/media/gstreamer/engine.cpp 2014-10-10 16:04:29 +0000
4@@ -146,6 +146,10 @@
5 {
6 }
7
8+ // Ensure the playbin is the last item destroyed
9+ // otherwise properties could try to access a dead playbin object
10+ gstreamer::Playbin playbin;
11+
12 std::shared_ptr<Engine::MetaDataExtractor> meta_data_extractor;
13 core::Property<Engine::State> state;
14 core::Property<std::tuple<media::Track::UriType, media::Track::MetaData>> track_meta_data;
15@@ -155,7 +159,7 @@
16 core::Property<media::Player::AudioStreamRole> audio_role;
17 core::Property<bool> is_video_source;
18 core::Property<bool> is_audio_source;
19- gstreamer::Playbin playbin;
20+
21 core::ScopedConnection about_to_finish_connection;
22 core::ScopedConnection on_state_changed_connection;
23 core::ScopedConnection on_tag_available_connection;
24
25=== modified file 'src/core/media/player_implementation.cpp'
26--- src/core/media/player_implementation.cpp 2014-09-25 16:26:13 +0000
27+++ src/core/media/player_implementation.cpp 2014-10-10 16:04:29 +0000
28@@ -386,6 +386,32 @@
29
30 media::PlayerImplementation::~PlayerImplementation()
31 {
32+ // Install null getters as these properties may be destroyed
33+ // after the engine has been destroyed since they are owned by the
34+ // base class.
35+ std::function<uint64_t()> position_getter = [this]()
36+ {
37+ return static_cast<uint64_t>(0);
38+ };
39+ position().install(position_getter);
40+
41+ std::function<uint64_t()> duration_getter = [this]()
42+ {
43+ return static_cast<uint64_t>(0);
44+ };
45+ duration().install(duration_getter);
46+
47+ std::function<bool()> video_type_getter = [this]()
48+ {
49+ return false;
50+ };
51+ is_video_source().install(video_type_getter);
52+
53+ std::function<bool()> audio_type_getter = [this]()
54+ {
55+ return false;
56+ };
57+ is_audio_source().install(audio_type_getter);
58 }
59
60 std::shared_ptr<media::TrackList> media::PlayerImplementation::track_list()
61
62=== modified file 'src/core/media/player_skeleton.cpp'
63--- src/core/media/player_skeleton.cpp 2014-09-10 21:05:34 +0000
64+++ src/core/media/player_skeleton.cpp 2014-10-10 16:04:29 +0000
65@@ -40,7 +40,7 @@
66 namespace dbus = core::dbus;
67 namespace media = core::ubuntu::media;
68
69-struct media::PlayerSkeleton::Private : public std::enable_shared_from_this<media::PlayerSkeleton::Private>
70+struct media::PlayerSkeleton::Private
71 {
72 Private(media::PlayerSkeleton* player,
73 const std::string& identity,
74@@ -348,6 +348,19 @@
75
76 media::PlayerSkeleton::~PlayerSkeleton()
77 {
78+ // The session object may outlive the private instance
79+ // so uninstall all method handlers.
80+ d->object->uninstall_method_handler<mpris::Player::Next>();
81+ d->object->uninstall_method_handler<mpris::Player::Previous>();
82+ d->object->uninstall_method_handler<mpris::Player::Pause>();
83+ d->object->uninstall_method_handler<mpris::Player::Stop>();
84+ d->object->uninstall_method_handler<mpris::Player::Play>();
85+ d->object->uninstall_method_handler<mpris::Player::PlayPause>();
86+ d->object->uninstall_method_handler<mpris::Player::Seek>();
87+ d->object->uninstall_method_handler<mpris::Player::SetPosition>();
88+ d->object->uninstall_method_handler<mpris::Player::OpenUri>();
89+ d->object->uninstall_method_handler<mpris::Player::CreateVideoSink>();
90+ d->object->uninstall_method_handler<mpris::Player::Key>();
91 }
92
93 const core::Property<bool>& media::PlayerSkeleton::can_play() const

Subscribers

People subscribed via source and target branches