Merge lp:~osomon/oxide/geolocation-doesnt-like-nans into lp:~oxide-developers/oxide/oxide.trunk

[Olivier Tilloy]

Shield ourselves against location sources that return invalid values for attributes (horizontal accuracy, vertical accuracy, speed).
Chromium silently fails to deliver a position update if the horizontal accuracy is NaN.

[David Barth (dbarth)]

Well done for debugging the problem this far !

On Tue, Sep 23, 2014 at 5:59 PM, Olivier Tilloy <
<email address hidden>> wrote:

> Olivier Tilloy has proposed merging
> lp:~osomon/oxide/geolocation-doesnt-like-nans into lp:oxide.
>
> Commit message:
> Shield ourselves against location sources that return invalid values for
> attributes (horizontal accuracy, vertical accuracy, speed).
> Chromium silently fails to deliver a position update if the horizontal
> accuracy is NaN.
>
> Requested reviews:
> Chris Coulson (chrisccoulson)
> Related bugs:
> Bug #1371166 in Oxide: "HERE location does not work in browser, but
> works in osmtouch"
> https://bugs.launchpad.net/oxide/+bug/1371166
>
> For more details, see:
>
> https://code.launchpad.net/~osomon/oxide/geolocation-doesnt-like-nans/+merge/235667
> --
>
> https://code.launchpad.net/~osomon/oxide/geolocation-doesnt-like-nans/+merge/235667
> Your team Oxide Developers is subscribed to branch lp:oxide.
>
> === modified file 'qt/core/browser/oxide_qt_location_provider.cc'
> --- qt/core/browser/oxide_qt_location_provider.cc 2014-05-06
> 21:14:38 +0000
> +++ qt/core/browser/oxide_qt_location_provider.cc 2014-09-23
> 15:58:36 +0000
> @@ -19,6 +19,7 @@
> #include "oxide_qt_location_provider_p.h"
>
> #include <cfloat>
> +#include <math.h>
>
> #include <QGeoCoordinate>
> #include <QGeoPositionInfo>
> @@ -43,16 +44,27 @@
> position.altitude = coord.altitude();
> if (info.hasAttribute(QGeoPositionInfo::HorizontalAccuracy)) {
> position.accuracy =
> info.attribute(QGeoPositionInfo::HorizontalAccuracy);
> + if (isnan(position.accuracy)) {
> + // shield ourselves against invalid data
> + position.accuracy = DBL_MAX;
> + }
> } else {
> // accuracy is mandatory
> position.accuracy = DBL_MAX;
> }
> if (info.hasAttribute(QGeoPositionInfo::VerticalAccuracy)) {
> - position.altitude_accuracy =
> - info.attribute(QGeoPositionInfo::VerticalAccuracy);
> + qreal accuracy = info.attribute(QGeoPositionInfo::VerticalAccuracy);
> + if (!isnan(accuracy)) {
> + // shield ourselves against invalid data
> + position.altitude_accuracy = accuracy;
> + }
> }
> if (info.hasAttribute(QGeoPositionInfo::GroundSpeed)) {
> - position.speed = info.attribute(QGeoPositionInfo::GroundSpeed);
> + qreal speed = info.attribute(QGeoPositionInfo::GroundSpeed);
> + if (!isnan(speed)) {
> + // shield ourselves against invalid data
> + position.speed = speed;
> + }
> }
> position.timestamp =
> base::Time::FromJsTime(info.timestamp().toMSecsSinceEpoch());
>
>
>

[lp:~osomon/oxide/geolocation-doesnt-like-nans]

766. By Olivier Tilloy on 2014-09-24

Use portable qIsNaN() function.

767. By Olivier Tilloy on 2014-09-24

Shield against invalid values for altitude.

[Chris Coulson (chrisccoulson)]

Thanks. I've added a couple of comments inline. Also, would it be possible for the mock plugin to emulate this behaviour so that we could have a test for it?

[lp:~osomon/oxide/geolocation-doesnt-like-nans]

768. By Olivier Tilloy on 2014-09-24

Fix typo.

769. By Olivier Tilloy on 2014-09-24

Use std::numeric_limits instead of macros from <cfloat>.

770. By Olivier Tilloy on 2014-09-24

Use base::IsNaN() from chromium instead of qIsNaN().

771. By Olivier Tilloy on 2014-09-24

Add a test case to check that invalid data in position updates is guarded against.

[Chris Coulson (chrisccoulson)]

review: Approve

=== modified file 'qt/core/browser/oxide_qt_location_provider.cc'
--- qt/core/browser/oxide_qt_location_provider.cc	2014-05-06 21:14:38 +0000
+++ qt/core/browser/oxide_qt_location_provider.cc	2014-09-24 11:57:16 +0000
@@ -18,7 +18,7 @@
 #include "oxide_qt_location_provider.h"
 #include "oxide_qt_location_provider_p.h"
 
