1070932 - Updates error handling code can fail

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[:gerard-majax]

While investigating bug 1059546, I ran into this:
> 09-22 11:46:21.078   291   291 E GeckoConsole: [JavaScript Error: "TypeError: app.downloadError is null" {file: "app://system.gaiamobile.org/js/updatable.js" line: 105}]

STR:
 0. make PRODUCTION=0 reset-gaia
 1. Check for updates, make sure you have not enabled the reviewers certificates for Marketplace
 2. Install "Stage" update

Expected:
 It should error

Actual:
 JS error in logcat, and download never stops

Comment 1

[:gerard-majax]

Attachment: Send proper app id with error code

Comment 2

[:gerard-majax]

Attachment: Send a valid appId with error code

An UpdateState message passed along with an invalid app id (e.g.,
undefined) will make the error code being not properly propagated. This
makes the system app unhappy, and unable to handle gracefully the error:
 - download notification will stay forever
 - app will not be marked as failed on the homescreen

Comment 3

[:gerard-majax]

Comment on attachment 8493141 [details] [diff] [review]
Send a valid appId with error code

Fabrice, investigating this I found that the root error for the downloadError not propagating properly to the system app was that we had an undefined id in this message.

However, I'm unsure if it's something that is expected: aNewApp.id being undefined, does it makes sense in this case of failure (namely, INVALID_SIGNATURE) ?

Also, I could not find any test directly related to Webapps:UpdateState, so I'm unsure where I should make sure this won't regress.

Comment 4

[:gerard-majax]

This may be a regression of bug 997717

Comment 5

[Johan Lorenzo [:jlorenzo]]

As the patch is here, bug 1060127 is a dupe of this bug. See video if needed: http://mzl.la/1AYUajU

[Blocking Requested - why for this release]: Bug 1060127 was flagged as 2.1+

Comment 7

[Gregor Wagner [:gwagner]]

Blocking a blocker

Comment 8

[[:fabrice] Fabrice Desré]

