830780 - Add some xpcshell tests for interrupted/corrupted OTA downloads

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: General, defect)

Description

[Dave Hylands [:dhylands]]

B2G deals with interrupted downloads slightly differently from non-B2G firefox.

We should add some tests to make sure that the B2G behaviour is retained.

In particular, we need to test the following:

- downloads interrupted and then resumed shouldn't re-download the update, but pickup where they left off

- downloads interrupted during staging shouldn't re-download the update.

- downloads which are detected to be corrupted, should re-download the update.

Comment 1

[Dave Hylands [:dhylands]]

- interrupt download while partway through downloading (status == downloading). Restarting the download should pick up where it left off.

- interrupt download while completely downloaded and doing verification (i.e. status == downloading, but update.mar file is complete). Restarting the download should re-verify and not do any further downloading.

- interrupt download while applying (status == pending or applying). Restarting the download should re-verify and re-apply.

- Do download/apply. Get Apply update. Choose Apply Later. Redo download, and we should get Apply prompt again.

- Create an update.mar file with all binary 0's. Make it look good (i.e. properly hashed). Download should fail at apply step. Make sure properly reported to UI.

- interupt download while partway through downloading. Corrupt the update.mar file. Restart the download. If should fail verification. Restart the download and it should attempt to download again.

- interrupt download while partway through downloading (status = downloading). Make a newer update available. Restart download should pickup where it left off on original download.

- interrupt download while completely downloaded and doing verification. (status = pending or applying). Make a newer update available. Restarting download should re-verify and not do any further downloading.

- interrupt download while applying (status == pending or applying). Make a newer update available. Restarting the download should re-verify and re-apply the original download.

For the previous 3 tests, performing a "Check Now" after interrupting should cause it to switch to the new update and abandon the old one.

Comment 2

[Dave Hylands [:dhylands]]

Add/modify tests to test was test_0061_manager.js, test_0062_manager.js and test_0064_manager.js were trying to test, but taking into consideration that gonk doesn't resume downloads at startup time.

Comment 3

[Dave Hylands [:dhylands]]

Also add some tests to check out permutations of bad permissions, bad ownership, updates/0 being a file instead of a directory etc.

We should test both /data/local/updates and /sdcard/updates (especially /sdcard/updates since this is a user-accessible thing and users can do weird things).

Also since /sdcard is FAT and /data/local is ext4/yaffs

Comment 4

[Dave Hylands [:dhylands]]

Attachment: Adds several gonk specific tests to the updater xpcshell

Comment 5

[Dave Hylands [:dhylands]]

Comment on attachment 723550 [details] [diff] [review]
Adds several gonk specific tests to the updater xpcshell

I'm not sure all of the tests that I originally thought could be done can be done purely in xpcshell.

This test file covers off the basic functionality for interrupted download behaviour.

Comment 6

[Dave Hylands [:dhylands]]

https://tbpl.mozilla.org/?tree=Try&rev=86e0de07be77

Comment 7

[Marshall Culpepper [:marshall_law]]

Comment on attachment 723550 [details] [diff] [review]
Adds several gonk specific tests to the updater xpcshell

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

nice, this looks good :)