-#include <cfloat>
+#include <limits>
 
 #include <QGeoCoordinate>
 #include <QGeoPositionInfo>
@@ -26,6 +26,7 @@
 #include <QThread>
 
 #include "base/bind.h"
+#include "base/float_util.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop_proxy.h"
@@ -41,18 +42,33 @@
   position.latitude = coord.latitude();
   position.longitude = coord.longitude();
   position.altitude = coord.altitude();
+  if (base::IsNaN(position.altitude)) {
+    // shield ourselves against invalid data
+    position.altitude = 0;
+  }
   if (info.hasAttribute(QGeoPositionInfo::HorizontalAccuracy)) {
     position.accuracy = info.attribute(QGeoPositionInfo::HorizontalAccuracy);
+    if (base::IsNaN(position.accuracy)) {
+      // shield ourselves against invalid data
+      position.accuracy = std::numeric_limits<double>::max();
+    }
   } else {
     // accuracy is mandatory
-    position.accuracy = DBL_MAX;
+    position.accuracy = std::numeric_limits<double>::max();
   }
   if (info.hasAttribute(QGeoPositionInfo::VerticalAccuracy)) {
-    position.altitude_accuracy =
-        info.attribute(QGeoPositionInfo::VerticalAccuracy);
+    qreal accuracy = info.attribute(QGeoPositionInfo::VerticalAccuracy);
+    if (!base::IsNaN(accuracy)) {
+      // shield ourselves against invalid data
+      position.altitude_accuracy = accuracy;
+    }
   }
   if (info.hasAttribute(QGeoPositionInfo::GroundSpeed)) {
-    position.speed = info.attribute(QGeoPositionInfo::GroundSpeed);
+    qreal speed = info.attribute(QGeoPositionInfo::GroundSpeed);
+    if (!base::IsNaN(speed)) {
+      // shield ourselves against invalid data
+      position.speed = speed;
+    }
   }
   position.timestamp =
       base::Time::FromJsTime(info.timestamp().toMSecsSinceEpoch());

=== modified file 'qt/tests/mock/position/source.cc'
--- qt/tests/mock/position/source.cc	2014-06-30 11:42:54 +0000
+++ qt/tests/mock/position/source.cc	2014-09-24 11:57:16 +0000
@@ -21,6 +21,7 @@
 #include <QGeoCoordinate>
 #include <QGuiApplication>
 #include <QtGlobal>
+#include <QtNumeric>
 
 namespace {
 
@@ -83,6 +84,8 @@
   } else if (testcase == "error-unavailable") {
     Q_EMIT QGeoPositionInfoSource::error(
         QGeoPositionInfoSource::UnknownSourceError);
+  } else if (testcase == "invaliddata") {
+    QTimer::singleShot((int) rand(1., 1000.), this, SLOT(sendInvalidUpdate()));
   } else {
     QTimer::singleShot((int) rand(1., 1000.), this, SLOT(sendUpdate()));
   }
@@ -92,3 +95,9 @@
   QGeoPositionInfo update = randomPosition();
   Q_EMIT positionUpdated(update);
 }
+
+void SourceMock::sendInvalidUpdate() {
+  QGeoPositionInfo update = randomPosition();
+  update.setAttribute(QGeoPositionInfo::HorizontalAccuracy, qQNaN());
+  Q_EMIT positionUpdated(update);
+}

=== modified file 'qt/tests/mock/position/source.h'
--- qt/tests/mock/position/source.h	2014-04-23 22:40:48 +0000
+++ qt/tests/mock/position/source.h	2014-09-24 11:57:16 +0000
@@ -41,6 +41,7 @@
 
  private Q_SLOTS:
   void sendUpdate();
+  void sendInvalidUpdate();
 
  private:
   QTimer timer_;

=== modified file 'qt/tests/qmltests/core/tst_geolocation.qml'
--- qt/tests/qmltests/core/tst_geolocation.qml	2014-09-11 20:15:31 +0000
+++ qt/tests/qmltests/core/tst_geolocation.qml	2014-09-24 11:57:16 +0000
@@ -26,6 +26,7 @@
     function test_geolocation_get_data() {
       return [
         { testcase: "", result: "OK" },
+        { testcase: "invaliddata", result: "OK" },
         { testcase: "timeout", result: "TIMEOUT" },
         { testcase: "error-permission", result: "PERMISSION DENIED" },
         { testcase: "error-unavailable", result: "POSITION UNAVAILABLE" }