Ensure B2G devices can apply FOTA updates

RESOLVED FIXED

Status

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: lsblakk, Assigned: marshall)

Tracking

({feature})

unspecified
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+)

Details

(Whiteboard: [B2GTest:Blocker][WebAPI:P1][LOE:M])

Attachments

(1 attachment, 3 obsolete attachments)

Once the updates are downloaded, we'll need to make sure the update is verified as good before being applied and that the applying of an update can reboot the phone (as needed).
bug 778084 comment 3 contains the background information that is needed to bootstrap the work on this.
blocking-basecamp: --- → ?
blocking-basecamp: ? → +
Brian, you've really won the FOTA lottery today :)  Feel free to re-assign or set to nobody if you're not a good owner for this.  Thanks.
Assignee: nobody → bsmith
Whiteboard: [B2GTest:Blocker] → [B2GTest:Blocker][WebAPI:P1]
Keywords: feature
Whiteboard: [B2GTest:Blocker][WebAPI:P1] → [B2GTest:Blocker][WebAPI:P1][LOE:L]
Whiteboard: [B2GTest:Blocker][WebAPI:P1][LOE:L] → [B2GTest:Blocker][WebAPI:P1][LOE:M]
OS: Mac OS X → Gonk
Hardware: x86 → ARM
Assignee: bsmith → marshall
Depends on: 794092
Posted patch Gonk FOTA updates - v1 (obsolete) — Splinter Review
This is the minimal code path to have AUS download and extract a Gonk FOTA update.mar, then apply it with the B2G UpdatePrompt using the new RecoveryService (see Bug 794092).
Attachment #665520 - Flags: review?(fabrice)
Attachment #665520 - Flags: review?(ehsan)
Comment on attachment 665520 [details] [diff] [review]
Gonk FOTA updates - v1

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

r=me with nits fixed. I *did not* review the nsUpdateDriver.(cpp|h) changes since I'm not familiar with this code at all.

