Bug 1296750 - Don't crash if multiple files have same timestamp in TelemetryJSONFilePingStore.deleteSmallestFiles r?grisha draft
authorAndrzej Hunt <ahunt@mozilla.com>
Fri, 19 Aug 2016 14:18:43 -0700
changeset 403497 4ea192dd2a6964094808cdcebdfa576f8b54f7da
parent 403436 7c7bc13fc1b626b044b9c20ebc07382e22e13efe
child 528916 89accc3e46b1977f8738ca6263e42c3ceeff0596
push id26924
push userahunt@mozilla.com
push dateFri, 19 Aug 2016 21:19:15 +0000
reviewersgrisha
bugs1296750
milestone51.0a1
Bug 1296750 - Don't crash if multiple files have same timestamp in TelemetryJSONFilePingStore.deleteSmallestFiles r?grisha Using a set is bad since it will silently ignore multiple files with the same timestamp, resulting in crashes when the set has fewer items than we expected. A list is probably the best choice in this instance. (Note: I am not familiar enough with this code to know whether or not this is an expected situation, i.e. whether or not timestamps should be unique.) MozReview-Commit-ID: 5TRNLgTsSHO
mobile/android/base/java/org/mozilla/gecko/telemetry/stores/TelemetryJSONFilePingStore.java
--- a/mobile/android/base/java/org/mozilla/gecko/telemetry/stores/TelemetryJSONFilePingStore.java
+++ b/mobile/android/base/java/org/mozilla/gecko/telemetry/stores/TelemetryJSONFilePingStore.java
@@ -121,27 +121,32 @@ public class TelemetryJSONFilePingStore 
         if (files == null) {
             return;
         }
 
         if (files.length < MAX_PING_COUNT) {
             return;
         }
 
-        final SortedSet<File> sortedFiles = new TreeSet<>(fileLastModifiedComparator);
-        sortedFiles.addAll(Arrays.asList(files));
+        // It's possible that multiple files will have the same timestamp: in this case they are treated
+        // as equal by the fileLastModifiedComparator. We therefore have to use a sorted list (as
+        // opposed to a set, or map).
+        final ArrayList<File> sortedFiles = new ArrayList<>(Arrays.asList(files));
+        Collections.sort(sortedFiles, fileLastModifiedComparator);
         deleteSmallestFiles(sortedFiles, files.length - MAX_PING_COUNT);
     }
 
-    private void deleteSmallestFiles(final SortedSet<File> files, final int numFilesToRemove) {
+    private void deleteSmallestFiles(final ArrayList<File> files, final int numFilesToRemove) {
         final Iterator<File> it = files.iterator();
         int i = 0;
+
         while (i < numFilesToRemove) {
             i += 1;
-            // Sorted set so we're iterating over ascending files.
+
+            // Sorted list so we're iterating over ascending files.
             final File file = it.next(); // file count > files to remove so this should not throw.
             file.delete();
         }
     }
 
     @Override
     public ArrayList<TelemetryPing> getAllPings() {
         final File[] fileArray = storeDir.listFiles(uuidFilenameFilter);