Merge lp:~mzanetti/mir/dont-crash-when-shooting-invalid-surface into lp:mir

[Michael Zanetti]

don't crash when we're trying to screenshot a non-existent surface.

This might happen when an app crashes and apport tracing it delays the applicationRemoved() for a couple of secs in which unity doesn't know the app is gone and might try to shoot it.

Proposed patch originally from Alexandros.

[lp:~mzanetti/mir/dont-crash-when-shooting-invalid-surface]

1416. By Michael Zanetti on 2014-02-20

don't crash when we're trying to screenshot a non-existent surface.

This might happen when an app crashes and apport tracing it delays the applicationRemoved() for a couple of secs in which unity doesn't know the app is gone and might try to shoot it.

[Alan Griffiths (alan-griffiths)]

Would be nice to have a test. But OK for now.

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1177
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/908/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/996
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/994/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/588/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/638/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/645/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/589/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/908/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1416
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/911/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/999
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/997/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/591/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/641/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/648/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/592/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/911/rebuild

review: Needs Fixing (continuous-integration)

[Kevin DuBois (kdub)]

looks like the unit tests need updating:
[ FAILED ] ApplicationSession.uses_snapshot_strategy (91 ms)

would it be better to throw here, and catch somewhere in unity-mir?

I patched up a throwing version with a test: lp:~mir-team/mir/dont-crash-when-shooting-invalid-surface if you're interested

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

I feel this syntax does not scale well:
   msh::Snapshot{{},{},nullptr}
It would probably be nicer to declare a const null_snapshot or something. But it's no big deal.

Kevin: I don't think throwing an exception is helpful. If the default_surface can be null under normal circumstances then I think take_snapshot should succeed transparently as in this proposal.

Also, there would seem to be a race between default_surface and take_snapshot_of. I'm assuming the only reason it's not racing is because surface is a shared_ptr.

Finally, I agree if the bug is simply crashing then it's a good candidate for an automated test case. But won't block on that. We should always welcome contributions from outside the team :)

review: Approve

[Daniel van Vugt (vanvugt)]

Oops.

[ RUN ] ApplicationSession.uses_snapshot_strategy
/tmp/buildd/mir-0.1.5+14.04.20140212/tests/unit-tests/scene/test_application_session.cpp:262: Failure
Actual function call count doesn't match EXPECT_CALL(*snapshot_strategy, take_snapshot_of(_,_))...
         Expected: to be called once
           Actual: never called - unsatisfied and active
unknown file: Failure
C++ exception with description "bad_function_call" thrown in the test body.
[ FAILED ] ApplicationSession.uses_snapshot_strategy (92 ms)

review: Needs Fixing

[Alexandros Frantzis (afrantzis)]

Superseded by https://code.launchpad.net/~afrantzis/mir/null-snapshot-for-session-without-surface/+merge/207932

Unmerged revisions

1416. By Michael Zanetti on 2014-02-20

don't crash when we're trying to screenshot a non-existent surface.

This might happen when an app crashes and apport tracing it delays the applicationRemoved() for a couple of secs in which unity doesn't know the app is gone and might try to shoot it.

=== modified file 'src/server/scene/application_session.cpp'
--- src/server/scene/application_session.cpp	2014-02-07 12:54:45 +0000
+++ src/server/scene/application_session.cpp	2014-02-20 17:34:55 +0000
@@ -99,7 +99,11 @@
 
 void ms::ApplicationSession::take_snapshot(msh::SnapshotCallback const& snapshot_taken)
 {
-    snapshot_strategy->take_snapshot_of(default_surface(), snapshot_taken);
+    auto surface = default_surface();
+    if (surface)
+        snapshot_strategy->take_snapshot_of(surface, snapshot_taken);
+    else
+        snapshot_taken(msh::Snapshot{{},{},nullptr});
 }
 
 std::shared_ptr<msh::Surface> ms::ApplicationSession::default_surface() const