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

RESOLVED FIXED

Status

P2
blocker
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: lecky.wanglei, Assigned: seinlin)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

5 years ago
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】:
(Reporter)

Updated

5 years ago
Severity: normal → blocker
blocking-b2g: --- → hd?
Priority: -- → P2
(Reporter)

Updated

5 years ago
Flags: needinfo?(wchang)
Is this an application update or system update?
blocking-b2g: hd? → ---
Flags: needinfo?(wchang) → needinfo?(lecky.wanglei)
(Reporter)

Comment 2

5 years ago
It is on system update.
Flags: needinfo?(lecky.wanglei)
(Reporter)

Updated

5 years ago
blocking-b2g: --- → hd?
Flags: needinfo?(wchang)
Hi Kaizhen,

Can you check if this is the expected behaviour at the moment or if this is a bug?
Flags: needinfo?(wchang) → needinfo?(kli)
(Assignee)

Comment 4

5 years ago
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?
Flags: needinfo?(kli) → needinfo?(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.
Flags: needinfo?(dhylands)
And is this a regular OTA or FOTA update?
(Assignee)

Comment 7

5 years ago
Created attachment 808948 [details]
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.
(Assignee)

Comment 8

5 years ago
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.
Flags: needinfo?(schien)
You can add log to find out when will we invoke cleanUpUpdatesDir() to delete the status file.
Flags: needinfo?(schien)
(Assignee)

Comment 10

5 years ago
Created attachment 811071 [details] [diff] [review]
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!
Flags: needinfo?(dhylands)
Flags: needinfo?(dhylands) → needinfo?(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).
Flags: needinfo?(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.
(Assignee)

Comment 13

5 years ago
Thansk Dave, I will submit a formal patch as you suggested and request you to review then.
(Assignee)

Comment 14

5 years ago
Created attachment 812551 [details] [diff] [review]
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.
Attachment #812551 - Flags: review?(dhylands)
(Reporter)

Updated

5 years ago
blocking-b2g: hd? → ---
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.
Attachment #812551 - Flags: review?(dhylands) → feedback+
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.
Attachment #811071 - Attachment is obsolete: true
Assignee: nobody → kli
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 18

5 years ago
(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 on attachment 812551 [details] [diff] [review]
bug-916033-fix.patch

I'll review this on 10/16
Attachment #812551 - Flags: review?(robert.bugzilla)
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.
Attachment #812551 - Flags: review?(robert.bugzilla) → review-
(Assignee)

Comment 21

5 years ago
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!
Attachment #812551 - Attachment is obsolete: true
Attachment #819686 - Flags: feedback?(robert.bugzilla)
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
Attachment #819686 - Flags: feedback?(robert.bugzilla)
(Assignee)

Comment 23

5 years ago
Created attachment 823936 [details] [diff] [review]
bug-916033-fix-3.patch
Attachment #819686 - Attachment is obsolete: true
Attachment #823936 - Flags: review?(robert.bugzilla)
Comment on attachment 823936 [details] [diff] [review]
bug-916033-fix-3.patch

># HG changeset patch
># User Kai-Zhen Li <kli@mozilla.com>
># 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?
Attachment #823936 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Comment 25

5 years ago
Created attachment 825252 [details] [diff] [review]
bug-916033-fix-4.patch
Attachment #823936 - Attachment is obsolete: true
(Assignee)

Comment 26

5 years ago
Robert, Thanks! I'll study how to create a test case for it and file a new bug then.
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b0554fc657e5
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.