Mir

Merge lp:~kdub/mir/sort-out-surface-ownership into lp:mir

Proposed by Kevin DuBois
Status: Merged
Approved by: Chris Halse Rogers
Approved revision: 3212
Merged at revision: 3240
Proposed branch: lp:~kdub/mir/sort-out-surface-ownership
Merge into: lp:mir
Diff against target: 1129 lines (+331/-299)
11 files modified
src/client/connection_surface_map.h (+2/-2)
src/client/mir_connection.cpp (+186/-18)
src/client/mir_connection.h (+27/-4)
src/client/mir_surface.cpp (+31/-179)
src/client/mir_surface.h (+16/-14)
src/client/mir_surface_api.cpp (+2/-4)
src/client/surface_map.cpp (+4/-10)
src/client/surface_map.h (+1/-0)
tests/unit-tests/client/test_client_mir_surface.cpp (+20/-61)
tests/unit-tests/client/test_connection_resource_map.cpp (+7/-6)
tests/unit-tests/client/test_mir_connection.cpp (+35/-1)
To merge this branch: bzr merge lp:~kdub/mir/sort-out-surface-ownership
Reviewer Review Type Date Requested Status
Chris Halse Rogers Approve
Alberto Aguirre (community) Approve
PS Jenkins bot (community) continuous-integration Approve
Mir CI Bot continuous-integration Approve
Alan Griffiths Approve
Review via email: mp+280481@code.launchpad.net

Commit message

client: sort out the lifetime and ownership of MirSurface. The MirSurface lifetimes are now managed by MirConnection, and SurfaceMap is where MirConnection stores the surfaces.

Previously, they were imperfectly managed by SurfaceMap, eg, MirSurface would call "delete this" on itself. This led to scenarios where SurfaceMap wasn't really sure if the surface was alive or dead. Since NBS increases the amount of Events being sent to the surface, this would tease out existing problems with this scenario around surface destruction more.

Description of the change

client: sort out the lifetime and ownership of MirSurface. The MirSurface lifetimes are now managed by MirConnection, and SurfaceMap is where MirConnection stores the surfaces.

Previously, they were imperfectly managed by SurfaceMap, eg, MirSurface would call "delete this" on itself sometimes. This led to scenarios where SurfaceMap wasn't really sure if the surface was alive or dead. Since NBS increases the amount of Events being sent to the surface, this would tease out existing problems with this scenario around surface destruction more.

I'll probably do something similar with BufferStream to reduce complexity even more (eg, SurfaceMap won't auto-add bufferstreams, and the funny deletion around l725 won't be needed anymore).

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

merge in mir, fix conflicts

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
3199. By Kevin DuBois

merge in mir

3200. By Kevin DuBois

extend the lifetime of the WaitHandle, as MirWaitHandles are a bit crazy too

3201. By Kevin DuBois

make sure to mop up a wait handle in an error case

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Kevin DuBois (kdub) wrote :

^still a FD leak for me to track down, probably on monday

3202. By Kevin DuBois

merge in mir

3203. By Kevin DuBois

find fd leak, and plug

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

PASSED: Continuous integration, rev:3203
http://jenkins.qa.ubuntu.com/job/mir-ci/5908/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5398
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4305
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5351
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-xenial-touch/223
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/234
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/234/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/234
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/234/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5348
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5348/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/7856
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/26297
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/219
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/219/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-xenial-touch/77
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/26301

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5908/rebuild