::: toolkit/mozapps/update/test/unit/test_gonk_general.js
@@ +174,5 @@
> +    // to add themselves.
> +    tm.mainThread.dispatch(function() {
> +    this._observer = observer.QueryInterface(AUS_Ci.nsIRequestObserver);
> +      this._ctxt = ctxt;
> +      this._observer.onStartRequest(this, this.ctxt);

not sure it matters since this callback is just a shim, but should this.ctxt be this._ctxt?

Comment 8

[Dave Hylands [:dhylands]]

Added [fxos:media] whiteboard for bugs assigned to me so that they can be triaged/prioritized, etc.

Comment 9

[Dave Hylands [:dhylands]]

Attachment: bug-830780-int-update.patch

Rebased patch to the latest master

Comment 10

[Dave Hylands [:dhylands]]

https://tbpl.mozilla.org/?tree=Try&rev=16674b73b8ec

Comment 11

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

Comment on attachment 8416825 [details] [diff] [review]
bug-830780-int-update.patch

The downloadInterruptedRecovery.js change looks fine but there is no downloadInterruptedGonk.js file which you added to the xpcshell.ini.

Also, instead of adding a new data file you could just return the data via code.

Comment 12

[Dave Hylands [:dhylands]]

Attachment: Add some gonk tests of interrupted downloads.

Weird. The try run has the right changes.

So I'll attach the patch again, and verify it has the right files.

Comment 13

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

Comment on attachment 8416995 [details] [diff] [review]
Add some gonk tests of interrupted downloads.

>diff --git a/toolkit/mozapps/update/tests/unit_aus_update/downloadInterruptedGonk.js b/toolkit/mozapps/update/tests/unit_aus_update/downloadInterruptedGonk.js
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/mozapps/update/tests/unit_aus_update/downloadInterruptedGonk.js
>@@ -0,0 +1,320 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
>+ */
>+
>+/* General MAR File Download Tests */
>+
>+const INC_CONTRACT_ID = "@mozilla.org/network/incremental-download;1";
>+
>+var gNextRunFunc;
>+var gStatusResult;
Remove. var gStatusResult is already decalred in head_update.js

>+var gExpectedStatusResult;
>+var gIncrementalDownloadClassID, gIncOldFactory;
>+var gIncrementalSuffix = "";
>+var gIncrementalExpectedFileSize = -1;
>+var gIncrementalDownloaderStarted = false;
>+
>+// gIncrementalDownloadErrorType is used to loop through each of the connection
>+// error types in the Mock incremental downloader.
>+var gIncrementalDownloadErrorType = 0;
>+
>+function run_test() {
>+  setupTestCommon();
>+
>+  logTestInfo("testing interrupted mar downloads and recovery for gonk");
>+
>+  Services.prefs.setBoolPref(PREF_APP_UPDATE_STAGING_ENABLED, false);
>+  Services.prefs.setIntPref(PREF_APP_UPDATE_RETRY_TIMEOUT, 5000);
>+  removeUpdateDirsAndFiles();
This isn't needed at the start now that all app update xpcshell tests use a custom updates and app directory.

>+
>+  // The HTTP server is only used for the mar file downloads which is slow
nit: consistency with other files makes it easier for me to check for differences between tests so please change the comment to
// The HTTP server is only used for the mar file downloads since it is slow

>+  start_httpserver();
>+  setUpdateURLOverride(gURLData + "update.xml");
>+  // The mock XMLHttpRequest is MUCH faster
>+  overrideXHR(callHandleEvent);
>+  standardInit();
>+  do_execute_soon(run_test_pt1);
>+}
>+
>+// The HttpServer must be stopped before calling do_test_finished
>+function finish_test() {
>+  stop_httpserver(do_test_finished);
Please use doTestFinish.


>+// Since the test doesn't actually apply the update, it should be sitting
>+// in the pending state. Make sure that when we download again that it
>+// doesn't actually download anything (i.e. the IncrementalDownloader
>+// shouldn't get called)
>+function run_test_pt2() {
>+  gIncrementalDownloaderStarted = false;
>+  run_test_helper_pt1("Download of simple.mar 2nd time",
>+                      AUS_Cr.NS_OK, run_test_pt3);
>+}
Aren't these first two tests essentially the same as
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/tests/unit_aus_update/downloadInterruptedRecovery.js#247
and
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/tests/unit_aus_update/downloadInterruptedRecovery.js#255

I'd like to see if these tests can just be added to downloadInterruptedRecovery.js. I don't want to take you away from your current work so I'll check if any or all of these tests can be adapted to desktop and if they can I'll post a patch.

Comment 14

[Dave Hylands [:dhylands]]

Not working on FxOS any more, so removing myself as assignee

Comment 15

[BMO Automation]

Firefox OS is not being worked on