Merge lp:~albaguirre/mir/fix-1498550 into lp:mir

[Alberto Aguirre]

[android] Attempt to recover from errors when posting to an external display

[android] Attempt to recover from errors when posting to an external display

I had some additional logic to handle scenario 3, but with latest lp:mir, it seems scenario 3's root cause may be tied to some object lifetime issues seen in a nested configuration (in which either "pure virtual method call" errors are seen, or exceptions after getting EGL_BADSURFACE.

I also opted to reuse the "hotplug" code path (I had some other logic that just attempted to restart the compositor threads) when handling the external display errors so that display configuration is forced - which in turns restarts the compositor threads (which is what is needed for recovery). This makes the diff much more smaller :)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2983
http://jenkins.qa.ubuntu.com/job/mir-ci/5102/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/4228
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3135
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4170
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/128/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1256
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1256/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/128/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4171
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4171/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6845
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/23858
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/129/console

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

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

Looks good.

Nit:

+ std::function<void()> error_handler

Could be const&

review: Approve

[Alan Griffiths (alan-griffiths)]

Without looking at implementation and usage details the design isn't clear.

You have:

    std::function<void()> error_handler

Now is that something that is to be called in a catch block? In which case an implementer might expect to do...

    try
    {
      rethrow;
    }
    ...
    catch (std::error const& x)
    {
      ...
    }

...to regain exception information (either for logging or for decision making).

But if needn't be in a catch block then the rethrow idiom is a bad idea.

And how much context does the handler need? Would it be better as void (*error_handler)(char const* error_message)

Whichever option is intended could be better expressed in the code.

E.g.

using CurrentExceptionHandler = std::function<void()>

    CurrentExceptionHandler error_handler

review: Needs Information

[Alberto Aguirre (albaguirre)]

"Now is that something that is to be called in a catch block?"

Yes.

[Alan Griffiths (alan-griffiths)]

> "Now is that something that is to be called in a catch block?"
>
> Yes.

could be better expressed in the code (or at least in a comment).

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

OK

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2984
http://jenkins.qa.ubuntu.com/job/mir-ci/5169/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/4313
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3220
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4255
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/213/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1323
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1323/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/213/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4256
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4256/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/6915
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/23996
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/214/console

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

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2985
http://jenkins.qa.ubuntu.com/job/mir-ci/5174/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/4318
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3225
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4260/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/218/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1328
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1328/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/218/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4261
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4261/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/6920/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/24002
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/219/console

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

review: Needs Fixing (continuous-integration)

[Kevin DuBois (kdub)]

lgtm too

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2985
http://jenkins.qa.ubuntu.com/job/mir-ci/5180/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/4324
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3231
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4266
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/224/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1334
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1334/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/224/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4267
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4267/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/6926
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/24009
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/225/console

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

review: Approve (continuous-integration)

=== modified file 'src/platforms/android/server/display.cpp'
--- src/platforms/android/server/display.cpp	2015-10-05 08:19:39 +0000
+++ src/platforms/android/server/display.cpp	2015-10-06 15:33:24 +0000
@@ -166,7 +166,8 @@
             gl_program_factory,
             gl_context,
             geom::Displacement{0,0},
