839071 - We're resetting some states when we shouldn't for hosted+appcache apps

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Julien Wajsberg [:julienw]]

There are a lot of different cases and symptoms but at least the following STR makes this visible:

* install an hosted+appcache app (for example, the app from http://everlong.org/mozilla/selfupdatinghosted/)
* stop the install
* reboot

Expected:
* the icon is in "pending mode"

Actual:
* the icon is in "installed mode"
(this happens if you have the network)

This specific problem is because we reset app.installState to "installed" in "updateHostedApp".

However we do that for other properties too, most notably "downloading", "downloadSize".

Asking for tef+ on that.

Comment 1

[Alex Keybl [:akeybl]]

If the app is in a non-working state, the user will uninstall/reinstall. Not a blocker.

Comment 2

[Jason Smith [:jsmith]]

If there's some user impact we aren't understanding, please clarify.

Comment 3

[Julien Wajsberg [:julienw]]

I'm not exactly sure. I think the appcache will download when the user launches the app. The impact is that he thinks the app is installed whereas it's not and we'll make him download stuff when he thinks he would not. This can be problematic if the user pays his data.

Is it sufficient to block ?

Also, Bug 839068 will kick in in the uninstall/reinstall case.

Comment 4

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

I hit this issue while looking into Bug 838909, so I tried to fix it.

Comment 5

[[:fabrice] Fabrice Desré]

Comment on attachment 712989 [details] [diff] [review]
v1

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

I don't disagree with this patch, but does this really solve this bug?

Comment 6

[Julien Wajsberg [:julienw]]

Comment on attachment 712989 [details] [diff] [review]
v1

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

Right, I was talking about the part in :

http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#1362

until line 1366.

This patch would fix the specific STR, but I think we must also fix lines 1362-1366 because otherwise we might have less visible bugs with installed apps.

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v2

Comment 9

[Julien Wajsberg [:julienw]]

Comment on attachment 713429 [details] [diff] [review]
v2

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

This seems correct with the following change.

I didn't had the time to try it yet (not before tomorrow) so I'm keeping the feedback? flag for now.

::: dom/apps/src/Webapps.jsm
@@ +1342,5 @@
>        }
>  
> +      // The installState would be changed to "updating" once we start
> +      // downloading the offline cache in startOfflineCacheDownload. Until
> +      // then, we keep its previous state.

there is the same in the "onlyCheckAppCache" case

Comment 10

[Julien Wajsberg [:julienw]]

The patch should really do the same changes in the "onlyCheckAppCache" case...

Comment 11

[Fernando Jiménez Moreno [:ferjm]]

Yes, I have another patch for this.

Comment 12

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v3

Comment 13

[Julien Wajsberg [:julienw]]

Comment on attachment 714451 [details] [diff] [review]
v3

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

still didn't have time to test it on the device but this feels good (except the app.downloadAvailable change).

::: dom/apps/src/Webapps.jsm
@@ -1169,5 @@
>      aApp.downloading = true;
> -    let cacheUpdate = aProfileDir
> -      ? updateSvc.scheduleCustomProfileUpdate(appcacheURI, docURI, aProfileDir)
> -      : updateSvc.scheduleAppUpdate(appcacheURI, docURI, aApp.localId, false);
> -

be careful as there is "app.progress = 0" in the current code here, we don't want to remove this. Maybe it should be moved just after aApp.downloading = true.

@@ +1346,5 @@
> +        }, true);
> +
> +        app.name = manifest.name;
> +        app.csp = manifest.csp || "";
> +        app.downloadAvailable = !!manifest.appcache_path;

I'd say you should remove this line.

downloadAvailable will be set later in the observer.

@@ +2822,5 @@
>        mustSave = (app.installState != aStatus);
>        app.installState = aStatus;
>        app.progress = aProgress;
>        if (aStatus == "installed") {
> +        app.updateTime = Date.now();

this doesn't apply directly either in current code but this is easy to merge

Comment 14

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v4

Comment 15

[Fernando Jiménez Moreno [:ferjm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/caa9e261c08c

Comment 16

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 715098 [details] [diff] [review]
v4

[Approval Request Comment]
User impact if declined: Comment 3
Testing completed: Local testing.
Risk to taking this patch (and alternatives if risky): Low risk.
String or UUID changes made by this patch: None

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/caa9e261c08c

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 715098 [details] [diff] [review]
v4

Approving for v1-train uplift, low risk and helps with network data usage.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/bdcc05097102