review: Approve (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

-TEST_F(MirClientSurfaceTest, create_wait_handle_really_blocks)

Is this test no longer relevant?

review: Needs Information
3204. By Kevin DuBois

merge in mir

Revision history for this message
Kevin DuBois (kdub) wrote :

Its more of a test of the wait handle, and the logic for creating a surface has moved to the MirConnection. I suppose I can port the test over instead of just removing it

3205. By Kevin DuBois

port over a test instead of deleting it. a quick scan suggests this is covered in other bits of testing, but may as well keep it

Revision history for this message
Kevin DuBois (kdub) wrote :

I think a case can be made for the removal of the test, but porting the test over was easy too.

3206. By Kevin DuBois

merge in mir

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

PASSED: Continuous integration, rev:3205
http://jenkins.qa.ubuntu.com/job/mir-ci/5937/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5430
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4337
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5386
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-xenial-touch/236
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/261
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/261/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/261
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/261/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5383
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5383/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/7879
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/26391
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/232
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/232/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-xenial-touch/90
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/26399

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5937/rebuild

review: Approve (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

OK. (I'm not entirely convinced it is needed, but I don't see any harm.)

review: Approve
Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:3205
https://mir-jenkins.ubuntu.com/job/mir-ci/18/
Executed test runs:
    None: https://mir-jenkins.ubuntu.com/job/generic-update-mp/18/console

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/18/rebuild

review: Approve (continuous-integration)
Revision history for this message
Chris Halse Rogers (raof) wrote :

127 + std::lock_guard<decltype(mutex)> lock(mutex);
...
136 + callback(surf.get(), context);

You're calling into client code with an internal mutex called. That's generally a bad idea, and I think it will deadlock under plausible client usage - MirConnection::disconnect() takes the same mutex, and calling mir_connection_release() in the error case for surface construction doesn't seem too farfetched.

716 + //a bit batty, but the creation handle has to exist for as long as the MirSurface does,
717 + //as we don't really manage the lifetime of MirWaitHandle sensibly.
718 + std::shared_ptr<MirWaitHandle> const creation_handle;

I'd prefer a unique_ptr here, but that's stylistic rather than blocking.

Also, is this constraint sufficiently strong? Answer: Huh, that's the current behaviour. YAY MirWaitHandle!

review: Needs Fixing
3207. By Kevin DuBois

correct a callback-under-lock scenario during an exception handling case

3208. By Kevin DuBois

merge in mir

3209. By Kevin DuBois

make sure to use const& where possible

Revision history for this message
Kevin DuBois (kdub) wrote :

@Chris, good catch, fixed the callback during the exception catching scenario. I kept the wait handle a shared ptr, was a bit easier to call at the appropriate time.

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

PASSED: Continuous integration, rev:3209
http://jenkins.qa.ubuntu.com/job/mir-ci/5979/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5486
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4393
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5442
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-xenial-touch/258
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/303
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/303/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/303
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/303/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5439
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5439/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/7924
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/26548
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/254
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/254/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-xenial-touch/112
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/26554

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5979/rebuild

review: Approve (continuous-integration)
3210. By Kevin DuBois

merge in mir

Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:3209
https://mir-jenkins.ubuntu.com/job/mir-ci/35/
Executed test runs:
    None: https://mir-jenkins.ubuntu.com/job/generic-update-mp/34/console

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/35/rebuild

review: Approve (continuous-integration)
Revision history for this message
Alberto Aguirre (albaguirre) wrote :

+ if (strncmp(surface->get_error_message(), "", 1))

Could just be if(!mir_surface_is_valid(surface))

review: Needs Fixing
Revision history for this message
Kevin DuBois (kdub) wrote :

@Alberto, fixed

3211. By Kevin DuBois

use a simpler way to check surface validitiy

3212. By Kevin DuBois

merge in mir

Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:3212
https://mir-jenkins.ubuntu.com/job/mir-ci/42/
Executed test runs:
    None: https://mir-jenkins.ubuntu.com/job/generic-update-mp/42/console

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/42/rebuild

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

PASSED: Continuous integration, rev:3212
http://jenkins.qa.ubuntu.com/job/mir-ci/6009/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5522
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4429
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5478
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-xenial-touch/273
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/333
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/333/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/333
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/333/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5475
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5475/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/7949
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/26625
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/269
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/269/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-xenial-touch/127
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/26633

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/6009/rebuild

review: Approve (continuous-integration)
Revision history for this message
Alberto Aguirre (albaguirre) wrote :

LGTM.

review: Approve
Revision history for this message
Chris Halse Rogers (raof) wrote :

LGTM now.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/client/connection_surface_map.h'
2--- src/client/connection_surface_map.h 2015-12-15 12:24:10 +0000
3+++ src/client/connection_surface_map.h 2016-01-13 19:10:37 +0000
4@@ -40,7 +40,7 @@
5 ~ConnectionSurfaceMap() noexcept;
6
7 void with_surface_do(frontend::SurfaceId surface_id, std::function<void(MirSurface*)> const& exec) const override;
8- void insert(frontend::SurfaceId surface_id, MirSurface* surface);
9+ void insert(frontend::SurfaceId surface_id, std::shared_ptr<MirSurface> const& surface);
10 void erase(frontend::SurfaceId surface_id);
11
12 void with_stream_do(frontend::BufferStreamId stream_id, std::function<void(ClientBufferStream*)> const& exec) const override;
13@@ -50,7 +50,7 @@
14
15 private:
16 std::shared_timed_mutex mutable guard;
17- std::unordered_map<frontend::SurfaceId, MirSurface*> surfaces;
18+ std::unordered_map<frontend::SurfaceId, std::shared_ptr<MirSurface>> surfaces;
19 std::unordered_map<frontend::BufferStreamId, StreamInfo> streams;
20 };
21
22
23=== modified file 'src/client/mir_connection.cpp'
24--- src/client/mir_connection.cpp 2016-01-04 07:26:59 +0000
25+++ src/client/mir_connection.cpp 2016-01-13 19:10:37 +0000
26@@ -53,6 +53,76 @@
27
28 namespace
29 {
30+mir::protobuf::SurfaceParameters serialize_spec(MirSurfaceSpec const& spec)
31+{
32+ mp::SurfaceParameters message;
33+
34+#define SERIALIZE_OPTION_IF_SET(option) \
35+ if (spec.option.is_set()) \
36+ message.set_##option(spec.option.value());
37+
38+ SERIALIZE_OPTION_IF_SET(width);
39+ SERIALIZE_OPTION_IF_SET(height);
40+ SERIALIZE_OPTION_IF_SET(pixel_format);
41+ SERIALIZE_OPTION_IF_SET(buffer_usage);
42+ SERIALIZE_OPTION_IF_SET(surface_name);
43+ SERIALIZE_OPTION_IF_SET(output_id);
44+ SERIALIZE_OPTION_IF_SET(type);
45+ SERIALIZE_OPTION_IF_SET(state);
46+ SERIALIZE_OPTION_IF_SET(pref_orientation);
47+ SERIALIZE_OPTION_IF_SET(edge_attachment);
48+ SERIALIZE_OPTION_IF_SET(min_width);
49+ SERIALIZE_OPTION_IF_SET(min_height);
50+ SERIALIZE_OPTION_IF_SET(max_width);
51+ SERIALIZE_OPTION_IF_SET(max_height);
52+ SERIALIZE_OPTION_IF_SET(width_inc);
53+ SERIALIZE_OPTION_IF_SET(height_inc);
54+ // min_aspect is a special case (below)
55+ // max_aspect is a special case (below)
56+
57+ if (spec.parent.is_set() && spec.parent.value() != nullptr)
58+ message.set_parent_id(spec.parent.value()->id());
59+
60+ if (spec.parent_id)
61+ {
62+ auto id = message.mutable_parent_persistent_id();
63+ id->set_value(spec.parent_id->as_string());
64+ }
65+
66+ if (spec.aux_rect.is_set())
67+ {
68+ message.mutable_aux_rect()->set_left(spec.aux_rect.value().left);
69+ message.mutable_aux_rect()->set_top(spec.aux_rect.value().top);
70+ message.mutable_aux_rect()->set_width(spec.aux_rect.value().width);
71+ message.mutable_aux_rect()->set_height(spec.aux_rect.value().height);
72+ }
73+
74+ if (spec.min_aspect.is_set())
75+ {
76+ message.mutable_min_aspect()->set_width(spec.min_aspect.value().width);
77+ message.mutable_min_aspect()->set_height(spec.min_aspect.value().height);
78+ }
79+
80+ if (spec.max_aspect.is_set())
81+ {
82+ message.mutable_max_aspect()->set_width(spec.max_aspect.value().width);
83+ message.mutable_max_aspect()->set_height(spec.max_aspect.value().height);
84+ }
85+
86+ if (spec.input_shape.is_set())
87+ {
88+ for (auto const& rect : spec.input_shape.value())
89+ {
90+ auto const new_shape = message.add_input_shape();
91+ new_shape->set_left(rect.left);
92+ new_shape->set_top(rect.top);
93+ new_shape->set_width(rect.width);
94+ new_shape->set_height(rect.height);
95+ }
96+ }
97+
98+ return message;
99+}
100
101 class MirDisplayConfigurationStore
102 {
103@@ -175,10 +245,106 @@
104 mir_surface_callback callback,
105 void * context)
106 {
107- auto surface = new MirSurface(this, server, &debug, buffer_stream_factory,
108- input_platform, spec, callback, context);
109-
110- return surface->get_create_wait_handle();
111+ auto response = std::make_shared<mp::Surface>();
112+ auto c = std::make_shared<MirConnection::SurfaceCreationRequest>(callback, context, spec);
113+ c->wh->expect_result();
114+ auto const message = serialize_spec(spec);
115+ {
116+ std::lock_guard<decltype(mutex)> lock(mutex);
117+ surface_requests.emplace_back(c);
118+ }
119+
120+ try
121+ {
122+ server.create_surface(&message, c->response.get(),
123+ gp::NewCallback(this, &MirConnection::surface_created, c.get()));
124+ }
125+ catch (std::exception const& ex)
126+ {
127+ std::unique_lock<decltype(mutex)> lock(mutex);
128+ auto request = std::find_if(surface_requests.begin(), surface_requests.end(),
129+ [&](std::shared_ptr<MirConnection::SurfaceCreationRequest> c2) { return c.get() == c2.get(); });
130+ if (request != surface_requests.end())
131+ {
132+ auto id = next_error_id(lock);
133+ auto surf = std::make_shared<MirSurface>(
134+ std::string{"Error creating surface: "} + boost::diagnostic_information(ex), this, id, (*request)->wh);
135+ surface_map->insert(id, surf);
136+ auto wh = (*request)->wh;
137+ surface_requests.erase(request);
138+
139+ lock.unlock();
140+ callback(surf.get(), context);
141+ wh->result_received();
142+ }
143+ }
144+ return c->wh.get();
145+}
146+
147+mf::SurfaceId MirConnection::next_error_id(std::unique_lock<std::mutex> const&)
148+{
149+ return mf::SurfaceId{surface_error_id--};
150+}
151+
152+void MirConnection::surface_created(SurfaceCreationRequest* request)
153+{
154+ std::unique_lock<decltype(mutex)> lock(mutex);
155+ std::shared_ptr<MirSurface> surf {nullptr};
156+ std::shared_ptr<mcl::ClientBufferStream> stream {nullptr};
157+ //make sure this request actually was made.
158+ auto request_it = std::find_if(surface_requests.begin(), surface_requests.end(),
159+ [&request](std::shared_ptr<MirConnection::SurfaceCreationRequest> r){ return request == r.get(); });
160+ if (request_it == surface_requests.end())
161+ return;
162+
163+ auto surface_proto = request->response;
164+ auto callback = request->cb;
165+ auto context = request->context;
166+ auto const& spec = request->spec;
167+
168+ try
169+ {
170+ stream = buffer_stream_factory->make_producer_stream(
171+ this, server, surface_proto->buffer_stream(), std::string{},
172+ {surface_proto->width(), surface_proto->height()});
173+ }
174+ catch (std::exception const& error)
175+ {
176+ if (!surface_proto->has_error())
177+ surface_proto->set_error(error.what());
178+ // failed to create buffer_stream, so clean up FDs it doesn't own
179+ for (auto i = 0; i < surface_proto->fd_size(); i++)
180+ ::close(surface_proto->fd(i));
181+ if (surface_proto->has_buffer_stream() && surface_proto->buffer_stream().has_buffer())
182+ for (int i = 0; i < surface_proto->buffer_stream().buffer().fd_size(); i++)
183+ ::close(surface_proto->buffer_stream().buffer().fd(i));
184+ }
185+
186+ if (surface_proto->has_error() || !surface_proto->has_id())
187+ {
188+ std::string reason;
189+ if (surface_proto->has_error())
190+ reason += surface_proto->error();
191+ if (surface_proto->has_error() && !surface_proto->has_id())
192+ reason += " and ";
193+ if (!surface_proto->has_id())
194+ reason += "Server assigned surface no id";
195+ auto id = next_error_id(lock);
196+ surf = std::make_shared<MirSurface>(reason, this, id, request->wh);
197+ surface_map->insert(id, surf);
198+ }
199+ else
200+ {
201+ surf = std::make_shared<MirSurface>(
202+ this, server, &debug, stream, input_platform, spec, *surface_proto, request->wh);
203+
204+ surface_map->insert(mf::SurfaceId{surface_proto->id().value()}, surf);
205+ }
206+
207+ callback(surf.get(), context);
208+ request->wh->result_received();
209+
210+ surface_requests.erase(request_it);
211 }
212
213 char const * MirConnection::get_error_message()
214@@ -228,9 +394,7 @@
215
216 void MirConnection::released(SurfaceRelease data)
217 {
218- surface_map->erase(mf::SurfaceId(data.surface->id()));
219-
220- // Erasing this surface from surface_map means that it will no longer receive events
221+ // releasing this surface from surface_map means that it will no longer receive events
222 // If it's still focused, send an unfocused event before we kill it entirely
223 if (data.surface->attrib(mir_surface_attrib_focus) == mir_surface_focused)
224 {
225@@ -239,7 +403,7 @@
226 }
227 data.callback(data.surface, data.context);
228 data.handle->result_received();
229- delete data.surface;
230+ surface_map->erase(mf::SurfaceId(data.surface->id()));
231 }
232
233 MirWaitHandle* MirConnection::release_surface(
234@@ -248,17 +412,26 @@
235 void * context)
236 {
237 auto new_wait_handle = new MirWaitHandle;
238+ {
239+ std::lock_guard<decltype(release_wait_handle_guard)> rel_lock(release_wait_handle_guard);
240+ release_wait_handles.push_back(new_wait_handle);
241+ }
242+
243+ if (!mir_surface_is_valid(surface))
244+ {
245+ new_wait_handle->expect_result();
246+ new_wait_handle->result_received();
247+ callback(surface, context);
248+ auto id = surface->id();
249+ surface_map->erase(mf::SurfaceId(id));
250+ return new_wait_handle;
251+ }
252
253 SurfaceRelease surf_release{surface, new_wait_handle, callback, context};
254
255 mp::SurfaceId message;
256 message.set_value(surface->id());
257
258- {
259- std::lock_guard<decltype(release_wait_handle_guard)> rel_lock(release_wait_handle_guard);
260- release_wait_handles.push_back(new_wait_handle);
261- }
262-
263 new_wait_handle->expect_result();
264 server.release_surface(&message, void_response.get(),
265 gp::NewCallback(this, &MirConnection::released, surf_release));
266@@ -537,11 +710,6 @@
267 surface_map->insert(mf::BufferStreamId(id), stream);
268 }
269
270-void MirConnection::on_surface_created(int id, MirSurface* surface)
271-{
272- surface_map->insert(mf::SurfaceId(id), surface);
273-}
274-
275 void MirConnection::register_lifecycle_event_callback(mir_lifecycle_event_callback callback, void* context)
276 {
277 lifecycle_control->set_callback(std::bind(callback, this, std::placeholders::_1, context));
278
279=== modified file 'src/client/mir_connection.h'
280--- src/client/mir_connection.h 2016-01-04 21:41:00 +0000
281+++ src/client/mir_connection.h 2016-01-13 19:10:37 +0000
282@@ -26,8 +26,10 @@
283
284 #include "mir/geometry/size.h"
285 #include "mir/client_platform.h"
286+#include "mir/frontend/surface_id.h"
287 #include "mir/client_context.h"
288 #include "mir_toolkit/mir_client_library.h"
289+#include "mir_surface.h"
290
291 #include <atomic>
292 #include <memory>
293@@ -100,9 +102,9 @@
294 mir_surface_callback callback,
295 void * context);
296 MirWaitHandle* release_surface(
297- MirSurface *surface,
298- mir_surface_callback callback,
299- void *context);
300+ MirSurface *surface,
301+ mir_surface_callback callback,
302+ void *context);
303
304 MirPromptSession* create_prompt_session();
305
306@@ -152,7 +154,6 @@
307 EGLNativeDisplayType egl_native_display();
308 MirPixelFormat egl_pixel_format(EGLDisplay, EGLConfig) const;
309
310- void on_surface_created(int id, MirSurface* surface);
311 void on_stream_created(int id, mir::client::ClientBufferStream* stream);
312
313 MirWaitHandle* configure_display(MirDisplayConfiguration* configuration);
314@@ -167,9 +168,28 @@
315 }
316
317 mir::client::rpc::DisplayServer& display_server();
318+ mir::client::rpc::DisplayServerDebug& debug_display_server();
319 std::shared_ptr<mir::logging::Logger> const& the_logger() const;
320
321 private:
322+ //google cant have callbacks with more than 2 args
323+ struct SurfaceCreationRequest
324+ {
325+ SurfaceCreationRequest(mir_surface_callback cb, void* context, MirSurfaceSpec const& spec) :
326+ cb(cb), context(context), spec(spec),
327+ response(std::make_shared<mir::protobuf::Surface>()),
328+ wh(std::make_shared<MirWaitHandle>())
329+ {
330+ }
331+ mir_surface_callback cb;
332+ void* context;
333+ MirSurfaceSpec const spec;
334+ std::shared_ptr<mir::protobuf::Surface> response;
335+ std::shared_ptr<MirWaitHandle> wh;
336+ };
337+ std::vector<std::shared_ptr<SurfaceCreationRequest>> surface_requests;
338+ void surface_created(SurfaceCreationRequest*);
339+
340 void populate_server_package(MirPlatformPackage& platform_package) override;
341 // MUST be first data member so it is destroyed last.
342 struct Deregisterer
343@@ -191,6 +211,9 @@
344 std::unique_ptr<mir::protobuf::Void> set_base_display_configuration_response;
345 std::atomic<bool> disconnecting{false};
346
347+ mir::frontend::SurfaceId next_error_id(std::unique_lock<std::mutex> const&);
348+ int surface_error_id{-1};
349+
350 std::shared_ptr<mir::client::ClientPlatformFactory> const client_platform_factory;
351 std::shared_ptr<mir::client::ClientPlatform> platform;
352 std::shared_ptr<EGLNativeDisplayType> native_display;
353
354=== modified file 'src/client/mir_surface.cpp'
355--- src/client/mir_surface.cpp 2015-12-17 15:58:42 +0000
356+++ src/client/mir_surface.cpp 2016-01-13 19:10:37 +0000
357@@ -49,77 +49,6 @@
358 {
359 std::mutex handle_mutex;
360 std::unordered_set<MirSurface*> valid_surfaces;
361-
362-mir::protobuf::SurfaceParameters serialize_spec(MirSurfaceSpec const& spec)
363-{
364- mp::SurfaceParameters message;
365-
366-#define SERIALIZE_OPTION_IF_SET(option) \
367- if (spec.option.is_set()) \
368- message.set_##option(spec.option.value());
369-
370- SERIALIZE_OPTION_IF_SET(width);
371- SERIALIZE_OPTION_IF_SET(height);
372- SERIALIZE_OPTION_IF_SET(pixel_format);
373- SERIALIZE_OPTION_IF_SET(buffer_usage);
374- SERIALIZE_OPTION_IF_SET(surface_name);
375- SERIALIZE_OPTION_IF_SET(output_id);
376- SERIALIZE_OPTION_IF_SET(type);
377- SERIALIZE_OPTION_IF_SET(state);
378- SERIALIZE_OPTION_IF_SET(pref_orientation);
379- SERIALIZE_OPTION_IF_SET(edge_attachment);
380- SERIALIZE_OPTION_IF_SET(min_width);
381- SERIALIZE_OPTION_IF_SET(min_height);
382- SERIALIZE_OPTION_IF_SET(max_width);
383- SERIALIZE_OPTION_IF_SET(max_height);
384- SERIALIZE_OPTION_IF_SET(width_inc);
385- SERIALIZE_OPTION_IF_SET(height_inc);
386- // min_aspect is a special case (below)
387- // max_aspect is a special case (below)
388-
389- if (spec.parent.is_set() && spec.parent.value() != nullptr)
390- message.set_parent_id(spec.parent.value()->id());
391-
392- if (spec.parent_id)
393- {
394- auto id = message.mutable_parent_persistent_id();
395- id->set_value(spec.parent_id->as_string());
396- }
397-
398- if (spec.aux_rect.is_set())
399- {
400- message.mutable_aux_rect()->set_left(spec.aux_rect.value().left);
401- message.mutable_aux_rect()->set_top(spec.aux_rect.value().top);
402- message.mutable_aux_rect()->set_width(spec.aux_rect.value().width);
403- message.mutable_aux_rect()->set_height(spec.aux_rect.value().height);
404- }
405-
406- if (spec.min_aspect.is_set())
407- {
408- message.mutable_min_aspect()->set_width(spec.min_aspect.value().width);
409- message.mutable_min_aspect()->set_height(spec.min_aspect.value().height);
410- }
411-
412- if (spec.max_aspect.is_set())
413- {
414- message.mutable_max_aspect()->set_width(spec.max_aspect.value().width);
415- message.mutable_max_aspect()->set_height(spec.max_aspect.value().height);
416- }
417-
418- if (spec.input_shape.is_set())
419- {
420- for (auto const& rect : spec.input_shape.value())
421- {
422- auto const new_shape = message.add_input_shape();
423- new_shape->set_left(rect.left);
424- new_shape->set_top(rect.top);
425- new_shape->set_width(rect.width);
426- new_shape->set_height(rect.height);
427- }
428- }
429-
430- return message;
431-}
432 }
433
434 MirSurfaceSpec::MirSurfaceSpec(
435@@ -159,10 +88,17 @@
436 return string_id;
437 }
438
439-MirSurface::MirSurface(std::string const& error)
440- : surface{mcl::make_protobuf_object<mir::protobuf::Surface>()}
441+MirSurface::MirSurface(
442+ std::string const& error,
443+ MirConnection* conn,
444+ mir::frontend::SurfaceId id,
445+ std::shared_ptr<MirWaitHandle> const& handle) :
446+ surface{mcl::make_protobuf_object<mir::protobuf::Surface>()},
447+ connection_(conn),
448+ creation_handle(handle)
449 {
450 surface->set_error(error);
451+ surface->mutable_id()->set_value(id.as_value());
452
453 std::lock_guard<decltype(handle_mutex)> lock(handle_mutex);
454 valid_surfaces.insert(this);
455@@ -172,25 +108,30 @@
456 MirConnection *allocating_connection,
457 mclr::DisplayServer& the_server,
458 mclr::DisplayServerDebug* debug,
459- std::shared_ptr<mcl::ClientBufferStreamFactory> const& buffer_stream_factory,
460+ std::shared_ptr<mcl::ClientBufferStream> const& buffer_stream,
461 std::shared_ptr<mircv::InputPlatform> const& input_platform,
462 MirSurfaceSpec const& spec,
463- mir_surface_callback callback, void * context)
464+ mir::protobuf::Surface const& surface_proto,
465+ std::shared_ptr<MirWaitHandle> const& handle)
466 : server{&the_server},
467 debug{debug},
468- surface{mcl::make_protobuf_object<mir::protobuf::Surface>()},
469+ surface{mcl::make_protobuf_object<mir::protobuf::Surface>(surface_proto)},
470 persistent_id{mcl::make_protobuf_object<mir::protobuf::PersistentSurfaceId>()},
471 name{spec.surface_name.is_set() ? spec.surface_name.value() : ""},
472 void_response{mcl::make_protobuf_object<mir::protobuf::Void>()},
473 modify_result{mcl::make_protobuf_object<mir::protobuf::Void>()},
474- connection(allocating_connection),
475- buffer_stream_factory(buffer_stream_factory),
476+ connection_(allocating_connection),
477+ buffer_stream(buffer_stream),
478 input_platform(input_platform),
479 keymapper(std::make_shared<mircv::XKBMapper>()),
480- configure_result{mcl::make_protobuf_object<mir::protobuf::SurfaceSetting>()}
481+ configure_result{mcl::make_protobuf_object<mir::protobuf::SurfaceSetting>()},
482+ creation_handle(handle)
483 {
484- for (int i = 0; i < mir_surface_attribs; i++)
485- attrib_cache[i] = -1;
486+ for(int i = 0; i < surface_proto.attributes_size(); i++)
487+ {
488+ auto const& attrib = surface_proto.attributes(i);
489+ attrib_cache[attrib.attrib()] = attrib.ivalue();
490+ }
491
492 if (spec.event_handler.is_set())
493 {
494@@ -201,16 +142,10 @@
495 spec.event_handler.value().context);
496 }
497
498- auto const message = serialize_spec(spec);
499- create_wait_handle.expect_result();
500- try
501- {
502- server->create_surface(&message, surface.get(), gp::NewCallback(this, &MirSurface::created, callback, context));
503- }
504- catch (std::exception const& ex)
505- {
506- surface->set_error(std::string{"Error invoking create surface: "} +
507- boost::diagnostic_information(ex));
508+ if (surface_proto.fd_size() > 0 && handle_event_callback)
509+ {
510+ input_thread = std::make_shared<md::ThreadedDispatcher>("Input dispatch",
511+ input_platform->create_input_receiver( surface_proto.fd(0), keymapper, handle_event_callback));
512 }
513
514 std::lock_guard<decltype(handle_mutex)> lock(handle_mutex);
515@@ -303,11 +238,6 @@
516 return &persistent_id_wait_handle;
517 }
518
519-MirWaitHandle* MirSurface::get_create_wait_handle()
520-{
521- return &create_wait_handle;
522-}
523-
524 /* todo: all these conversion functions are a bit of a kludge, probably
525 better to have a more developed MirPixelFormat that can handle this */
526 MirPixelFormat MirSurface::convert_ipc_pf_to_geometry(gp::int32 pf) const
527@@ -315,89 +245,6 @@
528 return static_cast<MirPixelFormat>(pf);
529 }
530
531-void MirSurface::created(mir_surface_callback callback, void * context)
532-{
533- {
534- std::lock_guard<decltype(mutex)> lock(mutex);
535- if (!surface->has_id())
536- {
537- if (!surface->has_error())
538- surface->set_error("Error processing surface create response, no ID (disconnected?)");
539-
540- callback(this, context);
541- create_wait_handle.result_received();
542- return;
543- }
544- }
545- try
546- {
547- {
548- geom::Size size{surface->width(), surface->height()};
549- auto stream = buffer_stream_factory->
550- make_producer_stream(connection, *server, surface->buffer_stream(), name, size);
551-
552- std::lock_guard<decltype(mutex)> lock(mutex);
553- buffer_stream = std::move(stream);
554- for(int i = 0; i < surface->attributes_size(); i++)
555- {
556- auto const& attrib = surface->attributes(i);
557- attrib_cache[attrib.attrib()] = attrib.ivalue();
558- }
559- }
560-
561- connection->on_surface_created(id(), this);
562- }
563- catch (std::exception const& error)
564- {
565- // failed to create buffer_stream, so clean up FDs it doesn't own
566- if (!buffer_stream)
567- for (int i = 0; i < surface->buffer_stream().buffer().fd_size(); i++)
568- ::close(surface->buffer_stream().buffer().fd(i));
569-
570- surface->set_error(std::string{"Error processing Surface creating response:"} +
571- boost::diagnostic_information(error));
572- }
573-
574- if (surface->fd_size() > 0 && handle_event_callback)
575- {
576- auto input_dispatcher = input_platform->create_input_receiver(surface->fd(0),
577- keymapper,
578- handle_event_callback);
579- input_thread = std::make_shared<md::ThreadedDispatcher>("Input dispatch", input_dispatcher);
580- }
581-
582- callback(this, context);
583- create_wait_handle.result_received();
584-}
585-
586-MirWaitHandle* MirSurface::release_surface(
587- mir_surface_callback callback,
588- void * context)
589-{
590- bool was_valid = false;
591- {
592- std::lock_guard<decltype(handle_mutex)> lock(handle_mutex);
593- if (valid_surfaces.count(this))
594- was_valid = true;
595- valid_surfaces.erase(this);
596- }
597- if (this->surface->has_error())
598- was_valid = false;
599-
600- MirWaitHandle* wait_handle{nullptr};
601- if (connection && was_valid)
602- {
603- wait_handle = connection->release_surface(this, callback, context);
604- }
605- else
606- {
607- callback(this, context);
608- delete this;
609- }
610-
611- return wait_handle;
612-}
613-
614 MirWaitHandle* MirSurface::configure_cursor(MirCursorConfiguration const* cursor)
615 {
616 mp::CursorSetting setting;
617@@ -779,3 +626,8 @@
618
619 return &modify_wait_handle;
620 }
621+
622+MirConnection* MirSurface::connection() const
623+{
624+ return connection_;
625+}
626
627=== modified file 'src/client/mir_surface.h'
628--- src/client/mir_surface.h 2015-12-17 15:51:08 +0000
629+++ src/client/mir_surface.h 2016-01-13 19:10:37 +0000
630@@ -25,6 +25,7 @@
631 #include "rpc/mir_display_server_debug.h"
632
633 #include "mir/client_platform.h"
634+#include "mir/frontend/surface_id.h"
635 #include "mir/optional_value.h"
636 #include "mir/geometry/dimensions.h"
637 #include "mir_toolkit/common.h"
638@@ -100,7 +101,7 @@
639 mir::optional_value<MirOrientationMode> pref_orientation;
640
641 mir::optional_value<MirSurface*> parent;
642- std::unique_ptr<MirPersistentId> parent_id;
643+ std::shared_ptr<MirPersistentId> parent_id;
644 mir::optional_value<MirRectangle> aux_rect;
645 mir::optional_value<MirEdgeAttachment> edge_attachment;
646
647@@ -140,28 +141,27 @@
648 MirSurface(MirSurface const &) = delete;
649 MirSurface& operator=(MirSurface const &) = delete;
650
651- MirSurface(std::string const& error);
652+ MirSurface(
653+ std::string const& error,
654+ MirConnection *allocating_connection,
655+ mir::frontend::SurfaceId surface_id,
656+ std::shared_ptr<MirWaitHandle> const& handle);
657
658 MirSurface(
659 MirConnection *allocating_connection,
660 mir::client::rpc::DisplayServer& server,
661 mir::client::rpc::DisplayServerDebug* debug,
662- std::shared_ptr<mir::client::ClientBufferStreamFactory> const& buffer_stream_factory,
663+ std::shared_ptr<mir::client::ClientBufferStream> const& buffer_stream,
664 std::shared_ptr<mir::input::receiver::InputPlatform> const& input_platform,
665- MirSurfaceSpec const& spec,
666- mir_surface_callback callback, void * context);
667+ MirSurfaceSpec const& spec, mir::protobuf::Surface const& surface_proto,
668+ std::shared_ptr<MirWaitHandle> const& handle);
669
670 ~MirSurface();
671
672- MirWaitHandle* release_surface(
673- mir_surface_callback callback,
674- void *context);
675-
676 MirSurfaceParameters get_parameters() const;
677 char const * get_error_message();
678 int id() const;
679
680- MirWaitHandle* get_create_wait_handle();
681 MirWaitHandle* configure(MirSurfaceAttrib a, int value);
682
683 // TODO: Some sort of extension mechanism so that this can be moved
684@@ -192,12 +192,12 @@
685 static bool is_valid(MirSurface* query);
686
687 MirWaitHandle* request_persistent_id(mir_surface_id_callback callback, void* context);
688+ MirConnection* connection() const;
689 private:
690 std::mutex mutable mutex; // Protects all members of *this
691
692 void on_configured();
693 void on_cursor_configured();
694- void created(mir_surface_callback callback, void* context);
695 void acquired_persistent_id(mir_surface_id_callback callback, void* context);
696 MirPixelFormat convert_ipc_pf_to_geometry(google::protobuf::int32 pf) const;
697
698@@ -213,14 +213,12 @@
699 MirWaitHandle modify_wait_handle;
700 std::unique_ptr<mir::protobuf::Void> const modify_result;
701
702- MirConnection* const connection{nullptr};
703+ MirConnection* const connection_;
704
705- MirWaitHandle create_wait_handle;
706 MirWaitHandle configure_wait_handle;
707 MirWaitHandle configure_cursor_wait_handle;
708 MirWaitHandle persistent_id_wait_handle;
709
710- std::shared_ptr<mir::client::ClientBufferStreamFactory> const buffer_stream_factory;
711 std::shared_ptr<mir::client::ClientBufferStream> buffer_stream;
712 std::shared_ptr<mir::input::receiver::InputPlatform> const input_platform;
713 std::shared_ptr<mir::input::receiver::XKBMapper> const keymapper;
714@@ -234,6 +232,10 @@
715 std::function<void(MirEvent const*)> handle_event_callback;
716 std::shared_ptr<mir::dispatch::ThreadedDispatcher> input_thread;
717 bool auto_resize_stream{true};
718+
719+ //a bit batty, but the creation handle has to exist for as long as the MirSurface does,
720+ //as we don't really manage the lifetime of MirWaitHandle sensibly.
721+ std::shared_ptr<MirWaitHandle> const creation_handle;
722 };
723
724 #endif /* MIR_CLIENT_PRIVATE_MIR_WAIT_HANDLE_H_ */
725
726=== modified file 'src/client/mir_surface_api.cpp'
727--- src/client/mir_surface_api.cpp 2015-10-14 16:16:08 +0000
728+++ src/client/mir_surface_api.cpp 2016-01-13 19:10:37 +0000
729@@ -152,9 +152,6 @@
730 }
731 catch (std::exception const& error)
732 {
733- auto error_surf = new MirSurface{std::string{"Failed to create surface: "} +
734- boost::diagnostic_information(error)};
735- (*callback)(error_surf, context);
736 return nullptr;
737 }
738 }
739@@ -258,9 +255,10 @@
740 MirSurface* surface,
741 mir_surface_callback callback, void* context)
742 {
743+ auto connection = surface->connection();
744 try
745 {
746- return surface->release_surface(callback, context);
747+ return connection->release_surface(surface, callback, context);
748 }
749 catch (std::exception const& ex)
750 {
751
752=== modified file 'src/client/surface_map.cpp'
753--- src/client/surface_map.cpp 2015-12-15 15:42:37 +0000
754+++ src/client/surface_map.cpp 2016-01-13 19:10:37 +0000
755@@ -31,7 +31,7 @@
756
757 mcl::ConnectionSurfaceMap::~ConnectionSurfaceMap() noexcept
758 {
759- std::unordered_map<frontend::SurfaceId, MirSurface*> surface_map;
760+ std::unordered_map<frontend::SurfaceId, std::shared_ptr<MirSurface>> surface_map;
761 std::unordered_map<frontend::BufferStreamId, StreamInfo> stream_map;
762 {
763 //Prevent TSAN from flagging lock ordering issues
764@@ -42,14 +42,8 @@
765 stream_map = std::move(streams);
766 }
767
768- // Unless the client has screwed up there should be no surfaces left
769+ // Unless the client has screwed up there should be no streams left
770 // here. (OTOH *we* don't need to leak memory when clients screw up.)
771- for (auto const& surface : surface_map)
772- {
773- if (MirSurface::is_valid(surface.second))
774- delete surface.second;
775- }
776-
777 for (auto const& info : stream_map)
778 {
779 if (info.second.owned)
780@@ -65,7 +59,7 @@
781 if (it != surfaces.end())
782 {
783 auto const surface = it->second;
784- exec(surface);
785+ exec(surface.get());
786 }
787 else
788 {
789@@ -75,7 +69,7 @@
790 }
791 }
792
793-void mcl::ConnectionSurfaceMap::insert(mf::SurfaceId surface_id, MirSurface* surface)
794+void mcl::ConnectionSurfaceMap::insert(mf::SurfaceId surface_id, std::shared_ptr<MirSurface> const& surface)
795 {
796 // get_buffer_stream has internal locks - call before locking mutex to
797 // avoid locking ordering issues
798
799=== modified file 'src/client/surface_map.h'
800--- src/client/surface_map.h 2015-07-16 07:03:19 +0000
801+++ src/client/surface_map.h 2016-01-13 19:10:37 +0000
802@@ -22,6 +22,7 @@
803 #include "mir/frontend/surface_id.h"
804 #include "mir/frontend/buffer_stream_id.h"
805 #include <functional>
806+#include <memory>
807
808 struct MirSurface;
809
810
811=== modified file 'tests/unit-tests/client/test_client_mir_surface.cpp'
812--- tests/unit-tests/client/test_client_mir_surface.cpp 2015-12-16 20:01:21 +0000
813+++ tests/unit-tests/client/test_client_mir_surface.cpp 2016-01-13 19:10:37 +0000
814@@ -51,8 +51,7 @@
815
816 #include <boost/throw_exception.hpp>
817 #include "mir/test/doubles/stub_client_buffer_factory.h"
818-#include "mir/test/doubles/stub_client_buffer_stream_factory.h"
819-#include "mir/test/doubles/mock_client_buffer_stream_factory.h"
820+#include "mir/test/doubles/stub_buffer_stream.h"
821 #include "mir/test/doubles/mock_client_buffer_stream.h"
822
823 #include "mir_test_framework/stub_client_platform_factory.h"
824@@ -139,7 +138,6 @@
825
826 static std::map<int, int> sent_surface_attributes;
827
828-private:
829 void generate_unique_buffer()
830 {
831 global_buffer_id++;
832@@ -278,10 +276,6 @@
833 {
834 }
835
836-void null_surface_callback(MirSurface*, void*)
837-{
838-}
839-
840 void null_event_callback(MirSurface*, MirEvent const*, void*)
841 {
842 }
843@@ -294,6 +288,8 @@
844 {
845 MirClientSurfaceTest()
846 {
847+ mock_server_tool->create_surface_response(&surface_proto);
848+ ON_CALL(*stub_buffer_stream, swap_interval()).WillByDefault(testing::Return(1));
849 start_test_server();
850 connect_to_test_server();
851 }
852@@ -333,50 +329,47 @@
853 connection.get(),
854 server_stub,
855 nullptr,
856- stub_buffer_stream_factory,
857+ stub_buffer_stream,
858 input_platform,
859 spec,
860- &null_surface_callback,
861- nullptr);
862+ surface_proto,
863+ wh);
864 }
865
866 std::shared_ptr<MirSurface> create_surface_with(
867 mclr::DisplayServer& server_stub,
868- std::shared_ptr<mcl::ClientBufferStreamFactory> const& buffer_stream_factory)
869+ std::shared_ptr<mcl::ClientBufferStream> const& buffer_stream)
870 {
871 return std::make_shared<MirSurface>(
872 connection.get(),
873 server_stub,
874 nullptr,
875- buffer_stream_factory,
876+ buffer_stream,
877 input_platform,
878 spec,
879- &null_surface_callback,
880- nullptr);
881+ surface_proto,
882+ wh);
883 }
884
885 std::shared_ptr<MirSurface> create_and_wait_for_surface_with(
886 mclr::DisplayServer& server_stub)
887 {
888 auto surface = create_surface_with(server_stub);
889- surface->get_create_wait_handle()->wait_for_all();
890 return surface;
891 }
892
893 std::shared_ptr<MirSurface> create_and_wait_for_surface_with(
894 mclr::DisplayServer& server_stub,
895- std::shared_ptr<mcl::ClientBufferStreamFactory> const& buffer_stream_factory)
896+ std::shared_ptr<mcl::ClientBufferStream> const& buffer_stream)
897 {
898- auto surface = create_surface_with(server_stub, buffer_stream_factory);
899- surface->get_create_wait_handle()->wait_for_all();
900+ auto surface = create_surface_with(server_stub, buffer_stream);
901 return surface;
902 }
903
904 std::shared_ptr<MirConnection> connection;
905
906 MirSurfaceSpec const spec{nullptr, 33, 45, mir_pixel_format_abgr_8888};
907- std::shared_ptr<mtd::StubClientBufferStreamFactory> const stub_buffer_stream_factory =
908- std::make_shared<mtd::StubClientBufferStreamFactory>();
909+ std::shared_ptr<mtd::MockClientBufferStream> stub_buffer_stream{std::make_shared<mtd::MockClientBufferStream>()};
910 std::shared_ptr<StubClientInputPlatform> const input_platform =
911 std::make_shared<StubClientInputPlatform>();
912 std::shared_ptr<MockServerPackageGenerator> const mock_server_tool =
913@@ -385,6 +378,8 @@
914 std::shared_ptr<mcl::ConnectionSurfaceMap> surface_map;
915 std::shared_ptr<mt::TestProtobufServer> test_server;
916 std::shared_ptr<mclr::DisplayServer> client_comm_channel;
917+ mir::protobuf::Surface surface_proto;
918+ std::shared_ptr<MirWaitHandle> const wh{std::make_shared<MirWaitHandle>()};
919
920 std::chrono::milliseconds const pause_time{10};
921 };
922@@ -403,22 +398,6 @@
923 }
924 }
925
926-TEST_F(MirClientSurfaceTest, create_wait_handle_really_blocks)
927-{
928- using namespace testing;
929-
930- FakeRpcChannel fake_channel;
931- mclr::DisplayServer unresponsive_server{mt::fake_shared(fake_channel)};
932-
933- auto const surface = create_surface_with(unresponsive_server);
934- auto wait_handle = surface->get_create_wait_handle();
935-
936- auto expected_end = std::chrono::steady_clock::now() + pause_time;
937- wait_handle->wait_for_pending(pause_time);
938-
939- EXPECT_GE(std::chrono::steady_clock::now(), expected_end);
940-}
941-
942 TEST_F(MirClientSurfaceTest, creates_input_thread_with_input_dispatcher_when_delegate_specified)
943 {
944 using namespace ::testing;
945@@ -432,9 +411,7 @@
946 .WillOnce(Return(mock_input_dispatcher));
947
948 MirSurface surface{connection.get(), *client_comm_channel, nullptr,
949- stub_buffer_stream_factory, mock_input_platform, spec, &null_surface_callback, nullptr};
950- auto wait_handle = surface.get_create_wait_handle();
951- wait_handle->wait_for_all();
952+ stub_buffer_stream, mock_input_platform, spec, surface_proto, wh};
953 surface.set_event_handler(null_event_callback, nullptr);
954
955 mock_input_dispatcher->trigger();
956@@ -455,9 +432,7 @@
957 .WillOnce(Return(mock_input_dispatcher));
958
959 MirSurface surface{connection.get(), *client_comm_channel, nullptr,
960- stub_buffer_stream_factory, mock_input_platform, spec, &null_surface_callback, nullptr};
961- auto wait_handle = surface.get_create_wait_handle();
962- wait_handle->wait_for_all();
963+ stub_buffer_stream, mock_input_platform, spec, surface_proto, wh};
964 surface.set_event_handler(null_event_callback, nullptr);
965
966 // Should now not get dispatched.
967@@ -478,9 +453,7 @@
968 EXPECT_CALL(*mock_input_platform, create_input_receiver(_, _, _)).Times(0);
969
970 MirSurface surface{connection.get(), *client_comm_channel, nullptr,
971- stub_buffer_stream_factory, mock_input_platform, spec, &null_surface_callback, nullptr};
972- auto wait_handle = surface.get_create_wait_handle();
973- wait_handle->wait_for_all();
974+ stub_buffer_stream, mock_input_platform, spec, surface_proto, wh};
975 }
976
977 TEST_F(MirClientSurfaceTest, valid_surface_is_valid)
978@@ -531,12 +504,7 @@
979 {
980 using namespace testing;
981 auto mock_stream = std::make_shared<mtd::MockClientBufferStream>();
982- auto mock_stream_factory = std::make_shared<NiceMock<mtd::MockClientBufferStreamFactory>>();
983 auto mock_input_platform = std::make_shared<NiceMock<MockClientInputPlatform>>();
984- ON_CALL(*mock_stream_factory, make_producer_stream(_,_,_,_,An<void*>()))
985- .WillByDefault(Return(mock_stream.get()));
986- ON_CALL(*mock_stream_factory, make_producer_stream(_,_,_,_,An<geom::Size>()))
987- .WillByDefault(Return(mock_stream));
988 ON_CALL(*mock_input_platform, create_input_receiver(_,_,_))
989 .WillByDefault(Return(std::make_shared<mt::TestDispatchable>([]{})));
990 ON_CALL(*mock_stream, rpc_id()).WillByDefault(Return(mir::frontend::BufferStreamId(2)));
991@@ -545,11 +513,8 @@
992 EXPECT_CALL(*mock_stream, set_size(size));
993 auto ev = mir::events::make_event(mir::frontend::SurfaceId(2), size);
994
995- //FIXME: difficult construction
996 MirSurface surface{connection.get(), *client_comm_channel, nullptr,
997- mock_stream_factory, mock_input_platform, spec, &null_surface_callback, nullptr};
998- auto wait_handle = surface.get_create_wait_handle();
999- wait_handle->wait_for_all();
1000+ mock_stream, mock_input_platform, spec, surface_proto, wh};
1001 surface.handle_event(*ev);
1002 surface_map->erase(mir::frontend::BufferStreamId(2));
1003 }
1004@@ -558,13 +523,8 @@
1005 {
1006 using namespace testing;
1007 auto mock_stream = std::make_shared<NiceMock<mtd::MockClientBufferStream>>();
1008- auto mock_stream_factory = std::make_shared<NiceMock<mtd::MockClientBufferStreamFactory>>();
1009 auto mock_input_platform = std::make_shared<NiceMock<MockClientInputPlatform>>();
1010 ON_CALL(*mock_stream, rpc_id()).WillByDefault(Return(mir::frontend::BufferStreamId(2)));
1011- ON_CALL(*mock_stream_factory, make_producer_stream(_,_,_,_,An<void*>()))
1012- .WillByDefault(Return(mock_stream.get()));
1013- ON_CALL(*mock_stream_factory, make_producer_stream(_,_,_,_,An<geom::Size>()))
1014- .WillByDefault(Return(mock_stream));
1015 ON_CALL(*mock_input_platform, create_input_receiver(_,_,_))
1016 .WillByDefault(Return(std::make_shared<mt::TestDispatchable>([]{})));
1017
1018@@ -572,8 +532,7 @@
1019 EXPECT_CALL(*mock_stream, set_size(size)).Times(0);
1020 auto ev = mir::events::make_event(mir::frontend::SurfaceId(2), size);
1021 MirSurface surface{connection.get(), *client_comm_channel, nullptr,
1022- mock_stream_factory, mock_input_platform, spec, &null_surface_callback, nullptr};
1023- surface.get_create_wait_handle()->wait_for_all();
1024+ mock_stream, mock_input_platform, spec, surface_proto, wh};
1025
1026 MirSurfaceSpec spec;
1027 std::vector<MirBufferStreamInfo> info =
1028
1029=== modified file 'tests/unit-tests/client/test_connection_resource_map.cpp'
1030--- tests/unit-tests/client/test_connection_resource_map.cpp 2015-07-16 07:03:19 +0000
1031+++ tests/unit-tests/client/test_connection_resource_map.cpp 2016-01-13 19:10:37 +0000
1032@@ -27,7 +27,8 @@
1033
1034 struct ConnectionResourceMap : testing::Test
1035 {
1036- MirSurface surface{"a string"};
1037+ std::shared_ptr<MirWaitHandle> wh { std::make_shared<MirWaitHandle>() };
1038+ std::shared_ptr<MirSurface> surface{std::make_shared<MirSurface>("a string", nullptr, mf::SurfaceId{2}, wh)};
1039 mtd::MockClientBufferStream stream;
1040 mf::SurfaceId const surface_id{43};
1041 mf::BufferStreamId const stream_id{43};
1042@@ -39,14 +40,14 @@
1043 auto surface_called = false;
1044 auto stream_called = false;
1045 mcl::ConnectionSurfaceMap map;
1046- map.insert(surface_id, &surface);
1047+ map.insert(surface_id, surface);
1048 map.with_surface_do(surface_id, [&](MirSurface* surf) {
1049- EXPECT_THAT(surf, Eq(&surface));
1050+ EXPECT_THAT(surf, Eq(surface.get()));
1051 surface_called = true;
1052 });
1053
1054 map.with_stream_do(stream_id, [&](mcl::ClientBufferStream* stream) {
1055- EXPECT_THAT(stream, Eq(surface.get_buffer_stream()));
1056+ EXPECT_THAT(stream, Eq(surface->get_buffer_stream()));
1057 stream_called = true;
1058 });
1059
1060@@ -59,9 +60,9 @@
1061 using namespace testing;
1062 auto stream_called = false;
1063 mcl::ConnectionSurfaceMap map;
1064- map.insert(surface_id, &surface);
1065+ map.insert(surface_id, surface);
1066 map.with_stream_do(stream_id, [&](mcl::ClientBufferStream* stream) {
1067- EXPECT_THAT(stream, Eq(surface.get_buffer_stream()));
1068+ EXPECT_THAT(stream, Eq(surface->get_buffer_stream()));
1069 stream_called = true;
1070 });
1071 EXPECT_TRUE(stream_called);
1072
1073=== modified file 'tests/unit-tests/client/test_mir_connection.cpp'
1074--- tests/unit-tests/client/test_mir_connection.cpp 2015-07-29 01:39:52 +0000
1075+++ tests/unit-tests/client/test_mir_connection.cpp 2016-01-13 19:10:37 +0000
1076@@ -63,7 +63,7 @@
1077 ON_CALL(*this, watch_fd()).WillByDefault(testing::Return(pollable_fd));
1078 }
1079
1080- void call_method(std::string const& name,
1081+ virtual void call_method(std::string const& name,
1082 google::protobuf::MessageLite const* parameters,
1083 google::protobuf::MessageLite* response,
1084 google::protobuf::Closure* complete)
1085@@ -83,6 +83,13 @@
1086 platform_operation(static_cast<mp::PlatformOperationMessage const*>(parameters),
1087 static_cast<mp::PlatformOperationMessage*>(response));
1088 }
1089+ else if (name == "create_surface")
1090+ {
1091+ auto response_message = static_cast<mp::Surface*>(response);
1092+ response_message->mutable_id()->set_value(33);
1093+ response_message->mutable_buffer_stream()->mutable_id()->set_value(33);
1094+ }
1095+
1096
1097 complete->Run();
1098 }
1099@@ -638,3 +645,30 @@
1100 EXPECT_THAT(mir_platform_message_get_opcode(returned_response), Eq(opcode));
1101 mir_platform_message_release(returned_response);
1102 }
1103+
1104+TEST_F(MirConnectionTest, create_wait_handle_really_blocks)
1105+{
1106+ using namespace testing;
1107+
1108+ std::chrono::milliseconds const pause_time{10};
1109+ struct FakeRpcChannel : public MockRpcChannel
1110+ {
1111+ void call_method(
1112+ std::string const&,
1113+ google::protobuf::MessageLite const*,
1114+ google::protobuf::MessageLite*,
1115+ google::protobuf::Closure* closure) override
1116+ {
1117+ delete closure;
1118+ }
1119+ };
1120+ TestConnectionConfiguration conf{mock_platform, std::make_shared<NiceMock<FakeRpcChannel>>()};
1121+ MirConnection connection(conf);
1122+ MirSurfaceSpec const spec{&connection, 33, 45, mir_pixel_format_abgr_8888};
1123+
1124+ auto wait_handle = connection.create_surface(spec, nullptr, nullptr);
1125+ auto expected_end = std::chrono::steady_clock::now() + pause_time;
1126+ wait_handle->wait_for_pending(pause_time);
1127+
1128+ EXPECT_GE(std::chrono::steady_clock::now(), expected_end);
1129+}

Subscribers

People subscribed via source and target branches