(In reply to Gregor Wagner [:gwagner] from comment #7)
> Blocking a blocker

Which one?

Comment 9

[Gregor Wagner [:gwagner]]

(In reply to Fabrice Desré [:fabrice] from comment #8)
> (In reply to Gregor Wagner [:gwagner] from comment #7)
> > Blocking a blocker
> 
> Which one?

Bug 1060127

Comment 10

[[:fabrice] Fabrice Desré]

Comment on attachment 8493141 [details] [diff] [review]
Send a valid appId with error code

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

Alex, r=me with this change. We have mochitests that test install/update and but not any that hit this error path :(

::: dom/apps/Webapps.jsm
@@ +3738,5 @@
>      this._saveApps().then(() => {
>        this.broadcastMessage("Webapps:UpdateState", {
>          app: aOldApp,
>          error: aError,
> +        id: aOldApp.id

I think it's safe to just use aId there.

Comment 11

[:gerard-majax]

Yeah, you are confirming what I saw about the mochitests :(. I'll fix the patch and investigate how to trigger the error code path from mochitest, thanks!

Comment 12

[:gerard-majax]

Nice,

> ./mach mochitest-plain dom/apps/tests/test_signed_pkg_install.html

on a debug gecko (mulet) triggered:
> 43 INFO TEST-UNEXPECTED-FAIL | /tests/dom/apps/tests/test_signed_pkg_install.html | uncaught exception - TypeError: gApp.downloadError is null at http://mochi.test:8888/tests/dom/apps/tests/test_signed_pkg_install.html:47

Comment 13

[:gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #12)
> Nice,
> 
> > ./mach mochitest-plain dom/apps/tests/test_signed_pkg_install.html
> 
> on a debug gecko (mulet) triggered:
> > 43 INFO TEST-UNEXPECTED-FAIL | /tests/dom/apps/tests/test_signed_pkg_install.html | uncaught exception - TypeError: gApp.downloadError is null at http://mochi.test:8888/tests/dom/apps/tests/test_signed_pkg_install.html:47

That was because I hacked the id to be null. On the test, I cannot reproduce the issue properly; aNewApp.id is NOT undefined.

Comment 14

[:gerard-majax]

> 30 INFO == TEST == Install a unsigned app from a trusted store
> -*- Webapps.jsm : at install package got app etag=app-unknown_issuer-version-1-1
> -*- Webapps.jsm : confirmInstall
> -*- Webapps.jsm : _writeManifestFile
> -*- Webapps.jsm : Saving /tmp/tmpkioA1F.mozrunner/webapps/{552227ac-858a-41f2-a922-1568669435b7}/update.webapp
> -*- Webapps.jsm : app.origin: app://{552227ac-858a-41f2-a922-1568669435b7}
> -*- Webapps.jsm : Saving /tmp/tmpkioA1F.mozrunner/webapps/webapps.json
> 31 INFO Application install returns successfully
> 32 INFO Got oninstall event
> -*- Webapps.jsm : onInstallSuccessAck CALLING downloadPackage newApp: id={552227ac-858a-41f2-a922-1568669435b7}
> -*- Webapps.jsm : Free storage: 29818912768. Download size: 4220
> -*- Webapps.jsm : About to download http://mochi.test:8888/tests/dom/apps/tests/signed/unknown_issuer_app_1.zip
> -*- Webapps.jsm : Saving /tmp/tmpkioA1F.mozrunner/webapps/webapps.json
> -*- Webapps.jsm : onProgress: 4220/4220
> -*- Webapps.jsm : Saving /tmp/tmpkioA1F.mozrunner/webapps/webapps.json
> -*- Webapps.jsm : File hash computed: 186127b556241df261d0765b45a077d1
> -*- Webapps.jsm : package open/read error: INVALID_SIGNATURE
> -*- Webapps.jsm : onInstallSuccessAck CALLING revertDownloadPackage newApp: id={552227ac-858a-41f2-a922-1568669435b7}
> -*- Webapps.jsm : Error downloading package: INVALID_SIGNATURE
> -*- Webapps.jsm : Saving /tmp/tmpkioA1F.mozrunner/webapps/webapps.json
> -*- Webapps.jsm : SENDING Webapps:UpdateState WITH: id={552227ac-858a-41f2-a922-1568669435b7}
> 33 INFO TEST-PASS | /tests/dom/apps/tests/test_signed_pkg_install.html | Download fails with expected error: INVALID_SIGNATURE 

Do the buggy code path is when calling revertDownloadPackage() from startDownload(), but when it's called from onInstallSuccessAck(), it is okay. That's why mochitest was always green.

Comment 15

[:gerard-majax]

Looks like we indeed completely lack testing of updating a signed app:

> ls dom/apps/tests/test_packaged_app_install.html dom/apps/tests/test_packaged_app_update.html dom/apps/tests/test_signed_pkg_install.html dom/apps/tests/test_signed_pkg_update.html
> ls: cannot access dom/apps/tests/test_signed_pkg_update.html: No such file or directory
> -rw-r--r-- 1 alex alex  12K Aug 31 12:10 dom/apps/tests/test_packaged_app_install.html
> -rw-r--r-- 1 alex alex 9.3K Jul 24 14:37 dom/apps/tests/test_packaged_app_update.html
> -rw-r--r-- 1 alex alex 7.9K Sep 24 15:01 dom/apps/tests/test_signed_pkg_install.html

Comment 16

[:gerard-majax]

So far I'm blocked with writing a proper test :(

Comment 17

[:gerard-majax]

Attachment: Send a valid appId with error code

An UpdateState message passed along with an invalid app id (e.g.,
undefined) will make the error code being not properly propagated. This
makes the system app unhappy, and unable to handle gracefully the error:
 - download notification will stay forever
 - app will not be marked as failed on the homescreen

Comment 18

[:gerard-majax]

Assign to fabrice to help me on the test

Comment 19

[[:fabrice] Fabrice Desré]

Attachment: app-fail-update.patch

Myk & Marco: I'm fine with any of you reviewing. The fix is simple, the test checks that if we update a signed app to a somewhat broken package we get the correct error code when downloading the update.

Comment 20

[Marco Castelluccio [:marco]]

Comment on attachment 8494861 [details] [diff] [review]
app-fail-update.patch

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

Looks good to me!

::: dom/apps/tests/test_signed_pkg_install.html
@@ +153,5 @@
> +    info("== TEST == Set state to invalid app");
> +    var url = gSJS + "?" + "nextApp=unsigned";
> +    var xhr = new XMLHttpRequest();
> +    xhr.addEventListener("load", function() {
> +      is(xhr.responseText, "OK", "nextApp=corrupt OK");

Nit: nextApp=unsigned

Comment 21

[[:fabrice] Fabrice Desré]

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

Comment 22

[[:fabrice] Fabrice Desré]

Comment on attachment 8494861 [details] [diff] [review]
app-fail-update.patch

Approval Request Comment
[Feature/regressing bug #]: Old standing bug
[User impact if declined]: No way for the user to know that an update failed.
[Describe test coverage new/current, TBPL]: new mochitests added.
[Risks and why]: Very low risk, the code is strictly more robust.
[String/UUID change made/needed]:

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8d26b1fe398d

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9a17dc3fa1c8

Comment 25

[Alissa（leave from Mozilla）]

Attachment: Flame 2.1 marketplace update

This issue has been failed verified on Flame 2.1, 2.2,Please check the log,whether the updating successful.
Issue steps:
1.Tap the update notification.
**The downloading notification of marketplace update will disappear whithout any prompt.
See attachment: marketupdate.3gp and logcat.txt
Reproducing rate: 5/5
Flame 2.1 build:
Gaia-Rev        8ae086c39011bc8842b2a19bb5267906fa22345a
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/ebbd5c65c3c1
Build-ID        20141124094013
Version         34.0
Flame 2.2 build:
Gaia-Rev        aad40f6d6eb8f626c6a20db55b9f00d2e832f113
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/be4ba3d5ca9a
Build-ID        20141124100136
Version         36.0a1

Comment 26

[Alissa（leave from Mozilla）]

Attachment: logcat of flame 2.1

Comment 27

[:gerard-majax]

Please be clear about the problem here. The behavior is the expected one, there is no error in logcat, and the apps icons on the homescreen do not show that there was an error during the update process.

Comment 28

[Alissa（leave from Mozilla）]

(In reply to Alexandre LISSY :gerard-majax from comment #27)
> Please be clear about the problem here. The behavior is the expected one,
> there is no error in logcat, and the apps icons on the homescreen do not
> show that there was an error during the update process.

OK,Because there is no prompt of update successfully,so I not sure whether it complete.Now it is correct.