589906 - Fennec hangs at startup with black screen when out of disk space

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Matt Brubeck (:mbrubeck)]

When Fennec runs out of internal storage during launch, it hangs at a black screen until the user presses the "home" button and restarts the phone or kills the process.  (This happens fairly often on Android phones, which often have as little as 256 MB of storage available for apps.)

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

this should be helped significantly by bug 588607

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

We've talked about trying to move our profile to the sd card if we need to. In the very least we should show an error dialog rather than hanging.

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

this is a patch to show an error message if we run out of space while unpacking our files (rather than hang).

I'll need polished strings for:
no space left on device
generic i/o error
can't find components.manifest
can't find file x in the zip file

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch (fixed some ws)

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

sorry, missing the components.manifest isn't fatal, so that shouldn't throw an exception and we don't need a polished string for it. Still need the other 3 though.

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 476726 [details] [diff] [review]
patch

Going on the assumption that the only one of these errors we expect users to see is the out of disk space error (I/O errors and being unable to find a file in the ZIP feel like far less common errors), that string is the only one that needs to be really polished.
>+            if (msg.equalsIgnoreCase("No space left on device"))
>+                showErrorDialog("There is not enough space available on the device for " + getAppName() + " to launch");

showErrorDialog("There is not enough space available for " + getAppName() + " to start.");

Do we want to recommend that they move Fennec to the SD card? Can we just actually present a dialog that offers that as an option? Froyo only, natch.

>+            else
>+                showErrorDialog(getAppName() + " encountered an unexpected I/O error and is unable to launch");

showErrorDialog("An error occurred when trying to load files required to run " + getAppName());

>+        if (fileEntry == null)
>+            throw new GeckoException("Can't find " + name + " in " + zip.getName());

This is fine.

uir+ with those changes.

Comment 7

[Michael Wu [:mwu]]

Comment on attachment 476726 [details] [diff] [review]
patch

Looks like a good simplification of the error paths.

>diff --git a/embedding/android/GeckoApp.java b/embedding/android/GeckoApp.java
>--- a/embedding/android/GeckoApp.java
>+++ b/embedding/android/GeckoApp.java
>@@ -65,11 +65,38 @@ abstract public class GeckoApp
>     public static GeckoSurfaceView surfaceView;
>     public static GeckoApp mAppContext;
>     ProgressDialog mProgressDialog;
Add a space here.

>+    void showErrorDialog(String message) {
Curly brace on the next line when starting a function/method.

>+        new AlertDialog.Builder(this)
>+            .setMessage(message)
>+            .setCancelable(false)
>+            .setPositiveButton("Exit",
>+                               new DialogInterface.OnClickListener() {
>+                                   public void onClick(DialogInterface dialog,
>+                                                       int id)
>+                                   {
>+                                       GeckoApp.this.finish();
>+                                   }
>+                               }).show();
>+    }
> 
>     void launch()
>     {
>         // unpack files in the components directory
>-        unpackComponents();
>+        try {
>+            unpackComponents();
>+        } catch (GeckoException ge) {
>+            // Gecko Exceptions should already have a message we can
>+            // show to the user
>+            showErrorDialog(ge.getMessage());
>+            return;
>+        } catch (IOException ie) {
>+            String msg = ie.getMessage();
>+            if (msg.equalsIgnoreCase("No space left on device"))
>+                showErrorDialog("There is not enough space available on the device for " + getAppName() + " to launch");
>+            else
>+                showErrorDialog(getAppName() + " encountered an unexpected I/O error and is unable to launch");
>+            return;
>+        }
How do we end up quitting?

>@@ -289,50 +316,44 @@ abstract public class GeckoApp
>     abstract public String getAppName();
>     abstract public String getContentProcessName();
> 
>-    protected void unpackComponents()
>+    class GeckoException extends Exception {
>+        GeckoException(String message) {
>+            super(message);
>+        }
>+    }
>+
>+    
>+    protected void unpackComponents() throws IOException, GeckoException
throws goes on its own line.

>     {
>         ZipFile zip;
>         InputStream listStream;
> 
>-        try {
>-            File componentsDir = new File("/data/data/org.mozilla." + getAppName() +"/components");
>-            componentsDir.mkdir();
>-            zip = new ZipFile(getApplication().getPackageResourcePath());
>-        } catch (Exception e) {
>-            Log.i("GeckoAppJava", e.toString());
>-            return;
>-        }
>+        File componentsDir = new File("/data/data/org.mozilla." + getAppName() +"/components");
Space after the last +

>@@ -353,14 +374,12 @@ abstract public class GeckoApp
>         } while (status != StreamTokenizer.TT_EOF);
>     }
> 
>-    private void unpackFile(ZipFile zip, byte[] buf, ZipEntry fileEntry, String name)
>+    private void unpackFile(ZipFile zip, byte[] buf, ZipEntry fileEntry, String name) throws IOException, GeckoException
throw on its own line.

>     {
>         if (fileEntry == null)
>             fileEntry = zip.getEntry(name);
>-        if (fileEntry == null) {
>-            Log.i("GeckoAppJava", "Can't find " + name + " in " + zip.getName());
>-            return;
>-        }
>+        if (fileEntry == null)
>+            throw new GeckoException("Can't find " + name + " in " + zip.getName());
throw new FileNotFoundException. Since this is the only instance where GeckoException is used, you can get rid of GeckoException.

>+        File dir = outFile.getParentFile();
>+        if (!outFile.exists())
>+            dir.mkdirs();
>+
>+        InputStream fileStream;
>+        fileStream = zip.getInputStream(fileEntry);
>+
>+        OutputStream outStream;
>+        outStream = new FileOutputStream(outFile);
Combine the definition and assignment while you're at it.

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch v.2

carrying beltzner's uir+

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #7)

> How do we end up quitting?

this does the trick:
> >+                                   public void onClick(DialogInterface dialog,
> >+                                                       int id)
> >+                                   {
> >+                                       GeckoApp.this.finish();

Comment 10

[Michael Wu [:mwu]]

Comment on attachment 478192 [details] [diff] [review]
patch v.2

The code in showErrorDialog is kinda ugly but I can't think of any way to improve it much. Yay Java.

Not sure if the comment about gecko exception makes sense as is.

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch v2.1

I updated this patch to use the new string resources. carrying r+ from mwu and ui-r+ from beltzner.

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/71ab2e2fa1cc

Comment 13

[Catalin Suciu [:csuciu]]

This issue is verified as fixed on Verified on Mozilla/5.0 (Android;Linux armv7l;rv:9.0a1)Gecko/20110919 Firefox/9.0a1 Fennec/9.0a1