Merge lp:~thomas-voss/platform-api/fix-1338610 into lp:platform-api

[Thomas Voß]

Check for null in functions accessing a session object.
  Make sure that exceptions cannot propagate for controller interactions.

Check for null in functions accessing a session object.
Make sure that exceptions cannot propagate for controller interactions.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:247
http://jenkins.qa.ubuntu.com/job/platform-api-ci/346/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/platform-api-utopic-amd64-ci/79
    SUCCESS: http://jenkins.qa.ubuntu.com/job/platform-api-utopic-armhf-ci/79
        deb: http://jenkins.qa.ubuntu.com/job/platform-api-utopic-armhf-ci/79/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/platform-api-utopic-i386-ci/79

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/platform-api-ci/346/rebuild

review: Approve (continuous-integration)

[Charles Kerr (charlesk)]

I'd forgotten that there are alphabetical equivalents of the bitwise operators, so "if (not session)" sent me reaching for my C++ book... :)

Anyway, the code looks straightforward and reasonable, nice to see e.what() logged when available.

review: Approve

=== modified file 'src/ubuntu/application/common/application/location/controller.cpp'
--- src/ubuntu/application/common/application/location/controller.cpp	2014-06-23 13:09:01 +0000
+++ src/ubuntu/application/common/application/location/controller.cpp	2014-07-28 20:16:38 +0000
@@ -55,19 +55,30 @@
 
     *out_flags = 0;
 
-    auto service = Instance::instance().get_service();
-
-    if (service->is_online().get())
-        *out_flags |= UA_LOCATION_SERVICE_ENABLED;
-    else
-        *out_flags |= UA_LOCATION_SERVICE_DISABLED;
-
-    if (service->does_satellite_based_positioning().get())
-        *out_flags |= UA_LOCATION_SERVICE_GPS_ENABLED;
-    else
-        *out_flags |= UA_LOCATION_SERVICE_GPS_DISABLED;
-
-    return U_STATUS_SUCCESS;
+    try
+    {
+        auto service = Instance::instance().get_service();
+
+        if (service->is_online().get())
+            *out_flags |= UA_LOCATION_SERVICE_ENABLED;
+        else
+            *out_flags |= UA_LOCATION_SERVICE_DISABLED;
+
+        if (service->does_satellite_based_positioning().get())
+            *out_flags |= UA_LOCATION_SERVICE_GPS_ENABLED;
+        else
+            *out_flags |= UA_LOCATION_SERVICE_GPS_DISABLED;
+
+        return U_STATUS_SUCCESS;
+    } catch(const std::exception& e)
+    {
+        std::cerr << "ua_location_service_controller_query_status: error accessing instance: " << e.what() << std::endl;
+    } catch(...)
+    {
+        std::cerr << "ua_location_service_controller_query_status: error accessing instance." << std::endl;
+    }
+
+    return U_STATUS_ERROR;
 }
 
 UStatus
@@ -76,10 +87,21 @@
 {
     (void) controller;
 
-    auto service = Instance::instance().get_service();
-    service->is_online().set(true);
-
-    return U_STATUS_SUCCESS;
+    try
+    {
+        auto service = Instance::instance().get_service();
+        service->is_online().set(true);
+
+        return U_STATUS_SUCCESS;
+    } catch(const std::exception& e)
+    {
+        std::cerr << "ua_location_service_controller_enable_service: error accessing instance: " << e.what() << std::endl;
+    } catch(...)
+    {
+        std::cerr << "ua_location_service_controller_enable_service: error accessing instance." << std::endl;
+    }
+
+    return U_STATUS_ERROR;
 }
 
 UStatus
@@ -88,10 +110,21 @@
 {
     (void) controller;
 
-    auto service = Instance::instance().get_service();
-    service->is_online().set(false);
-    
-    return U_STATUS_SUCCESS;
+    try
+    {
+        auto service = Instance::instance().get_service();
+        service->is_online().set(false);
+
+        return U_STATUS_SUCCESS;
+    } catch(const std::exception& e)
+    {
+        std::cerr << "ua_location_service_controller_disable_service: error accessing instance: " << e.what() << std::endl;
+    } catch(...)
+    {
+        std::cerr << "ua_location_service_controller_disable_service: error accessing instance." << std::endl;
+    }
+
+    return U_STATUS_ERROR;
 }
 
 UStatus
