Merge lp:~albaguirre/platform-api/fix-1206146 into lp:platform-api

[Alberto Aguirre]

Fix memory leak on sensor event notifications (LP: #1206146)

Fix memory leak on sensor event notifications (LP: #1206146)

The object instantiated by make_holder was leaked.

I did not see why two heap allocations and a copy where needed for an object whose lifetime is only the duration of the notification callback.

The callback receives an opaque pointer to such object and no api is provided to delete the event object provided.

So I've opted to just make a stack allocation.

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[Thomas Voß (thomas-voss)]

LGTM.

review: Approve

[Ricardo Salveti (rsalveti)]

From IRC (sorry, missed your ping):
<AlbertA2> rsalveti: do I need to go through the silo process for this: https://code.launchpad.net/~albaguirre/platform-api/fix-1206146/+merge/235066 to get it merged?
<AlbertA2> rsalveti: given that it's only touching android code...
<AlbertA2> rsalveti: I'm unsure of what the process is in this situations...

Yeah, please create a silo, test, and land as a package. Once that's done, ping me and I'll sync the code on the android side.

Thanks!

=== modified file 'android/default/default_ubuntu_application_sensor.cpp'
--- android/default/default_ubuntu_application_sensor.cpp	2014-05-27 07:54:20 +0000
+++ android/default/default_ubuntu_application_sensor.cpp	2014-09-17 23:40:39 +0000
@@ -33,21 +33,6 @@
 
 namespace
 {
-template<typename T>
-struct Holder
-{
-    Holder(const T&value = T()) : value(value)
-    {
-    }
-
-    T value;
-};
-
-template<typename T>
-Holder<T>* make_holder(const T& value)
-{
-    return new Holder<T>(value);
-}
 
 enum sensor_value_t { MIN_DELAY, MIN_VALUE, MAX_VALUE, RESOLUTION };
 template<ubuntu::application::sensors::SensorType sensor_type>
@@ -56,7 +41,8 @@
     SensorListener() : on_accelerometer_event(NULL),
                        on_proximity_event(NULL),
                        on_light_event(NULL),
-                       on_orientation_event(NULL)
+                       on_orientation_event(NULL),
+                       context(nullptr)
     {
     }
 
@@ -69,16 +55,15 @@
                 if (!on_orientation_event)
                     return;
 
-                ubuntu::application::sensors::OrientationEvent::Ptr ev(
-                    new ubuntu::application::sensors::OrientationEvent(
+                ubuntu::application::sensors::OrientationEvent ev(
                         reading->timestamp,
                         reading->vector[0],
                         reading->vector[1],
-                        reading->vector[2])
+                        reading->vector[2]
                         );
 
                 on_orientation_event(
-                    make_holder(ev), this->context
+                    &ev, this->context
                     );
 
                 break;
@@ -88,16 +73,15 @@
                 if (!on_accelerometer_event)
                     return;
 
-                ubuntu::application::sensors::AccelerometerEvent::Ptr ev(
-                    new ubuntu::application::sensors::AccelerometerEvent(
+                ubuntu::application::sensors::AccelerometerEvent ev(
                         reading->timestamp,
                         reading->acceleration[0],
                         reading->acceleration[1],
-                        reading->acceleration[2])
+                        reading->acceleration[2]
                         );
 
                 on_accelerometer_event(
-                    make_holder(ev), this->context
+                    &ev, this->context
                     );
 
                 break;
@@ -107,14 +91,13 @@
                 if (!on_proximity_event)
                     return;
 
-                ubuntu::application::sensors::ProximityEvent::Ptr ev(
-                    new ubuntu::application::sensors::ProximityEvent(
+                ubuntu::application::sensors::ProximityEvent ev(
                         static_cast<uint64_t>(reading->timestamp),
-                        reading->distance)
+                        reading->distance
                     );
 
                 on_proximity_event(
-                    make_holder(ev), this->context
+                    &ev, this->context
                     );
 
                 break;
@@ -124,14 +107,13 @@
                 if (!on_light_event)
                     return;
 
-                ubuntu::application::sensors::LightEvent::Ptr ev(
-                    new ubuntu::application::sensors::LightEvent(
+                ubuntu::application::sensors::LightEvent ev(
                         reading->timestamp,
-                        reading->light)
+                        reading->light
                     );
 
                 on_light_event(
-                    make_holder(ev), this->context
+                    &ev, this->context
                     );
 
                 break;
@@ -309,18 +291,18 @@
 uas_proximity_event_get_timestamp(
     UASProximityEvent* event)
 {
-    auto ev = static_cast<Holder<ubuntu::application::sensors::ProximityEvent::Ptr>*>(event);
+    auto ev = static_cast<ubuntu::application::sensors::ProximityEvent*>(event);
 
-    return ev->value->get_timestamp();
+    return ev->get_timestamp();
 }
 
 UASProximityDistance
 uas_proximity_event_get_distance(
     UASProximityEvent* event)
 {
-    auto ev = static_cast<Holder<ubuntu::application::sensors::ProximityEvent::Ptr>*>(event);
+    auto ev = static_cast<ubuntu::application::sensors::ProximityEvent*>(event);
 
-    if (ev->value->get_distance() == proximity->max_value())
+    if (ev->get_distance() == proximity->max_value())
         return U_PROXIMITY_FAR;
 
     return U_PROXIMITY_NEAR;
@@ -469,8 +451,8 @@
 uas_light_event_get_timestamp(
     UASLightEvent* event)
 {
-    auto ev = static_cast<Holder<ubuntu::application::sensors::LightEvent::Ptr>*>(event);
-    return ev->value->get_timestamp();
+    auto ev = static_cast<ubuntu::application::sensors::LightEvent*>(event);
+    return ev->get_timestamp();
 }
 
 UStatus
@@ -481,8 +463,8 @@
     if (event == NULL || value == NULL)
         return U_STATUS_ERROR;
 
-    auto ev = static_cast<Holder<ubuntu::application::sensors::LightEvent::Ptr>*>(event);
-    *value = ev->value->get_light();
+    auto ev = static_cast<ubuntu::application::sensors::LightEvent*>(event);
+    *value = ev->get_light();
 
     return U_STATUS_SUCCESS;
 }
@@ -629,8 +611,8 @@
 uas_accelerometer_event_get_timestamp(
     UASAccelerometerEvent* event)
 {
-    auto ev = static_cast<Holder<ubuntu::application::sensors::AccelerometerEvent::Ptr>*>(event);
-    return ev->value->get_timestamp();
+    auto ev = static_cast<ubuntu::application::sensors::AccelerometerEvent*>(event);
+    return ev->get_timestamp();
 }
 
 UStatus
@@ -641,8 +623,8 @@
     if (event == NULL || value == NULL)
         return U_STATUS_ERROR;
     
-    auto ev = static_cast<Holder<ubuntu::application::sensors::AccelerometerEvent::Ptr>*>(event);
-    *value = ev->value->get_x();
+    auto ev = static_cast<ubuntu::application::sensors::AccelerometerEvent*>(event);
+    *value = ev->get_x();
 
     return U_STATUS_SUCCESS;
 }
@@ -655,8 +637,8 @@
     if (event == NULL || value == NULL)
         return U_STATUS_ERROR;
 
-    auto ev = static_cast<Holder<ubuntu::application::sensors::AccelerometerEvent::Ptr>*>(event);
-    *value = ev->value->get_y();
+    auto ev = static_cast<ubuntu::application::sensors::AccelerometerEvent*>(event);
+    *value = ev->get_y();
 
     return U_STATUS_SUCCESS;
 }
@@ -669,8 +651,8 @@
     if (event == NULL || value == NULL)
         return U_STATUS_ERROR;
 
-    auto ev = static_cast<Holder<ubuntu::application::sensors::AccelerometerEvent::Ptr>*>(event);
-    *value = ev->value->get_z();
+    auto ev = static_cast<ubuntu::application::sensors::AccelerometerEvent*>(event);
+    *value = ev->get_z();
 
     return U_STATUS_SUCCESS;
 }
@@ -817,8 +799,8 @@
 uas_orientation_event_get_timestamp(
     UASOrientationEvent* event)
 {
-    auto ev = static_cast<Holder<ubuntu::application::sensors::OrientationEvent::Ptr>*>(event);
-    return ev->value->get_timestamp();
+    auto ev = static_cast<ubuntu::application::sensors::OrientationEvent*>(event);
+    return ev->get_timestamp();
 }
 
 UStatus
@@ -828,9 +810,9 @@
 {
     if (event == NULL || value == NULL)
         return U_STATUS_ERROR;
-    
-    auto ev = static_cast<Holder<ubuntu::application::sensors::OrientationEvent::Ptr>*>(event);
-    *value = ev->value->get_azimuth();
+
+    auto ev = static_cast<ubuntu::application::sensors::OrientationEvent*>(event);
+    *value = ev->get_azimuth();
 
     return U_STATUS_SUCCESS;
 }
@@ -843,8 +825,8 @@
     if (event == NULL || value == NULL)
         return U_STATUS_ERROR;
 
-    auto ev = static_cast<Holder<ubuntu::application::sensors::OrientationEvent::Ptr>*>(event);
-    *value = ev->value->get_pitch();
+    auto ev = static_cast<ubuntu::application::sensors::OrientationEvent*>(event);
+    *value = ev->get_pitch();
 
     return U_STATUS_SUCCESS;
 }
@@ -857,8 +839,8 @@
     if (event == NULL || value == NULL)
         return U_STATUS_ERROR;
 
-    auto ev = static_cast<Holder<ubuntu::application::sensors::OrientationEvent::Ptr>*>(event);
-    *value = ev->value->get_roll();
+    auto ev = static_cast<ubuntu::application::sensors::OrientationEvent*>(event);
+    *value = ev->get_roll();
 
     return U_STATUS_SUCCESS;
 }

=== modified file 'android/include/private/application/sensors/events.h'
--- android/include/private/application/sensors/events.h	2014-05-27 06:56:28 +0000
+++ android/include/private/application/sensors/events.h	2014-09-17 23:40:39 +0000
@@ -28,7 +28,7 @@
 {
 namespace sensors
 {
-class OrientationEvent : public platform::ReferenceCountedBase
+class OrientationEvent
 {
 public:
     OrientationEvent(uint64_t timestamp, float azimuth, float pitch, float roll)
@@ -38,8 +38,6 @@
           roll(roll)
     {}
 
-    typedef ubuntu::platform::shared_ptr<OrientationEvent> Ptr;
-
     uint64_t get_timestamp()
     {
         return this->timestamp;
@@ -56,13 +54,11 @@
     float roll;
 
 protected:
-    virtual ~OrientationEvent() {}
-
     OrientationEvent(const OrientationEvent&) = delete;
     OrientationEvent& operator=(const OrientationEvent&) = delete;
 };
 
-class AccelerometerEvent : public platform::ReferenceCountedBase
+class AccelerometerEvent
 {
 public:
     AccelerometerEvent(uint64_t timestamp, float x, float y, float z)
@@ -72,8 +68,6 @@
           z(z)
     {}
 
-    typedef ubuntu::platform::shared_ptr<AccelerometerEvent> Ptr;
-
     uint64_t get_timestamp()
     {
         return this->timestamp;
@@ -90,20 +84,16 @@
     float z;
 
 protected:
-    virtual ~AccelerometerEvent() {}
-
     AccelerometerEvent(const AccelerometerEvent&) = delete;
     AccelerometerEvent& operator=(const AccelerometerEvent&) = delete;
 };
 
-class ProximityEvent : public platform::ReferenceCountedBase
+class ProximityEvent
 {
 public:
     ProximityEvent(uint64_t timestamp, float distance) : timestamp(timestamp),
                                                          distance(distance)
     {}
-    
-    typedef ubuntu::platform::shared_ptr<ProximityEvent> Ptr;
 
     uint64_t get_timestamp()
     {
@@ -117,20 +107,16 @@
     float distance;
 
 protected:
-    virtual ~ProximityEvent() {}
-
     ProximityEvent(const ProximityEvent&) = delete;
     ProximityEvent& operator=(const ProximityEvent&) = delete;
 };
 
-class LightEvent : public platform::ReferenceCountedBase
+class LightEvent
 {
 public:
     LightEvent(uint64_t timestamp, float light) : timestamp(timestamp),
                                                   light(light)
     {}
-    
-    typedef ubuntu::platform::shared_ptr<LightEvent> Ptr;
 
     uint64_t get_timestamp()
     {
@@ -144,8 +130,6 @@
     float light;
 
 protected:
-    virtual ~LightEvent() {}
-
     LightEvent(const LightEvent&) = delete;
     LightEvent& operator=(const LightEvent&) = delete;
 };