813770 - Rebooting after update is downloaded but before it's applied doesn't show prompt to re-apply on subsequent restart

Status: RESOLVED FIXED

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

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

STR, after bug 813451
 (1) Set up device to have update available
 (2) Download update, confirm in logcat
 (3) |adb reboot| before applying update finishes

When b2g comes back up, the normal update pings don't show a prompt to re-apply or even re-download the update.  Force-checking doesn't do that either.  So the device seems to be stuck on that version.

Absolutely bb+, but leaving bb? to triage assignee.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The same problem would occur if the b2g process crashes after an update is downloaded.

Borderline dogfood-blocker but fairly edge-case-y.

Comment 2

[JP Rosevear [:jpr]]

Per comment #1, is this a duplicate of bug 799482 (and maybe fixed by the dependency?)

Comment 3

[Andrew Overholt [:overholt]]

My guess is this was fixed by bug 791829.

Comment 4

[Andrew Overholt [:overholt]]

Setting priority based on triage discussions.  Feel free to decrease priority if you disagree.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to JP Rosevear [:jpr] from comment #2)
> Per comment #1, is this a duplicate of bug 799482 (and maybe fixed by the
> dependency?)

No, that bug happens when a *download* is interrupted.  This bug happens after the download has completed.

(In reply to Andrew Overholt [:overholt] from comment #3)
> My guess is this was fixed by bug 791829.

Different bugs.

Comment 6

[Damon Sicore (:damons)]

OK, If Marshall is too busy, can we get someone else to take a look at testing this and finding root cause?  Seems like this is a test case we should get into update automation, too.

Comment 7

[Andrew Overholt [:overholt]]

Could bbondy or dhylands help out?  This is on Marshall's list but not at the top.

Comment 8

[Dave Hylands [:dhylands]]

It would be #3 on my list, but I could look at it if I get there before Marshal.

Comment 9

[Andrew Overholt [:overholt]]

Brian, do you have time for this?

Comment 10

[Brian R. Bondy [:bbondy]]

No. There are only a few days left that I'm working before the holidays and I won't be able to get to this within those days.

Comment 11

[Andrew Overholt [:overholt]]

(In reply to Brian R. Bondy [:bbondy] from comment #10)
> No. There are only a few days left that I'm working before the holidays and
> I won't be able to get to this within those days.

Okay, back to Marshall :)

Comment 12

[Dave Hylands [:dhylands]]

Dietrich asked me to take this one.

Comment 13

[Dave Hylands [:dhylands]]

So, the state seems to change as follows:

1. state = downloading
2. download happens
3. update.mar verification happens
4. state = pending
5. updater is launched
6. state = applying
7. updater applies update to /system/b2g/updated directory
8. state = applied
9. updater exits
10. User is prompted whether to install update or not.

I'm assuming that reboots during steps 1-3 will be treated as an interrupted download and that it will re-attempt to do the download.

I'm also assuming that this bug refers to reboots occurring after step 4 and before step 8.

Rebooting prior to step 4 resulted in the download happening automatically at startup (I don't think that this is the desired behaviour, but that's for bug 819548 to fix).

Rebooting while sitting in step 10, resulted in the update being applied without prompting.

So this bug seems to apply to a reboot happening in steps 5 thru 7 (i.e. state = pending or applying).

Questions:
Should it start automatically? Or should the user need to initiate?

If it should start automatically, when should the application of the update occur?
  - I'm assuming that we want to at least wait until the phone has booted up and gone idle for some amount of time.

Comment 14

[Dave Hylands [:dhylands]]

Looking at the code, there is no state changes between downloading and verification, so there is currently no way to detect that the download portion was completed, but we got restarted while performing validation.

Validation of a full OTA download (~50Mb) takes about 15 seconds. Currently, if the phone reboots during the validation phase, then it will attempt to re-download rather than just re-validate. If it is desirable to not re-download, then we should probably create a new bug to introduce a new state.

Comment 16

[Marshall Culpepper [:marshall_law]]

(In reply to Dave Hylands [:dhylands] from comment #13)
> Questions:
> Should it start automatically? Or should the user need to initiate?

I would argue that it should start automatically -- if the update process got all the way to "applying" (actually "staging") the update before getting rebooted, it should be safe to start the process over without intervention.

In fact, the applying process only stages first because we need to prompt the user for a process restart to finish applying the changes (Step 10).

In the case of this bug, the device has already rebooted. This gives us the opportunity to both extract and apply the update before b2g is even booted. We would need to code this logic early into the boot process, though. Probably in the standalone updater itself.

>  
> If it should start automatically, when should the application of the update
> occur?
>   - I'm assuming that we want to at least wait until the phone has booted up
> and gone idle for some amount of time.

See above. We _could_ wait until some timeout to try again after b2g is already up, but this might be unnecessary if we can preempt the process startup by staging and applying up front.

Thoughts?

Comment 17

[Dave Hylands [:dhylands]]

I actually got the automatic bit working (so automatically applying the update at reboot time).

But for some types of updates (I've been testing with full updates), then phone looks dead while it's doing this (I could see what was going on in logcat, but the LCD was black).

And since the applying portion can take 30 seconds or more, it looks like the phone is hung for this time.

So my current solution is that the phone will show that there is an update to apply, and look like a normal update, but when the user clicks on it, it will detect that the download has already occurred and go right into "Uncompressing..."

Comment 18

[Dave Hylands [:dhylands]]

Attachment: Allows interrupted downloads to resume properly the next time the update prompt shows

This patch does 2 things:

1 - Detects and ignores interrupted downloads at boot time
2 - Provides the correct callbacks so that the UI gets updated properly the next time a download is started.

Conceivably, if we detect an interrupted download we may also wish to "force" an update check, so that the user sees the notifcation fairly soon after boot rather than waiting until the timer fires.

Comment 19

[Dave Hylands [:dhylands]]

This is some interaction between this bug and bug 819548.

In particular, the #ifdef MOZ_WIDGET_GONK stuff from patch 819548 can be removed as part of landing this, or after, since this code essentially does the same thing a few lines earlier.

Comment 20

[Julien Wajsberg [:julienw]]

I agree.

Is the 2nd part of the patch about the case where we were interrupted during uncompressing ? I didn't test this in bug 819548, good catch :)

Comment 21

[Dave Hylands [:dhylands]]

(In reply to Julien Wajsberg [:julienw] from comment #20)
> I agree.
> 
> Is the 2nd part of the patch about the case where we were interrupted during
> uncompressing ? I didn't test this in bug 819548, good catch :)

Yep.

The first part of my patch was directly lifted from your patch. I went around a few different ways and eventually settled on what's in the attached patch.

Comment 22

[Marshall Culpepper [:marshall_law]]

Comment on attachment 697232 [details] [diff] [review]
Allows interrupted downloads to resume properly the next time the update prompt shows

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

Dave and I chatted about the patch and he's going to update it based on changes in Bug 817821

Comment 24

[Dave Hylands [:dhylands]]

Attachment: Allows interrupted downloads to resume properly the next time the update prompt shows - v2

Removed onProgress
Added fake _request object so that _verifyDownload works properly

Comment 25

[Marshall Culpepper [:marshall_law]]

Comment on attachment 699121 [details] [diff] [review]
Allows interrupted downloads to resume properly the next time the update prompt shows - v2

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

This looks good to me, but I'd feel more comfortable if an update peer also gave this a look over, since we're calling onStopRequest with a fake object to get this behavior.

Brian, I'm tagging you but feel free to change or suggest someone else if you can't get to it :)

Comment 26

[Brian R. Bondy [:bbondy]]

Comment on attachment 699121 [details] [diff] [review]
Allows interrupted downloads to resume properly the next time the update prompt shows - v2

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

rstrong mentioned he'd take a look

Comment 27

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

Comment on attachment 699121 [details] [diff] [review]
Allows interrupted downloads to resume properly the next time the update prompt shows - 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
>@@ -747,24 +747,31 @@ function writeStatusFile(dir, state) {
>   writeStringToFile(statusFile, state);
> }
> 
> /**
>  * Reads the link file from the update.link file in the
>  * specified directory.
Something like:
Reads the link file specified in the update.link file in the specified directory and returns the nsIFile for the corresponding file.

>  * @param   dir
>  *          The dir to look for an update.link file in
>- * @return  The contents of the update.link file.
>+ * @return  A file object corresponding to the filename found 
>+ *          in the update.link file, or null if the update.link
>+ *          file doesn't exist.
A nsIFile for the file path specified in the update.link file or null if the update.link file doesn't exist.

>  */
> function readLinkFile(dir) {
Please change to getLinkedFile (or similar)

>   var linkFile = dir.clone();
>   linkFile.append(FILE_UPDATE_LINK);
>-  var link = readStringFromFile(linkFile) || STATE_NONE;
>-  LOG("readLinkFile - link: " + link + ", path: " + linkFile.path);
>-  return status;
>+  var link = readStringFromFile(linkFile);
>+  LOG("readLinkFile linkFile.path: " + linkFile.path + ", link: " + link);
>+  if (!link) {
>+    return null;
>+  }
>+  let file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile);
>+  file.initWithPath(link);
>+  return file;
> }
Why not just add this for GONK to _getUpdateArchiveFile?

> 
> /**
>  * Creates a link file, which allows the actual patch to live in
>  * a directory different from the update directory.
>  * @param   dir
>  *          The patch directory where the update.link file
>  *          should be written.
>@@ -1753,16 +1760,26 @@ UpdateService.prototype = {
>       cleanupActiveUpdate();
>       return;
>     }
> 
>     var um = Cc["@mozilla.org/updates/update-manager;1"].
>              getService(Ci.nsIUpdateManager);
>     var update = um.activeUpdate;
nit: this should be after the new code below.

> 
>+#ifdef MOZ_WIDGET_GONK
>+    if ((status == STATE_DOWNLOADING) ||
Please add a comment as to why STATE_DOWNLOADING is also included for this case.

>+        (status == STATE_PENDING) ||
>+        (status == STATE_APPLYING)) {
The following should work just fine
if (status == STATE_DOWNLOADING ||
    status == STATE_PENDING ||
    status == STATE_APPLYING) {

>+      LOG("UpdateService:_postUpdateProcessing - interrupted download detected - wait for user consent");
This text is incorrect since it also applies to STATE_PENDING and STATE_APPLYING

>+      um.activeUpdate = null;
Won't this cause a currently downloading update to end up redownloading the entire update after the next check and if so, is that really what you want to do instead of the logic below?

>+      return;
>+    }
>+#endif
>+
>     if (status == STATE_DOWNLOADING) {
>       LOG("UpdateService:_postUpdateProcessing - patch found in downloading " +
>           "state");
>       if (update && update.state != STATE_SUCCEEDED) {
>         // Resume download
>         var status = this.downloadUpdate(update, true);
>         if (status == STATE_NONE)
>           cleanupActiveUpdate();
>@@ -3444,17 +3461,55 @@ Downloader.prototype = {
>     // to download.
>     this._patch = this._selectPatch(update, updateDir);
>     if (!this._patch) {
>       LOG("Downloader:downloadUpdate - no patch to download");
>       return readStatusFile(updateDir);
>     }
>     this.isCompleteUpdate = this._patch.type == "complete";
> 
>-    var patchFile = this._getUpdateArchiveFile();
>+    var patchFile;
>+
>+#ifdef MOZ_WIDGET_GONK
>+    var state = readStatusFile(updateDir);
>+    if ((state == STATE_PENDING) || (state == STATE_APPLYING)) {
>+      // It looks like the patch was downloaded, but got interrupted while
>+      // it was being applied. So we'll try to skip the downloading portion.
>+
>+      writeStatusFile(updateDir, STATE_PENDING);
>+
>+      patchFile = readLinkFile(updateDir);
>+      if (!patchFile) {
>+        // No link file. We'll just assume that the update.mar is in the
>+        // update directory.
>+        patchFile = updateDir.clone();
>+        patchFile.append(FILE_UPDATE_ARCHIVE);
>+      }
Why not just add this for GONK to _getUpdateArchiveFile?

>+
>+      // Since the code expects the onStopRequest callback to happen
>+      // asynchronously (And you have to call AUS_addDownloadListener
>+      // after calling AUS_downloadUpdate) we need to defer this.
>+
>+      this._downloadTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
>+      this._downloadTimer.initWithCallback(function() {
>+        this._downloadTimer = null;
>+        // Send a fake onStopRequest. Filling in the destination allows
>+        // _verifyDownload to work, and then the update will be applied.
>+        this._request = {destination: patchFile};
>+        this.onStopRequest(this._request, null, Cr.NS_OK);
>+      }.bind(this), 0, Ci.nsITimer.TYPE_ONE_SHOT);
>+
>+      // Returning STATE_DOWNLOADING makes UpdatePrompt think we're
>+      // downloading. The onStopRequest that we spoofed above will make it
>+      // look like the download finished.
>+      return STATE_DOWNLOADING;
>+    }
>+#endif
>+
>+    patchFile = this._getUpdateArchiveFile();
>     if (!patchFile) {
>       return STATE_NONE;
>     }
> 
>     if (patchFile.path.indexOf(updateDir.path) != 0) {
>       // The patchFile is in a directory which is different from the
>       // updateDir, create a link file.
>       writeLinkFile(updateDir, patchFile);

I should be able to review an updated patch later tonight Mountain View time.

Comment 28

[Dave Hylands [:dhylands]]

(In reply to Robert Strong [:rstrong] (do not email) from comment #27)
> Comment on attachment 699121 [details] [diff] [review]
> Allows interrupted downloads to resume properly the next time the update
> prompt shows - v2

> >   var linkFile = dir.clone();
> >   linkFile.append(FILE_UPDATE_LINK);
> >-  var link = readStringFromFile(linkFile) || STATE_NONE;
> >-  LOG("readLinkFile - link: " + link + ", path: " + linkFile.path);
> >-  return status;
> >+  var link = readStringFromFile(linkFile);
> >+  LOG("readLinkFile linkFile.path: " + linkFile.path + ", link: " + link);
> >+  if (!link) {
> >+    return null;
> >+  }
> >+  let file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile);
> >+  file.initWithPath(link);
> >+  return file;
> > }
> Why not just add this for GONK to _getUpdateArchiveFile?

_getUpdateArchiveFile returns the location that we should store the patch file. This is what we want for a non-interrupted download. So since the patch file and link files don't exist yet, _getUpdateArchiveFile has no reason to read a link file.

_getUpdateArchiveFile will return one of two locations for the patch, /data/local/updates/0 or /mnt/sdcard/updates/0. When /mnt/sdcard/updates/0 is used as the location for the patchfile, then /data/local/updates/0 will contain a link file.

For an interrupted update we want the actual location of the patch file associated with the update.

> >+      um.activeUpdate = null;
> Won't this cause a currently downloading update to end up redownloading the
> entire update after the next check and if so, is that really what you want
> to do instead of the logic below?

Clearing um.activeUpdate will cause _selectAndInstallUpdate to not return early.

What's important here, is that we don't want to call cleanActiveUpdate. If we leave the update.state file and patchfile around, then when the user starts the download again the incremental download will pick up where it left off.

As I wrote this, I realized that there is a small bug in the logic, which I'll address.

> >-    var patchFile = this._getUpdateArchiveFile();
> >+    var patchFile;
> >+
> >+#ifdef MOZ_WIDGET_GONK
> >+    var state = readStatusFile(updateDir);
> >+    if ((state == STATE_PENDING) || (state == STATE_APPLYING)) {
> >+      // It looks like the patch was downloaded, but got interrupted while
> >+      // it was being applied. So we'll try to skip the downloading portion.
> >+
> >+      writeStatusFile(updateDir, STATE_PENDING);
> >+
> >+      patchFile = readLinkFile(updateDir);
> >+      if (!patchFile) {
> >+        // No link file. We'll just assume that the update.mar is in the
> >+        // update directory.
> >+        patchFile = updateDir.clone();
> >+        patchFile.append(FILE_UPDATE_ARCHIVE);
> >+      }
> Why not just add this for GONK to _getUpdateArchiveFile?

This code path is for interrupted updates. _getUpdateArchiveFile is for new updates.

Comment 29

[Dave Hylands [:dhylands]]

Attachment: Allows interrupted downloads to resume properly the next time the update prompt shows - v3

Addressed review comments

Comment 30

[Dave Hylands [:dhylands]]

I noticed a bunch of trailing white space (in the splinter review). I'll clean that up before landing.

Comment 31

[Dave Hylands [:dhylands]]

Attachment: Allows interrupted downloads to resume properly the next time the update prompt shows - v4

Cleaned up trailing whitespace in the whole file.

Comment 32

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

Comment on attachment 700803 [details] [diff] [review]
Allows interrupted downloads to resume properly the next time the update prompt shows - v4

Looks much better! Mostly just nits. r=me with the following assuming tests pass.

>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
>@@ -748,28 +748,36 @@ function readStatusFile(dir) {
>  */
> function writeStatusFile(dir, state) {
>   var statusFile = dir.clone();
>   statusFile.append(FILE_UPDATE_STATUS);
>   writeStringToFile(statusFile, state);
> }
> 

Since the following is gonk specific please ifdef the entire block.
> /**
>- * Reads the link file from the update.link file in the
>- * specified directory.
>+ * Reads the link file specified in the update.link file in the
>+ * specified directory and returns the nsIFile for the
>+ * corresponding file.
>  * @param   dir
>  *          The dir to look for an update.link file in
>- * @return  The contents of the update.link file.
>+ * @return  A nsIFile for the file path specified in the
>+ *          update.link file or null if the update.link file
>+ *          doesn't exist.
>  */
>-function readLinkFile(dir) {
>+function getLinkFile(dir) {
I suggested getLinkedFile because it is the file linked to from the link file and getLinkFile implies that it is the link file such as update.link. How about just getFileFromLinkFile so it is clearer.

>   var linkFile = dir.clone();
>   linkFile.append(FILE_UPDATE_LINK);
>-  var link = readStringFromFile(linkFile) || STATE_NONE;
>-  LOG("readLinkFile - link: " + link + ", path: " + linkFile.path);
>-  return status;
>+  var link = readStringFromFile(linkFile);
>+  LOG("getLinkFile linkFile.path: " + linkFile.path + ", link: " + link);
>+  if (!link) {
>+    return null;
>+  }
>+  let file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile);
>+  file.initWithPath(link);
>+  return file;
> }

>@@ -816,25 +824,39 @@ function releaseSDCardMountLock() {
>   if (gSDCardMountLock) {
>     gSDCardMountLock.unlock();
>     gSDCardMountLock = null;
>   }
> #endif
> }
> 

Since the following is gonk specific please ifdef the entire block.
> /**
>+ * Determines if the state corresponds to an interrupted update.
>+ * This could either be because the download was interrupted, or
>+ * because staging the update was interrupted.
>+ *
>+ * @return true if the state corresponds to an interrupted
>+ *         update.
>+ */
>+function isInterruptedUpdate(status) {
>+  return ((status == STATE_DOWNLOADING) ||
>+          (status == STATE_PENDING) ||
>+          (status == STATE_APPLYING));
remove the unneeded parens
return (status == STATE_DOWNLOADING ||
        status == STATE_PENDING ||
        status == STATE_APPLYING);

>+}

>@@ -1756,16 +1778,39 @@ UpdateService.prototype = {
>     if (status == STATE_NONE) {
>       LOG("UpdateService:_postUpdateProcessing - no status, no update");
>       cleanupActiveUpdate();
>       return;
>     }
> 
>     var um = Cc["@mozilla.org/updates/update-manager;1"].
>              getService(Ci.nsIUpdateManager);
>+
>+#ifdef MOZ_WIDGET_GONK
>+    // This code is called very early in the boot process, before we've even
>+    // had a chance to setup the UI and give any feedback to the user.
nit: s/and give any feedback to the user/so we can give feedback to the user/

>+    //
>+    // Since the download may be occuring over a link which has associated
>+    // cost, we want to require user-consent before resuming the download.
>+    // Similarly, applying an already downloaded update now is undesireable
nit: s/Similarly/Also/

>+    // since the phone looks dead (at boot time) while the update is being
>+    // applied, which can take several minutes. We would rather wait until
>+    // later when the UI is initialized and we can give feedback to theuser
>+    // about what's going on.
nit: since the phone will look dead while the update is being applied when booting which can take several minutes. Instead we wait until the UI is initialized so it is possible to give feedback to and get consent to update from the user.

>+    //
>+    // We clear um.activeUpdate (so _selectAndInstallUpdate doesn't return
>+    // early), and leave the files so that an interrupted download can resume
s/interrupted download/interrupted update/

>+    // where it left off.
>+    if (isInterruptedUpdate(status)) {
>+      LOG("UpdateService:_postUpdateProcessing - interrupted update detected - wait for user consent");
>+      um.activeUpdate = null;
>+      return;
>+    }
>+#endif
>+

Comment 33

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

(In reply to Robert Strong [:rstrong] (do not email) from comment #32)
>...
> >-function readLinkFile(dir) {
> >+function getLinkFile(dir) {
> I suggested getLinkedFile because it is the file linked to from the link
> file and getLinkFile implies that it is the link file such as update.link.
> How about just getFileFromLinkFile so it is clearer.
Actually, I think getFileFromUpdateLink would be better

Also, put isInterruptedUpdate above this so they can both use the same ifdef which will keep at least some of the gonk specific code in the same place.

Comment 34

[Dave Hylands [:dhylands]]

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=069f8873741f

Comment 35

[Dave Hylands [:dhylands]]

I cancelled the previous push to try because I didn't want to build the world.
Pushing again:
 https://tbpl.mozilla.org/?tree=Try&rev=cc4220db0777

Comment 36

[Dave Hylands [:dhylands]]

Attachment: Allows interrupted downloads to resume properly the next time the update prompt shows - v5

Carrying forward the r+
Address rstrong's review comments.

Comment 37

[Dave Hylands [:dhylands]]

Attachment: Fixes to the test suite to make the new code pass

Changes to the test code to allow the new code to pass.

Comment 38

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

Comment on attachment 701227 [details] [diff] [review]
Fixes to the test suite to make the new code pass

I want to review this one

Comment 39

[Dave Hylands [:dhylands]]

Attachment: Fixes to the test suite to make the new code pass - v2

Removed a trailing space.

Comment 40

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

Comment on attachment 701228 [details] [diff] [review]
Fixes to the test suite to make the new code pass - v2

Stealing again per comment #38

Comment 41

[Dave Hylands [:dhylands]]

Pushed to try:
 https://tbpl.mozilla.org/?tree=Try&rev=eafea80a2af1

Comment 42

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

Comment on attachment 701228 [details] [diff] [review]
Fixes to the test suite to make the new code pass - v2

>diff --git a/toolkit/mozapps/update/test/unit/test_0030_general.js b/toolkit/mozapps/update/test/unit/test_0030_general.js
>--- a/toolkit/mozapps/update/test/unit/test_0030_general.js
>+++ b/toolkit/mozapps/update/test/unit/test_0030_general.js
>@@ -187,19 +185,25 @@ function run_test_pt11() {
>   run_test_helper_pt1("mar download with the mar not found",
>                       AUS_Cr.NS_ERROR_UNEXPECTED, run_test_pt12);
> }
> 
> // mar download with a valid MD5 hash but invalid file size
> function run_test_pt12() {
>   const arbitraryFileSize = 1024000;
>   setResponseBody("MD5", MD5_HASH_SIMPLE_MAR ,arbitraryFileSize);
>+  // There seems to be a race on the web server side when the patchFile is
>+  // stored on the SDCard. Sometimes, the webserver will serve up an error
>+  // 416 and the contents of the file, and sometimes it will serve up an error
>+  // 200 and no contents. This can cause either NS_ERROR_UNEXPECTED or
>+  // NS_ERROR_CONTENT_CORRUPTED.
>+  // Bug 828858 was filed to follow up on this issue.
>   run_test_helper_pt1("mar download with a valid MD5 hash but invalid file size",
>-                      AUS_Cr.NS_ERROR_UNEXPECTED, run_test_pt13,
>-                      AUS_Cr.NS_ERROR_CORRUPTED_CONTENT);
>+                      [AUS_Cr.NS_ERROR_UNEXPECTED, 
>+                       AUS_Cr.NS_ERROR_CORRUPTED_CONTENT], run_test_pt13);
I'd like this workaround to be GONK specific instead of for all platforms and I'd also like the workaround to be completely outside of the normal helper functions to make it clear what needs to be removed and that it is a workaround.

Something like:

// There seems to be a race on the web server side when the patchFile is
// stored on the SDCard. Sometimes, the webserver will serve up an error
// 416 and the contents of the file, and sometimes it will serve up an error
// 200 and no contents. This can cause either NS_ERROR_UNEXPECTED or
// NS_ERROR_CONTENT_CORRUPTED.
// Bug 828858 was filed to follow up on this issue.
if (IS_TOOLKIT_GONK) {
  run_test_helper_bug828858_pt1("mar download with a valid MD5 hash but invalid file size",
                                AUS_Cr.NS_ERROR_UNEXPECTED,
                                AUS_Cr.NS_ERROR_CORRUPTED_CONTENT, run_test_pt13);
} else {
  run_test_helper_pt1("mar download with a valid MD5 hash but invalid file size",
                      AUS_Cr.NS_ERROR_UNEXPECTED, run_test_pt13,
                      AUS_Cr.NS_ERROR_CORRUPTED_CONTENT);
}

along with new helper functions as follows

// The following 3 functions are a workaround for GONK due to Bug 828858 and can
// be removed after it is fixed and the callers are changed to use the regular
// helper functions.
function run_test_helper_bug828858_pt1(aMsg, aExpectedStatusResult, aNextRunFunc) {
  gUpdates = null;
  gUpdateCount = null;
  gStatusResult = null;
  gCheckFunc = check_test_helper_bug828858_pt1_1;
  gNextRunFunc = aNextRunFunc;
  gExpectedStatusResult = aExpectedStatusResult;
  logTestInfo(aMsg, Components.stack.caller);
  gUpdateChecker.checkForUpdates(updateCheckListener, true);
}

function check_test_helper_bug828858_pt1_1() {
  do_check_eq(gUpdateCount, 1);
  gCheckFunc = check_test_helper_bug828858_pt1_2;
  var bestUpdate = gAUS.selectUpdate(gUpdates, gUpdateCount);
  var state = gAUS.downloadUpdate(bestUpdate, false);
  if (state == STATE_NONE || state == STATE_FAILED)
    do_throw("nsIApplicationUpdateService:downloadUpdate returned " + state);
  gAUS.addDownloadListener(downloadListener);
}

function check_test_helper_bug828858_pt1_2() {
  for (var i = 0; i < gExpectedStatusResult.length; i++) {
    if (gStatusResult == gExpectedStatusResult[i]) {
      do_check_eq(gStatusResult, gExpectedStatusResult[i]);
      break;
    }
  }
  if (i >= gExpectedStatusResult.length) {
    do_check_eq(gStatusResult, gExpectedStatusResult[0]);
  }
  gAUS.removeDownloadListener(downloadListener);
  gNextRunFunc();
}

Also, please either add a comment in bug 828858 or file a new bug to remove the workaround from this test.

r=me with that

Comment 43

[Dave Hylands [:dhylands]]

(In reply to Robert Strong [:rstrong] (do not email) from comment #42)
> Comment on attachment 701228 [details] [diff] [review]
> Fixes to the test suite to make the new code pass - v2
> 
> I'd like this workaround to be GONK specific instead of for all platforms
> and I'd also like the workaround to be completely outside of the normal
> helper functions to make it clear what needs to be removed and that it is a
> workaround.

Done.

I restored all of the helper functions back to their original form, and added the 3 new helper functions.

> Also, please either add a comment in bug 828858 or file a new bug to remove
> the workaround from this test.

I added a note to bug 828858.

I've submitted to try again:
https://tbpl.mozilla.org/?tree=Try&rev=2041db645ef1

Comment 44

[Dave Hylands [:dhylands]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/285310da9412
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea86f0fce98a

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/285310da9412
https://hg.mozilla.org/mozilla-central/rev/ea86f0fce98a

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/daab716630a7
https://hg.mozilla.org/releases/mozilla-b2g18/rev/8f34a5cf39ba