Closed Bug 596662 Opened 11 years ago Closed 11 years ago

New update mechanism does not work on Android 2.1

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(fennec2.0b1+)

VERIFIED FIXED
Tracking Status
fennec 2.0b1+ ---

People

(Reporter: alexp, Assigned: alexp)

References

Details

Attachments

(1 file, 4 obsolete files)

checkAndLaunchUpdate() function uses API Level 8 available only in Android 2.2.
Attached patch Fix (obsolete) — Splinter Review
Avoid moving the update file to a download dir, just set read permissions instead. This allows running the APK directly from Fennec update dir.
Assignee: nobody → alexp
Status: NEW → ASSIGNED
Attachment #475560 - Flags: review?(blassey.bugs)
tracking-fennec: --- → ?
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.
(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?
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.
(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.
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.
Attached patch Fix v2 (obsolete) — Splinter Review
Set file permissions in the update service.
Attachment #475560 - Attachment is obsolete: true
Attachment #475611 - Flags: review?(robert.bugzilla)
Attachment #475560 - Flags: review?(blassey.bugs)
Attachment #475611 - Flags: review?(blassey.bugs)
Attachment #475611 - Flags: review?(blassey.bugs) → review+
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
Attachment #475611 - Flags: review?(robert.bugzilla) → review+
(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;
(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).
Attached patch Fix v3 (obsolete) — Splinter Review
Fixed review comments.
r=rs,blassey
Attachment #475611 - Attachment is obsolete: true
Attachment #475643 - Flags: review+
tracking-fennec: ? → 2.0b1+
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.
(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!
Attached patch Fix v3.1 (obsolete) — Splinter Review
The actual fix of the bug as described.
r=rs,blassey
Attachment #475643 - Attachment is obsolete: true
Attachment #476307 - Flags: review+
checkin-needed?
(In reply to comment #15)
> checkin-needed?

I'd say so.
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.
How one could tell the patch is approved for check in to start the actual check-in process?
This bug is blocking beta1, so you are automatically approved. a=blocking-fennec
OK then. Thanks for the clarification!
Updated to the latest code, ready to push.
r=rs,blassey, a=blocking-fennec
Attachment #476307 - Attachment is obsolete: true
Attachment #477328 - Flags: review+
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Comment on attachment 477328 [details] [diff] [review]
[checked-in] Fix v3.2

http://hg.mozilla.org/mozilla-central/rev/607938e4b608
Attachment #477328 - Attachment description: Fix v3.2 → [checked-in] Fix v3.2
Depends on: 662075
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.