Bug 1236507 - DownloadContentService: Skip download if file system is not writable. r?rnewman draft
authorSebastian Kaspari <s.kaspari@gmail.com>
Tue, 22 Dec 2015 10:28:10 +0100
changeset 318623 0a1c7e38a87f76ed27065ac83e69eaecccff57d5
parent 318622 55dc093df961d8e7962d3c6689a7666a924660e6
child 318624 b5bcf1febb5232fa89d77b2eb5af97cb9f4cb442
push id8896
push users.kaspari@gmail.com
push dateMon, 04 Jan 2016 13:57:41 +0000
reviewersrnewman
bugs1236507
milestone46.0a1
Bug 1236507 - DownloadContentService: Skip download if file system is not writable. r?rnewman
mobile/android/base/java/org/mozilla/gecko/dlc/DownloadAction.java
mobile/android/tests/background/junit4/src/org/mozilla/gecko/dlc/TestDownloadAction.java
--- a/mobile/android/base/java/org/mozilla/gecko/dlc/DownloadAction.java
+++ b/mobile/android/base/java/org/mozilla/gecko/dlc/DownloadAction.java
@@ -82,16 +82,21 @@ public class DownloadAction extends Base
                 if (destinationFile.exists() && verify(destinationFile, content.getChecksum())) {
                     Log.d(LOGTAG, "Content already exists and is up-to-date.");
                     catalog.markAsDownloaded(content);
                     continue;
                 }
 
                 temporaryFile = createTemporaryFile(context, content);
 
+                if (!canWrite(temporaryFile, destinationFile)) {
+                    throw new RecoverableDownloadContentException(RecoverableDownloadContentException.DISK_IO,
+                                                                  "Temporary or destination file not writeable");
+                }
+
                 if (!hasEnoughDiskSpace(content, destinationFile, temporaryFile)) {
                     Log.d(LOGTAG, "Not enough disk space to save content. Skipping download.");
                     continue;
                 }
 
                 // TODO: Check space on disk before downloading content (bug 1220145)
                 final String url = createDownloadURL(content);
 
@@ -120,17 +125,17 @@ public class DownloadAction extends Base
                 if (callback != null) {
                     callback.onContentDownloaded(content);
                 }
 
                 if (temporaryFile != null && temporaryFile.exists()) {
                     temporaryFile.delete();
                 }
             } catch (RecoverableDownloadContentException e) {
-                Log.w(LOGTAG, "Downloading content failed (Recoverable): " + content , 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);
@@ -331,9 +336,19 @@ public class DownloadAction extends Base
         final File destinationDirectory = destinationFile.getParentFile();
         // We need some more space to extract the file (getSize() returns the uncompressed size)
         if (destinationDirectory.getUsableSpace() < content.getSize() * 2) {
             return false;
         }
 
         return true;
     }
+
+    protected boolean canWrite(File... files) {
+        for (File file : files) {
+            if (!file.canWrite()) {
+                return false;
+            }
+        }
+
+        return true;
+    }
 }
