Bugfix: prevent race condition in TrackPointCreator.

This race condition leads to a crash in TrackStatisticsUpdater as the time might appear to move backwards if the created TrackPoints are not stored in order. Might happen if multiple sensors (BLE or GPS) are used in simultaneously.

Every method affected by a race condition is now synchronized.

Problem was introduced in c4b8082efc

Fixes #1151.

src/androidTest/java/de/dennisguse/opentracks/io/file/importer/ExportImportTest.java

@@ -469,7 +469,7 @@ public class ExportImportTest {
                 return sensorDataSet;
             }
         });
-        trackPointCreator.onChange(null);
+        trackPointCreator.onChange(new SensorDataSet());
     }
 
     private void mockAltitudeChange(TrackPointCreator trackPointCreator, float altitudeGain) {

src/androidTest/java/de/dennisguse/opentracks/services/handlers/GPSHandlerTest.java

@@ -22,7 +22,6 @@ import org.mockito.junit.MockitoJUnitRunner;
 import java.time.Instant;
 
 import de.dennisguse.opentracks.data.models.Distance;
-import de.dennisguse.opentracks.data.models.TrackPoint;
 import de.dennisguse.opentracks.settings.PreferencesUtils;
 
 @RunWith(MockitoJUnitRunner.class)
@@ -62,7 +61,7 @@ public class GPSHandlerTest {
         locationHandler.onLocationChanged(createLocation(45f, 35f, 3, 5, System.currentTimeMillis()));
 
         // then
-        verify(trackPointCreator, times(1)).onNewTrackPoint(any(TrackPoint.class));
+        verify(trackPointCreator, times(1)).onChange(any(Location.class));
     }
 
     /**
@@ -78,7 +77,7 @@ public class GPSHandlerTest {
         locationHandler.onLocationChanged(createLocation(latitude, 35f, 3, 5, System.currentTimeMillis()));
 
         // then
-        verify(trackPointCreator, times(0)).onNewTrackPoint(any(TrackPoint.class));
+        verify(trackPointCreator, times(0)).onChange(any(Location.class));
     }
 
     /**
@@ -94,7 +93,7 @@ public class GPSHandlerTest {
 
         // then
         // no newTrackPoint called
-        verify(trackPointCreator, times(0)).onNewTrackPoint(any(TrackPoint.class));
+        verify(trackPointCreator, times(0)).onChange(any(Location.class));
     }
 
     @Test
@@ -107,7 +106,7 @@ public class GPSHandlerTest {
         locationHandler.onLocationChanged(createLocation(99.0, 35.0, Long.MAX_VALUE, 15, System.currentTimeMillis()));
 
         // then
-        verify(trackPointCreator, times(1)).onNewTrackPoint(any(TrackPoint.class));
+        verify(trackPointCreator, times(1)).onChange(any(Location.class));
     }
 
     /**

src/main/java/de/dennisguse/opentracks/services/handlers/GPSHandler.java

@@ -12,6 +12,7 @@ import androidx.annotation.NonNull;
 import androidx.annotation.VisibleForTesting;
 
 import java.time.Duration;
+import java.time.Instant;
 
 import de.dennisguse.opentracks.R;
 import de.dennisguse.opentracks.data.models.Distance;
@@ -109,25 +110,23 @@ public class GPSHandler implements LocationListener, GpsStatus.GpsStatusListener
             return;
         }
 
-        TrackPoint trackPoint = new TrackPoint(location, trackPointCreator.createNow());
-        boolean isAccurate = trackPoint.fulfillsAccuracy(thresholdHorizontalAccuracy);
-        boolean isValid = LocationUtils.isValidLocation(location);
-
         if (gpsStatus != null) {
+            // Send each update to the status; please note that this TrackPoint is not stored.
+            TrackPoint trackPoint = new TrackPoint(location, Instant.ofEpochMilli(location.getTime()));
             gpsStatus.onLocationChanged(trackPoint);
         }
 
-        if (!isValid) {
+        if (!LocationUtils.isValidLocation(location)) {
             Log.w(TAG, "Ignore newTrackPoint. location is invalid.");
             return;
         }
 
-        if (!isAccurate) {
+        if (!LocationUtils.fulfillsAccuracy(location, thresholdHorizontalAccuracy)) {
             Log.d(TAG, "Ignore newTrackPoint. Poor accuracy.");
             return;
         }
 
-        trackPointCreator.onNewTrackPoint(trackPoint);
+        trackPointCreator.onChange(location);
     }
 
     @Override

src/main/java/de/dennisguse/opentracks/services/handlers/TrackPointCreator.java

@@ -1,6 +1,7 @@
 package de.dennisguse.opentracks.services.handlers;
 
 import android.content.Context;
+import android.location.Location;
 import android.util.Log;
 import android.util.Pair;
 
@@ -47,7 +48,7 @@ public class TrackPointCreator implements BluetoothRemoteSensorManager.SensorDat
         this.gpsHandler = gpsHandler;
     }
 
-    public void start(@NonNull Context context) {
+    public synchronized void start(@NonNull Context context) {
         this.context = context;
 
         gpsHandler.onStart(context);
@@ -71,7 +72,7 @@ public class TrackPointCreator implements BluetoothRemoteSensorManager.SensorDat
         gpsHandler.onStop();
     }
 
-    public void reset() {
+    public synchronized void reset() {
         if (remoteSensorManager == null || altitudeSumManager == null) {
             Log.d(TAG, "No recording running and no reset necessary.");
             return;
@@ -80,7 +81,7 @@ public class TrackPointCreator implements BluetoothRemoteSensorManager.SensorDat
         altitudeSumManager.reset();
     }
 
-    private SensorDataSet fill(TrackPoint trackPoint) {
+    private SensorDataSet addSensorData(TrackPoint trackPoint) {
         if (!isStarted()) {
             Log.w(TAG, "Not started, should not be called.");
             return null;
@@ -98,7 +99,7 @@ public class TrackPointCreator implements BluetoothRemoteSensorManager.SensorDat
         return sensorDataSet;
     }
 
-    public void stop() {
+    public synchronized void stop() {
         gpsHandler.onStop();
 
         if (remoteSensorManager != null) {
@@ -114,16 +115,21 @@ public class TrackPointCreator implements BluetoothRemoteSensorManager.SensorDat
         this.context = null;
     }
 
+    public synchronized void onChange(@NonNull Location location) {
+        onNewTrackPoint(new TrackPoint(location, createNow()));
+    }
+
     /**
      * Got a new TrackPoint from Bluetooth only; contains no GPS location.
      */
     @Override
-    public void onChange(SensorDataSet sensorDataSet) {
+    public synchronized void onChange(@NonNull SensorDataSet unused) {
         onNewTrackPoint(new TrackPoint(TrackPoint.Type.SENSORPOINT, createNow()));
     }
 
-    public synchronized void onNewTrackPoint(@NonNull TrackPoint trackPoint) {
-        fill(trackPoint);
+    @VisibleForTesting
+    public void onNewTrackPoint(@NonNull TrackPoint trackPoint) {
+        addSensorData(trackPoint);
 
         boolean stored = service.newTrackPoint(trackPoint, gpsHandler.getThresholdHorizontalAccuracy());
         if (stored) {
@@ -131,13 +137,13 @@ public class TrackPointCreator implements BluetoothRemoteSensorManager.SensorDat
         }
     }
 
-    public TrackPoint createSegmentStartManual() {
+    public synchronized TrackPoint createSegmentStartManual() {
         return TrackPoint.createSegmentStartManualWithTime(createNow());
     }
 
-    public TrackPoint createSegmentEnd() {
+    public synchronized TrackPoint createSegmentEnd() {
         TrackPoint segmentEnd = TrackPoint.createSegmentEndWithTime(createNow());
-        fill(segmentEnd);
+        addSensorData(segmentEnd);
         reset();
         return segmentEnd;
     }
@@ -158,11 +164,12 @@ public class TrackPointCreator implements BluetoothRemoteSensorManager.SensorDat
             currentTrackPoint.setLatitude(lastStoredTrackPointWithLocation.getLatitude());
         }
 
-        SensorDataSet sensorDataSet = fill(currentTrackPoint);
+        SensorDataSet sensorDataSet = addSensorData(currentTrackPoint);
 
         return new Pair<>(currentTrackPoint, sensorDataSet);
     }
 
+    @VisibleForTesting
     Instant createNow() {
         return Instant.now(clock);
     }

src/main/java/de/dennisguse/opentracks/util/LocationUtils.java

@@ -17,6 +17,8 @@ package de.dennisguse.opentracks.util;
 
 import android.location.Location;
 
+import de.dennisguse.opentracks.data.models.Distance;
+
 /**
  * Utility class for decimating tracks at a given level of precision.
  *
@@ -40,4 +42,11 @@ public class LocationUtils {
                 && Math.abs(location.getLatitude()) <= 90
                 && Math.abs(location.getLongitude()) <= 180;
     }
+
+    public static boolean fulfillsAccuracy(Location location, Distance thresholdHorizontalAccuracy) {
+        return location.hasAccuracy() &&
+                Distance.of(location.getAccuracy())
+                        .lessThan(thresholdHorizontalAccuracy);
+
+    }
 }