Bug 817842 - Announcements: use previous Date header for next If-Modified-Since to avoid clock skew. r=nalexander

mobile/android/base/background/announcements/AnnouncementsBroadcastService.java

@@ -156,7 +156,7 @@ public class AnnouncementsBroadcastService extends IntentService {
       final SharedPreferences sharedPreferences = this.getSharedPreferences(AnnouncementsConstants.PREFS_BRANCH,
                                                                             BackgroundConstants.SHARED_PREFERENCES_MODE);
       final Editor editor = sharedPreferences.edit();
-      editor.remove(AnnouncementsConstants.PREF_LAST_FETCH);
+      editor.remove(AnnouncementsConstants.PREF_LAST_FETCH_LOCAL_TIME);
       editor.remove(AnnouncementsConstants.PREF_EARLIEST_NEXT_ANNOUNCE_FETCH);
       editor.commit();
     }

mobile/android/base/background/announcements/AnnouncementsConstants.java.in

@@ -17,7 +17,8 @@ public class AnnouncementsConstants {
   public static final String ACTION_ANNOUNCEMENTS_PREF = "@ANDROID_PACKAGE_NAME@.ANNOUNCEMENTS_PREF";
 
   static final String PREFS_BRANCH = "background";
-  static final String PREF_LAST_FETCH  = "last_fetch";
+  static final String PREF_LAST_FETCH_LOCAL_TIME  = "last_fetch";
+  static final String PREF_LAST_FETCH_SERVER_DATE = "last_announce_date";
   static final String PREF_LAST_LAUNCH = "last_firefox_launch";
   static final String PREF_ANNOUNCE_SERVER_BASE_URL  = "announce_server_base_url";
   static final String PREF_EARLIEST_NEXT_ANNOUNCE_FETCH = "earliest_next_announce_fetch";

mobile/android/base/background/announcements/AnnouncementsFetchDelegate.java

@@ -13,6 +13,11 @@ public interface AnnouncementsFetchDelegate {
    */
   public long getLastFetch();
 
+  /**
+   * @return the Date header string of the last response, or null if not present.
+   */
+  public String getLastDate();
+
   /**
    * @return the current system locale (e.g., en_us).
    */
@@ -30,9 +35,12 @@ public interface AnnouncementsFetchDelegate {
 
   /*
    * Callback methods.
+   * Note that we provide both a local fetch time and a server date here.
+   * This is so we can track how long we've waited (local), and supply the
+   * date back to the server for If-Modified-Since.
    */
-  public void onNoNewAnnouncements(long fetched);
-  public void onNewAnnouncements(List<Announcement> snippets, long fetched);
+  public void onNoNewAnnouncements(long localFetchTime, String serverDate);
+  public void onNewAnnouncements(List<Announcement> snippets, long localFetchTime, String serverDate);
   public void onLocalError(Exception e);
   public void onRemoteError(Exception e);
   public void onRemoteFailure(int status);

mobile/android/base/background/announcements/AnnouncementsFetchResourceDelegate.java

@@ -20,11 +20,13 @@ import org.mozilla.gecko.sync.net.Resource;
 import org.mozilla.gecko.sync.net.SyncResourceDelegate;
 import org.mozilla.gecko.sync.net.SyncResponse;
 
+import ch.boye.httpclientandroidlib.Header;
 import ch.boye.httpclientandroidlib.HttpResponse;
 import ch.boye.httpclientandroidlib.client.ClientProtocolException;
 import ch.boye.httpclientandroidlib.client.methods.HttpRequestBase;
 import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;
 import ch.boye.httpclientandroidlib.impl.cookie.DateUtils;
+import ch.boye.httpclientandroidlib.protocol.HTTP;
 
 /**
  * Converts HTTP resource callbacks into AnnouncementsFetchDelegate callbacks.
@@ -56,11 +58,10 @@ public class AnnouncementsFetchResourceDelegate extends SyncResourceDelegate {
     request.addHeader("Connection", "close");
 
     // Set If-Modified-Since to avoid re-fetching content.
-    final long ifModifiedSince = delegate.getLastFetch();
-    if (ifModifiedSince > 0) {
-      final String imsHeader = DateUtils.formatDate(new Date(ifModifiedSince));
-      Logger.info(LOG_TAG, "If-Modified-Since: " + imsHeader);
-      request.addHeader("If-Modified-Since", imsHeader);
+    final String ifModifiedSince = delegate.getLastDate();
+    if (ifModifiedSince != null) {
+      Logger.info(LOG_TAG, "If-Modified-Since: " + ifModifiedSince);
+      request.addHeader("If-Modified-Since", ifModifiedSince);
     }
 
     // Just in case.
@@ -87,15 +88,29 @@ public class AnnouncementsFetchResourceDelegate extends SyncResourceDelegate {
 
   @Override
   public void handleHttpResponse(HttpResponse response) {
-    SyncResponse r = new SyncResponse(response);    // For convenience.
+    final Header dateHeader = response.getFirstHeader(HTTP.DATE_HEADER);
+    String date = null;
+    if (dateHeader != null) {
+      // Note that we are deliberately not validating the server time here.
+      // We pass it directly back to the server; we don't care about the
+      // contents, and if we reject a value we essentially re-initialize
+      // the client, which will cause stale announcements to be re-fetched.
+      date = dateHeader.getValue();
+    }
+    if (date == null) {
+      // Use local clock, because skipping is better than re-fetching.
+      date = DateUtils.formatDate(new Date());
+      Logger.warn(LOG_TAG, "No fetch date; using local time " + date);
+    }
 
+    final SyncResponse r = new SyncResponse(response);    // For convenience.
     try {
       final int statusCode = r.getStatusCode();
       Logger.debug(LOG_TAG, "Got announcements response: " + statusCode);
 
       if (statusCode == 204 || statusCode == 304) {
         BaseResource.consumeEntity(response);
-        delegate.onNoNewAnnouncements(startTime);
+        delegate.onNoNewAnnouncements(startTime, date);
         return;
       }
 
@@ -107,7 +122,7 @@ public class AnnouncementsFetchResourceDelegate extends SyncResourceDelegate {
           delegate.onRemoteError(e);
           return;
         }
-        delegate.onNewAnnouncements(snippets, startTime);
+        delegate.onNewAnnouncements(snippets, startTime, date);
         return;
       }
 

mobile/android/base/background/announcements/AnnouncementsService.java

@@ -184,12 +184,25 @@ public class AnnouncementsService extends IntentService implements Announcements
   }
 
   protected void setLastFetch(final long fetch) {
-    this.getSharedPreferences().edit().putLong(AnnouncementsConstants.PREF_LAST_FETCH, fetch).commit();
+    this.getSharedPreferences().edit().putLong(AnnouncementsConstants.PREF_LAST_FETCH_LOCAL_TIME, fetch).commit();
+  }
+
+  public long getLastFetch() {
+    return this.getSharedPreferences().getLong(AnnouncementsConstants.PREF_LAST_FETCH_LOCAL_TIME, 0L);
+  }
+
+  protected String setLastDate(final String fetch) {
+    if (fetch == null) {
+      this.getSharedPreferences().edit().remove(AnnouncementsConstants.PREF_LAST_FETCH_SERVER_DATE).commit();
+      return null;
+    }
+    this.getSharedPreferences().edit().putString(AnnouncementsConstants.PREF_LAST_FETCH_SERVER_DATE, fetch).commit();
+    return fetch;
   }
 
   @Override
-  public long getLastFetch() {
-    return getSharedPreferences().getLong(AnnouncementsConstants.PREF_LAST_FETCH, 0L);
+  public String getLastDate() {
+    return this.getSharedPreferences().getString(AnnouncementsConstants.PREF_LAST_FETCH_SERVER_DATE, null);
   }
 
   /**
@@ -235,16 +248,23 @@ public class AnnouncementsService extends IntentService implements Announcements
     return AnnouncementsConstants.ANNOUNCE_USER_AGENT;
   }
 
-  @Override
-  public void onNoNewAnnouncements(long fetched) {
-    Logger.info(LOG_TAG, "No new announcements to display.");
+  protected void persistTimes(long fetched, String date) {
     setLastFetch(fetched);
+    if (date != null) {
+      setLastDate(date);
+    }
   }
 
   @Override
-  public void onNewAnnouncements(List<Announcement> announcements, long fetched) {
+  public void onNoNewAnnouncements(long fetched, String date) {
+    Logger.info(LOG_TAG, "No new announcements to display.");
+    persistTimes(fetched, date);
+  }
+
+  @Override
+  public void onNewAnnouncements(List<Announcement> announcements, long fetched, String date) {
     Logger.info(LOG_TAG, "Processing announcements: " + announcements.size());
-    setLastFetch(fetched);
+    persistTimes(fetched, date);
     processAnnouncements(announcements);
   }