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

RESOLVED FIXED in Firefox 21

Status

Core Graveyard
DOM: Apps
RESOLVED FIXED
5 years ago
2 months ago

People

(Reporter: jsmith, Assigned: fabrice)

Tracking

Trunk
mozilla21
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:-, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed, b2g18-v1.0.0 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment)

(Reporter)

Description

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

Comment 1

5 years ago
And...this regression has returned. :(.
blocking-b2g: --- → tef?
Keywords: dataloss, regression
(Reporter)

Updated

5 years ago
Blocks: 728081
(Assignee)

Updated

5 years ago
Assignee: nobody → fabrice
(Assignee)

Comment 2

5 years ago
Created attachment 708899 [details] [diff] [review]
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.
Attachment #708899 - Flags: review?(ferjmoreno)
Flags: needinfo?(felash)
(Reporter)

Comment 3

5 years ago
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?
(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.
Flags: needinfo?(felash)
(Reporter)

Comment 5

5 years ago
I'm going to dig into this a bit more on today's build for comparison.
Keywords: qawanted
Summary: Trying and failing to update a 3rd party app update due to an error - cannot launch the app anymore → Trying and failing to update for the maps packaged app due to an error - cannot launch the app anymore
(Reporter)

Comment 6

5 years ago
Holding off on triage until I get more info.
blocking-b2g: tef? → ---
(Assignee)

Comment 7

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

Comment 8

5 years ago
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.
blocking-b2g: --- → tef?
Keywords: qawanted
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
Attachment #708899 - Flags: review?(ferjmoreno) → review+
(Reporter)

Comment 10

5 years ago
Turns out the root cause is being fixed bug 836859. I'll morph the bug to what's being worked on here.
blocking-b2g: tef? → ---
Keywords: dataloss, regression
Summary: Trying and failing to update for the maps packaged app due to an error - cannot launch the app anymore → If the caller to download tries to start a download but we have nothing to download, throw an error
(Assignee)

Comment 11

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

Comment 13

5 years ago
(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...
Yep, I meant that we need your patch even if we have one in Gaia :)
I filed Bug 837193.
(Assignee)

Comment 16

5 years ago
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.
Attachment #708899 - Flags: approval-mozilla-b2g18?
(Assignee)

Comment 17

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b97c1f76275
https://hg.mozilla.org/mozilla-central/rev/7b97c1f76275
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
(Reporter)

Updated

5 years ago
Whiteboard: [qa-]
Attachment #708899 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
https://hg.mozilla.org/releases/mozilla-b2g18/rev/239dbf0b6dbd
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → affected
status-firefox19: --- → wontfix
status-firefox20: --- → wontfix
status-firefox21: --- → fixed
Does this need to land on v1.0.0, too?
yes please
blocking-b2g: --- → tef?
Low risk, go ahead with uplift.
blocking-b2g: tef? → tef+
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_0/rev/309ce5a54fb0
status-b2g18-v1.0.0: affected → fixed

Updated

5 years ago
blocking-b2g: tef+ → -

Updated

2 months ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.