Bug 1336001 - Fix a racing condition, allowing batching uploader to deal correctly with multiple batches r=rnewman draft
authorGrigory Kruglov <gkruglov@mozilla.com>
Thu, 09 Feb 2017 12:20:55 -0800
changeset 481434 7f7ebb0d39eeee8d6b125f413ca2da3740fe59db
parent 469123 f3d187bd0733b1182dffc97b5dfe623e18f92a44
child 545179 9d04e2ee6fa9b7152bfff1e1a12dcf977fa9720e
push id44795
push usergkruglov@mozilla.com
push dateThu, 09 Feb 2017 20:25:31 +0000
reviewersrnewman
bugs1336001
milestone54.0a1
Bug 1336001 - Fix a racing condition, allowing batching uploader to deal correctly with multiple batches r=rnewman Two threads were racing to get to batchMeta - one to reset its state, and the other to read its internal state to construct a network request. This patch synchronizes access when necessary using a re-openable gate, and ensures that network request ownre receives all of the information necessary to construct a request (GET params & X-I-U-S header value). Some tests adjustments were necessary as well. MozReview-Commit-ID: 9hFs3fXGaGM
mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/SyncStorageRequest.java
mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/uploaders/BatchMeta.java
mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/uploaders/BatchingUploader.java
mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/uploaders/PayloadUploadDelegate.java
mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/uploaders/RecordUploadRunnable.java
mobile/android/tests/background/junit4/src/org/mozilla/android/sync/test/TestServer11RepositorySession.java
mobile/android/tests/background/junit4/src/org/mozilla/gecko/sync/repositories/uploaders/BatchingUploaderTest.java
mobile/android/tests/background/junit4/src/org/mozilla/gecko/sync/repositories/uploaders/PayloadUploadDelegateTest.java
mobile/android/tests/background/junit4/src/org/mozilla/gecko/sync/repositories/uploaders/RecordUploadRunnableTest.java
--- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/SyncStorageRequest.java
+++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/SyncStorageRequest.java
@@ -67,19 +67,24 @@ public class SyncStorageRequest implemen
   public SyncStorageRequest(String uri) throws URISyntaxException {
     this(new URI(uri));
   }
 
   /**
    * @param uri
    */
   public SyncStorageRequest(URI uri) {
+    this(uri, null);
+  }
+
+  public SyncStorageRequest(URI uri, String ifUnmodifiedSince) {
     this.resource = new BaseResource(uri);
     this.resourceDelegate = this.makeResourceDelegate(this);
     this.resource.delegate = this.resourceDelegate;
+    this.ifUnmodifiedSince = ifUnmodifiedSince;
   }
 
   @Override
   public URI getURI() {
     return this.resource.getURI();
   }
 
   @Override
@@ -148,35 +153,35 @@ public class SyncStorageRequest implemen
     @Override
     public void handleTransportException(GeneralSecurityException e) {
       this.request.delegate.handleRequestError(e);
     }
 
     @Override
     public void addHeaders(HttpRequestBase request, DefaultHttpClient client) {
       // Clients can use their delegate interface to specify X-If-Unmodified-Since.
-      String ifUnmodifiedSince = this.request.delegate.ifUnmodifiedSince();
+      String ifUnmodifiedSince = this.request.ifUnmodifiedSince;
+      if (ifUnmodifiedSince == null) {
+        ifUnmodifiedSince = this.request.delegate.ifUnmodifiedSince();
+      }
       if (ifUnmodifiedSince != null) {
         Logger.debug(LOG_TAG, "Making request with X-If-Unmodified-Since = " + ifUnmodifiedSince);
         request.setHeader("x-if-unmodified-since", ifUnmodifiedSince);
       }
       if (request.getMethod().equalsIgnoreCase("DELETE")) {
         request.addHeader("x-confirm-delete", "1");
       }
     }
   }
 
+  protected final String ifUnmodifiedSince;
   protected BaseResourceDelegate resourceDelegate;
   public SyncStorageRequestDelegate delegate;
   protected BaseResource resource;
 
