916033 - [B2G][Helix][OTA][ChenHoulai]After the update file is downloaded, reboot the phone, the update can not execute immediately.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P2)

Description

[lecky]

User Agent: Mozilla/5.0 (compatible; MSIE 9.0; Windows NT 6.1; Trident/5.0; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0; .NET4.0C; .NET4.0E; aff-kingsoft-ciba)

Steps to reproduce:

【Detail Description*】：
    After the update file is downloaded, reboot the phone, the update can not execute immediately.
【Repro Steps*】：
   1、Enter into the settings, click 'check now' to get update info.
    2、Download the update file, when prompt install now or not, click 'later'.
    3、Reboot the phone.
    4、Enter into the settings, click 'check now' to get update info.
【Expect Result*】：
   4、The update file can be install normally, no need to download again.
【Real Result*】：
   4、The update file is needed to download again.
【Test Count*】：5
【Found Count*】：5
【Gaia commit ID*】:c0ea0a4943dc8d3751b07f5b5c5d3abe06364a14  
【Gecko commit ID*】: 170f9e477571127cd40997fa2abe262ed43f0e4d  
【Log*】：NA
【Network environment】：
【Resume operation】：
【Carrier】：

Comment 1

[Wayne Chang [:wchang]]

Is this an application update or system update?

Comment 2

[lecky]

It is on system update.

Comment 3

[Wayne Chang [:wchang]]

Hi Kaizhen,

Can you check if this is the expected behaviour at the moment or if this is a bug?

Comment 4

[Kai-Zhen Li [:seinlin][:kli]]

Hi Dave,

I can reproduce the same result on inari with gecko 18. So I think the root cause will be the same.

After the device is rebooted this dir, "/data/local/updates/0/", will become empty. So update service will try to download update.mar again, even when "/mnt/sdcard/updates/0/update.mar" exists and it is the same as the one on server.

Do you have some suggestion?

Comment 5

[Dave Hylands [:dhylands]]