-            overlay_option)),
+            overlay_option),
+            [this] { on_hotplug(); }), //Recover from exception by forcing a configuration change
     overlay_option(overlay_option)
 {
     //Some drivers (depending on kernel state) incorrectly report an error code indicating that the display is already on. Ignore the first failure.

=== renamed file 'src/platforms/android/server/display_disconnected_exception.h' => 'src/platforms/android/server/display_device_exceptions.h'
--- src/platforms/android/server/display_disconnected_exception.h	2015-09-23 19:09:59 +0000
+++ src/platforms/android/server/display_device_exceptions.h	2015-10-06 15:33:24 +0000
@@ -16,10 +16,11 @@
  * Authored by: Alberto Aguirre <[e-mail]>
  */
 
-#ifndef MIR_GRAPHICS_ANDROID_DISPLAY_DISCONNECTED_EXCEPTION_H_
-#define MIR_GRAPHICS_ANDROID_DISPLAY_DISCONNECTED_EXCEPTION_H_
+#ifndef MIR_GRAPHICS_ANDROID_DISPLAY_DEVICE_EXCEPTIONS_H_
+#define MIR_GRAPHICS_ANDROID_DISPLAY_DEVICE_EXCEPTIONS_H_
 
 #include <stdexcept>
+#include <string>
 
 namespace mir
 {
@@ -31,11 +32,17 @@
 class DisplayDisconnectedException : public std::runtime_error
 {
 public:
-    DisplayDisconnectedException() : std::runtime_error("display disconnected") {}
-};
-
-}
-}
-}
-
-#endif /* MIR_GRAPHICS_ANDROID_DISPLAY_DISCONNECTED_EXCEPTION_H_ */
+    DisplayDisconnectedException(std::string const& message) : std::runtime_error(message) {}
+};
+
+class ExternalDisplayError : public std::runtime_error
+{
+public:
+    ExternalDisplayError(std::string const& message) : std::runtime_error(message) {}
+};
+
+}
+}
+}
+
+#endif /* MIR_GRAPHICS_ANDROID_DISPLAY_DEVICE_EXCEPTIONS_H_ */

=== modified file 'src/platforms/android/server/display_group.cpp'
--- src/platforms/android/server/display_group.cpp	2015-09-23 19:09:59 +0000
+++ src/platforms/android/server/display_group.cpp	2015-10-06 15:33:24 +0000
@@ -18,7 +18,7 @@
 
 #include "display_group.h"
 #include "configurable_display_buffer.h"
-#include "display_disconnected_exception.h"
+#include "display_device_exceptions.h"
 #include <boost/throw_exception.hpp>
 #include <stdexcept>
 
@@ -28,12 +28,21 @@
 
 mga::DisplayGroup::DisplayGroup(
     std::shared_ptr<mga::DisplayDevice> const& device,
-    std::unique_ptr<mga::ConfigurableDisplayBuffer> primary_buffer) :
-    device(device)
+    std::unique_ptr<mga::ConfigurableDisplayBuffer> primary_buffer,
+    ExceptionHandler const& exception_handler) :
+    device(device),
+    exception_handler(exception_handler)
 {
     dbs.emplace(std::make_pair(mga::DisplayName::primary, std::move(primary_buffer)));
 }
 
+mga::DisplayGroup::DisplayGroup(
+    std::shared_ptr<mga::DisplayDevice> const& device,
+    std::unique_ptr<mga::ConfigurableDisplayBuffer> primary_buffer)
+    : DisplayGroup(device, std::move(primary_buffer), []{})
+{
+}
+
 void mga::DisplayGroup::for_each_display_buffer(std::function<void(mg::DisplayBuffer&)> const& f)
 {
     std::unique_lock<decltype(guard)> lk(guard);
@@ -87,10 +96,18 @@
     {
         device->commit(contents);
     }
-    catch (mga::DisplayDisconnectedException& e)
+    catch (mga::DisplayDisconnectedException const&)
     {
         //Ignore disconnect errors as they are not fatal
     }
+    catch (mga::ExternalDisplayError const&)
+    {
+        //NOTE: We allow Display to inject an error handler (which can then attempt to recover
+        // from this error) as post is called directly by the compositor and we don't want to propagate
+        // handling of android platform specific exceptions in mir core.
+        exception_handler();
+    }
+
 }
 
 std::chrono::milliseconds mga::DisplayGroup::recommended_sleep() const

=== modified file 'src/platforms/android/server/display_group.h'
--- src/platforms/android/server/display_group.h	2015-09-23 19:09:59 +0000
+++ src/platforms/android/server/display_group.h	2015-10-06 15:33:24 +0000
@@ -38,9 +38,14 @@
 class DisplayGroup : public graphics::DisplaySyncGroup
 {
 public:
+    using ExceptionHandler = std::function<void()>;
     DisplayGroup(
         std::shared_ptr<DisplayDevice> const& device,
-        std::unique_ptr<ConfigurableDisplayBuffer> primary_buffer);
+        std::unique_ptr<ConfigurableDisplayBuffer> primary_buffer,
+        ExceptionHandler const& handler);
+    DisplayGroup(
+           std::shared_ptr<DisplayDevice> const& device,
+           std::unique_ptr<ConfigurableDisplayBuffer> primary_buffer);
 
     void for_each_display_buffer(std::function<void(graphics::DisplayBuffer&)> const& f) override;
     void post() override;