-  public SyncStorageRequest() {
-    super();
-  }
-
   // Default implementation. Override this.
   protected BaseResourceDelegate makeResourceDelegate(SyncStorageRequest request) {
     return new SyncStorageResourceDelegate(request);
   }
 
   @Override
   public void get() {
     this.resource.get();
--- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/uploaders/BatchMeta.java
+++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/uploaders/BatchMeta.java
@@ -36,19 +36,22 @@ public class BatchMeta extends BufferSiz
 
     // Accessed by synchronously running threads.
     /* @GuardedBy("accessLock") */ private final List<String> successRecordGuids = new ArrayList<>();
 
     /* @GuardedBy("accessLock") */ private boolean needsCommit = false;
 
     protected final Long collectionLastModified;
 
+    private final BatchingUploader.ReclosableGate metaUpdatesGate = new BatchingUploader.ReclosableGate();
+
     public BatchMeta(@NonNull Object payloadLock, long maxBytes, long maxRecords, @Nullable Long collectionLastModified) {
         super(payloadLock, maxBytes, maxRecords);
         this.collectionLastModified = collectionLastModified;
+        this.metaUpdatesGate.open();
     }
 
     protected void setIsUnlimited(boolean isUnlimited) {
         synchronized (accessLock) {
             this.isUnlimited = isUnlimited;
         }
     }
 
@@ -75,16 +78,22 @@ public class BatchMeta extends BufferSiz
         }
     }
 
     protected synchronized String getToken() {
         return token;
     }
 
     protected synchronized void setToken(final String newToken, boolean isCommit) throws TokenModifiedException {
+        try {
+            metaUpdatesGate.await();
+        } catch (InterruptedException e) {
+            Thread.currentThread().interrupt();
+        }
+
         // Set token once in a batching mode.
         // In a non-batching mode, this.token and newToken will be null, and this is a no-op.
         if (token == null) {
             token = newToken;
             return;
         }
 
         // Sanity checks.
@@ -107,16 +116,22 @@ public class BatchMeta extends BufferSiz
     protected synchronized Long getLastModified() {
         if (lastModified == null) {
             return collectionLastModified;
         }
         return lastModified;
     }
 
     protected synchronized void setLastModified(final Long newLastModified, final boolean expectedToChange) throws LastModifiedChangedUnexpectedly, LastModifiedDidNotChange {
+        try {
+            metaUpdatesGate.await();
+        } catch (InterruptedException e) {
+            Thread.currentThread().interrupt();
+        }
+
         if (lastModified == null) {
             lastModified = newLastModified;
             return;
         }
 
         if (!expectedToChange && !lastModified.equals(newLastModified)) {
             Logger.debug(LOG_TAG, "Last-Modified timestamp changed when we didn't expect it");
             throw new LastModifiedChangedUnexpectedly();
@@ -155,11 +170,20 @@ public class BatchMeta extends BufferSiz
     @Override
     protected void reset() {
         synchronized (accessLock) {
             super.reset();
             token = null;
             lastModified = null;
             successRecordGuids.clear();
             needsCommit = false;
+            metaUpdatesGate.open();
         }
     }
+
+    /**
+     * Allows callers to block attempts to update Last-Modified and Token until {@link #reset()} has
+     * been called. Threads trying to update L-M and Token will be forced to wait if necessary.
+     */
+    /* package-local */ void blockMetaUpdatesUntilReset() {
+        metaUpdatesGate.close();
+    }
 }
\ No newline at end of file
--- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/uploaders/BatchingUploader.java
+++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/uploaders/BatchingUploader.java
@@ -134,33 +134,31 @@ public class BatchingUploader {
                 addAndFlushIfNecessary(recordDeltaByteCount, recordBytes, guid);
 
             // Batch won't fit the record.
             } else {
                 Logger.debug(LOG_TAG, "Current batch won't fit incoming record, committing batch.");
                 flush(true, false);
 
                 Logger.debug(LOG_TAG, "Recording the incoming record into a new batch");
-                batchMeta.reset();
 
                 // Keep track of the overflow record.
                 addAndFlushIfNecessary(recordDeltaByteCount, recordBytes, guid);
             }
         }
     }
 
     // Convenience function used from the process method; caller must hold a payloadLock.
     private void addAndFlushIfNecessary(long byteCount, byte[] recordBytes, String guid) {
         boolean isPayloadFull = payload.addAndEstimateIfFull(byteCount, recordBytes, guid);
         boolean isBatchFull = batchMeta.addAndEstimateIfFull(byteCount);
 
         // Preemptive commit batch or upload a payload if they're estimated to be full.
         if (isBatchFull) {
             flush(true, false);
-            batchMeta.reset();
         } else if (isPayloadFull) {
             flush(false, false);
         }
     }
 
     public void noMoreRecordsToUpload() {
         Logger.debug(LOG_TAG, "Received 'no more records to upload' signal.");
 
@@ -301,28 +299,88 @@ public class BatchingUploader {
 
         // Even though payload object itself is thread-safe, we want to ensure we get these altogether
         // as a "unit". Another approach would be to create a wrapper object for these values, but this works.
         synchronized (payloadLock) {
             outgoing = payload.getRecordsBuffer();
             outgoingGuids = payload.getRecordGuidsBuffer();
             byteCount = payload.getByteCount();
         }
+        payload.reset();
 
-        workQueue.execute(new RecordUploadRunnable(
-                new BatchingAtomicUploaderMayUploadProvider(),
-                collectionUri,
-                batchMeta,
-                new PayloadUploadDelegate(this, outgoingGuids, isCommit, isLastPayload),
-                outgoing,
-                byteCount,
-                isCommit
-        ));
+        // If we're going to reset batchMeta, lock the gate and wait before resetting until the worker
+        // below opens the gate. This avoids a race between the worker and the reset call.
+        // See Bug 1336001.
+        final boolean batchWillReset = isCommit && !isLastPayload;
+        final ReclosableGate batchMetaGate = new ReclosableGate();
+        if (batchWillReset) {
+            batchMetaGate.close();
+
+            // This sort of a simple gated access to batchMeta allows for a theoretical possibility
+            // of a race between network request/PayloadUploadDelegate and the reset call below.
+            // Even though timing of network requests makes this race a fairly improbable event,
+            // let's rule it out completely and force updates to wait until reset has completed.
+            batchMeta.blockMetaUpdatesUntilReset();
+        }
+
+        // Note that workQueue is expected to be a SingleThreadExecutor.
+        workQueue.execute(new BatchContextRunnable(isCommit) {
+            @Override
+            public void run() {
+                final String batchToken;
+                final Long batchLastModified;
+
+                synchronized (batchMeta) {
+                    batchToken = batchMeta.getToken();
+                    batchLastModified = batchMeta.getLastModified();
+                }
+
+                // If batchMeta needs to be reset, unlock the gate since we are now done with it.
+                if (batchWillReset) {
+                    batchMetaGate.open();
+                }
 
-        payload.reset();
+                new RecordUploadRunnable(
+                        new BatchingAtomicUploaderMayUploadProvider(),
+                        collectionUri,
+                        batchToken,
+                        batchLastModified,
+                        new PayloadUploadDelegate(BatchingUploader.this, outgoingGuids, isCommit, isLastPayload),
+                        outgoing,
+                        byteCount,
+                        isCommit
+                ).run();
+            }
+        });
+
+        // We're done if we don't need to reset the batch.
+        if (!batchWillReset) {
+            return;
+        }
+
+        // Wait for worker above to unlock the gate before resetting.
+        try {
+            batchMetaGate.await();
+        } catch (InterruptedException e) {
+            Thread.currentThread().interrupt();
+        }
+
+        batchMeta.reset();
+    }
+
+    /**
+     * Allows tests to easily peek into the flow of upload tasks.
+     */
+    @VisibleForTesting
+    abstract class BatchContextRunnable implements Runnable {
+        boolean isCommit;
+
+        BatchContextRunnable(boolean isCommit) {
+            this.isCommit = isCommit;
+        }
     }
 
     private class BatchingAtomicUploaderMayUploadProvider implements MayUploadProvider {
         public boolean mayUpload() {
             return !recordUploadFailed;
         }
     }
 
@@ -336,9 +394,39 @@ public class BatchingUploader {
         private static final long serialVersionUID = 1L;
     }
     public static class LastModifiedChangedUnexpectedly extends BatchingUploaderException {
         private static final long serialVersionUID = 1L;
     }
     public static class TokenModifiedException extends BatchingUploaderException {
         private static final long serialVersionUID = 1L;
     };
+
+    /**
+     * Re-closable gate which is used to synchronize read/write access to batchMeta while flushing
+     * requests.
+     */
+    static class ReclosableGate {
+        private boolean isOpen;
+        private int generation;
+
+        /* package-local */ synchronized void close() {
+            isOpen = false;
+        }
+
+        /* package-local */ synchronized void open() {
+            generation++;
+            isOpen = true;
+            notifyAll();
+        }
+
+        // Upon arrival at the gate, threads receive a "generation" number.
+        // They will be let through if the gate has been opened at least once since their generation
+        // number has been issued.
+        // This is to avoid blocking threads if the gate rapidly opens and closes.
+        /* package-local */ synchronized void await() throws InterruptedException {
+            int arrivalGeneration = generation;
+            while (!isOpen && arrivalGeneration == generation) {
+                wait();
+            }
+        }
+    }
 }
