Bug 961526 - Part 1: Fix SQLiteConstraintException in FHR. r=rnewman, r=mcomella, a=sledru
authorMichael Comella <michael.l.comella@gmail.com>
Tue, 04 Feb 2014 15:10:55 -0800
changeset 182747 c90044c11e4e43b58577074c90d881dcd5534baf
parent 182746 dd51a2464396d1de5c730cb810456d52f5b08c8c
child 182748 f9028e3008d5e18011f05fa1ec700f2225843286
push id3343
push userffxbld
push dateMon, 17 Mar 2014 21:55:32 +0000
treeherdermozilla-beta@2f7d3415f79f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrnewman, mcomella, sledru
bugs961526
milestone29.0a2
Bug 961526 - Part 1: Fix SQLiteConstraintException in FHR. r=rnewman, r=mcomella, a=sledru
mobile/android/base/background/healthreport/HealthReportDatabaseStorage.java
mobile/android/base/background/healthreport/prune/PrunePolicyDatabaseStorage.java
mobile/android/tests/background/junit3/src/healthreport/MockHealthReportDatabaseStorage.java
mobile/android/tests/background/junit3/src/healthreport/TestHealthReportDatabaseStorage.java
--- a/mobile/android/base/background/healthreport/HealthReportDatabaseStorage.java
+++ b/mobile/android/base/background/healthreport/HealthReportDatabaseStorage.java
@@ -533,31 +533,33 @@ public class HealthReportDatabaseStorage
     }
 
     @Override
     public synchronized int getID() throws IllegalStateException {
       if (this.fieldID == UNKNOWN_TYPE_OR_FIELD_ID) {
         this.fieldID = integerQuery("named_fields", "field_id",
                                     "measurement_name = ? AND measurement_version = ? AND field_name = ?",
                                     new String[] {measurementName, measurementVersion, fieldName},
-                                    -1);
+                                    UNKNOWN_TYPE_OR_FIELD_ID);
         if (this.fieldID == UNKNOWN_TYPE_OR_FIELD_ID) {
           throw new IllegalStateException("No field with name " + fieldName +
                                           " (" + measurementName + ", " + measurementVersion + ")");
         }
       }
       return this.fieldID;
     }
   }
 
   // `envs` and `fields` look similar, but they are touched differently and
   // store differently stable kinds of data, hence type difference.
   // Note that we don't pre-populate the environment cache. We'll typically only
   // handle one per session.
