Bug 1428165 - Part 1: ensure that 'modified' and 'created' timestamps are set when inserting history from sync r=nalexander
authorGrigory Kruglov <gkruglov@mozilla.com>
Fri, 05 Jan 2018 21:15:55 -0500
changeset 398077 d9e432fa5a8cd5a597bd2db25b9aaaca5a5aa180
parent 398076 97d9e1823553fec9684c6937d0d82cf288d037c3
child 398078 ef89f9d60f81063ac58fdcae049792d6e59a3233
push id57620
push usergkruglov@mozilla.com
push dateSat, 06 Jan 2018 02:30:33 +0000
treeherderautoland@ef89f9d60f81 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnalexander
bugs1428165, 1291821
milestone59.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1428165 - Part 1: ensure that 'modified' and 'created' timestamps are set when inserting history from sync r=nalexander This fixes a regression introduced in Bug 1291821. History records would be bulk-inserted from sync, and our ContentProvider would erroneously forget to set these two timestamp fields. MozReview-Commit-ID: 2k0afijN62H
mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/HistoryDataAccessor.java
mobile/android/services/src/test/java/org/mozilla/gecko/db/BrowserProviderHistoryTest.java
--- a/mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
+++ b/mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
@@ -2824,45 +2824,53 @@ public class BrowserProvider extends Sha
                     // Do not sync these changes.
                     false
             );
         }
     }
 
     private int bulkInsertHistory(final SQLiteDatabase db, ContentValues[] values) {
         int inserted = 0;
+        // Set 'modified' and 'created' timestamps to current wall time.
+        // 'modified' specifically is used by Sync for change tracking, and so we must ensure it's
+        // set to our own clock (as opposed to record's modified timestamp as record by the server).
+        final long now = System.currentTimeMillis();
         final String fullInsertSqlStatement = "INSERT INTO " + History.TABLE_NAME + " (" +
                 History.GUID + "," +
                 History.TITLE + "," +
                 History.URL + "," +
                 History.DATE_LAST_VISITED + "," +
                 History.REMOTE_DATE_LAST_VISITED + "," +
                 History.VISITS + "," +
-                History.REMOTE_VISITS + ") VALUES (?, ?, ?, ?, ?, ?, ?)";
+                History.REMOTE_VISITS + "," +
+                History.DATE_MODIFIED + "," +
+                History.DATE_CREATED + ") VALUES (?, ?, ?, ?, ?, ?, ?, " + now + "," + now + ")";
         final String shortInsertSqlStatement = "INSERT INTO " + History.TABLE_NAME + " (" +
                 History.GUID + "," +
                 History.TITLE + "," +
-                History.URL + ") VALUES (?, ?, ?)";
+                History.URL + "," +
+                History.DATE_MODIFIED + "," +
+                History.DATE_CREATED + ") VALUES (?, ?, ?, " + now + "," + now + ")";
         final SQLiteStatement compiledFullStatement = db.compileStatement(fullInsertSqlStatement);
         final SQLiteStatement compiledShortStatement = db.compileStatement(shortInsertSqlStatement);
         SQLiteStatement statementToExec;
 
         beginWrite(db);
         try {
             for (ContentValues cv : values) {
                 final String guid = cv.getAsString(History.GUID);
                 final String title = cv.getAsString(History.TITLE);
                 final String url = cv.getAsString(History.URL);
                 final Long dateLastVisited = cv.getAsLong(History.DATE_LAST_VISITED);
                 final Long remoteDateLastVisited = cv.getAsLong(History.REMOTE_DATE_LAST_VISITED);
                 final Integer visits = cv.getAsInteger(History.VISITS);
 
                 // If dateLastVisited is null, so will be remoteDateLastVisited and visits.
                 // We will use the short compiled statement in this case.
-                // See implementation in AndroidBrowserHistoryDataAccessor@getContentValues.
+                // See implementation in HistoryDataAccessor#getContentValues.
                 if (dateLastVisited == null) {
                     statementToExec = compiledShortStatement;
                 } else {
                     statementToExec = compiledFullStatement;
                 }
 
                 statementToExec.clearBindings();
                 statementToExec.bindString(1, guid);
--- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/HistoryDataAccessor.java
+++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/HistoryDataAccessor.java
@@ -28,24 +28,26 @@ public class HistoryDataAccessor extends
 
   @Override
   protected Uri getUri() {
     return BrowserContractHelpers.HISTORY_CONTENT_URI;
   }
 
   @Override
   protected ContentValues getContentValues(Record record) {
-    ContentValues cv = new ContentValues();
-    HistoryRecord rec = (HistoryRecord) record;
+    // NB: these two sets of values (with or without visit information) must agree with the
+    // BrowserProvider#bulkInsertHistory implementation.
+    final ContentValues cv = new ContentValues();
+    final HistoryRecord rec = (HistoryRecord) record;
     cv.put(BrowserContract.History.GUID, rec.guid);
     cv.put(BrowserContract.History.TITLE, rec.title);
     cv.put(BrowserContract.History.URL, rec.histURI);
     if (rec.visits != null) {
-      JSONArray visits = rec.visits;
-      long mostRecent = getLastVisited(visits);
+      final JSONArray visits = rec.visits;
+      final long mostRecent = getLastVisited(visits);
 
       // Fennec stores history timestamps in milliseconds, and visit timestamps in microseconds.
       // The rest of Sync works in microseconds. This is the conversion point for records coming form Sync.
       cv.put(BrowserContract.History.DATE_LAST_VISITED, mostRecent / 1000);
       cv.put(BrowserContract.History.REMOTE_DATE_LAST_VISITED, mostRecent / 1000);
       cv.put(BrowserContract.History.VISITS, visits.size());
     }
     return cv;
@@ -55,16 +57,18 @@ public class HistoryDataAccessor extends
   protected String[] getAllColumns() {
     return BrowserContractHelpers.HistoryColumns;
   }
 
   @Override
   public Uri insert(Record record) {
     HistoryRecord rec = (HistoryRecord) record;
 
+    // TODO this could use BrowserContract.METHOD_INSERT_HISTORY_WITH_VISITS_FROM_SYNC, both for
+    // speed (one transaction instead of one), and for the benefit of data consistency concerns.
     Logger.debug(LOG_TAG, "Storing record " + record.guid);
     Uri newRecordUri = super.insert(record);
 
     Logger.debug(LOG_TAG, "Storing visits for " + record.guid);
     context.getContentResolver().bulkInsert(
             BrowserContract.Visits.CONTENT_URI,
             VisitsHelper.getVisitsContentValues(rec.guid, rec.visits)
     );
@@ -96,18 +100,18 @@ public class HistoryDataAccessor extends
             BrowserContract.Visits.CONTENT_URI,
             VisitsHelper.getVisitsContentValues(newGUID, rec.visits)
     );
   }
 
   /**
    * Insert records.
    * <p>
-   * This inserts all the records (using <code>ContentProvider.bulkInsert</code>),
-   * then inserts all the visit information (also using <code>ContentProvider.bulkInsert</code>).
+   * This inserts all the records and their visit information using a custom ContentProvider interface.
+   * Underlying ContentProvider must handle "call" method {@link BrowserContract#METHOD_INSERT_HISTORY_WITH_VISITS_FROM_SYNC}.
    *
    * @param records
    *          the records to insert.
    * @return
    *          the number of records actually inserted.
    * @throws NullCursorException
    */
   public boolean bulkInsert(ArrayList<HistoryRecord> records) throws NullCursorException {
--- a/mobile/android/services/src/test/java/org/mozilla/gecko/db/BrowserProviderHistoryTest.java
+++ b/mobile/android/services/src/test/java/org/mozilla/gecko/db/BrowserProviderHistoryTest.java
@@ -34,16 +34,17 @@ public class BrowserProviderHistoryTest 
 
     private static final long THREE_MONTHS = 1000L * 60L * 60L * 24L * 30L * 3L;
 
     @Before
     @Override
     public void setUp() throws Exception {
         super.setUp();
         final ShadowContentResolver cr = new ShadowContentResolver();
+
         thumbnailClient = cr.acquireContentProviderClient(BrowserContract.Thumbnails.CONTENT_URI);
         thumbnailTestUri = testUri(BrowserContract.Thumbnails.CONTENT_URI);
         expireHistoryNormalUri = testUri(BrowserContract.History.CONTENT_OLD_URI).buildUpon()
                 .appendQueryParameter(
                         BrowserContract.PARAM_EXPIRE_PRIORITY,
                         BrowserContract.ExpirePriority.NORMAL.toString()
                 ).build();
         expireHistoryAggressiveUri = testUri(BrowserContract.History.CONTENT_OLD_URI).buildUpon()
@@ -255,52 +256,119 @@ public class BrowserProviderHistoryTest 
             assertTrue(true);
 
             // NB: same values as above, to ensure throwing update didn't actually change anything.
             assertHistoryAggregates(BrowserContract.History.URL + " = ?", new String[] {url},
                     2, 19, lastVisited3, 8, lastVisited3);
         }
     }
 
+    private void assertHistoryValuesForGuidsFromSync(int expectedCount, String title, String url, Long remoteLastVisited, Integer visits) throws RemoteException {
+        final Cursor c = historyClient.query(historyTestUri, new String[] {
+                BrowserContract.History.TITLE,
+                BrowserContract.History.VISITS,
+                BrowserContract.History.URL,
+                BrowserContract.History.LOCAL_VISITS,
+                BrowserContract.History.REMOTE_VISITS,
+                BrowserContract.History.LOCAL_DATE_LAST_VISITED,
+                BrowserContract.History.REMOTE_DATE_LAST_VISITED,
+                BrowserContract.History.DATE_CREATED,
+                BrowserContract.History.DATE_MODIFIED
+        }, null, null, BrowserContract.History._ID + " DESC");
+
+        // 3m ago, since one of the tests manually rewinds timestamps to 2 months ago.
+        final long reasonablyRecentTimestamp = System.currentTimeMillis() - 1000L * 60L * 60L * 24L * 30L * 3L;
+
+        assertNotNull(c);
+        assertEquals(expectedCount, c.getCount());
+        try {
+            final int titleCol = c.getColumnIndexOrThrow(BrowserContract.History.TITLE);
+            final int urlCol = c.getColumnIndexOrThrow(BrowserContract.History.URL);
+            final int visitsCol = c.getColumnIndexOrThrow(BrowserContract.History.VISITS);
+            final int localVisitsCol = c.getColumnIndexOrThrow(BrowserContract.History.LOCAL_VISITS);
+            final int remoteVisitsCol = c.getColumnIndexOrThrow(BrowserContract.History.REMOTE_VISITS);
+            final int localDateLastVisitedCol = c.getColumnIndexOrThrow(BrowserContract.History.LOCAL_DATE_LAST_VISITED);
+            final int remoteDateLastVisitedCol = c.getColumnIndexOrThrow(BrowserContract.History.REMOTE_DATE_LAST_VISITED);
+            final int dateCreatedCol = c.getColumnIndexOrThrow(BrowserContract.History.DATE_CREATED);
+            final int dateModifiedCol = c.getColumnIndexOrThrow(BrowserContract.History.DATE_MODIFIED);
+
+            while (c.moveToNext()) {
+                assertEquals(title, c.getString(titleCol));
+                assertEquals(url, c.getString(urlCol));
+
+                // History is inserted 'from sync', so expect local visit counts and local visit dates
+                // to be null.
+                assertEquals(0, c.getInt(localVisitsCol));
+                assertEquals(0, c.getLong(localDateLastVisitedCol));
+
+                if (remoteLastVisited == null) {
+                    assertEquals(0, c.getInt(remoteDateLastVisitedCol));
+                } else {
+                    assertEquals(remoteLastVisited, (Long) c.getLong(remoteDateLastVisitedCol));
+                    assertEquals(visits, (Integer) c.getInt(remoteVisitsCol));
+                    assertEquals(visits, (Integer) c.getInt(visitsCol));
+                }
+
+                // Make sure 'modified' and 'created' timestamps are present.
+                assertFalse(c.isNull(dateCreatedCol));
+                assertFalse(c.isNull(dateModifiedCol));
+
+                // Make sure these timestamps are also reasonable. Hopefully this doesn't fail due
+                // to poorly timed clock drift.
+                final long createdTimestamp = c.getLong(dateCreatedCol);
+                final long modifiedTimestamp = c.getLong(dateModifiedCol);
+                assertTrue(createdTimestamp + " must be greater than " + reasonablyRecentTimestamp,
+                        reasonablyRecentTimestamp < createdTimestamp);
+                assertTrue(modifiedTimestamp + " must be greater than " + reasonablyRecentTimestamp,
+                        reasonablyRecentTimestamp < c.getLong(dateModifiedCol));
+            }
+        } finally {
+            c.close();
+        }
+    }
+
     @Test
     public void testBulkHistoryInsert() throws Exception {
         Bundle result;
 
         // Test basic error conditions.
         String historyTestUriArg = historyTestUri.toString();
         try {
-            result = historyClient.call(BrowserContract.METHOD_INSERT_HISTORY_WITH_VISITS_FROM_SYNC, historyTestUriArg, new Bundle());
+            historyClient.call(BrowserContract.METHOD_INSERT_HISTORY_WITH_VISITS_FROM_SYNC, historyTestUriArg, new Bundle());
             fail();
         } catch (IllegalArgumentException e) {}
 
         final Bundle data = new Bundle();
 
         Bundle[] recordBundles = new Bundle[0];
         data.putSerializable(BrowserContract.METHOD_PARAM_DATA, recordBundles);
         result = historyClient.call(BrowserContract.METHOD_INSERT_HISTORY_WITH_VISITS_FROM_SYNC, historyTestUriArg, data);
         assertNotNull(result);
         assertNull(result.getSerializable(BrowserContract.METHOD_RESULT));
         assertRowCount(historyClient, historyTestUri, 0);
 
         // Test insert three history records with 10 visits each.
         recordBundles = new Bundle[3];
         for (int i = 0; i < 3; i++) {
             final Bundle bundle = new Bundle();
-            bundle.putParcelable(BrowserContract.METHOD_PARAM_OBJECT, buildHistoryCV("guid" + i, "Test", "https://www.mozilla.org/" + i, 10L, 10L, 10));
+            bundle.putParcelable(BrowserContract.METHOD_PARAM_OBJECT, buildHistoryCV("guid" + i, "Test", "https://www.mozilla.org/", 10L, 10L, 10));
             bundle.putSerializable(BrowserContract.History.VISITS, buildHistoryVisitsCVs(10, "guid" + i, 1L, 3, false));
             recordBundles[i] = bundle;
         }
         data.putSerializable(BrowserContract.METHOD_PARAM_DATA, recordBundles);
 
         result = historyClient.call(BrowserContract.METHOD_INSERT_HISTORY_WITH_VISITS_FROM_SYNC, historyTestUriArg, data);
         assertNotNull(result);
         assertNull(result.getSerializable(BrowserContract.METHOD_RESULT));
         assertRowCount(historyClient, historyTestUri, 3);
         assertRowCount(visitsClient, visitsTestUri, 30);
 
+        // Ensure that bulk-inserted records look sane.
+        assertHistoryValuesForGuidsFromSync(3, "Test", "https://www.mozilla.org/", 10L, 10);
+
         // Test insert mixed data.
         recordBundles = new Bundle[3];
         final Bundle bundle = new Bundle();
         bundle.putParcelable(BrowserContract.METHOD_PARAM_OBJECT, buildHistoryCV("guid4", null, "https://www.mozilla.org/1", null, null, null));
         bundle.putSerializable(BrowserContract.History.VISITS, new ContentValues[0]);
         recordBundles[0] = bundle;
         final Bundle bundle2 = new Bundle();
         bundle2.putParcelable(BrowserContract.METHOD_PARAM_OBJECT, buildHistoryCV("guid5", "Test", "https://www.mozilla.org/2", null, null, null));
@@ -317,16 +385,118 @@ public class BrowserProviderHistoryTest 
         assertNull(result.getSerializable(BrowserContract.METHOD_RESULT));
         assertRowCount(historyClient, historyTestUri, 6);
         assertRowCount(visitsClient, visitsTestUri, 35);
 
         assertHistoryAggregates(BrowserContract.History.URL + " = ?", new String[] {"https://www.mozilla.org/3"},
                 5, 0, 0, 5, 5);
     }
 
+    /**
+     * Tests that bulk-inserted history records without visits look correct.
+     */
+    @Test
+    public void testBulkHistoryInsertWithoutVisits() throws Exception {
+        final Bundle data = new Bundle();
+        // Test insert three history records with 10 visits each.
+        final int insertedRecordCount = 10;
+
+        Bundle[] recordBundles = new Bundle[insertedRecordCount];
+        for (int i = 0; i < insertedRecordCount; i++) {
+            final Bundle bundle = new Bundle();
+            bundle.putParcelable(BrowserContract.METHOD_PARAM_OBJECT, buildHistoryCV("guid" + i, "Test", "https://www.mozilla.org/", null, null, null));
+            bundle.putSerializable(BrowserContract.History.VISITS, new ContentValues[0]);
+            recordBundles[i] = bundle;
+        }
+        data.putSerializable(BrowserContract.METHOD_PARAM_DATA, recordBundles);
+
+        Bundle result = historyClient.call(BrowserContract.METHOD_INSERT_HISTORY_WITH_VISITS_FROM_SYNC, historyTestUri.toString(), data);
+        assertNotNull(result);
+        assertNull(result.getSerializable(BrowserContract.METHOD_RESULT));
+        assertRowCount(historyClient, historyTestUri, insertedRecordCount);
+        assertRowCount(visitsClient, visitsTestUri, 0);
+
+        assertHistoryValuesForGuidsFromSync(insertedRecordCount, "Test", "https://www.mozilla.org/", null, null);
+    }
+
+    /**
+     * Tests bulk-inserting history records, resetting modified/created timestamps to be older than
+     * the normal expiration 'keepAfter' threshold, and then running a normal expiration.
+     */
+    @Test
+    public void testBullkHistoryInsertThenNormalExpire() throws Exception {
+        final Bundle data = new Bundle();
+        // Test insert three history records with 10 visits each.
+        final int insertedRecordCount = 3000;
+
+        Bundle[] recordBundles = new Bundle[insertedRecordCount];
+        for (int i = 0; i < insertedRecordCount; i++) {
+            final Bundle bundle = new Bundle();
+            bundle.putParcelable(BrowserContract.METHOD_PARAM_OBJECT, buildHistoryCV("guid" + i, "Test", "https://www.mozilla.org/", 10L, 10L, 10));
+            bundle.putSerializable(BrowserContract.History.VISITS, buildHistoryVisitsCVs(10, "guid" + i, 1L, 3, false));
+            recordBundles[i] = bundle;
+        }
+        data.putSerializable(BrowserContract.METHOD_PARAM_DATA, recordBundles);
+
+        Bundle result = historyClient.call(BrowserContract.METHOD_INSERT_HISTORY_WITH_VISITS_FROM_SYNC, historyTestUri.toString(), data);
+        assertNotNull(result);
+        assertNull(result.getSerializable(BrowserContract.METHOD_RESULT));
+        assertRowCount(historyClient, historyTestUri, insertedRecordCount);
+        assertRowCount(visitsClient, visitsTestUri, insertedRecordCount * 10);
+
+        assertHistoryValuesForGuidsFromSync(insertedRecordCount, "Test", "https://www.mozilla.org/", 10L, 10);
+
+        long twoMonthsAgo = System.currentTimeMillis() - 1000L * 60 * 60 * 12 * 60;
+        // inserting a new entry sets the date created and modified automatically, so let's reset them
+        ContentValues cv = new ContentValues();
+        cv.put(BrowserContract.History.DATE_CREATED, twoMonthsAgo);
+        cv.put(BrowserContract.History.DATE_MODIFIED, twoMonthsAgo);
+        assertEquals(insertedRecordCount, historyClient.update(historyTestUri, cv, null, null));
+
+        historyClient.delete(expireHistoryNormalUri, null, null);
+
+        // Expect that we're left with just 2000 records.
+        assertHistoryValuesForGuidsFromSync(2000, "Test", "https://www.mozilla.org/", 10L, 10);
+        assertRowCount(visitsClient, visitsTestUri, 2000 * 10);
+    }
+
+    /**
+     * Tests bulk-inserting history records, and then running a aggressive expiration.
+     */
+    @Test
+    public void testBullkHistoryInsertThenAggressiveExpire() throws Exception {
+        final Bundle data = new Bundle();
+        // Test insert three history records with 10 visits each.
+        final int insertedRecordCount = 1000;
+
+        Bundle[] recordBundles = new Bundle[insertedRecordCount];
+        for (int i = 0; i < insertedRecordCount; i++) {
+            final Bundle bundle = new Bundle();
+            bundle.putParcelable(BrowserContract.METHOD_PARAM_OBJECT, buildHistoryCV("guid" + i, "Test", "https://www.mozilla.org/", 10L, 10L, 10));
+            bundle.putSerializable(BrowserContract.History.VISITS, buildHistoryVisitsCVs(10, "guid" + i, 1L, 3, false));
+            recordBundles[i] = bundle;
+        }
+        data.putSerializable(BrowserContract.METHOD_PARAM_DATA, recordBundles);
+
+        Bundle result = historyClient.call(BrowserContract.METHOD_INSERT_HISTORY_WITH_VISITS_FROM_SYNC, historyTestUri.toString(), data);
+        assertNotNull(result);
+        assertNull(result.getSerializable(BrowserContract.METHOD_RESULT));
+        assertRowCount(historyClient, historyTestUri, insertedRecordCount);
+        assertRowCount(visitsClient, visitsTestUri, insertedRecordCount * 10);
+
+        assertHistoryValuesForGuidsFromSync(insertedRecordCount, "Test", "https://www.mozilla.org/", 10L, 10);
+
+        // Aggressive expiration doesn't care about record timestamps.
+        historyClient.delete(expireHistoryAggressiveUri, null, null);
+
+        // Expect that we're left with just 500 records.
+        assertHistoryValuesForGuidsFromSync(500, "Test", "https://www.mozilla.org/", 10L, 10);
+        assertRowCount(visitsClient, visitsTestUri, 500 * 10);
+    }
+
     private ContentValues[] buildHistoryVisitsCVs(int numberOfVisits, String guid, long baseDate, int visitType, boolean isLocal) {
         final ContentValues[] visits = new ContentValues[numberOfVisits];
         for (int i = 0; i < numberOfVisits; i++) {
             final ContentValues visit = new ContentValues();
             visit.put(BrowserContract.Visits.HISTORY_GUID, guid);
             visit.put(BrowserContract.Visits.DATE_VISITED, baseDate + i);
             visit.put(BrowserContract.Visits.VISIT_TYPE, visitType);
             visit.put(BrowserContract.Visits.IS_LOCAL, isLocal ? BrowserContract.Visits.VISIT_IS_LOCAL : BrowserContract.Visits.VISIT_IS_REMOTE);