\ No newline at end of file
--- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/uploaders/PayloadUploadDelegate.java
+++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/uploaders/PayloadUploadDelegate.java
@@ -5,17 +5,16 @@
 package org.mozilla.gecko.sync.repositories.uploaders;
 
 import org.json.simple.JSONArray;
 import org.mozilla.gecko.background.common.log.Logger;
 import org.mozilla.gecko.sync.ExtendedJSONObject;
 import org.mozilla.gecko.sync.HTTPFailureException;
 import org.mozilla.gecko.sync.NonArrayJSONException;
 import org.mozilla.gecko.sync.NonObjectJSONException;
-import org.mozilla.gecko.sync.Utils;
 import org.mozilla.gecko.sync.net.AuthHeaderProvider;
 import org.mozilla.gecko.sync.net.SyncResponse;
 import org.mozilla.gecko.sync.net.SyncStorageRequestDelegate;
 import org.mozilla.gecko.sync.net.SyncStorageResponse;
 
 import java.util.ArrayList;
 
 public class PayloadUploadDelegate implements SyncStorageRequestDelegate {
@@ -37,21 +36,17 @@ public class PayloadUploadDelegate imple
 
     @Override
     public AuthHeaderProvider getAuthHeaderProvider() {
         return uploader.getRepositorySession().getServerRepository().getAuthHeaderProvider();
     }
 
     @Override
     public String ifUnmodifiedSince() {
-        final Long lastModified = uploader.getCurrentBatch().getLastModified();
-        if (lastModified == null) {
-            return null;
-        }
-        return Utils.millisecondsToDecimalSecondsString(lastModified);
+        throw new IllegalStateException("Request must be configured with a X-I-U-S header");
     }
 
     @Override
     public void handleRequestSuccess(final SyncStorageResponse response) {
         // First, do some sanity checking.
         if (response.getStatusCode() != 200 && response.getStatusCode() != 202) {
             handleRequestError(
                 new IllegalStateException("handleRequestSuccess received a non-200/202 response: " + response.getStatusCode())
--- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/uploaders/RecordUploadRunnable.java
+++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/uploaders/RecordUploadRunnable.java
@@ -1,19 +1,21 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 package org.mozilla.gecko.sync.repositories.uploaders;
 
 import android.net.Uri;
+import android.support.annotation.Nullable;
 import android.support.annotation.VisibleForTesting;
 
 import org.mozilla.gecko.background.common.log.Logger;
 import org.mozilla.gecko.sync.Server11PreviousPostFailedException;
+import org.mozilla.gecko.sync.Utils;
 import org.mozilla.gecko.sync.net.SyncStorageRequest;
 import org.mozilla.gecko.sync.net.SyncStorageRequestDelegate;
 
 import java.io.IOException;
 import java.io.OutputStream;
 import java.net.URI;
 import java.net.URISyntaxException;
 import java.util.ArrayList;
@@ -40,30 +42,33 @@ public class RecordUploadRunnable implem
 
     private final ArrayList<byte[]> outgoing;
     private final long byteCount;
 
     // Used to construct POST URI during run().
     @VisibleForTesting
     public final boolean isCommit;
     private final Uri collectionUri;
-    private final BatchMeta batchMeta;
+    private final String batchToken;
+    private final long batchLastModified;
 
     public RecordUploadRunnable(MayUploadProvider mayUploadProvider,
                                 Uri collectionUri,
-                                BatchMeta batchMeta,
+                                String batchToken,
+                                long batchLastModified,
                                 SyncStorageRequestDelegate uploadDelegate,
                                 ArrayList<byte[]> outgoing,
                                 long byteCount,
                                 boolean isCommit) {
         this.mayUploadProvider = mayUploadProvider;
         this.uploadDelegate = uploadDelegate;
         this.outgoing = outgoing;
         this.byteCount = byteCount;
-        this.batchMeta = batchMeta;
+        this.batchToken = batchToken;
+        this.batchLastModified = batchLastModified;
         this.collectionUri = collectionUri;
         this.isCommit = isCommit;
     }
 
     public static class ByteArraysContentProducer implements ContentProducer {
         ArrayList<byte[]> outgoing;
         public ByteArraysContentProducer(ArrayList<byte[]> arrays) {
             outgoing = arrays;
@@ -139,28 +144,29 @@ public class RecordUploadRunnable implem
         }
 
         Logger.trace(LOG_TAG, "Running upload task. Outgoing records: " + outgoing.size());
 
         // We don't want the task queue to proceed until this request completes.
         // Fortunately, BaseResource is currently synchronous.
         // If that ever changes, you'll need to block here.
 
-        final URI postURI = buildPostURI(isCommit, batchMeta, collectionUri);
-        final SyncStorageRequest request = new SyncStorageRequest(postURI);
+        final URI postURI = buildPostURI(isCommit, batchToken, collectionUri);
+        final SyncStorageRequest request = new SyncStorageRequest(
+                postURI, Utils.millisecondsToDecimalSecondsString(batchLastModified)
+        );
         request.delegate = uploadDelegate;
 
         ByteArraysEntity body = new ByteArraysEntity(outgoing, byteCount);
         request.post(body);
     }
 
     @VisibleForTesting
-    public static URI buildPostURI(boolean isCommit, BatchMeta batchMeta, Uri collectionUri) {
+    public static URI buildPostURI(boolean isCommit, @Nullable String batchToken, Uri collectionUri) {
         final Uri.Builder uriBuilder = collectionUri.buildUpon();
-        final String batchToken = batchMeta.getToken();
 
         if (batchToken != null) {
             uriBuilder.appendQueryParameter(QUERY_PARAM_BATCH, batchToken);
         } else {
             uriBuilder.appendQueryParameter(QUERY_PARAM_BATCH, QUERY_PARAM_TRUE);
         }
 
         if (isCommit) {
--- a/mobile/android/tests/background/junit4/src/org/mozilla/android/sync/test/TestServer11RepositorySession.java
+++ b/mobile/android/tests/background/junit4/src/org/mozilla/android/sync/test/TestServer11RepositorySession.java
@@ -67,17 +67,22 @@ public class TestServer11RepositorySessi
   static final String LOCAL_COUNTS_URL    = LOCAL_INFO_BASE_URL + "collection_counts";
 
   // Corresponds to rnewman+atest1@mozilla.com, local.
   static final String TEST_USERNAME          = "n6ec3u5bee3tixzp2asys7bs6fve4jfw";
   static final String TEST_PASSWORD          = "passowrd";
   static final String SYNC_KEY          = "eh7ppnb82iwr5kt3z3uyi5vr44";
 
   public final AuthHeaderProvider authHeaderProvider = new BasicAuthHeaderProvider(TEST_USERNAME, TEST_PASSWORD);
-  protected final InfoCollections infoCollections = new InfoCollections();
+  protected final InfoCollections infoCollections = new InfoCollections() {
+    @Override
+    public Long getTimestamp(String collection) {
+      return 0L;
+    }
+  };
   protected final InfoConfiguration infoConfiguration = new InfoConfiguration();
 
   // Few-second timeout so that our longer operations don't time out and cause spurious error-handling results.
   private static final int SHORT_TIMEOUT = 10000;
 
   public AuthHeaderProvider getAuthHeaderProvider() {
     return new BasicAuthHeaderProvider(TEST_USERNAME, TEST_PASSWORD);
   }
--- a/mobile/android/tests/background/junit4/src/org/mozilla/gecko/sync/repositories/uploaders/BatchingUploaderTest.java
+++ b/mobile/android/tests/background/junit4/src/org/mozilla/gecko/sync/repositories/uploaders/BatchingUploaderTest.java
@@ -29,19 +29,20 @@ import java.util.concurrent.ExecutorServ
 public class BatchingUploaderTest {
     class MockExecutorService implements Executor {
         public int totalPayloads = 0;
         public int commitPayloads = 0;
 
         @Override
         public void execute(@NonNull Runnable command) {
             ++totalPayloads;
-            if (((RecordUploadRunnable) command).isCommit) {
+            if (((BatchingUploader.BatchContextRunnable) command).isCommit) {
                 ++commitPayloads;
             }
+            command.run();
         }
     }
 
     class MockStoreDelegate implements RepositorySessionStoreDelegate {
         public int storeFailed = 0;
         public int storeSucceeded = 0;
         public int storeCompleted = 0;
 
@@ -425,17 +426,22 @@ public class BatchingUploaderTest {
 
         InfoConfiguration infoConfiguration = new InfoConfiguration(infoConfigurationJSON);
 
         try {
             return new Server11Repository(
                     "dummyCollection",
                     "http://dummy.url/",
                     null,
-                    new InfoCollections(),
+                    new InfoCollections() {
+                        @Override
+                        public Long getTimestamp(String collection) {
+                            return 0L;
+                        }
+                    },
                     infoConfiguration
             );
         } catch (URISyntaxException e) {
             // Won't throw, and this won't happen.
             return null;
         }
     }
 }
\ No newline at end of file
--- a/mobile/android/tests/background/junit4/src/org/mozilla/gecko/sync/repositories/uploaders/PayloadUploadDelegateTest.java
+++ b/mobile/android/tests/background/junit4/src/org/mozilla/gecko/sync/repositories/uploaders/PayloadUploadDelegateTest.java
@@ -367,28 +367,33 @@ public class PayloadUploadDelegateTest {
                 ((MockUploader) batchingUploader).failedRecords.get("testGuid3").getClass());
 
         payloadUploadDelegate = new PayloadUploadDelegate(batchingUploader, postedGuids, false, true);
         payloadUploadDelegate.handleRequestFailure(new SyncStorageResponse(response));
         assertEquals(3, ((MockUploader) batchingUploader).failedRecords.size());
         assertTrue(((MockUploader) batchingUploader).didLastPayloadFail);
     }
 
-    @Test
-    public void testIfUnmodifiedSince() {
+    @Test(expected = IllegalStateException.class)
+    public void testIfUnmodifiedSinceNoLM() {
         PayloadUploadDelegate payloadUploadDelegate = new PayloadUploadDelegate(
                 batchingUploader, new ArrayList<String>(), false, false);
 
-        assertNull(payloadUploadDelegate.ifUnmodifiedSince());
+        payloadUploadDelegate.ifUnmodifiedSince();
+    }
 
+    @Test(expected = IllegalStateException.class)
+    public void testIfUnmodifiedSinceWithLM() {
+        PayloadUploadDelegate payloadUploadDelegate = new PayloadUploadDelegate(
+                batchingUploader, new ArrayList<String>(), false, false);
         try {
             batchingUploader.getCurrentBatch().setLastModified(1471645412480L, true);
         } catch (BatchingUploader.BatchingUploaderException e) {}
 
-        assertEquals("1471645412.480", payloadUploadDelegate.ifUnmodifiedSince());
+        payloadUploadDelegate.ifUnmodifiedSince();
     }
 
     private SyncStorageResponse makeSyncStorageResponse(int code, String body, String lastModified) {
         BasicHttpResponse response = new BasicHttpResponse(
                 new BasicStatusLine(new ProtocolVersion("HTTP", 1, 1), code, null));
 
         if (body != null) {
             BasicHttpEntity entity = new BasicHttpEntity();
--- a/mobile/android/tests/background/junit4/src/org/mozilla/gecko/sync/repositories/uploaders/RecordUploadRunnableTest.java
+++ b/mobile/android/tests/background/junit4/src/org/mozilla/gecko/sync/repositories/uploaders/RecordUploadRunnableTest.java
@@ -12,27 +12,25 @@ import org.mozilla.gecko.background.test
 import java.net.URI;
 
 import static org.junit.Assert.*;
 
 @RunWith(TestRunner.class)
 public class RecordUploadRunnableTest {
     @Test
     public void testBuildPostURI() throws Exception {
-        BatchMeta batchMeta = new BatchMeta(new Object(), 1, 1, null);
         URI postURI = RecordUploadRunnable.buildPostURI(
-                false, batchMeta, Uri.parse("http://example.com/"));
+                false, null, Uri.parse("http://example.com/"));
         assertEquals("http://example.com/?batch=true", postURI.toString());
 
         postURI = RecordUploadRunnable.buildPostURI(
-                true, batchMeta, Uri.parse("http://example.com/"));
+                true, null, Uri.parse("http://example.com/"));
         assertEquals("http://example.com/?batch=true&commit=true", postURI.toString());
 
-        batchMeta.setToken("MTIzNA", false);
         postURI = RecordUploadRunnable.buildPostURI(
-                false, batchMeta, Uri.parse("http://example.com/"));
+                false, "MTIzNA", Uri.parse("http://example.com/"));
         assertEquals("http://example.com/?batch=MTIzNA", postURI.toString());
 
         postURI = RecordUploadRunnable.buildPostURI(
-                true, batchMeta, Uri.parse("http://example.com/"));
+                true, "MTIzNA", Uri.parse("http://example.com/"));
         assertEquals("http://example.com/?batch=MTIzNA&commit=true", postURI.toString());
     }
 }
\ No newline at end of file