From 0c44faad12e41732362b01e9cc0a014ea9b680ab Mon Sep 17 00:00:00 2001 From: Richard Newman Date: Tue, 4 Dec 2012 10:58:07 -0800 Subject: [PATCH] Bug 817842 - Announcements: use previous Date header for next If-Modified-Since to avoid clock skew. r=nalexander --- .../AnnouncementsBroadcastService.java | 2 +- .../AnnouncementsConstants.java.in | 3 +- .../AnnouncementsFetchDelegate.java | 12 +++++-- .../AnnouncementsFetchResourceDelegate.java | 31 +++++++++++----- .../announcements/AnnouncementsService.java | 36 ++++++++++++++----- 5 files changed, 64 insertions(+), 20 deletions(-) diff --git a/mobile/android/base/background/announcements/AnnouncementsBroadcastService.java b/mobile/android/base/background/announcements/AnnouncementsBroadcastService.java index b2fcf8a8e7d..b2c005b19ff 100644 --- a/mobile/android/base/background/announcements/AnnouncementsBroadcastService.java +++ b/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(); } diff --git a/mobile/android/base/background/announcements/AnnouncementsConstants.java.in b/mobile/android/base/background/announcements/AnnouncementsConstants.java.in index 61bae0a090b..0aef99e93bb 100644 --- a/mobile/android/base/background/announcements/AnnouncementsConstants.java.in +++ b/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"; diff --git a/mobile/android/base/background/announcements/AnnouncementsFetchDelegate.java b/mobile/android/base/background/announcements/AnnouncementsFetchDelegate.java index 6f1771afe6a..b75de86f411 100644 --- a/mobile/android/base/background/announcements/AnnouncementsFetchDelegate.java +++ b/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 snippets, long fetched); + public void onNoNewAnnouncements(long localFetchTime, String serverDate); + public void onNewAnnouncements(List snippets, long localFetchTime, String serverDate); public void onLocalError(Exception e); public void onRemoteError(Exception e); public void onRemoteFailure(int status); diff --git a/mobile/android/base/background/announcements/AnnouncementsFetchResourceDelegate.java b/mobile/android/base/background/announcements/AnnouncementsFetchResourceDelegate.java index 41c4967ea92..d2bb4751232 100644 --- a/mobile/android/base/background/announcements/AnnouncementsFetchResourceDelegate.java +++ b/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; } diff --git a/mobile/android/base/background/announcements/AnnouncementsService.java b/mobile/android/base/background/announcements/AnnouncementsService.java index 527a063b9db..d15fd9b487c 100644 --- a/mobile/android/base/background/announcements/AnnouncementsService.java +++ b/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 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 announcements, long fetched, String date) { Logger.info(LOG_TAG, "Processing announcements: " + announcements.size()); - setLastFetch(fetched); + persistTimes(fetched, date); processAnnouncements(announcements); }