-  private final ConcurrentHashMap<String, Integer> envs = new ConcurrentHashMap<String, Integer>();
+  //
+  // protected for testing purposes only.
+  protected final ConcurrentHashMap<String, Integer> envs = new ConcurrentHashMap<String, Integer>();
 
   /**
    * An {@link Environment} that knows how to persist to and from our database.
    */
   public static class DatabaseEnvironment extends Environment {
     protected final HealthReportDatabaseStorage storage;
 
     @Override
@@ -850,16 +852,22 @@ public class HealthReportDatabaseStorage
 
   /**
    * Cache the lookup from measurement and field specifier to field instance.
    * This allows us to memoize the field ID lookup, too.
    */
   private HashMap<String, Field> fields = new HashMap<String, Field>();
   private boolean fieldsCacheUpdated = false;
 
+  private void invalidateFieldsCache() {
+    synchronized (this.fields) {
+      fieldsCacheUpdated = false;
+    }
+  }
+
   private String getFieldKey(String mName, int mVersion, String fieldName) {
     return mVersion + "." + mName + "/" + fieldName;
   }
 
   @Override
   public Field getField(String mName, int mVersion, String fieldName) {
     final String key = getFieldKey(mName, mVersion, fieldName);
     synchronized (fields) {
@@ -1009,19 +1017,17 @@ public class HealthReportDatabaseStorage
       v.put("flags", field.type);
       Logger.debug(LOG_TAG, "M: " + measurementID + " F: " + field.name + " (" + field.type + ")");
       db.insert("fields", null, v);
     }
 
     notifyMeasurementVersionUpdated(measurement, version);
 
     // Let's be easy for now.
-    synchronized (fields) {
-      fieldsCacheUpdated = false;
-    }
+    invalidateFieldsCache();
   }
 
   /**
    * Return a cursor over the measurements and fields in the DB.
    * Columns are {@link HealthReportDatabaseStorage#COLUMNS_MEASUREMENT_AND_FIELD_DETAILS}.
    */
   @Override
   public Cursor getFieldVersions() {
@@ -1147,20 +1153,26 @@ public class HealthReportDatabaseStorage
 
     final ContentValues v = new ContentValues();
     v.put("env", env);
     v.put("field", field);
     v.put("date", day);
 
     final SQLiteDatabase db = this.helper.getWritableDatabase();
     putValue(v, value);
-    try {
-      db.insertOrThrow(table, null, v);
-    } catch (SQLiteConstraintException e) {
-      throw new IllegalStateException("Event did not reference existing an environment or field.", e);
+
+    // Using SQLiteDatabase.insertOrThrow throws SQLiteConstraintException we cannot catch for
+    // unknown reasons (bug 961526 comment 13). We believe these are thrown because we attempt to
+    // record events using environment IDs removed from the database by the prune service. We
+    // invalidate the currentEnvironment ID after pruning, preventing further propagation,
+    // however, any event recording waiting for the prune service to complete on the background
+    // thread may carry an invalid ID: we expect an insertion failure and drop these events here.
+    final long res = db.insert(table, null, v);
+    if (res == -1) {
+      Logger.error(LOG_TAG, "Unable to record daily discrete event. Ignoring.");
     }
   }
 
   @Override
   public void recordDailyDiscrete(int env, int day, int field, JSONObject value) {
     this.recordDailyDiscrete(env, day, field, value == null ? "null" : value.toString(), EVENTS_TEXTUAL);
   }
 
@@ -1511,38 +1523,46 @@ public class HealthReportDatabaseStorage
 
   @Override
   public void deleteMeasurements() {
     final SQLiteDatabase db = this.helper.getWritableDatabase();
     db.beginTransaction();
     try {
       // Cascade will clear the rest.
       db.delete("measurements", null, null);
+
+      // Clear measurements and fields cache, because some of their IDs are now invalid.
+      invalidateFieldsCache(); // Let it repopulate on its own.
+      populateMeasurementVersionsCache(db); // Performed at Storage init so repopulate now.
+
       db.setTransactionSuccessful();
     } finally {
       db.endTransaction();
     }
   }
 
   /**
    * Prunes the given number of least-recently used environments. Note that orphaned environments
-   * are not removed.
+   * are not removed and the environment cache is cleared.
    */
   public void pruneEnvironments(final int numToPrune) {
     final SQLiteDatabase db = this.helper.getWritableDatabase();
     db.beginTransaction();
     try {
       db.delete("environments",
           "id in (SELECT env " +
           "       FROM events " +
           "       GROUP BY env " +
           "       ORDER BY MAX(date), env " +
           "       LIMIT " + numToPrune + ")",
           null);
       db.setTransactionSuccessful();
+
+      // Clear environment cache, because some of their IDs are now invalid.
+      this.envs.clear();
     } finally {
       db.endTransaction();
     }
   }
 
   /**
    * Prunes up to a maximum of the given number of the oldest events. While it is more correct to
    * prune the exact given amount, there is no unique identifier among events so we cannot be so
--- a/mobile/android/base/background/healthreport/prune/PrunePolicyDatabaseStorage.java
+++ b/mobile/android/base/background/healthreport/prune/PrunePolicyDatabaseStorage.java
@@ -38,16 +38,21 @@ public class PrunePolicyDatabaseStorage 
   }
 
   public void pruneEvents(final int count) {
     getStorage().pruneEvents(count);
   }
 
   public void pruneEnvironments(final int count) {
     getStorage().pruneEnvironments(count);
+
+    // Re-populate the DB and environment cache with the current environment in the unlikely event
+    // that it was deleted.
+    this.currentEnvironmentID = -1;
+    getCurrentEnvironmentID();
   }
 
   /**
    * Deletes data recorded before the given time. Note that if this method fails to retrieve the
    * current environment from the profile cache, it will not delete data so be sure to prune by
    * other methods (e.g. {@link pruneEvents}) as well.
    */
   public int deleteDataBefore(final long time) {
--- a/mobile/android/tests/background/junit3/src/healthreport/MockHealthReportDatabaseStorage.java
+++ b/mobile/android/tests/background/junit3/src/healthreport/MockHealthReportDatabaseStorage.java
@@ -1,15 +1,16 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
 package org.mozilla.gecko.background.healthreport;
 
 import java.io.File;
 import java.util.ArrayList;
+import java.util.concurrent.ConcurrentHashMap;
 
 import org.json.JSONObject;
 import org.mozilla.gecko.background.common.GlobalConstants;
 import org.mozilla.gecko.background.healthreport.HealthReportDatabaseStorage;
 import org.mozilla.gecko.background.healthreport.HealthReportStorage.MeasurementFields.FieldSpec;
 
 import android.content.ContentValues;
 import android.content.Context;
@@ -37,16 +38,20 @@ public class MockHealthReportDatabaseSto
   public int getGivenDaysAgo(int numDays) {
     return super.getDay(this.getGivenDaysAgoMillis(numDays));
   }
 
   public long getGivenDaysAgoMillis(int numDays) {
     return now - numDays * GlobalConstants.MILLISECONDS_PER_DAY;
   }
 
+  public ConcurrentHashMap<String, Integer> getEnvironmentCache() {
+    return this.envs;
+  }
+
   public MockHealthReportDatabaseStorage(Context context, File fakeProfileDirectory) {
     super(context, fakeProfileDirectory);
   }
 
   public SQLiteDatabase getDB() {
     return this.helper.getWritableDatabase();
   }
 
--- a/mobile/android/tests/background/junit3/src/healthreport/TestHealthReportDatabaseStorage.java
+++ b/mobile/android/tests/background/junit3/src/healthreport/TestHealthReportDatabaseStorage.java
@@ -322,36 +322,48 @@ public class TestHealthReportDatabaseSto
     final int nonExistentEnvID = DBHelpers.getNonExistentID(db, "environments");
     final int nonExistentFieldID = DBHelpers.getNonExistentID(db, "fields");
 
     try {
       storage.incrementDailyCount(nonExistentEnvID, storage.getToday(), counterFieldID);
       fail("Should throw - event_integer(env) references environments(id), which is given as a non-existent value.");
     } catch (IllegalStateException e) { }
     try {
-      storage.recordDailyDiscrete(nonExistentEnvID, storage.getToday(), discreteFieldID, "iu");
-      fail("Should throw - event_textual(env) references environments(id), which is given as a non-existent value.");
-    } catch (IllegalStateException e) { }
-    try {
       storage.recordDailyLast(nonExistentEnvID, storage.getToday(), discreteFieldID, "iu");
       fail("Should throw - event_textual(env) references environments(id), which is given as a non-existent value.");
     } catch (IllegalStateException e) { }
 
     try {
       storage.incrementDailyCount(envID, storage.getToday(), nonExistentFieldID);
       fail("Should throw - event_integer(field) references fields(id), which is given as a non-existent value.");
     } catch (IllegalStateException e) { }
     try {
-      storage.recordDailyDiscrete(envID, storage.getToday(), nonExistentFieldID, "iu");
-      fail("Should throw - event_textual(field) references fields(id), which is given as a non-existent value.");
-    } catch (IllegalStateException e) { }
-    try {
       storage.recordDailyLast(envID, storage.getToday(), nonExistentFieldID, "iu");
       fail("Should throw - event_textual(field) references fields(id), which is given as a non-existent value.");
     } catch (IllegalStateException e) { }
+
+    // Test dropped events due to constraint violations that do not throw (see bug 961526).
+    final String eventValue = "a value not in the database";
+    assertFalse(isEventInDB(db, eventValue)); // Better safe than sorry.
+
+    storage.recordDailyDiscrete(nonExistentEnvID, storage.getToday(), discreteFieldID, eventValue);
+    assertFalse(isEventInDB(db, eventValue));
+
+    storage.recordDailyDiscrete(envID, storage.getToday(), nonExistentFieldID, "iu");
+    assertFalse(isEventInDB(db, eventValue));
+  }
+
+  private static boolean isEventInDB(final SQLiteDatabase db, final String value) {
+    final Cursor c = db.query("events_textual", new String[] {"value"}, "value = ?",
+        new String[] {value}, null, null, null);
+    try {
+      return c.getCount() > 0;
+    } finally {
+      c.close();
+    }
   }
 
   // Largely taken from testDeleteEnvAndEventsBefore and testDeleteOrphanedAddons.
   public void testDeleteDataBefore() throws Exception {
     final PrepopulatedMockHealthReportDatabaseStorage storage =
         new PrepopulatedMockHealthReportDatabaseStorage(context, fakeProfileDirectory);
     final SQLiteDatabase db = storage.getDB();
 
@@ -548,17 +560,20 @@ public class TestHealthReportDatabaseSto
     assertEquals(0, storage.getEnvironmentCount());
   }
 
   public void testPruneEnvironments() throws Exception {
     final PrepopulatedMockHealthReportDatabaseStorage storage =
         new PrepopulatedMockHealthReportDatabaseStorage(context, fakeProfileDirectory, 2);
     final SQLiteDatabase db = storage.getDB();
     assertEquals(5, DBHelpers.getRowCount(db, "environments"));
+    assertEquals(5, storage.getEnvironmentCache().size());
+
     storage.pruneEnvironments(1);
+    assertEquals(0, storage.getEnvironmentCache().size());
     assertTrue(!getEnvAppVersions(db).contains("v3"));
     storage.pruneEnvironments(2);
     assertTrue(!getEnvAppVersions(db).contains("v2"));
     assertTrue(!getEnvAppVersions(db).contains("v1"));
     storage.pruneEnvironments(1);
     assertTrue(!getEnvAppVersions(db).contains("v123"));
     storage.pruneEnvironments(1);
     assertTrue(!getEnvAppVersions(db).contains("v4"));