Bug 1215106 - DownloadContentService: Mark content as permanently failed after multiple errors of the same type. r=rnewman

--HG--
extra : commitid : AuFevajQBmB
extra : rebase_source : 55dc093df961d8e7962d3c6689a7666a924660e6

mobile/android/base/java/org/mozilla/gecko/dlc/BaseAction.java

@@ -6,6 +6,7 @@
 package org.mozilla.gecko.dlc;
 
 import android.content.Context;
+import android.support.annotation.IntDef;
 import android.util.Log;
 
 import org.mozilla.gecko.background.nativecode.NativeCrypto;
@@ -29,12 +30,40 @@ public abstract class BaseAction {
     /* package-private */ static class RecoverableDownloadContentException extends Exception {
         private static final long serialVersionUID = -2246772819507370734L;
 
-        public RecoverableDownloadContentException(String message) {
+        @IntDef({MEMORY, DISK_IO, SERVER, NETWORK})
+        public @interface ErrorType {}
+        public static final int MEMORY = 1;
+        public static final int DISK_IO = 2;
+        public static final int SERVER = 3;
+        public static final int NETWORK = 4;
+
+        private int errorType;
+
+        public RecoverableDownloadContentException(@ErrorType int errorType, String message) {
             super(message);
+            this.errorType = errorType;
         }
 
-        public RecoverableDownloadContentException(Throwable cause) {
+        public RecoverableDownloadContentException(@ErrorType int errorType, Throwable cause) {
             super(cause);
+            this.errorType = errorType;
+        }
+
+        @ErrorType
+        public int getErrorType() {
+            return errorType;
+        }
+
+        /**
+         * Should this error be counted as failure? If this type of error will happen multiple times in a row then this
+         * error will be treated as permanently and the operation will not be tried again until the content changes.
+         */
+        public boolean shouldBeCountedAsFailure() {
+            if (NETWORK == errorType) {
+                return false; // Always retry after network errors
+            }
+
+            return true;
         }
     }
 
@@ -74,7 +103,8 @@ public abstract class BaseAction {
 
             byte[] ctx = NativeCrypto.sha256init();
             if (ctx == null) {
-                throw new RecoverableDownloadContentException("Could not create SHA-256 context");
+                throw new RecoverableDownloadContentException(RecoverableDownloadContentException.MEMORY,
+                                                              "Could not create SHA-256 context");
             }
 
             byte[] buffer = new byte[4096];
@@ -94,7 +124,7 @@ public abstract class BaseAction {
             return true;
         } catch (IOException e) {
             // Recoverable: Just I/O discontinuation
-            throw new RecoverableDownloadContentException(e);
+            throw new RecoverableDownloadContentException(RecoverableDownloadContentException.DISK_IO, e);
         } finally {
             IOUtils.safeStreamClose(inputStream);
         }

mobile/android/base/java/org/mozilla/gecko/dlc/DownloadAction.java

@@ -126,6 +126,11 @@ public class DownloadAction extends BaseAction {
                 }
             } catch (RecoverableDownloadContentException e) {
                 Log.w(LOGTAG, "Downloading content failed (Recoverable): " + content , e);
+
+                if (e.shouldBeCountedAsFailure()) {
+                    catalog.rememberFailure(content, e.getErrorType());
+                }
+
                 // TODO: Reschedule download (bug 1209498)
             } catch (UnrecoverableDownloadContentException e) {
                 Log.w(LOGTAG, "Downloading content failed (Unrecoverable): " + content, e);
@@ -161,7 +166,8 @@ public class DownloadAction extends BaseAction {
                 // TODO: This is guesstimating at best. We want to implement failure counters (Bug 1215106).
                 if (status >= 500) {
                     // Recoverable: Server errors 5xx
-                    throw new RecoverableDownloadContentException("(Recoverable) Download failed. Status code: " + status);
+                    throw new RecoverableDownloadContentException(RecoverableDownloadContentException.SERVER,
+                                                                  "(Recoverable) Download failed. Status code: " + status);
                 } else if (status >= 400) {
                     // Unrecoverable: Client errors 4xx - Unlikely that this version of the client will ever succeed.
                     throw new UnrecoverableDownloadContentException("(Unrecoverable) Download failed. Status code: " + status);
@@ -176,7 +182,7 @@ public class DownloadAction extends BaseAction {
             final HttpEntity entity = response.getEntity();
             if (entity == null) {
                 // Recoverable: Should not happen for a valid asset
-                throw new RecoverableDownloadContentException("Null entity");
+                throw new RecoverableDownloadContentException(RecoverableDownloadContentException.SERVER, "Null entity");
             }
 
             inputStream = new BufferedInputStream(entity.getContent());
@@ -188,7 +194,7 @@ public class DownloadAction extends BaseAction {
             outputStream.close();
         } catch (IOException e) {
             // Recoverable: Just I/O discontinuation
-            throw new RecoverableDownloadContentException(e);
+            throw new RecoverableDownloadContentException(RecoverableDownloadContentException.NETWORK, e);
         } finally {
             IOUtils.safeStreamClose(inputStream);
             IOUtils.safeStreamClose(outputStream);
@@ -229,7 +235,7 @@ public class DownloadAction extends BaseAction {
             move(temporaryFile, destinationFile);
         } catch (IOException e) {
             // We could not extract to the destination: Keep temporary file and try again next time we run.
-            throw new RecoverableDownloadContentException(e);
+            throw new RecoverableDownloadContentException(RecoverableDownloadContentException.DISK_IO, e);
         } finally {
             IOUtils.safeStreamClose(inputStream);
             IOUtils.safeStreamClose(outputStream);
@@ -272,7 +278,8 @@ public class DownloadAction extends BaseAction {
 
         if (!cacheDirectory.exists() && !cacheDirectory.mkdirs()) {
             // Recoverable: File system might not be mounted NOW and we didn't download anything yet anyways.
-            throw new RecoverableDownloadContentException("Could not create cache directory: " + cacheDirectory);
+            throw new RecoverableDownloadContentException(RecoverableDownloadContentException.DISK_IO,
+                                                          "Could not create cache directory: " + cacheDirectory);
         }
 
         return new File(cacheDirectory, content.getDownloadChecksum() + "-" + content.getId());
@@ -308,7 +315,7 @@ public class DownloadAction extends BaseAction {
         } catch (IOException e) {
             // We could not copy the temporary file to its destination: Keep the temporary file and
             // try again the next time we run.
-            throw new RecoverableDownloadContentException(e);
+            throw new RecoverableDownloadContentException(RecoverableDownloadContentException.DISK_IO, e);
         } finally {
             IOUtils.safeStreamClose(inputStream);
             IOUtils.safeStreamClose(outputStream);

mobile/android/base/java/org/mozilla/gecko/dlc/catalog/DownloadContent.java

@@ -23,6 +23,8 @@ public class DownloadContent {
     private static final String KEY_KIND = "kind";
     private static final String KEY_SIZE = "size";
     private static final String KEY_STATE = "state";
+    private static final String KEY_FAILURES = "failures";
+    private static final String KEY_LAST_FAILURE_TYPE = "last_failure_type";
 
     @IntDef({STATE_NONE, STATE_SCHEDULED, STATE_DOWNLOADED, STATE_FAILED, STATE_IGNORED})
     public @interface State {}
@@ -50,6 +52,8 @@ public class DownloadContent {
     private final String kind;
     private final long size;
     private int state;
+    private int failures;
+    private int lastFailureType;
 
     private DownloadContent(@NonNull String id, @NonNull String location, @NonNull String filename,
                             @NonNull String checksum, @NonNull String downloadChecksum, @NonNull long lastModified,
@@ -121,6 +125,24 @@ public class DownloadContent {
         return TYPE_ASSET_ARCHIVE.equals(type);
     }
 
+    /* package-private */ int getFailures() {
+        return failures;
+    }
+
+    /* package-private */ void rememberFailure(int failureType) {
+        if (lastFailureType != failureType) {
+            lastFailureType = failureType;
+            failures = 1;
+        } else {
+            failures++;
+        }
+    }
+
+    /* package-private */ void resetFailures() {
+        failures = 0;
+        lastFailureType = 0;
+    }
+
     public static DownloadContent fromJSON(JSONObject object) throws JSONException {
         return new Builder()
                 .setId(object.getString(KEY_ID))
@@ -133,6 +155,7 @@ public class DownloadContent {
                 .setKind(object.getString(KEY_KIND))
                 .setSize(object.getLong(KEY_SIZE))
                 .setState(object.getInt(KEY_STATE))
+                .setFailures(object.optInt(KEY_FAILURES), object.optInt(KEY_LAST_FAILURE_TYPE))
                 .build();
     }
 
@@ -148,6 +171,12 @@ public class DownloadContent {
         object.put(KEY_KIND, kind);
         object.put(KEY_SIZE, size);
         object.put(KEY_STATE, state);
+
+        if (failures > 0) {
+            object.put(KEY_FAILURES, failures);
+            object.put(KEY_LAST_FAILURE_TYPE, lastFailureType);
+        }
+
         return object;
     }
 
@@ -166,11 +195,16 @@ public class DownloadContent {
         private String kind;
         private long size;
         private int state;
+        private int failures;
+        private int lastFailureType;
 
         public DownloadContent build() {
             DownloadContent content = new DownloadContent(id, location, filename, checksum, downloadChecksum,
                                                           lastModified, type, kind, size);
             content.setState(state);
+            content.failures = failures;
+            content.lastFailureType = lastFailureType;
+
             return content;
         }
 
@@ -223,5 +257,12 @@ public class DownloadContent {
             this.state = state;
             return this;
         }
+
+        /* package-private */ Builder setFailures(int failures, int lastFailureType) {
+            this.failures = failures;
+            this.lastFailureType = lastFailureType;
+
+            return this;
+        }
     }
 }

mobile/android/base/java/org/mozilla/gecko/dlc/catalog/DownloadContentCatalog.java

@@ -31,6 +31,8 @@ public class DownloadContentCatalog {
     private static final String LOGTAG = "GeckoDLCCatalog";
     private static final String FILE_NAME = "download_content_catalog";
 
+    private static final int MAX_FAILURES_UNTIL_PERMANENTLY_FAILED = 10;
+
     private final AtomicFile file;          // Guarded by 'file'
     private List<DownloadContent> content;  // Guarded by 'this'
     private boolean hasLoadedCatalog;       // Guarded by 'this
@@ -107,6 +109,7 @@ public class DownloadContentCatalog {
 
     public synchronized void markAsDownloaded(DownloadContent content) {
         content.setState(DownloadContent.STATE_DOWNLOADED);
+        content.resetFailures();
         hasCatalogChanged = true;
     }
 
@@ -120,6 +123,17 @@ public class DownloadContentCatalog {
         hasCatalogChanged = true;
     }
 
+    public synchronized void rememberFailure(DownloadContent content, int failureType) {
+        if (content.getFailures() >= MAX_FAILURES_UNTIL_PERMANENTLY_FAILED) {
+            Log.d(LOGTAG, "Maximum number of failures reached. Marking content has permanently failed.");
+
+            markAsPermanentlyFailed(content);
+        } else {
+            content.rememberFailure(failureType);
+            hasCatalogChanged = true;
+        }
+    }
+
     public void persistChanges() {
         new Thread(LOGTAG + "-Persist") {
             public void run() {

mobile/android/tests/background/junit4/src/org/mozilla/gecko/dlc/TestDownloadAction.java

@@ -426,6 +426,83 @@ public class TestDownloadAction {
         Assert.assertTrue(action.hasEnoughDiskSpace(content, destinationFile, temporaryFile));
     }
 
+    /**
+     * Scenario: Download failed with network I/O error.
+     *
+     * Verify that:
+     *  * Error is not counted as failure
+     */
+    @Test
+    public void testNetworkErrorIsNotCountedAsFailure() throws Exception {
+        DownloadContent content = createFont();
+        DownloadContentCatalog catalog = mockCatalogWithScheduledDownloads(content);
+
+        DownloadAction action = spy(new DownloadAction(null));
+        doReturn(true).when(action).isConnectedToNetwork(RuntimeEnvironment.application);
+        doReturn(false).when(action).isActiveNetworkMetered(RuntimeEnvironment.application);
+        doReturn(mockNotExistingFile()).when(action).createTemporaryFile(RuntimeEnvironment.application, content);
+        doReturn(mockNotExistingFile()).when(action).getDestinationFile(RuntimeEnvironment.application, content);
+        doReturn(true).when(action).hasEnoughDiskSpace(eq(content), any(File.class), any(File.class));
+
+        HttpClient client = mock(HttpClient.class);
+        doThrow(IOException.class).when(client).execute(any(HttpUriRequest.class));
+        doReturn(client).when(action).buildHttpClient();
+
+        action.perform(RuntimeEnvironment.application, catalog);
+
+        verify(catalog, never()).rememberFailure(eq(content), anyInt());
+        verify(catalog, never()).markAsDownloaded(content);
+    }
+
+    /**
+     * Scenario: Disk IO Error when extracting file.
+     *
+     * Verify that:
+     * * Error is counted as failure
+     * * After multiple errors the content is marked as permanently failed
+     */
+    @Test
+    public void testDiskIOErrorIsCountedAsFailure() throws Exception {
+        DownloadContent content = createFont();
+        DownloadContentCatalog catalog = mockCatalogWithScheduledDownloads(content);
+        doCallRealMethod().when(catalog).rememberFailure(eq(content), anyInt());
+        doCallRealMethod().when(catalog).markAsPermanentlyFailed(content);
+
+        Assert.assertEquals(DownloadContent.STATE_NONE, content.getState());
+
+        DownloadAction action = spy(new DownloadAction(null));
+        doReturn(true).when(action).isConnectedToNetwork(RuntimeEnvironment.application);
+        doReturn(false).when(action).isActiveNetworkMetered(RuntimeEnvironment.application);
+        doReturn(mockNotExistingFile()).when(action).createTemporaryFile(RuntimeEnvironment.application, content);
+        doReturn(mockNotExistingFile()).when(action).getDestinationFile(RuntimeEnvironment.application, content);
+        doReturn(true).when(action).hasEnoughDiskSpace(eq(content), any(File.class), any(File.class));
+        doNothing().when(action).download(any(HttpClient.class), anyString(), any(File.class));
+        doReturn(true).when(action).verify(any(File.class), anyString());
+
+        File destinationFile = mock(File.class);
+        doReturn(false).when(destinationFile).exists();
+        File parentFile = mock(File.class);
+        doReturn(false).when(parentFile).mkdirs();
+        doReturn(false).when(parentFile).exists();
+        doReturn(parentFile).when(destinationFile).getParentFile();
+        doReturn(destinationFile).when(action).getDestinationFile(RuntimeEnvironment.application, content);
+
+        for (int i = 0; i < 10; i++) {
+            action.perform(RuntimeEnvironment.application, catalog);
+
+            Assert.assertEquals(DownloadContent.STATE_NONE, content.getState());
+        }
+
+        action.perform(RuntimeEnvironment.application, catalog);
+
+        Assert.assertEquals(DownloadContent.STATE_FAILED, content.getState());
+        verify(catalog, times(11)).rememberFailure(eq(content), anyInt());
+    }
+
+    private DownloadContent createFont() {
+        return createFontWithSize(102400L);
+    }
+
     private DownloadContent createFontWithSize(long size) {
         return new DownloadContent.Builder()
                 .setKind(DownloadContent.KIND_FONT)

mobile/android/tests/background/junit4/src/org/mozilla/gecko/dlc/catalog/TestDownloadContentCatalog.java

@@ -206,4 +206,56 @@ public class TestDownloadContentCatalog {
         Assert.assertTrue(catalog.getScheduledDownloads().contains(content4));
         Assert.assertTrue(catalog.getScheduledDownloads().contains(content5));
     }
+
+    /**
+     * Scenario: Calling rememberFailure() on a catalog with varying values
+     */
+    @Test
+    public void testRememberingFailures() {
+        DownloadContentCatalog catalog = new DownloadContentCatalog(mock(AtomicFile.class));
+        Assert.assertFalse(catalog.hasCatalogChanged());
+
+        DownloadContent content = new DownloadContent.Builder().build();
+        Assert.assertEquals(0, content.getFailures());
+
+        catalog.rememberFailure(content, 42);
+        Assert.assertEquals(1, content.getFailures());
+        Assert.assertTrue(catalog.hasCatalogChanged());
+
+        catalog.rememberFailure(content, 42);
+        Assert.assertEquals(2, content.getFailures());
+
+        // Failure counter is reset if different failure has been reported
+        catalog.rememberFailure(content, 23);
+        Assert.assertEquals(1, content.getFailures());
+
+        // Failure counter is reset after successful download
+        catalog.markAsDownloaded(content);
+        Assert.assertEquals(0, content.getFailures());
+    }
+
+    /**
+     * Scenario: Content has failed multiple times with the same failure type.
+     *
+     * Verify that:
+     *  * Content is marked as permanently failed
+     */
+    @Test
+    public void testContentWillBeMarkedAsPermanentlyFailedAfterMultipleFailures() {
+        DownloadContentCatalog catalog = new DownloadContentCatalog(mock(AtomicFile.class));
+
+        DownloadContent content = new DownloadContent.Builder().build();
+        Assert.assertEquals(DownloadContent.STATE_NONE, content.getState());
+
+        for (int i = 0; i < 10; i++) {
+            catalog.rememberFailure(content, 42);
+
+            Assert.assertEquals(i + 1, content.getFailures());
+            Assert.assertEquals(DownloadContent.STATE_NONE, content.getState());
+        }
+
+        catalog.rememberFailure(content, 42);
+        Assert.assertEquals(10, content.getFailures());
+        Assert.assertEquals(DownloadContent.STATE_FAILED, content.getState());
+    }
 }