@@ -100,10 +133,21 @@
 {
     (void) controller;
 
-    auto service = Instance::instance().get_service();
-    service->does_satellite_based_positioning().set(true);
-
-    return U_STATUS_SUCCESS;
+    try
+    {
+        auto service = Instance::instance().get_service();
+        service->does_satellite_based_positioning().set(true);
+
+        return U_STATUS_SUCCESS;
+    } catch(const std::exception& e)
+    {
+        std::cerr << "ua_location_service_controller_enable_gps: error accessing instance: " << e.what() << std::endl;
+    } catch(...)
+    {
+        std::cerr << "ua_location_service_controller_enable_gps: error accessing instance." << std::endl;
+    }
+
+    return U_STATUS_ERROR;
 }
 
 UStatus
@@ -112,8 +156,19 @@
 {
     (void) controller;
 
-    auto service = Instance::instance().get_service();
-    service->does_satellite_based_positioning().set(false);
-
-    return U_STATUS_SUCCESS;
+    try
+    {
+        auto service = Instance::instance().get_service();
+        service->does_satellite_based_positioning().set(false);
+
+        return U_STATUS_SUCCESS;
+    } catch(const std::exception& e)
+    {
+        std::cerr << "ua_location_service_controller_disable_gps: error accessing instance: " << e.what() << std::endl;
+    } catch(...)
+    {
+        std::cerr << "ua_location_service_controller_disable_gps: error accessing instance." << std::endl;
+    }
+
+    return U_STATUS_ERROR;
 }

=== modified file 'src/ubuntu/application/common/application/location/instance.h'
--- src/ubuntu/application/common/application/location/instance.h	2014-06-23 11:17:42 +0000
+++ src/ubuntu/application/common/application/location/instance.h	2014-07-28 20:16:38 +0000
@@ -56,10 +56,16 @@
 
     ~Instance() noexcept
     {
-        bus->stop();
+        try
+        {
+            bus->stop();
 
-        if (worker.joinable())
-            worker.join();
+            if (worker.joinable())
+                worker.join();
+        } catch(...)
+        {
+            // We silently ignore errors to fulfill our noexcept guarantee.
+        }
     }
 
     core::dbus::Bus::Ptr bus;

=== modified file 'src/ubuntu/application/common/application/location/service.cpp'
--- src/ubuntu/application/common/application/location/service.cpp	2014-06-24 06:22:58 +0000
+++ src/ubuntu/application/common/application/location/service.cpp	2014-07-28 20:16:38 +0000
@@ -35,29 +35,54 @@
 ua_location_service_create_session_for_low_accuracy(
     UALocationServiceRequirementsFlags /*flags*/)
 {
-    return new UbuntuApplicationLocationServiceSession{
-        Instance::instance().get_service()->create_session_for_criteria(cul::Criteria{})};
+    // Creating the instance might fail for a number of reason and
+    // we cannot allow exceptions to propagate to prevent applications
+    // from aborting. For that, we catch all exceptions, provide some error
+    // information to std::cerr and return a nullptr in case of errors.
+    try
+    {
+        return new UbuntuApplicationLocationServiceSession
+        {
+            // Creating the instance might fail for a number of reasons.
+
+            Instance::instance().get_service()->create_session_for_criteria(cul::Criteria{})
+        };
+    } catch(const std::exception& e)
+    {
+        std::cerr << "ua_location_service_create_session_for_low_accuracy: Error creating instance: " << e.what() << std::endl;
+    } catch(...)
+    {
+        std::cerr << "ua_location_service_create_session_for_low_accuracy: Error creating instance." << std::endl;
+    }
+
+    return nullptr;
 }
 
 UALocationServiceSession*
 ua_location_service_create_session_for_high_accuracy(
     UALocationServiceRequirementsFlags /*flags*/)
 {
+    // Creating the instance might fail for a number of reason and
+    // we cannot allow exceptions to propagate to prevent applications
+    // from aborting. For that, we catch all exceptions, provide some error
+    // information to std::cerr and return a nullptr in case of errors.
     try
     {
         return new UbuntuApplicationLocationServiceSession
         {
+            // Creating the instance might fail for a number of reasons.
+
             Instance::instance().get_service()->create_session_for_criteria(cul::Criteria{})
         };
     } catch(const std::exception& e)
     {
-        fprintf(stderr, "Error creating session for high accuracy: %s \n", e.what());
+        std::cerr << "ua_location_service_create_session_for_high_accuracy: Error creating instance: " << e.what() << std::endl;
     } catch(...)
     {
-        fprintf(stderr, "Error creating session for high accuracy.\n");
+        std::cerr << "ua_location_service_create_session_for_high_accuracy: Error creating instance." << std::endl;
     }
 
-    return NULL;
+    return nullptr;
 }
 
 UALocationServiceController*

