Closed
Bug 836909
Opened 12 years ago
Closed 12 years ago
If the caller to download tries to start a download but we have nothing to download, throw an error
Categories
(Core Graveyard :: DOM: Apps, defect)
Tracking
(blocking-b2g:-, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed, b2g18-v1.0.0 fixed)
People
(Reporter: jsmith, Assigned: fabrice)
References
Details
(Whiteboard: [qa-])
Attachments
(1 file)
(deleted),
patch
|
ferjm
:
review+
overholt
:
approval-mozilla-b2g18+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
And...this regression has returned. :(.
blocking-b2g: --- → tef?
Keywords: dataloss,
regression
Reporter | ||
Updated•12 years ago
|
Blocks: b2g-app-updates
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → fabrice
Assignee | ||
Comment 2•12 years ago
|
||
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•12 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?
Comment 4•12 years ago
|
||
(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•12 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•12 years ago
|
||
Holding off on triage until I get more info.
blocking-b2g: tef? → ---
Assignee | ||
Comment 7•12 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•12 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 9•12 years ago
|
||
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•12 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•12 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.
Comment 12•12 years ago
|
||
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•12 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...
Comment 14•12 years ago
|
||
Yep, I meant that we need your patch even if we have one in Gaia :)
Comment 15•12 years ago
|
||
I filed Bug 837193.
Assignee | ||
Comment 16•12 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•12 years ago
|
||
Comment 18•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Reporter | ||
Updated•12 years ago
|
Whiteboard: [qa-]
Updated•12 years ago
|
Attachment #708899 -
Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Comment 19•12 years ago
|
||
status-b2g18:
--- → fixed
status-b2g18-v1.0.0:
--- → affected
status-firefox19:
--- → wontfix
status-firefox20:
--- → wontfix
status-firefox21:
--- → fixed
Comment 20•12 years ago
|
||
Does this need to land on v1.0.0, too?
Comment 23•12 years ago
|
||
Updated•12 years ago
|
blocking-b2g: tef+ → -
Updated•7 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•