::: toolkit/mozapps/update/nsIUpdateService.idl
@@ +197,5 @@
>     */
>    attribute boolean isSecurityUpdate;
>  
>    /**
> +   * Whether not the update being downloaded is an OS update. This is generally

Not sure if this is a full grammatically correct English sentence.

::: toolkit/mozapps/update/nsUpdateService.js
@@ +1659,5 @@
>  
> +  _getOSUpdateStatus: function AUS__getOSUpdateStatus() {
> +#ifdef MOZ_WIDGET_GONK
> +    var recoveryService = Cc["@mozilla.org/recovery-service;1"].
> +                          getService(Ci.nsIRecoveryService);

nit: align .getService() with ["...
Posted patch Gonk FOTA updates - v2 (obsolete) — Splinter Review
Rebased and removed the applied-os status which turned out to be unnecessary.
Attachment #665520 - Attachment is obsolete: true
Attachment #665520 - Flags: review?(fabrice)
Attachment #665520 - Flags: review?(ehsan)
Attachment #665641 - Flags: review?(fabrice)
Attachment #665641 - Flags: review?(ehsan)

Comment 6

7 years ago
Comment on attachment 665641 [details] [diff] [review]
Gonk FOTA updates - v2

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

I have a set up comments below.  I mostly don't understand yet why this patch is needed, and what it's trying to do at a high level, so some information on that will be highly appreciated.  Also, make sure that you ask rstrong for review as well when you get my r+.  :-)

::: b2g/components/UpdatePrompt.js
@@ +195,5 @@
> +      log("Error: Update has no osApplyToDir");
> +      return;
> +    }
> +
> +    this.finishOSUpdate(osApplyToDir + "/update.zip");

So you don't perform any validation checks on the apply-to dir here.  Is that acceptable?

@@ +225,5 @@
> +      recoveryService.installFotaUpdate(aOsUpdatePath);
> +    } catch(e) {
> +      log("Error: Couldn't reboot into recovery to apply FOTA update " +
> +          aOsUpdatePath);
> +    }

Where is the log flushed here?  Is it accessible after the update is applied?

::: toolkit/mozapps/update/nsUpdateService.js
@@ +1668,5 @@
> +    if (fotaStatus == Ci.nsIRecoveryService.FOTA_UPDATE_SUCCESS) {
> +      return STATE_SUCCEEDED;
> +    }
> +
> +    return STATE_FAILED + ":" + FOTA_GENERAL_ERROR;

The convention is: "failed: %d".

But I also don't see why any of this is necessary.  Why can't we get the status of the update as we normally do?

::: toolkit/mozapps/update/updater/updater.cpp
@@ +2097,1 @@
>        rv = CopyInstallDirToDestDir();

Why are you doing this?  This seems like it should break background OS updates.

@@ +3391,5 @@
>  int AddPreCompleteActions(ActionList *list)
>  {
> +  if (sIsOSUpdate) {
> +    return OK;
> +  }

I'm not entirely sure that the implications of doing this are, rstrong needs to weigh in here.

::: toolkit/xre/nsUpdateDriver.cpp
@@ +610,5 @@
> +#endif
> +  return rv;
> +}
> +
> +static nsresult

Nit: please make this void.

@@ +613,5 @@
> +
> +static nsresult
> +GetOSApplyToDir(nsACString& applyToDir)
> +{
> +#ifdef MOZ_WIDGET_GONK

Please don't define this function if MOZ_WIDGET_GONK is defined, and adjust the caller with that in mind.  It's best to make mistakes compile time errors if we can.

@@ +618,5 @@
> +  // TODO add logic to check available storage space,
> +  // and iterate through pref(s) to find alternative dirs if
> +  // necessary.
> +  applyToDir.Assign(getenv("EXTERNAL_STORAGE"));
> +  applyToDir.Append("/updates");

I'm assuming that the right thing is going to happen if the EXTERNAL_STORAGE environment variable is not defined...

@@ +1036,5 @@
> +nsUpdateProcessor::SetOSApplyToDir(nsIUpdate* aUpdate)
> +{
> +  nsresult rv;
> +  nsCOMPtr<nsIWritablePropertyBag> updateProperties;
> +  updateProperties = do_QueryInterface(aUpdate, &rv);

Nit: please initialize when declaring.

::: toolkit/xre/nsUpdateDriver.h
@@ +93,4 @@
>    };
>  
>  private:
> +  bool SetOSApplyToDir(nsIUpdate* aUpdate);

This should be a static function as far as I can tell.
Attachment #665641 - Flags: review?(ehsan) → review-
(In reply to Ehsan Akhgari [:ehsan] from comment #6)
> I have a set up comments below.  I mostly don't understand yet why this
> patch is needed, and what it's trying to do at a high level, so some
> information on that will be highly appreciated.

Thanks for the quick and in depth review! 

The high level purpose of this patch is to let B2G use the existing update service for downloading and extracting a FOTA (full-over-the-air / "os") update. Once the update is extracted, it is handed off to a Gonk-specific recovery partition that applies the update.

There are some more details about our plan here:
https://bugzilla.mozilla.org/show_bug.cgi?id=778084#c5

Comment 8

7 years ago
(In reply to comment #7)
> (In reply to Ehsan Akhgari [:ehsan] from comment #6)
> > I have a set up comments below.  I mostly don't understand yet why this
> > patch is needed, and what it's trying to do at a high level, so some
> > information on that will be highly appreciated.
> 
> Thanks for the quick and in depth review! 
> 
> The high level purpose of this patch is to let B2G use the existing update
> service for downloading and extracting a FOTA (full-over-the-air / "os")
> update. Once the update is extracted, it is handed off to a Gonk-specific
> recovery partition that applies the update.
> 
> There are some more details about our plan here:
> https://bugzilla.mozilla.org/show_bug.cgi?id=778084#c5

OK, but let's assume that you have not written this patch yet.  What is the minimal set of things that you need to teach the updater in order to be able to proceed with that plan?
Posted patch Gonk FOTA updates - v3 (obsolete) — Splinter Review
Moved gonk specific GetOSApplyToDir code into the B2G directory provider, and
addressed some review comments.
Attachment #665641 - Attachment is obsolete: true
Attachment #665641 - Flags: review?(fabrice)
Attachment #666037 - Flags: review?(netzen)
Attachment #666037 - Flags: review?(ehsan)
To clarify a bit, this patch only attempts to achieve the following:
1. Find and download an OS update MAR with AUS
2. Extract the update.zip from the MAR with the background updater
3. Invoke the recovery partition to apply the update

The rest of the plan laid out in b2g-fota-updates will be covered in other patches.


(In reply to Ehsan Akhgari [:ehsan] from comment #6)
> 
> @@ +225,5 @@
> > +      recoveryService.installFotaUpdate(aOsUpdatePath);
> > +    } catch(e) {
> > +      log("Error: Couldn't reboot into recovery to apply FOTA update " +
> > +          aOsUpdatePath);
> > +    }
> 
> Where is the log flushed here?  Is it accessible after the update is applied?

This logs to logcat, which is semi-persistent. Keep in mind that installing a FOTA update actually means rebooting the device into the recovery partition. If an exception is thrown here, it will be because the reboot didn't occur :)


> ::: toolkit/mozapps/update/nsUpdateService.js
> @@ +1668,5 @@
> > +    if (fotaStatus == Ci.nsIRecoveryService.FOTA_UPDATE_SUCCESS) {
> > +      return STATE_SUCCEEDED;
> > +    }
> > +
> > +    return STATE_FAILED + ":" + FOTA_GENERAL_ERROR;
> 
> The convention is: "failed: %d".
> 
> But I also don't see why any of this is necessary.  Why can't we get the
> status of the update as we normally do?

For our purposes, the recovery partition is basically a black box. Since the code here in _postUpdateProcessing is happening as soon as XPCOM is ready (in profile-after-change), it's probably the soonest we could hope to query and save the FOTA update status.


> ::: toolkit/mozapps/update/updater/updater.cpp
> @@ +2097,1 @@
> >        rv = CopyInstallDirToDestDir();
> 
> Why are you doing this?  This seems like it should break background OS
> updates.

The recovery partition will be the one responsible for any staging / flashing / copying of files. It doesn't really make sense to copy all of gecko if the FOTA update is going to flash a wholly new system image, for example :)

> 
> @@ +3391,5 @@
> >  int AddPreCompleteActions(ActionList *list)
> >  {
> > +  if (sIsOSUpdate) {
> > +    return OK;
> > +  }
> 
> I'm not entirely sure that the implications of doing this are, rstrong needs
> to weigh in here.

This is just another case of needing to skip any action that isn't simply extracting the MAR. For OS updates, everything else is the domain of the recovery partition.
Attachment #666037 - Flags: review?(ehsan) → review?(robert.bugzilla)
This removes the refactoring done in nsUpdateDriver.cpp per IRC convo w/ rstrong.
Attachment #666037 - Attachment is obsolete: true
Attachment #666037 - Flags: review?(robert.bugzilla)
Attachment #666037 - Flags: review?(netzen)
Attachment #666106 - Flags: review?(robert.bugzilla)
Attachment #666106 - Flags: review?(netzen)
Comment on attachment 666106 [details] [diff] [review]
Gonk FOTA updates - v4

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

I'd prefer if rstrong looked at this after me since there's a lot of code here I don't know. But overall looks good to me.

::: b2g/components/DirectoryProvider.js
@@ +61,5 @@
> +    let dir = Cc["@mozilla.org/file/local;1"]
> +                 .createInstance(Ci.nsILocalFile)
> +    dir.initWithPath(path);
> +    if (!dir.exists()) {
> +      throw Cr.NS_ERROR_FILE_NOT_FOUND;

should we fall back to LOCAL_PATH in this case instead of an error?

@@ +65,5 @@
> +      throw Cr.NS_ERROR_FILE_NOT_FOUND;
> +    }
> +
> +    dir.appendRelativePath("updates");
> +    persistent.value = true;

If EXTERNAL_STORAGE can change maybe consider making this persistent.value = false; so the value won't be cached by the dir service.

::: toolkit/mozapps/update/nsUpdateService.js
@@ +1662,5 @@
> +    // the recovery partition won't write to update.status for us
> +    var recoveryService = Cc["@mozilla.org/recovery-service;1"].
> +                          getService(Ci.nsIRecoveryService);
> +    if (!recoveryService) {
> +      return STATE_FAILED + ": " + FOTA_GENERAL_ERROR;

This should ALWAYS exist right? If not consider having FOTA_NO_RECOVERY_SERVICE_ERROR?

@@ +1669,5 @@
> +    var fotaStatus = recoveryService.getFotaUpdateStatus();
> +    if (fotaStatus == Ci.nsIRecoveryService.FOTA_UPDATE_SUCCESS) {
> +      return STATE_SUCCEEDED;
> +    }
> +

Consider having different codes for both of these if they are both used by the recovery service:
  FOTA_UPDATE_UNKNOWN = 0,
  FOTA_UPDATE_FAIL = 1,

::: toolkit/mozapps/update/updater/updater.cpp
@@ +2257,5 @@
>        sReplaceRequest = true;
>      }
>    }
>  
> +  if (getenv("MOZ_OS_UPDATE")) {

Not fully sure what gets restarted after the update by this process, but maybe consider clearing this env var after it is checked here.
Attachment #666106 - Flags: review?(netzen) → review+
(In reply to Brian R. Bondy [:bbondy] from comment #12)
> I'd prefer if rstrong looked at this after me since there's a lot of code
> here I don't know. But overall looks good to me.

\o/ thanks again for the late review. Your comments and questions all sound reasonable, I'll update the patch to address them.
Comment on attachment 666106 [details] [diff] [review]
Gonk FOTA updates - v4

Discussed over irc
Move the code in _getOSUpdateStatus into the "if (status == STATE_APPLIED && update && update.isOSUpdate) {" block and #ifdef MOZ_WIDGET_GONK that entirely.

Just #if defined(MOZ_WIDGET_GONK) this so the call from c++ to js doesn't need to happen when it will never be needed.

>+  bool isOSUpdate;
>+  if (NS_SUCCEEDED(aUpdate->GetIsOSUpdate(&isOSUpdate)) &&
>+      isOSUpdate) {
>+    mInfo.mIsOSUpdate = true;
>+#if defined(MOZ_WIDGET_GONK)
>+    SetOSApplyToDir(aUpdate);
>+#endif
>+  }

r=me with these and Brian's comments addressed.
Attachment #666106 - Flags: review?(robert.bugzilla) → review+
Try run for da6dcd186132 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=da6dcd186132
Results (out of 289 total builds):
    success: 272
    warnings: 15
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mculpepper@mozilla.com-da6dcd186132
https://hg.mozilla.org/mozilla-central/rev/f7505a5f3770
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Summary: Ensure B2G devices can verify and apply FOTA updates → Ensure B2G devices can apply FOTA updates
You need to log in before you can comment on or make changes to this bug.