=== modified file 'src/ubuntu/application/common/application/location/session.cpp'
--- src/ubuntu/application/common/application/location/session.cpp	2014-06-26 15:07:29 +0000
+++ src/ubuntu/application/common/application/location/session.cpp	2014-07-28 20:16:38 +0000
@@ -30,6 +30,9 @@
 ua_location_service_session_ref(
     UALocationServiceSession *session)
 {
+    if (not session)
+        return;
+
     auto s = static_cast<UbuntuApplicationLocationServiceSession*>(session);
     s->ref();
 }
@@ -38,6 +41,9 @@
 ua_location_service_session_unref(
     UALocationServiceSession *session)
 {
+    if (not session)
+        return;
+
     auto s = static_cast<UbuntuApplicationLocationServiceSession*>(session);
     s->unref();
 }
@@ -48,7 +54,11 @@
     UALocationServiceSessionPositionUpdatesHandler handler,
     void *context)
 {
+    if (not session)
+        return;
+
     auto s = static_cast<UbuntuApplicationLocationServiceSession*>(session);
+
     try
     {
         std::lock_guard<std::mutex> lg(s->position_updates.guard);
@@ -69,7 +79,11 @@
     UALocationServiceSessionHeadingUpdatesHandler handler,
     void *context)
 {
+    if (not session)
+        return;
+
     auto s = static_cast<UbuntuApplicationLocationServiceSession*>(session);
+
     try
     {
         std::lock_guard<std::mutex> lg(s->heading_updates.guard);
@@ -90,7 +104,11 @@
     UALocationServiceSessionVelocityUpdatesHandler handler,
     void *context)
 {
+    if (not session)
+        return;
+
     auto s = static_cast<UbuntuApplicationLocationServiceSession*>(session);
+
     try
     {
         std::lock_guard<std::mutex> lg(s->velocity_updates.guard);
@@ -109,9 +127,10 @@
 ua_location_service_session_start_position_updates(
     UALocationServiceSession *session)
 {
+    if (not session)
+        return U_STATUS_ERROR;
+
     auto s = static_cast<UbuntuApplicationLocationServiceSession*>(session);
-    if (!s)
-        return U_STATUS_ERROR;
 
     try
     {
@@ -129,9 +148,10 @@
 ua_location_service_session_stop_position_updates(
     UALocationServiceSession *session)
 {
+    if (not session)
+        return;
+
     auto s = static_cast<UbuntuApplicationLocationServiceSession*>(session);
-    if (!s)
-        return;
 
     try
     {
@@ -146,9 +166,10 @@
 ua_location_service_session_start_heading_updates(
     UALocationServiceSession *session)
 {
+    if (not session)
+        return U_STATUS_ERROR;
+
     auto s = static_cast<UbuntuApplicationLocationServiceSession*>(session);
-    if (!s)
-        return U_STATUS_ERROR;
 
     try
     {
@@ -166,9 +187,10 @@
 ua_location_service_session_stop_heading_updates(
     UALocationServiceSession *session)
 {
+    if (not session)
+        return;
+
     auto s = static_cast<UbuntuApplicationLocationServiceSession*>(session);
-    if (!s)
-        return;
 
     try
     {
@@ -183,9 +205,10 @@
 ua_location_service_session_start_velocity_updates(
     UALocationServiceSession *session)
 {
+    if (not session)
+        return U_STATUS_ERROR;
+
     auto s = static_cast<UbuntuApplicationLocationServiceSession*>(session);
-    if (!s)
-        return U_STATUS_ERROR;
 
     try
     {
@@ -203,9 +226,10 @@
 ua_location_service_session_stop_velocity_updates(
     UALocationServiceSession *session)
 {
+    if (not session)
+        return;
+
     auto s = static_cast<UbuntuApplicationLocationServiceSession*>(session);
-    if (!s)
-        return;
 
     try
     {