596662 - New update mechanism does not work on Android 2.1

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Alex Pakhotin (:alexp)]

checkAndLaunchUpdate() function uses API Level 8 available only in Android 2.2.

Comment 1

[Alex Pakhotin (:alexp)]

Attachment: Fix

Avoid moving the update file to a download dir, just set read permissions instead. This allows running the APK directly from Fennec update dir.

Comment 2

[Michael Wu [:mwu]]

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


>+            Runtime.getRuntime().exec("chmod 644 " + file.getPath());
>+            return true;

That's pretty sketchy though we do tend to try to run a lot of commands. In this case, it would be better if the code that downloaded the apk also set the right permissions.

Comment 3

[Alex Pakhotin (:alexp)]

(In reply to comment #2)
> In this case, it would be better if the code that downloaded the apk also set
> the right permissions.

Yes, it might be better, but the current Android approach is very special, so I'd like to keep most of this specifics out of the common update service code.

Robert, what do you think?

Comment 4

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

What are the permissions on the file after it is downloaded? I would think that read access is already set and there must be something more going on here than what I can gather from the comments so far.

Comment 5

[Alex Pakhotin (:alexp)]

(In reply to comment #4)
> What are the permissions on the file after it is downloaded? I would think that
> read access is already set and there must be something more going on here than
> what I can gather from the comments so far.

The permissions of a downloaded file are 600, i.e. the file is not accessible by the system installer process, which installs the .APK files.

Comment 6

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

Makes sense. I'm fine with having app update change the permissions to 0644 after the download though I agree keeping the Android changes to app update to a minimum.

Comment 7

[Alex Pakhotin (:alexp)]

Attachment: Fix v2

Set file permissions in the update service.

Comment 8

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

Comment on attachment 475611 [details] [diff] [review]
Fix v2

>diff --git a/toolkit/mozapps/update/nsUpdateService.js b/toolkit/mozapps/update/nsUpdateService.js
>--- a/toolkit/mozapps/update/nsUpdateService.js
>+++ b/toolkit/mozapps/update/nsUpdateService.js
>@@ -2589,16 +2589,25 @@ Downloader.prototype = {
>         state = STATE_PENDING;
> 
>         // We only need to explicitly show the prompt if this is a backround
>         // download, since otherwise some kind of UI is already visible and
>         // that UI will notify.
>         if (this.background)
>           shouldShowPrompt = true;
> 
>+#ifdef ANDROID
>+        // Make the .APK file accessible by the system installer
>+        let file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsILocalFile);
>+        let patchFile = getUpdatesDir();
>+        patchFile.append(FILE_UPDATE_ARCHIVE);
>+        file.initWithPath(patchFile.path);
>+        file.permissions = 0644;
I think the following is cleaner
// Give read permissions to everyone so the .APK file is accessible by
// the system installer. This is Android only since users that don't
// have write access to the install directory could modify the update
// mar and then a user with write access could apply the modified update
// mar.let patchFile = getUpdatesDir().QueryInterface(Ci.nsILocalFile);
patchFile.append(FILE_UPDATE_ARCHIVE);
patchFile.permissions = FileUtils.PERMS_FILE;

r=me with that

Comment 9

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

(In reply to comment #8)
> Comment on attachment 475611 [details] [diff] [review]
> Fix v2
> 
> >diff --git a/toolkit/mozapps/update/nsUpdateService.js b/toolkit/mozapps/update/nsUpdateService.js
> >--- a/toolkit/mozapps/update/nsUpdateService.js
> >+++ b/toolkit/mozapps/update/nsUpdateService.js
> >@@ -2589,16 +2589,25 @@ Downloader.prototype = {
> >         state = STATE_PENDING;
> > 
> >         // We only need to explicitly show the prompt if this is a backround
> >         // download, since otherwise some kind of UI is already visible and
> >         // that UI will notify.
> >         if (this.background)
> >           shouldShowPrompt = true;
> > 
> >+#ifdef ANDROID
> >+        // Make the .APK file accessible by the system installer
> >+        let file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsILocalFile);
> >+        let patchFile = getUpdatesDir();
> >+        patchFile.append(FILE_UPDATE_ARCHIVE);
> >+        file.initWithPath(patchFile.path);
> >+        file.permissions = 0644;
> I think the following is cleaner
> // Give read permissions to everyone so the .APK file is accessible by
> // the system installer. This is Android only since users that don't
> // have write access to the install directory could modify the update
> // mar and then a user with write access could apply the modified update
> // mar.let patchFile = getUpdatesDir().QueryInterface(Ci.nsILocalFile);
> patchFile.append(FILE_UPDATE_ARCHIVE);
> patchFile.permissions = FileUtils.PERMS_FILE;
> 
> r=me with that
editor is acting up... it should be:
// Give read permissions to everyone so the .APK file is accessible by
// the system installer. This is Android only since users that don't
// have write access to the install directory could modify the update
// mar and then a user with write access could apply the modified update
// mar.
let patchFile = getUpdatesDir().QueryInterface(Ci.nsILocalFile);
patchFile.append(FILE_UPDATE_ARCHIVE);
patchFile.permissions = FileUtils.PERMS_FILE;

Comment 10

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

(In reply to comment #9)
> // Give read permissions to everyone so the .APK file is accessible by
> // the system installer. This is Android only since users that don't
> // have write access to the install directory could modify the update
> // mar and then a user with write access could apply the modified update
> // mar.
Actually, that is for the 0666 case. To prevent someone from removing the ifdef please just note this bug in the comment as follows in case I get run over by a bus

// Give read permissions to everyone so the .APK file is accessible by
// the system installer (bug 596662).

Comment 11

[Alex Pakhotin (:alexp)]

Attachment: Fix v3

Fixed review comments.
r=rs,blassey

Comment 12

[Michael Wu [:mwu]]

Comment on attachment 475643 [details] [diff] [review]
Fix v3


>@@ -458,27 +458,27 @@ abstract public class GeckoApp
> 
>         if (!updateFile.exists())
>             return;
> 
>         Log.i("GeckoAppJava", "Update is available!");
> 
>         // Launch APK
>         File downloadDir = Environment.getExternalStoragePublicDirectory(Environment.DIRECTORY_DOWNLOADS);

Did you mean to leave this line? Seems like getting rid of this line was a primary objective.

Comment 13

[Alex Pakhotin (:alexp)]

(In reply to comment #12)
> >         File downloadDir = Environment.getExternalStoragePublicDirectory(Environment.DIRECTORY_DOWNLOADS);
> 
> Did you mean to leave this line? Seems like getting rid of this line was a
> primary objective.

This is crazy! It's good to have someone with a fresh view.
With that switched focus the main goal was left out.
Thank you Michael!

Comment 14

[Alex Pakhotin (:alexp)]

Attachment: Fix v3.1

The actual fix of the bug as described.
r=rs,blassey

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

checkin-needed?

Comment 16

[Alex Pakhotin (:alexp)]

(In reply to comment #15)
> checkin-needed?

I'd say so.

Comment 17

[Michael Wu [:mwu]]

Please upload a patch with the appropriate r= and a= and set checkin-needed in the keywords. After that, adding this bug to https://wiki.mozilla.org/LandingQueue will get your patch in sooner by getting the attention of people currently checking in.

Comment 18

[Alex Pakhotin (:alexp)]

How one could tell the patch is approved for check in to start the actual check-in process?

Comment 19

[Michael Wu [:mwu]]

This bug is blocking beta1, so you are automatically approved. a=blocking-fennec

Comment 20

[Alex Pakhotin (:alexp)]

OK then. Thanks for the clarification!

Comment 21

[Alex Pakhotin (:alexp)]

Attachment: [checked-in] Fix v3.2

Updated to the latest code, ready to push.
r=rs,blassey, a=blocking-fennec

Comment 22

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 477328 [details] [diff] [review]
[checked-in] Fix v3.2

http://hg.mozilla.org/mozilla-central/rev/607938e4b608

Comment 23

[Catalin Suciu [:csuciu]]

This issue is not reproducible on Mozilla/5.0 (Android;Linux armv7l;rv:9.0a1)Gecko/20110913 Firefox/9.0a1 Fennec/9.0a1
Device: HTC Desire
OS: Android 2.2