Updates error handling code can fail

RESOLVED FIXED in Firefox 34

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gerard-majax, Assigned: fabrice)

Tracking

({regression})

unspecified
2.1 S5 (26sep)
ARM
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:2.1+, firefox33 wontfix, firefox34 fixed, firefox35 fixed, b2g-v2.1 verified, b2g-v2.2 verified)

Details

(Whiteboard: [systemsfe])

Attachments

(3 attachments, 3 obsolete attachments)

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
Reporter

Comment 2

5 years ago
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
Reporter

Updated

5 years ago
Attachment #8493125 - Attachment is obsolete: true
Reporter

Comment 3

5 years ago
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.
Attachment #8493141 - Flags: feedback?(fabrice)
Reporter

Comment 4

5 years ago
This may be a regression of bug 997717
Depends on: 997717
Flags: needinfo?(myk)
Flags: needinfo?(mhaigh)
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+
blocking-b2g: --- → 2.1?
Keywords: regression
Duplicate of this bug: 1060127
Blocking a blocker
Assignee: nobody → lissyx+mozillians
blocking-b2g: 2.1? → 2.1+
Whiteboard: [systemsfe]
Assignee

Comment 8

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

Which one?
(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
Target Milestone: --- → 2.1 S5 (26sep)
Assignee

Comment 10

5 years ago
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.
Reporter

Comment 11

5 years ago
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!
Reporter

Comment 12

5 years ago
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
Reporter

Comment 13

5 years ago
(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.
Reporter

Comment 14

5 years ago
> 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.
Reporter

Comment 15

5 years ago
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
Reporter

Comment 16

5 years ago
So far I'm blocked with writing a proper test :(
Reporter

Comment 17

5 years ago
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
Reporter

Updated

5 years ago
Attachment #8493141 - Attachment is obsolete: true
Attachment #8493141 - Flags: feedback?(fabrice)
Reporter

Comment 18

5 years ago
Assign to fabrice to help me on the test
Assignee: lissyx+mozillians → fabrice
Assignee

Comment 19

5 years ago
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.
Attachment #8494577 - Attachment is obsolete: true
Attachment #8494861 - Flags: review?(myk)
Attachment #8494861 - Flags: review?(mar.castelluccio)
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
Attachment #8494861 - Flags: review?(mar.castelluccio) → review+
Flags: needinfo?(mhaigh)
Assignee

Comment 22

5 years ago
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]:
Attachment #8494861 - Flags: review?(myk) → approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/8d26b1fe398d
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Attachment #8494861 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(myk)
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
Posted file logcat of flame 2.1
Flags: needinfo?(hlu)
Reporter

Comment 27

5 years ago
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.
Flags: needinfo?(hlu) → needinfo?(huayu.li)
(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.
Flags: needinfo?(huayu.li)
You need to log in before you can comment on or make changes to this bug.