--- a/mobile/android/tests/background/junit4/src/org/mozilla/gecko/dlc/TestDownloadAction.java
+++ b/mobile/android/tests/background/junit4/src/org/mozilla/gecko/dlc/TestDownloadAction.java
@@ -163,16 +163,17 @@ public class TestDownloadAction {
 
         DownloadAction action = spy(new DownloadAction(null));
         doReturn(false).when(action).isActiveNetworkMetered(RuntimeEnvironment.application);
 
         File file = mockNotExistingFile();
         doReturn(file).when(action).createTemporaryFile(RuntimeEnvironment.application, content);
         doReturn(file).when(action).getDestinationFile(RuntimeEnvironment.application, content);
 
+        doReturn(true).when(action).canWrite(any(File.class), any(File.class));
         doReturn(false).when(action).verify(eq(file), anyString());
         doNothing().when(action).download(any(HttpClient.class), anyString(), eq(file));
         doReturn(true).when(action).verify(eq(file), anyString());
         doNothing().when(action).extract(eq(file), eq(file), anyString());
 
         action.perform(RuntimeEnvironment.application, catalog);
 
         verify(action).buildHttpClient();
@@ -197,16 +198,17 @@ public class TestDownloadAction {
                 .setSize(4223)
                 .build();
 
         DownloadContentCatalog catalog = mock(DownloadContentCatalog.class);
         doReturn(Collections.singletonList(content)).when(catalog).getScheduledDownloads();
 
         DownloadAction action = spy(new DownloadAction(null));
         doReturn(false).when(action).isActiveNetworkMetered(RuntimeEnvironment.application);
+        doReturn(true).when(action).canWrite(any(File.class), any(File.class));
 
         File temporaryFile = mockFileWithSize(1337L);
         doReturn(temporaryFile).when(action).createTemporaryFile(RuntimeEnvironment.application, content);
 
         ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
         doReturn(outputStream).when(action).openFile(eq(temporaryFile), anyBoolean());
 
         HttpClient client = mockHttpClient(HttpStatus.SC_PARTIAL_CONTENT, "HelloWorld");
@@ -296,16 +298,17 @@ public class TestDownloadAction {
         doReturn(false).when(action).isActiveNetworkMetered(RuntimeEnvironment.application);
 
         File temporaryFile = mockFileWithSize(1337L);
         doReturn(temporaryFile).when(action).createTemporaryFile(RuntimeEnvironment.application, content);
 
         File destinationFile = mockNotExistingFile();
         doReturn(destinationFile).when(action).getDestinationFile(RuntimeEnvironment.application, content);
 
+        doReturn(true).when(action).canWrite(any(File.class), any(File.class));
         doReturn(true).when(action).verify(eq(temporaryFile), anyString());
         doNothing().when(action).extract(eq(temporaryFile), eq(destinationFile), anyString());
 
         action.perform(RuntimeEnvironment.application, catalog);
 
         verify(action, never()).download(any(HttpClient.class), anyString(), eq(temporaryFile));
         verify(action).verify(eq(temporaryFile), anyString());
         verify(action).extract(eq(temporaryFile), eq(destinationFile), anyString());
@@ -328,16 +331,17 @@ public class TestDownloadAction {
                 .setSize(1337L)
                 .build();
 
         DownloadContentCatalog catalog = mock(DownloadContentCatalog.class);
         doReturn(Collections.singletonList(content)).when(catalog).getScheduledDownloads();
 
         DownloadAction action = spy(new DownloadAction(null));
         doReturn(false).when(action).isActiveNetworkMetered(RuntimeEnvironment.application);
+        doReturn(true).when(action).canWrite(any(File.class), any(File.class));
         doNothing().when(action).download(any(HttpClient.class), anyString(), any(File.class));
         doReturn(false).when(action).verify(any(File.class), anyString());
 
         File temporaryFile = mockNotExistingFile();
         doReturn(temporaryFile).when(action).createTemporaryFile(RuntimeEnvironment.application, content);
 
         File destinationFile = mockNotExistingFile();
         doReturn(destinationFile).when(action).getDestinationFile(RuntimeEnvironment.application, content);
@@ -438,16 +442,17 @@ public class TestDownloadAction {
         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));
+        doReturn(true).when(action).canWrite(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());
@@ -494,16 +499,42 @@ public class TestDownloadAction {
         }
 
         action.perform(RuntimeEnvironment.application, catalog);
 
         Assert.assertEquals(DownloadContent.STATE_FAILED, content.getState());
         verify(catalog, times(11)).rememberFailure(eq(content), anyInt());
     }
 
+    /**
+     * Scenario: Temporary or destination file is not writable.
+     *
+     * Verify that:
+     *  * No download is performed
+     *  * Error is counted as failure
+     */
+    @Test
+    public void testNoDownIsPerformedIfFilesAreNotWritable() 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(false).when(action).canWrite(any(File.class), any(File.class));
+
+        action.perform(RuntimeEnvironment.application, catalog);
+
+        verify(action).canWrite(any(File.class), any(File.class));
+        verify(action, never()).download(any(HttpClient.class), anyString(), any(File.class));
+        verify(catalog).rememberFailure(eq(content), anyInt());
+    }
+
     private DownloadContent createFont() {
         return createFontWithSize(102400L);
     }
 
     private DownloadContent createFontWithSize(long size) {
         return new DownloadContent.Builder()
                 .setKind(DownloadContent.KIND_FONT)
                 .setType(DownloadContent.TYPE_ASSET_ARCHIVE)