QtMir

Merge lp:~gerboland/qtmir/tests-use-stack-instead-of-heap into lp:qtmir

tests-use-stack-instead-of-heap
Merge into trunk

Proposed by Gerry Boland on 2016-06-22

Status:	Merged
Approved by:	Daniel d'Andrada on 2016-06-23
Approved revision:	523
Merged at revision:	536
Proposed branch:	lp:~gerboland/qtmir/tests-use-stack-instead-of-heap
Merge into:	lp:qtmir
Diff against target:	513 lines (+73/-110) 1 file modified tests/modules/ApplicationManager/application_manager_test.cpp (+73/-110)
To merge this branch:	bzr merge lp:~gerboland/qtmir/tests-use-stack-instead-of-heap
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Unity8 CI Bot	continuous-integration		Needs Fixing on 2016-06-23
Daniel d'Andrada (community)			Approve on 2016-06-23
Andreas Pokorny (community)		2016-06-22	Approve on 2016-06-23
Review via email: mp+298122@code.launchpad.net

Commit Message

Tests: AppManTest: use stack instead of heap when possible, better cleanup of resources

Unity8 CI Bot (unity8-ci-bot) wrote on 2016-06-22:

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/287/rebuild

review: Needs Fixing (continuous-integration)

Andreas Pokorny (andreas-pokorny) wrote on 2016-06-23:

sure

review: Approve

Daniel d'Andrada (dandrader) wrote on 2016-06-23:

The order of deletion is important. Session will kill itself with deleteLater() once it has no more surfaces.

unexpectedStopOfForegroundWebapp is now leaking a Session object, for instance (I think there are some others like that).

"""
delete surface;

qtApp.sendPostedEvents(nullptr, QEvent::DeferredDelete);
"""

Surface goes away. Session sees that and calls deleteLater() on itself. We then call that sendPostedEvents() to force immediate processing of that deleteLater() call.

review: Needs Fixing

Daniel d'Andrada (dandrader) wrote on 2016-06-23:

Oh, and please also update the copyright header.

lp:~gerboland/qtmir/tests-use-stack-instead-of-heap updated on 2016-06-23

522. By Gerry Boland on 2016-06-23: Copyright
523. By Gerry Boland on 2016-06-23: Undo changes to tests which leaked.

Gerry Boland (gerboland) wrote on 2016-06-23:

> The order of deletion is important. Session will kill itself with
> deleteLater() once it has no more surfaces.
>
> unexpectedStopOfForegroundWebapp is now leaking a Session object, for instance
> (I think there are some others like that).
>
> """
> delete surface;
>
> qtApp.sendPostedEvents(nullptr, QEvent::DeferredDelete);
> """
>
> Surface goes away. Session sees that and calls deleteLater() on itself. We
> then call that sendPostedEvents() to force immediate processing of that
> deleteLater() call.

So I see. I've re-run under Valgrind and restored those tests it indicated I had made leak. Which wasn't all. Which surprised me!

I'll keep in mind that Session is deleting itself - I'm not so sure I like that idea. But, later.

What do you think about it now?

Daniel d'Andrada (dandrader) on 2016-06-23:

review: Approve

Unity8 CI Bot (unity8-ci-bot) wrote on 2016-06-23:

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/290/rebuild

