594897 - Check for an update and launch it if available on Fennec restart

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Alex Pakhotin (:alexp)]

When the update service downloads an update it restarts Fennec.
We have to check for a downloaded update APK and launch it.

Comment 1

[Alex Pakhotin (:alexp)]

Attachment: Fix

User may restart Fennec manually, so check for updates in GeckoApp.onCreate() on a normal start.

Comment 2

[Robert Strong (they/them - no direct email)]

Comment on attachment 473671 [details] [diff] [review]
Fix

>+            // Update the status file
>+            String status;
>+            if (succeeded)
>+                status = "succeeded\n";
>+            else
>+                status = "failed\n";

failed should be followed by ": x" where x is the error.
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#1211

If you need to add a new error for Android due to the existing ones not being adequate please do so.
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#117

Here is where they are copied from
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/readstrings/errors.h#42

Comment 3

[Michael Wu [:mwu]]

Comment on attachment 473671 [details] [diff] [review]
Fix

>@@ -439,9 +441,81 @@ abstract public class GeckoApp
>             Log.i("GeckoAppJava", e.toString());
>         }
>         System.exit(0);
>     }
> 
>     public void handleNotification(String action, String alertName, String alertCookie) {
>         GeckoAppShell.handleNotification(action, alertName, alertCookie);
>     }
>+
>+    private void checkAndLaunchUpdate() {
>+        Log.i("GeckoAppJava", "Checking for an update");
>+
>+        boolean succeeded = false;
>+
>+        String updateDir = "/data/data/org.mozilla." + getAppName() + "/updates/0/";
>+        File updateFile = new File(updateDir + "update.apk");
>+
>+        if (updateFile.exists()) {
if (!updateFile.exists()) return;

>+            Log.i("GeckoAppJava", "Update is available!");
>+
>+            // Launch APK
>+            File downloadDir = Environment.getExternalStoragePublicDirectory(Environment.DIRECTORY_DOWNLOADS);
>+            File updateFileToRun = new File(downloadDir, getAppName() + "-update.apk");
>+            try {
>+                if (moveFile(updateFile, updateFileToRun)) {
>+                    String amCmd = "/system/bin/am start -a android.intent.action.VIEW " +
>+                                   "-n com.android.packageinstaller/.PackageInstallerActivity -d file://" +
>+                                   updateFileToRun.getPath();
>+                    Log.i("GeckoAppJava", amCmd);
>+                    Runtime.getRuntime().exec(amCmd);
>+                    succeeded = true;
>+                } else {
>+                    Log.i("GeckoAppJava", "Cannot move the update file!");
>+                }
>+            } catch (Exception e) {
>+                Log.i("GeckoAppJava", e.toString());
>+            }
>+
>+            // Update the status file
>+            String status;
String status = stats ? "succeeded\n" : "failed\n";

>+            if (succeeded)
>+                status = "succeeded\n";
>+            else
>+                status = "failed\n";
>+
>+            File statusFile = new File(updateDir + "update.status");
>+            OutputStream outStream;
>+            try {
>+                byte[] buf = status.getBytes("UTF-8");
>+                outStream = new FileOutputStream(statusFile);
>+                outStream.write(buf, 0, buf.length);
>+                outStream.close();
>+            } catch (Exception e) {
>+                Log.i("GeckoAppJava", e.toString());
>+            }
>+        }
>+
>+        if (succeeded)
>+            System.exit(0);
>+    }
>+
>+    private static boolean moveFile(File fromFile, File toFile) {
Try using File.renameTo

>+        try {
>+            FileInputStream inStream = new FileInputStream(fromFile);
>+            FileOutputStream outStream = new FileOutputStream(toFile);
>+            byte[] buf = new byte[8192];
>+            int read = 0;
>+            while ((read = inStream.read(buf)) > 0) {
>+                outStream.write(buf, 0, read);
>+            }
>+            inStream.close();
>+            outStream.close();
>+
>+            fromFile.delete();
>+        } catch (Exception e) {
>+            Log.i("GeckoAppJava", e.toString());
>+            return false;
>+        }
>+        return true;
>+    }
> }

Comment 4

[Alex Pakhotin (:alexp)]