Can you provide a logcat of the update (so actually 2 logcats, one that covers the actual download/application of the update, and one of after the phone reboots.

Comment 6

[Dave Hylands [:dhylands]]

And is this a regular OTA or FOTA update?

Comment 7

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: Bug 916033 - download update file.log

Yes, it is a regular FOTA update. Attached file is logcat of 3 update actions. It is clearly commented.

Comment 8

[Kai-Zhen Li [:seinlin][:kli]]

I think this dir "/data/local/updates/0/" becomes empty is the cause, but don't know why it will become empty after the device is reboot.

Comment 9

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

You can add log to find out when will we invoke cleanUpUpdatesDir() to delete the status file.

Comment 10

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: updateService.diff

Hi, Dave, 

If I add an update status "installed" and do something as the attached diff file, this issue can be solved.

Do you think add one more update status does make sense?

Thanks!

Comment 11

[Dave Hylands [:dhylands]]

So I was able to reproduce the problem on my unagi.

The basic flow goes something like this:

1 - We download the update
2 - We unpack the .mar file and create the .zip file. The update.status becomes applied
3 - The user chooses Later, which leaves the update in the "applied" state
4 - The phone reboots
5 - The update startup code detects that an applied FOTA update exists, so it calls librecovery:getFotaUpdateStatus which says that FOTA update was applied successfully.
6 - Since the FOTA was applied successfully, it goes ahead and calls cleanupActiveUpdate, which removes the files in /data/local/updates/0

So the real problem is that step 5 is reporting stale information (it's reporting that *some* previous update was successul, but since we didn't actually try to initiate the recovery yet, it doesn't mean anything).

Comment 12

[Dave Hylands [:dhylands]]

Comment on attachment 811071 [details] [diff] [review]
updateService.diff

Review of attachment 811071 [details] [diff] [review]:
-----------------------------------------------------------------

Currently, the checking of a FOTA update being applied is in nsUpdateService.js and the initiation of a FOTA update is in nsUpdatePrompt.js.

I think it makes sense to put both of these pieces of functionality into nsUpdateService.js, and my comments are based on this.

So I'm going to propose that we add the function

  bool applyOsUpdate(in nsIUpdate update);

to nsIApplicationUpdateService. This function can contain most of the functionality from UpdatePrompt.js finishUpdate and all of finishOSUpdate functions. It would also call writeStatusFile to update the status to be os-applied just before calling the recovery service. If for some reason, the recovery service fails, then we should restore the status to applied.

::: b2g/components/UpdatePrompt.js
@@ +374,5 @@
>    },
>  
>    finishOSUpdate: function UP_finishOSUpdate(aOsUpdatePath) {
> +    this._update.state = "installed";
> +    Services.um.setUpdateStatus(this._update);

Once this gets moved into nsUpdateService.js, then we can call writeStatusFile directly.

Note that if the recovery fails for any reason, we'll also need to set the status back to applied.

::: toolkit/mozapps/update/nsIUpdateService.idl
@@ +229,4 @@
>     *   "succeeded"          The update was successfully applied.
>     *   "download-failed"    The update failed to be downloaded.
>     *   "failed"             The update failed to be applied.
> +   *   "installed"          The update is selected to be installed.

Change to os-applied since this status is only for os updates (the code uses isOSUpdate)

@@ +494,5 @@
> +
> +  /**
> +   * Update the update.status based on the update.state.
> +   */
> +  void setUpdateStatus(in nsIUpdate update);

Aside: Any non comment changes to an idl file require that the uuid be updated as well.

Lets not do this and instead use the new applyOSUpdate function.

::: toolkit/mozapps/update/nsUpdateService.js
@@ +128,4 @@
>  const STATE_SUCCEEDED       = "succeeded";
>  const STATE_DOWNLOAD_FAILED = "download-failed";
>  const STATE_FAILED          = "failed";
> +const STATE_INSTALLED       = "installed";

Lets call this os-applied instead of installed (and use STATE_OS_APPLIED)

@@ +1800,2 @@
>      if (status == STATE_APPLIED && update && update.isOSUpdate) {
> +      LOG("UpdateService:_postUpdateProcessing - applied update not installed");

I'd add a comment that this situation occurs when the user downloads a FOTA update, chooses to install Later, and then the phone is rebooted for some reason other than entering recovery mode to apply the update.

@@ +1800,4 @@
>      if (status == STATE_APPLIED && update && update.isOSUpdate) {
> +      LOG("UpdateService:_postUpdateProcessing - applied update not installed");
> +      return;
> +    }

This check should be inside the #ifdef MOZ_WIDGET_GONK below.

@@ +2917,4 @@
>      }
>    },
>  
> +  setUpdateStatus: function UM_setUpdateStatus(update) { 

nit: trailing space.

I think that function should be removed. Once we move the applyOSUpdate function, it can just call writeStatusFile directly.

Comment 13

[Kai-Zhen Li [:seinlin][:kli]]

Thansk Dave, I will submit a formal patch as you suggested and request you to review then.

Comment 14

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: bug-916033-fix.patch

-Add a new status 'os-applied' for nsIUpdateService.
-Add applyOsUpdate in nsIApplicationUpdateService and it is used to replace finishOSUpdate in b2g/components/UpdatePrompt.js.

Comment 15

[Kai-Zhen Li [:seinlin][:kli]]

Try server test result:
https://tbpl.mozilla.org/?tree=Try&rev=53b630bdda42

Comment 16

[Dave Hylands [:dhylands]]

Comment on attachment 812551 [details] [diff] [review]
bug-916033-fix.patch

Review of attachment 812551 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry to take so long on this (I was away on PTO for a couple of weeks).

This looks good to me. I'm not a module peer, so I technically can't give this an r+.

I've reached out to some appropriate individuals, so hopefully we can get this reviewed.

Comment 17

[Dave Hylands [:dhylands]]

Comment on attachment 811071 [details] [diff] [review]
updateService.diff

This looks like it should have been obsoleted, so I'm going to go ahead and mark it as such.

Comment 18

[Kai-Zhen Li [:seinlin][:kli]]

(In reply to Dave Hylands [:dhylands] from comment #17)
> Comment on attachment 811071 [details] [diff] [review]
> updateService.diff
> 
> This looks like it should have been obsoleted, so I'm going to go ahead and
> mark it as such.

Thanks Dave, 
Yes, it should be obsoleted as it is WIP for feedback only. 
Can you help to loop the appropriate individuals and get the patch reviewed?

Comment 19

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

Comment on attachment 812551 [details] [diff] [review]
bug-916033-fix.patch

I'll review this on 10/16

Comment 20

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

Comment on attachment 812551 [details] [diff] [review]
bug-916033-fix.patch

Looks fine overall and the next review after the comments are addressed or reasons why they shouldn't be addressed will be quick. Thanks!

>diff --git a/toolkit/mozapps/update/nsIUpdateService.idl b/toolkit/mozapps/update/nsIUpdateService.idl
>--- a/toolkit/mozapps/update/nsIUpdateService.idl
>+++ b/toolkit/mozapps/update/nsIUpdateService.idl
>...
>@@ -395,16 +396,21 @@ interface nsIApplicationUpdateService : 
>   void removeDownloadListener(in nsIRequestObserver listener);
> 
>   /**
>    *
>    */
>   AString downloadUpdate(in nsIUpdate update, in boolean background);
> 
>   /**
>+   * Finish the OS update when installed is selected.
I'm having trouble parsing this sentence. Can you be more specific?

>+   */
>+  void applyOsUpdate(in nsIUpdate update);
Please add the param to the comment.

>+
>+  /**
>    * Get the Active Updates directory
>    * @returns An nsIFile for the active updates directory.
>    */
>   nsIFile getUpdatesDirectory();
> 
>   /**
>    * Pauses the active update download process
>    */
>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
>@@ -117,16 +117,17 @@ const STATE_DOWNLOADING     = "downloadi
> const STATE_PENDING         = "pending";
> const STATE_PENDING_SVC     = "pending-service";
> const STATE_APPLYING        = "applying";
> const STATE_APPLIED         = "applied";
> const STATE_APPLIED_SVC     = "applied-service";
> const STATE_SUCCEEDED       = "succeeded";
> const STATE_DOWNLOAD_FAILED = "download-failed";
> const STATE_FAILED          = "failed";
>+const STATE_OS_APPLIED      = "os-applied";
I'd prefer going with applied-os here and elsewhere for consistency with the other status codes (e.g. applied and applied-service) and please place this between applied and applied-service.

>@@ -3134,16 +3141,57 @@ UpdateService.prototype = {
> 
>   /**
>    * See nsIUpdateService.idl
>    */
>   get isDownloading() {
>     return this._downloader && this._downloader.isBusy;
>   },
> 
>+  /**
>+   * See nsIUpdateService.idl
>+   */
>+  applyOsUpdate: function UM_applyOsUpdate(update) {
s/update/aUpdate/g

>+    if (!update.isOSUpdate) {
>+      return;
I'm tempted to have this throw NS_ERROR_FAILURE... any reason not to? If not, please add it and include in the javadoc comment that it throws.

>+    }

I think it would be a good thing to also check the current state before trying to apply.

>+
>+    let osApplyToDir;
>+    try {
>+      update.QueryInterface(Ci.nsIWritablePropertyBag);
>+      osApplyToDir = update.getProperty("osApplyToDir");
>+    } catch (e) {}
>+
>+    if (!osApplyToDir) {
>+      handleUpdateFailure(update, FOTA_UNKNOWN_ERROR);
>+      return;
>+    }
>+
>+    let updateFile = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile);
>+    updateFile.initWithPath(osApplyToDir + "/update.zip");
>+    if (!updateFile.exists()) {
>+      LOG("Error: FOTA update not found at " + updateFile.path);
>+      handleUpdateFailure(update, FOTA_UNKNOWN_ERROR);
>+      return;
>+    }
>+
>+    writeStatusFile(getUpdatesDir(), update.state = STATE_OS_APPLIED);
>+    LOG("Rebooting into recovery to apply FOTA update: " + updateFile.path);
>+    try {
>+      let recoveryService = Cc["@mozilla.org/recovery-service;1"]
>+                            .getService(Ci.nsIRecoveryService);
>+      recoveryService.installFotaUpdate(updateFile.path);
>+    } catch(e) {
>+      LOG("Error: Couldn't reboot into recovery to apply FOTA update " +
>+          updateFile.path);
>+      writeStatusFile(getUpdatesDir(), update.state = STATE_APPLIED);
>+      handleUpdateFailure(update, FOTA_UNKNOWN_ERROR);
>+    }
>+  },
I think it would be a good thing to have specific errors for these cases instead of overloading FOTA_UNKNOWN_ERROR

>diff --git a/toolkit/mozapps/update/test/sharedUpdateXML.js b/toolkit/mozapps/update/test/sharedUpdateXML.js
>--- a/toolkit/mozapps/update/test/sharedUpdateXML.js
>+++ b/toolkit/mozapps/update/test/sharedUpdateXML.js
>@@ -44,16 +44,17 @@ const STATE_DOWNLOADING     = "downloadi
> const STATE_PENDING         = "pending";
> const STATE_PENDING_SVC     = "pending-service";
> const STATE_APPLYING        = "applying";
> const STATE_APPLIED         = "applied";
> const STATE_APPLIED_SVC     = "applied-service";
> const STATE_SUCCEEDED       = "succeeded";
> const STATE_DOWNLOAD_FAILED = "download-failed";
> const STATE_FAILED          = "failed";
>+const STATE_OS_APPLIED      = "os-applied";
Only add this if you have a test for it.

Comment 21

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: bug-916033-fix-2.patch

Robert, could you kindly help me have a look to this patch if throws all error during applyOsUpdate is a good practice? Thanks!

Comment 22

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

Comment on attachment 819686 [details] [diff] [review]
bug-916033-fix-2.patch

(In reply to Kai-Zhen Li from comment #21)
> Created attachment 819686 [details] [diff] [review]
> bug-916033-fix-2.patch
> 
> Robert, could you kindly help me have a look to this patch if throws all
> error during applyOsUpdate is a good practice? Thanks!
My comment regarding throwing was for the case where it silently returned and I was fine with handling the other cases by calling handleUpdateFailure. I was also concerned with FOTA_UNKNOWN_ERROR being overloaded for all of the failure cases and suggested splitting them out along with making it so the failure reason could be identified.

While I'm here:
>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
>@@ -117,16 +117,17 @@ const STATE_DOWNLOADING     = "downloadi
> const STATE_PENDING         = "pending";
> const STATE_PENDING_SVC     = "pending-service";
> const STATE_APPLYING        = "applying";
> const STATE_APPLIED         = "applied";
> const STATE_APPLIED_SVC     = "applied-service";
> const STATE_SUCCEEDED       = "succeeded";
> const STATE_DOWNLOAD_FAILED = "download-failed";
> const STATE_FAILED          = "failed";
>+const STATE_APPLIED_OS      = "applied-os";
nit: per previous comment "please place this between applied and applied-service."

>@@ -3134,16 +3141,59 @@ UpdateService.prototype = {
> 
>   /**
>    * See nsIUpdateService.idl
>    */
>   get isDownloading() {
>     return this._downloader && this._downloader.isBusy;
>   },
> 
>+  /**
>+   * See nsIUpdateService.idl
>+   */
>+  applyOsUpdate: function UM_applyOsUpdate(aUpdate) {
nit: s/UM_applyOsUpdate/AUS_applyOsUpdate/

AUS is shorthand for ApplicationUpdateService while UM is shorthand for UpdateManager

Comment 23

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: bug-916033-fix-3.patch

Comment 24

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

Comment on attachment 823936 [details] [diff] [review]
bug-916033-fix-3.patch

># HG changeset patch
># User Kai-Zhen Li <[e-mail]>
># Date 1382954805 -28800
># Node ID 52d561e3941f0139fb1a0bfad1e58be786e55060
># Parent  3e9059077e07982f51187b37c9046fdc344c74b3
>Bug 916033: Add a new status 'applied-os' for nsIUpdateService. r=rstrong

>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
>...
>@@ -3134,16 +3145,59 @@ UpdateService.prototype = {
> 
>   /**
>    * See nsIUpdateService.idl
>    */
>   get isDownloading() {
>     return this._downloader && this._downloader.isBusy;
>   },
> 
>+  /**
>+   * See nsIUpdateService.idl
>+   */
>+  applyOsUpdate: function AUS_applyOsUpdate(aUpdate) {
>+    if (!aUpdate.isOSUpdate || aUpdate.state != STATE_APPLIED) {
>+      aUpdate.statusText = "fota-state-error";
>+      throw Cr.NS_ERROR_FAILURE;
>+    }
>+
>+    let osApplyToDir;
>+    try {
>+      aUpdate.QueryInterface(Ci.nsIWritablePropertyBag);
>+      osApplyToDir = aUpdate.getProperty("osApplyToDir");
>+    } catch (e) {}
>+
>+    if (!osApplyToDir) {
Add a LOG statement
LOG("UpdateService:applyOsUpdate - Error: osApplyToDir is not defined in the nsIUpdate!");

>+      handleUpdateFailure(aUpdate, FOTA_FILE_OPERATION_ERROR);
>+      return;
>+    }
>+
>+    let updateFile = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile);
>+    updateFile.initWithPath(osApplyToDir + "/update.zip");
>+    if (!updateFile.exists()) {
>+      LOG("Error: OS update is not found at " + updateFile.path);
nit: prefix the logged text with "UpdateService:applyOsUpdate - "

>+      handleUpdateFailure(aUpdate, FOTA_FILE_OPERATION_ERROR);
>+      return;
>+    }
>+
>+    writeStatusFile(getUpdatesDir(), aUpdate.state = STATE_APPLIED_OS);
>+    LOG("Rebooting into recovery to apply FOTA update: " + updateFile.path);
>+    try {
>+      let recoveryService = Cc["@mozilla.org/recovery-service;1"]
>+                            .getService(Ci.nsIRecoveryService);
>+      recoveryService.installFotaUpdate(updateFile.path);
>+    } catch (e) {
>+      LOG("Error: Couldn't reboot into recovery to apply FOTA update " +
>+          updateFile.path);
nit: prefix the logged text with "UpdateService:applyOsUpdate - "

>+      writeStatusFile(getUpdatesDir(), aUpdate.state = STATE_APPLIED);
>+      handleUpdateFailure(aUpdate, FOTA_RECOVERY_ERROR);
>+      return;
No need for the return

>+    }
>+  },

Is it possible to create a test for this?

Comment 25

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: bug-916033-fix-4.patch

Comment 26

[Kai-Zhen Li [:seinlin][:kli]]

Robert, Thanks! I'll study how to create a test case for it and file a new bug then.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/b0554fc657e5

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b0554fc657e5