review: Needs Fixing (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Daniel d'Andrada

Gerry Boland

Mir development team

1	=== modified file 'tests/modules/ApplicationManager/application_manager_test.cpp'
2	--- tests/modules/ApplicationManager/application_manager_test.cpp 2016-05-30 14:50:26 +0000
3	+++ tests/modules/ApplicationManager/application_manager_test.cpp 2016-06-23 14:01:51 +0000
4	@@ -1,5 +1,5 @@
5	/*
6	- * Copyright (C) 2013-2015 Canonical, Ltd.
7	+ * Copyright (C) 2013-2016 Canonical, Ltd.
8	*
9	* This program is free software: you can redistribute it and/or modify it under
10	* the terms of the GNU Lesser General Public License version 3, as published by
11	@@ -91,7 +91,7 @@
12	QByteArray cmdLine( "/usr/bin/dialer-app --desktop_file_hint=dialer-app");
13	QByteArray secondcmdLine( "/usr/bin/dialer-app");
14
15	- FakeMirSurface *surface = new FakeMirSurface;
16	+ FakeMirSurface surface;
17
18	EXPECT_CALL(procInfo,command_line(firstProcId))
19	.Times(1)
20	@@ -106,8 +106,8 @@
21	applicationManager.authorizeSession(firstProcId, authed);
22	ASSERT_EQ(true, authed);
23	onSessionStarting(mirSession);
24	- onSessionCreatedSurface(mirSession.get(), surface);
25	- surface->drawFirstFrame();
26	+ onSessionCreatedSurface(mirSession.get(), &surface);
27	+ surface.drawFirstFrame();
28	Application * application = applicationManager.findApplication(dialer_app_id);
29	ASSERT_NE(nullptr,application);
30	ASSERT_EQ(Application::InternalState::Running, application->internalState());
31	@@ -118,8 +118,6 @@
32
33	EXPECT_FALSE(authed);
34	EXPECT_EQ(application, applicationManager.findApplication(dialer_app_id));
35	-
36	- delete surface;
37	}
38
39	TEST_F(ApplicationManagerTests,application_dies_while_starting)
40	@@ -315,7 +313,7 @@
41	const pid_t a_procId = 5921;
42	const char an_app_id[] = "some_app";
43	QByteArray a_cmd( "/usr/bin/app1 --desktop_file_hint=some_app");
44	- FakeMirSurface *aSurface = new FakeMirSurface;
45	+ FakeMirSurface aSurface;
46
47	ON_CALL(procInfo,command_line(_)).WillByDefault(Return(a_cmd));
48
49	@@ -328,8 +326,8 @@
50	applicationManager.authorizeSession(a_procId, authed);
51
52	onSessionStarting(first_session);
53	- onSessionCreatedSurface(first_session.get(), aSurface);
54	- aSurface->drawFirstFrame();
55	+ onSessionCreatedSurface(first_session.get(), &aSurface);
56	+ aSurface.drawFirstFrame();
57	onSessionStarting(second_session);
58
59	Application * the_app = applicationManager.findApplication(an_app_id);
60	@@ -340,7 +338,6 @@
61
62	onSessionStopping(first_session);
63	onSessionStopping(second_session);
64	- delete aSurface;
65	qtApp.sendPostedEvents(nullptr, QEvent::DeferredDelete);
66	}
67
68	@@ -348,7 +345,7 @@
69	{
70	using namespace ::testing;
71	const pid_t procId = 5921;
72	- FakeMirSurface *aSurface = new FakeMirSurface;
73	+ FakeMirSurface aSurface;
74	QByteArray cmdLine( "/usr/bin/app --desktop_file_hint=app");
75
76	EXPECT_CALL(procInfo,command_line(procId))
77	@@ -371,13 +368,11 @@
78
79	// Signal app is ready now
80	applicationManager.onProcessStarting("app");
81	- onSessionCreatedSurface(session.get(), aSurface);
82	- aSurface->drawFirstFrame();
83	+ onSessionCreatedSurface(session.get(), &aSurface);
84	+ aSurface.drawFirstFrame();
85
86	// now that its ready, suspend process should have begun
87	EXPECT_EQ(Application::InternalState::SuspendingWaitSession, app->internalState());
88	-
89	- delete aSurface;
90	}
91
92	TEST_F(ApplicationManagerTests,requestFocusApplication)
93	@@ -759,16 +754,14 @@
94	applicationManager.authorizeSession(procId, authed);
95	onSessionStarting(session);
96
97	- FakeMirSurface *surface = new FakeMirSurface;
98	+ FakeMirSurface surface;
99
100	- onSessionCreatedSurface(session.get(), surface);
101	- surface->drawFirstFrame();
102	+ onSessionCreatedSurface(session.get(), &surface);
103	+ surface.drawFirstFrame();
104
105	// Check application state is correctly set
106	Application *theApp = applicationManager.findApplication(appId);
107	EXPECT_EQ(theApp->state(), Application::Running);
108	-
109	- delete surface;
110	}
111
112	/*
113	@@ -837,13 +830,13 @@
114	applicationManager.authorizeSession(procId, authed);
115	onSessionStarting(session);
116
117	- FakeMirSurface *surface = new FakeMirSurface;
118	- onSessionCreatedSurface(session.get(), surface);
119	+ QScopedPointer<FakeMirSurface> surface(new FakeMirSurface);
120	+ onSessionCreatedSurface(session.get(), surface.data());
121	surface->drawFirstFrame();
122
123	QSignalSpy countSpy(&applicationManager, SIGNAL(countChanged()));
124
125	- QSignalSpy closeRequestedSpy(surface, SIGNAL(closeRequested()));
126	+ QSignalSpy closeRequestedSpy(surface.data(), SIGNAL(closeRequested()));
127
128	// Stop app
129	applicationManager.stopApplication(appId);
130	@@ -853,8 +846,7 @@
131	EXPECT_EQ(1, closeRequestedSpy.count());
132
133	// comply
134	- delete surface;
135	- surface = nullptr;
136	+ surface.reset();
137
138	// now it's the turn of the application process itself to go away, since its last surface has gone
139	EXPECT_EQ(Application::InternalState::Closing, app->internalState());
140	@@ -890,9 +882,9 @@
141	applicationManager.authorizeSession(procId, authed);
142	onSessionStarting(session);
143
144	- FakeMirSurface *surface = new FakeMirSurface;
145	- onSessionCreatedSurface(session.get(), surface);
146	- surface->drawFirstFrame();
147	+ FakeMirSurface surface;
148	+ onSessionCreatedSurface(session.get(), &surface);
149	+ surface.drawFirstFrame();
150
151	QSignalSpy countSpy(&applicationManager, SIGNAL(countChanged()));
152
153	@@ -902,8 +894,6 @@
154
155	EXPECT_EQ(2, countSpy.count()); //FIXME(greyback)
156	EXPECT_EQ(0, applicationManager.count());
157	-
158	- delete surface;
159	}
160
161	/*
162	@@ -930,9 +920,9 @@
163	applicationManager.authorizeSession(procId, authed);
164	onSessionStarting(session);
165
166	- FakeMirSurface *surface = new FakeMirSurface;
167	- onSessionCreatedSurface(session.get(), surface);
168	- surface->drawFirstFrame();
169	+ FakeMirSurface surface;
170	+ onSessionCreatedSurface(session.get(), &surface);
171	+ surface.drawFirstFrame();
172
173	QSignalSpy countSpy(&applicationManager, SIGNAL(countChanged()));
174
175	@@ -946,8 +936,6 @@
176
177	EXPECT_EQ(countSpy.count(), 2); //FIXME(greyback)
178	EXPECT_EQ(applicationManager.count(), 0);
179	-
180	- delete surface;
181	}
182
183	/*
184	@@ -976,9 +964,9 @@
185	applicationManager.authorizeSession(procId, authed);
186	onSessionStarting(session);
187
188	- FakeMirSurface *surface = new FakeMirSurface;
189	- onSessionCreatedSurface(session.get(), surface);
190	- surface->drawFirstFrame();
191	+ FakeMirSurface surface;
192	+ onSessionCreatedSurface(session.get(), &surface);
193	+ surface.drawFirstFrame();
194
195	suspend(app);
196
197	@@ -998,8 +986,6 @@
198
199	EXPECT_EQ(0, countSpy.count());
200	EXPECT_EQ(1, applicationManager.count());
201	-
202	- delete surface;
203	}
204
205	/*
206	@@ -1030,9 +1016,9 @@
207	applicationManager.authorizeSession(procId, authed);
208	onSessionStarting(session);
209
210	- FakeMirSurface *surface = new FakeMirSurface;
211	- onSessionCreatedSurface(session.get(), surface);
212	- surface->drawFirstFrame();
213	+ FakeMirSurface surface;
214	+ onSessionCreatedSurface(session.get(), &surface);
215	+ surface.drawFirstFrame();
216
217	suspend(app);
218
219	@@ -1052,8 +1038,6 @@
220
221	EXPECT_EQ(countSpy.count(), 0);
222	EXPECT_EQ(applicationManager.count(), 1);
223	-
224	- delete surface;
225	}
226
227	/*
228	@@ -1112,9 +1096,9 @@
229	onSessionStarting(session);
230
231	// Associate a surface so AppMan considers app Running, check focused
232	- FakeMirSurface *surface = new FakeMirSurface;
233	- onSessionCreatedSurface(session.get(), surface);
234	- surface->drawFirstFrame();
235	+ FakeMirSurface surface;
236	+ onSessionCreatedSurface(session.get(), &surface);
237	+ surface.drawFirstFrame();
238
239	QSignalSpy countSpy(&applicationManager, SIGNAL(countChanged()));
240
241	@@ -1123,8 +1107,6 @@
242
243	EXPECT_EQ(countSpy.count(), 2); //FIXME(greyback)
244	EXPECT_EQ(applicationManager.count(), 0);
245	-
246	- delete surface;
247	}
248
249	/*
250	@@ -1152,9 +1134,9 @@
251	onSessionStarting(session);
252
253	// Associate a surface so AppMan considers app Running, check focused
254	- FakeMirSurface *surface = new FakeMirSurface;
255	- onSessionCreatedSurface(session.get(), surface);
256	- surface->drawFirstFrame();
257	+ FakeMirSurface surface;
258	+ onSessionCreatedSurface(session.get(), &surface);
259	+ surface.drawFirstFrame();
260
261	QSignalSpy countSpy(&applicationManager, SIGNAL(countChanged()));
262
263	@@ -1166,8 +1148,6 @@
264
265	Application *app = applicationManager.findApplication(appId);
266	EXPECT_EQ(nullptr, app);
267	-
268	- delete surface;
269	}
270
271	/*
272	@@ -1201,9 +1181,9 @@
273	ASSERT_EQ(Application::InternalState::Starting, app->internalState());
274
275	// Associate a surface so AppMan considers app Running
276	- FakeMirSurface *surface = new FakeMirSurface;
277	- onSessionCreatedSurface(session.get(), surface);
278	- surface->drawFirstFrame();
279	+ FakeMirSurface surface;
280	+ onSessionCreatedSurface(session.get(), &surface);
281	+ surface.drawFirstFrame();
282
283	ASSERT_EQ(Application::InternalState::SuspendingWaitSession, app->internalState());
284
285	@@ -1222,8 +1202,6 @@
286	EXPECT_EQ(1, applicationManager.count());
287
288	EXPECT_EQ(Application::Stopped, app->state());
289	-
290	- delete surface;
291	}
292
293	/*
294	@@ -1424,8 +1402,8 @@
295	onSessionStarting(session);
296
297	// App creates surface, puts it in background, then is OOM killed.
298	- FakeMirSurface *surface = new FakeMirSurface;
299	- onSessionCreatedSurface(session.get(), surface);
300	+ QScopedPointer<FakeMirSurface> surface(new FakeMirSurface);
301	+ onSessionCreatedSurface(session.get(), surface.data());
302	surface->drawFirstFrame();
303	suspend(app);
304
305	@@ -1436,7 +1414,7 @@
306
307	EXPECT_EQ(Application::Stopped, app->state());
308
309	- delete surface; surface = nullptr;
310	+ surface.reset();
311
312	// Session should have called deleteLater() on itself, as it's zombie and doesn't hold any surface
313	// But DeferredDelete is special: likes to be called out specifically or it won't come out
314	@@ -1477,9 +1455,9 @@
315	onSessionStarting(session);
316
317	// App creates surface, focuses it so state is running
318	- FakeMirSurface *surface = new FakeMirSurface;
319	- onSessionCreatedSurface(session.get(), surface);
320	- surface->drawFirstFrame();
321	+ FakeMirSurface surface;
322	+ onSessionCreatedSurface(session.get(), &surface);
323	+ surface.drawFirstFrame();
324
325	// Test normal lifecycle management as a control group
326	ASSERT_EQ(Application::InternalState::Running, the_app->internalState());
327	@@ -1514,8 +1492,6 @@
328
329	EXPECT_EQ(Application::Running, the_app->state());
330	ASSERT_EQ(Application::InternalState::Running, the_app->internalState());
331	-
332	- delete surface;
333	}
334
335	/*
336	@@ -1543,9 +1519,9 @@
337	onSessionStarting(session);
338
339	// App creates surface, focuses it so state is running
340	- FakeMirSurface *surface = new FakeMirSurface;
341	- onSessionCreatedSurface(session.get(), surface);
342	- surface->drawFirstFrame();
343	+ FakeMirSurface surface;
344	+ onSessionCreatedSurface(session.get(), &surface);
345	+ surface.drawFirstFrame();
346
347	// Mark app as exempt
348	application->setExemptFromLifecycle(true);
349	@@ -1555,8 +1531,6 @@
350	EXPECT_FALSE(sharedWakelock.enabled());
351	ASSERT_EQ(Application::InternalState::RunningInBackground, application->internalState());
352	EXPECT_EQ(Application::Running, application->state());
353	-
354	- delete surface;
355	}
356
357	/*
358	@@ -1668,9 +1642,9 @@
359	onSessionStarting(session);
360
361	// Have app in fully Running state
362	- FakeMirSurface *aSurface = new FakeMirSurface;
363	- onSessionCreatedSurface(session.get(), aSurface);
364	- aSurface->drawFirstFrame();
365	+ FakeMirSurface aSurface;
366	+ onSessionCreatedSurface(session.get(), &aSurface);
367	+ aSurface.drawFirstFrame();
368	ASSERT_EQ(Application::InternalState::Running, the_app->internalState());
369
370	// Create fake QML cache for this app
371	@@ -1686,8 +1660,6 @@
372
373	EXPECT_EQ(0, applicationManager.count());
374	EXPECT_TRUE(dir.exists());
375	-
376	- delete aSurface;
377	}
378
379	/*
380	@@ -1702,18 +1674,16 @@
381	Application* app = startApplication(procId, appId);
382	std::shared_ptr<mir::scene::Session> session = app->session()->session();
383
384	- FakeMirSurface *surface = new FakeMirSurface;
385	- onSessionCreatedSurface(session.get(), surface);
386	- surface->drawFirstFrame();
387	+ FakeMirSurface surface;
388	+ onSessionCreatedSurface(session.get(), &surface);
389	+ surface.drawFirstFrame();
390
391	- QSignalSpy spy(surface, SIGNAL(closeRequested()));
392	+ QSignalSpy spy(&surface, SIGNAL(closeRequested()));
393
394	// Stop app
395	applicationManager.stopApplication(appId);
396
397	EXPECT_EQ(1, spy.count());
398	-
399	- delete surface;
400	}
401
402
403	@@ -1794,8 +1764,8 @@
404	applicationManager.authorizeSession(procId, authed);
405	onSessionStarting(session);
406
407	- FakeMirSurface *surface = new FakeMirSurface;
408	- onSessionCreatedSurface(session.get(), surface);
409	+ QScopedPointer<FakeMirSurface> surface(new FakeMirSurface);
410	+ onSessionCreatedSurface(session.get(), surface.data());
411	surface->drawFirstFrame();
412
413	EXPECT_EQ(Application::InternalState::Running, app->internalState());
414	@@ -1808,7 +1778,7 @@
415	.Times(1)
416	.WillOnce(Return(true));
417
418	- QSignalSpy closeRequestedSpy(surface, SIGNAL(closeRequested()));
419	+ QSignalSpy closeRequestedSpy(surface.data(), SIGNAL(closeRequested()));
420
421	applicationManager.stopApplication(appId);
422
423	@@ -1817,8 +1787,7 @@
424	EXPECT_EQ(1, closeRequestedSpy.count());
425
426	// comply
427	- delete surface;
428	- surface = nullptr;
429	+ surface.reset();
430
431	// now it's the turn of the application process itself to go away, since its last surface has gone
432	EXPECT_EQ(Application::InternalState::Closing, app->internalState());
433	@@ -1871,17 +1840,17 @@
434	applicationManager.authorizeSession(procId1, authed);
435	onSessionStarting(session1);
436
437	- FakeMirSurface *surface1 = new FakeMirSurface;
438	- surface1->setSession(app1->session());
439	- onSessionCreatedSurface(session1.get(), surface1);
440	- surface1->drawFirstFrame();
441	+ FakeMirSurface surface1;
442	+ surface1.setSession(app1->session());
443	+ onSessionCreatedSurface(session1.get(), &surface1);
444	+ surface1.drawFirstFrame();
445
446	EXPECT_EQ(Application::InternalState::Running, app1->internalState());
447
448	QSignalSpy focusedApplicationIdChangedSpy(&applicationManager,
449	&unityapi::ApplicationManagerInterface::focusedApplicationIdChanged);
450
451	- MirFocusController::instance()->setFocusedSurface(surface1);
452	+ MirFocusController::instance()->setFocusedSurface(&surface1);
453	qtApp.processEvents(); // process queued signal-slot connections
454
455	EXPECT_EQ(1, focusedApplicationIdChangedSpy.count());
456	@@ -1898,28 +1867,24 @@
457	applicationManager.authorizeSession(procId2, authed);
458	onSessionStarting(session2);
459
460	- FakeMirSurface *surface2 = new FakeMirSurface;
461	- surface2->setSession(app2->session());
462	- onSessionCreatedSurface(session2.get(), surface2);
463	- surface2->drawFirstFrame();
464	+ FakeMirSurface surface2;
465	+ surface2.setSession(app2->session());
466	+ onSessionCreatedSurface(session2.get(), &surface2);
467	+ surface2.drawFirstFrame();
468
469	EXPECT_EQ(Application::InternalState::Running, app2->internalState());
470
471	- MirFocusController::instance()->setFocusedSurface(surface2);
472	+ MirFocusController::instance()->setFocusedSurface(&surface2);
473	qtApp.processEvents(); // process queued signal-slot connections
474
475	EXPECT_EQ(2, focusedApplicationIdChangedSpy.count());
476	EXPECT_EQ(appId2, applicationManager.focusedApplicationId());
477
478	- MirFocusController::instance()->setFocusedSurface(surface1);
479	+ MirFocusController::instance()->setFocusedSurface(&surface1);
480	qtApp.processEvents(); // process queued signal-slot connections
481
482	EXPECT_EQ(3, focusedApplicationIdChangedSpy.count());
483	EXPECT_EQ(appId1, applicationManager.focusedApplicationId());
484	-
485	- // clean up
486	- delete surface1;
487	- delete surface2;
488	}
489
490	/*
491	@@ -1949,18 +1914,16 @@
492	applicationManager.authorizeSession(procId, authed);
493	onSessionStarting(session);
494
495	- FakeMirSurface *surface = new FakeMirSurface;
496	- onSessionCreatedSurface(session.get(), surface);
497	- surface->drawFirstFrame();
498	+ FakeMirSurface surface;
499	+ onSessionCreatedSurface(session.get(), &surface);
500	+ surface.drawFirstFrame();
501
502	EXPECT_EQ(Application::InternalState::Running, app->internalState());
503
504	QSignalSpy focusRequestedSpy(&applicationManager,
505	&unityapi::ApplicationManagerInterface::focusRequested);
506
507	- surface->requestFocus();
508	+ surface.requestFocus();
509
510	EXPECT_EQ(1, focusRequestedSpy.count());
511	-
512	- delete surface;
513	}

QtMir

Merge lp:~gerboland/qtmir/tests-use-stack-instead-of-heap into lp:qtmir

Commit Message

Description of the Change

Preview Diff

Subscribers