@@ -55,6 +60,7 @@
     std::mutex mutable guard;
     std::shared_ptr<DisplayDevice> const device;
     std::map<DisplayName, std::unique_ptr<ConfigurableDisplayBuffer>> dbs;
+    ExceptionHandler const exception_handler;
 };
 
 }

=== modified file 'src/platforms/android/server/real_hwc_wrapper.cpp'
--- src/platforms/android/server/real_hwc_wrapper.cpp	2015-09-23 19:09:59 +0000
+++ src/platforms/android/server/real_hwc_wrapper.cpp	2015-10-06 15:33:24 +0000
@@ -18,7 +18,7 @@
 
 #include "real_hwc_wrapper.h"
 #include "hwc_report.h"
-#include "display_disconnected_exception.h"
+#include "display_device_exceptions.h"
 #include <boost/throw_exception.hpp>
 #include <stdexcept>
 #include <sstream>
@@ -120,13 +120,17 @@
     if (auto rc = hwc_device->set(hwc_device.get(), num_displays,
         const_cast<hwc_display_contents_1**>(displays.data())))
     {
-        if (num_displays > 1 && !display_connected(DisplayName::external))
-        {
-            BOOST_THROW_EXCEPTION(mga::DisplayDisconnectedException());
-        }
-
         std::stringstream ss;
         ss << "error during hwc set(). rc = " << std::hex << rc;
+
+        if (num_displays > 1)
+        {
+            if (!display_connected(DisplayName::external))
+                BOOST_THROW_EXCEPTION(mga::DisplayDisconnectedException(ss.str()));
+            else
+                BOOST_THROW_EXCEPTION(mga::ExternalDisplayError(ss.str()));
+        }
+
         BOOST_THROW_EXCEPTION(std::runtime_error(ss.str()));
     }
     report->report_set_done(displays);

=== modified file 'tests/unit-tests/graphics/android/test_display_group.cpp'
--- tests/unit-tests/graphics/android/test_display_group.cpp	2015-10-01 08:58:30 +0000
+++ tests/unit-tests/graphics/android/test_display_group.cpp	2015-10-06 15:33:24 +0000
@@ -18,7 +18,7 @@
 
 #include "src/platforms/android/server/configurable_display_buffer.h"
 #include "src/platforms/android/server/display_group.h"
-#include "src/platforms/android/server/display_disconnected_exception.h"
+#include "src/platforms/android/server/display_device_exceptions.h"
 #include "mir/test/doubles/mock_display_device.h"
 #include "mir/test/doubles/stub_renderable_list_compositor.h"
 #include "mir/test/doubles/stub_swapping_gl_context.h"
@@ -91,7 +91,7 @@
     NiceMock<mtd::MockDisplayDevice> mock_device;
     mga::DisplayGroup group(mt::fake_shared(mock_device), std::make_unique<StubConfigurableDB>());
     EXPECT_CALL(mock_device, commit(_))
-        .WillOnce(Throw(mga::DisplayDisconnectedException()))
+        .WillOnce(Throw(mga::DisplayDisconnectedException("")))
         .WillOnce(Throw(std::runtime_error("")));
 
     EXPECT_NO_THROW({group.post();});
@@ -100,3 +100,18 @@
         group.post();
     }, std::runtime_error);
 }
+
+TEST(DisplayGroup, calls_error_handler_on_external_display_error)
+{
+    using namespace testing;
+    NiceMock<mtd::MockDisplayDevice> mock_device;
+    bool error_handler_called{false};
+
+    mga::DisplayGroup group(mt::fake_shared(mock_device), std::make_unique<StubConfigurableDB>(),
+        [&error_handler_called] { error_handler_called = true; });
+    EXPECT_CALL(mock_device, commit(_))
+        .WillOnce(Throw(mga::ExternalDisplayError("")));
+
+    EXPECT_NO_THROW({group.post();});
+    EXPECT_TRUE(error_handler_called);
+}