(In reply to comment #3)
> >+        if (updateFile.exists()) {
> if (!updateFile.exists()) return;

Right.
Originally I had some additional logic at the end of the function.

> String status = stats ? "succeeded\n" : "failed\n";

Hmm, now a comment for the failure code I'm adding will look ambiguous.

> >+    private static boolean moveFile(File fromFile, File toFile) {
> Try using File.renameTo

Unfortunately renameTo doesn't work across filesystems.

Comment 5

[Alex Pakhotin (:alexp)]

Attachment: Fix v2

Implemented review comments.

Comment 6

[Alex Pakhotin (:alexp)]

Attachment: Fix v2 (real)

Comment 7

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

(In reply to comment #2)
> Comment on attachment 473671 [details] [diff] [review]
> Fix
> 
> >+            // Update the status file
> >+            String status;
> >+            if (succeeded)
> >+                status = "succeeded\n";
> >+            else
> >+                status = "failed\n";
> 
> failed should be followed by ": x" where x is the error.
> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#1211
Rob, is this supposed to be the error text or the error code?

Comment 8

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

Comment on attachment 474862 [details] [diff] [review]
Fix v2 (real)


>+        if (updateFile.exists()) {
>+            Log.i("GeckoAppJava", "Update is available!");

as mwu suggested, return early here

>+            // Update the status file
>+            String status = statusCode == 0 ? "succeeded\n" : "failed: "+ statusCode + "\n";

I believe this is supposed to be an error string, not an error code. Rob, can you comment?

Comment 9

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

Comment on attachment 474862 [details] [diff] [review]
Fix v2 (real)


>+    private static boolean moveFile(File fromFile, File toFile) {
>+        try {
>+            FileInputStream inStream = new FileInputStream(fromFile);
>+            FileOutputStream outStream = new FileOutputStream(toFile);
>+            byte[] buf = new byte[8192];
>+            int read = 0;
>+            while ((read = inStream.read(buf)) > 0) {
>+                outStream.write(buf, 0, read);
>+            }
>+            inStream.close();
>+            outStream.close();
>+
>+            fromFile.delete();
>+        } catch (Exception e) {
>+            Log.i("GeckoAppJava", e.toString());
>+            return false;
>+        }
>+        return true;
>+    }
> }

This would be better if you created two FileChannel's and used transferFrom().

Additionally you could special case moving on the same file system and use renameTo(). Perhaps try that first and if it fails use the FileChannels.

Comment 10

[Alex Pakhotin (:alexp)]

(In reply to comment #8)
> >+            // Update the status file
> >+            String status = statusCode == 0 ? "succeeded\n" : "failed: "+ statusCode + "\n";
> 
> I believe this is supposed to be an error string, not an error code. Rob, can
> you comment?

It's obvious from here: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#1213:

        update.errorCode = parseInt(ary[1])

Comment 11

[Michael Wu [:mwu]]

It would be best to avoid moving files across filesystems altogether. The SD card isn't necessarily fast or even there.

Comment 12

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

(In reply to comment #10)
> (In reply to comment #8)
> > >+            // Update the status file
> > >+            String status = statusCode == 0 ? "succeeded\n" : "failed: "+ statusCode + "\n";
> > 
> > I believe this is supposed to be an error string, not an error code. Rob, can
> > you comment?
> 
> It's obvious from here:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#1213:
> 
>         update.errorCode = parseInt(ary[1])


or here http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/updater/updater.cpp#1393 (more obvious to the js-illiterate among us)

Comment 13

[Alex Pakhotin (:alexp)]

(In reply to comment #11)
> It would be best to avoid moving files across filesystems altogether. The SD
> card isn't necessarily fast or even there.

I guess we will figure this out later as a follow-up. The best way would be to make it possible for the installer to access the APK right from our updates dir.

Comment 14

[Alex Pakhotin (:alexp)]

Attachment: Fix v3

- Missed review comment.
- Use FileChannel.transferTo to move the file.

Comment 15

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

pushed http://hg.mozilla.org/mozilla-central/rev/c4e8f9514580

Comment 16

[Aaron Train [:aaronmt]]

Filed bug 597599 because if the updater fails, i.e, 'connection timed out' - there is no way to check for updates again

Comment 17

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Verified:

Mozilla/5.0 (Android; Linux armv71; rv2.0b7pre) Gecko/20101007 Firefox/4.0b7pre Fennec/4.0b2pre

(technically 20101006 build)

Comment 18

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

litmus test cases created:
https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=13647
https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=13648