Make the site engagement service more robust to clock changes and time inconsistencies.

chrome/browser/engagement/site_engagement_service.cc

Index: chrome/browser/engagement/site_engagement_service.cc
diff --git a/chrome/browser/engagement/site_engagement_service.cc b/chrome/browser/engagement/site_engagement_service.cc
index 4437b54d108e40c9691a0789fe3473d65a794641..0b1ff519a30c7967c859dbc706acc45385eb352a 100644
--- a/chrome/browser/engagement/site_engagement_service.cc
+++ b/chrome/browser/engagement/site_engagement_service.cc
@@ -299,30 +299,46 @@ void SiteEngagementService::CleanupEngagementScores(
       GetEngagementContentSettings(settings_map);
 
   // We want to rebase last engagement times relative to MaxDecaysPerScore
-  // periods of decay in the past.
+  // periods of decay in the past. If |update_last_engagement_time| = true, we
+  // should have last_engagement_time < rebase_time < now.
   base::Time now = clock_->Now();
   base::Time last_engagement_time = GetLastEngagementTime();
   base::Time rebase_time = now - GetMaxDecayPeriod();
   base::Time new_last_engagement_time;
+  DCHECK(!update_last_engagement_time ||
+         (last_engagement_time < rebase_time && rebase_time < now));
+
   for (const auto& site : *engagement_settings) {
     GURL origin(site.primary_pattern.ToString());
 
     if (origin.is_valid()) {
       SiteEngagementScore score = CreateEngagementScore(origin);
       if (update_last_engagement_time) {
-        // Work out the offset between this score's last engagement time and the
-        // last time the service recorded any engagement. Set the score's last
-        // engagement time to rebase_time - offset to preserve its state,
-        // relative to the rebase date. This ensures that the score will decay
-        // the next time it is used, but will not decay too much.
-        DCHECK_LE(score.last_engagement_time(), rebase_time);
-        base::TimeDelta offset =
-            last_engagement_time - score.last_engagement_time();
-        base::Time rebase_score_time = rebase_time - offset;
-        score.set_last_engagement_time(rebase_score_time);
-        if (rebase_score_time > new_last_engagement_time)
-          new_last_engagement_time = rebase_score_time;
-
+        // Catch cases of users moving their clocks, or a potential race where
+        // a score content setting is written out to prefs, but the updated
+        // |last_engagement_time| was not written, as both are lossy
+        // preferences.
+        if (score.last_engagement_time() > rebase_time) {
+          score.set_last_engagement_time(now);
+        } else if (score.last_engagement_time() > last_engagement_time) {
+          // This score is newer than |last_engagement_time|, but older than
+          // |rebase_time|. It should still be rebased with no offset as we
+          // don't accurately know what the offset should be.
+          score.set_last_engagement_time(rebase_time);
+        } else {
+          // Work out the offset between this score's last engagement time and
+          // the last time the service recorded any engagement. Set the score's
+          // last engagement time to rebase_time - offset to preserve its state,
+          // relative to the rebase date. This ensures that the score will decay
+          // the next time it is used, but will not decay too much.
+          base::TimeDelta offset =
+              last_engagement_time - score.last_engagement_time();
+          base::Time rebase_score_time = rebase_time - offset;
+          score.set_last_engagement_time(rebase_score_time);
+        }
+
+        if (score.last_engagement_time() > new_last_engagement_time)
+          new_last_engagement_time = score.last_engagement_time();
         score.Commit();
       }