836909 - If the caller to download tries to start a download but we have nothing to download, throw an error

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Jason Smith [:jsmith]]

Build: B2G 1/31/2013
Device: Unagi

Steps:

1. Install a packaged app
2. Update it on the server to something that will cause an error (e.g. make the app certified on update)
3. Check for updates
4. Apply the update and fail

Expected:

The old app should be able to still launch.

Actual:

The old app can no longer launch. We enter the limbo state of where there's a generic icon on the homescreen that will try to restart the download if you click it.

Comment 1

[Jason Smith [:jsmith]]

And...this regression has returned. :(.

Comment 2

[[:fabrice] Fabrice Desré]

Attachment: defense in depth patch

I can't reproduce Jason's error, but what I'm seeing is that gaia is constantly showing an "update available" notification after the first update fails.

Gaia is calling download() while downloadAvailable is false, which makes no sense but we were not guarding against that on the gecko side. This patch adds this defense, and sends back a DOWNLOAD_CANCELED in these cases.

I wonder if gaia is confused by the INVALID_SECURITY_LEVEL error during the update.

Comment 3

[Jason Smith [:jsmith]]

For context - I tested this with the maps packaged app. So I'm wondering if the failure in the other bug causes this issue. Maybe it's not a general problem for all apps?

Comment 4

[Julien Wajsberg [:julienw]]

(In reply to Jason Smith [:jsmith] from comment #0)

> The old app can no longer launch. We enter the limbo state of where there's
> a generic icon on the homescreen that will try to restart the download if
> you click it.

Really, this should not happen for updates at all. This should happen only for installs.

To me, this can only happen if the App state is bad on the gecko side: installState was reset to "pending". Could you check this Jason (in the usual webapps.json file) ?

However, I've seen that the update manager in System app might have a bug because it merely checks for "installState === 'installed'" and not 'updating', but this is not related to this bug. It would be nice that we fix this in the process though.

(In reply to Fabrice Desré [:fabrice] from comment #2)
> Gaia is calling download() while downloadAvailable is false, which makes no
> sense but we were not guarding against that on the gecko side. This patch
> adds this defense, and sends back a DOWNLOAD_CANCELED in these cases.
> 
> I wonder if gaia is confused by the INVALID_SECURITY_LEVEL error during the
> update.

The System app assumes that if there was a download available, and there is an error, then a download is still available. So we're not removing the notification and we're not even checking this property again.

If this assumption is false then yes, there is a bug in the System app.

Comment 5

[Jason Smith [:jsmith]]

I'm going to dig into this a bit more on today's build for comparison.

Comment 6

[Jason Smith [:jsmith]]

Holding off on triage until I get more info.

Comment 7

[[:fabrice] Fabrice Desré]

(In reply to Julien Wajsberg [:julienw] from comment #4)

> 
> The System app assumes that if there was a download available, and there is
> an error, then a download is still available. So we're not removing the
> notification and we're not even checking this property again.
> 
> If this assumption is false then yes, there is a bug in the System app.

Pretty much like gevcko should not assume that the content side will not do silly calls, gaia should not assume anything and always check the app state after getting an event. Can you check if this is what's happening here?

Comment 8

[Jason Smith [:jsmith]]

Definitely reproduces for me with the maps packaged app.

  "m.here.com": {
    "origin": "app://m.here.com",
    "installOrigin": "https://marketplace.firefox.com",
    "receipt": null,
    "installTime": 132333986000,
    "manifestURL": "https://marketplace.firefox.com/app/7eccfd71-2765-458d-983f-
078580b46a11/manifest.webapp",
    "removable": true,
    "localId": 1021,
    "etag": null,
    "packageEtag": null,
    "appStatus": 1,
    "basePath": "/data/local/webapps",
    "id": "m.here.com",
    "name": "HERE Maps",
    "csp": "",
    "lastCheckedUpdate": 1359740269723,
    "downloadAvailable": false,
    "downloadSize": 3328462,
    "retryingDownload": true,
    "downloading": false,
    "installState": "pending",
    "progress": 2688
  }

That's the state of the app after the failed download.

Comment 9

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 708899 [details] [diff] [review]
defense in depth patch

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

FWIW I am not able to reproduce this issue neither.

::: dom/apps/src/Webapps.jsm
@@ +945,4 @@
>      debug("startDownload for " + aManifestURL);
>      let id = this._appIdForManifestURL(aManifestURL);
>      let app = this.webapps[id];
>      if (!app) {

This is not related to this issue, but should we also send an error if no app is found instead of silently return?

@@ +954,5 @@
> +    // download, send an error.
> +    if (!app.downloadAvailable) {
> +      this.broadcastMessage("Webapps:PackageEvent",
> +                            { type: "canceled",
> +                              manifestURL:  app.manifestURL,

nit: extra space

Comment 10

[Jason Smith [:jsmith]]

Turns out the root cause is being fixed bug 836859. I'll morph the bug to what's being worked on here.

Comment 11

[[:fabrice] Fabrice Desré]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #9)
> Comment on attachment 708899 [details] [diff] [review]

> 
> This is not related to this issue, but should we also send an error if no
> app is found instead of silently return?

I thought about that, we since we have no app, we can't fire an event on an app, and the current method signature is returning void... We'll need some api change to do that I think.

Comment 12

[Julien Wajsberg [:julienw]]

I believe we can do also a patch on Gaia, to not show the notification. I'll file a new bug for that.

However, the patch must be in Gecko too, because |download| might be called by content code too.

Comment 13

[[:fabrice] Fabrice Desré]

(In reply to Julien Wajsberg [:julienw] from comment #12)
> I believe we can do also a patch on Gaia, to not show the notification. I'll
> file a new bug for that.
> 
> However, the patch must be in Gecko too, because |download| might be called
> by content code too.

this is what this patch is doing...

Comment 14

[Julien Wajsberg [:julienw]]

Yep, I meant that we need your patch even if we have one in Gaia :)

Comment 15

[Julien Wajsberg [:julienw]]

I filed Bug 837193.

Comment 16

[[:fabrice] Fabrice Desré]

Comment on attachment 708899 [details] [diff] [review]
defense in depth patch

[Approval Request Comment]
This patch offer defense in depth against some misuse of the mozApps api that could happen in any app. It's not risky at all.

Comment 17

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b97c1f76275

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7b97c1f76275

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/239dbf0b6dbd

Comment 20

[Andrew Overholt [:overholt]]

Does this need to land on v1.0.0, too?

Comment 21

[Julien Wajsberg [:julienw]]

yes please

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

Low risk, go ahead with uplift.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_0/rev/309ce5a54fb0