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

RESOLVED FIXED in Firefox 21

Status

defect
RESOLVED FIXED
7 years ago
2 years ago

People

(Reporter: julienw, Assigned: ferjm)

Tracking

Trunk
mozilla21
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

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

Details

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

7 years ago
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.
If the app is in a non-working state, the user will uninstall/reinstall. Not a blocker.
blocking-b2g: tef? → -
tracking-b2g18: --- → +
If there's some user impact we aren't understanding, please clarify.
Reporter

Comment 3

7 years ago
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.
Assignee: nobody → ferjmoreno
Posted patch v1 (obsolete) — Splinter Review
I hit this issue while looking into Bug 838909, so I tried to fix it.
Attachment #712989 - Flags: review?(fabrice)
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?
Attachment #712989 - Flags: review?(fabrice) → feedback?(felash)
Reporter

Comment 6

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

Updated

6 years ago
Attachment #712989 - Flags: feedback?(felash)
Reporter

Updated

6 years ago
Duplicate of this bug: 839864
Posted patch v2 (obsolete) — Splinter Review
Attachment #712989 - Attachment is obsolete: true
Attachment #713429 - Flags: review?(fabrice)
Attachment #713429 - Flags: feedback?(felash)
Reporter

Comment 9

6 years ago
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
Attachment #713429 - Flags: review?(fabrice) → review+
Reporter

Comment 10

6 years ago
The patch should really do the same changes in the "onlyCheckAppCache" case...
Yes, I have another patch for this.
Posted patch v3 (obsolete) — Splinter Review
Attachment #713429 - Attachment is obsolete: true
Attachment #713429 - Flags: feedback?(felash)
Attachment #714451 - Flags: review?(fabrice)
Attachment #714451 - Flags: feedback?(felash)
Reporter

Comment 13

6 years ago
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
Attachment #714451 - Flags: feedback?(felash) → feedback+
Posted patch v4Splinter Review
Attachment #714451 - Attachment is obsolete: true
Attachment #714451 - Flags: review?(fabrice)
Attachment #715098 - Flags: review?(fabrice)
Attachment #715098 - Flags: review?(fabrice) → review+
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
Attachment #715098 - Flags: approval-mozilla-b2g18?
https://hg.mozilla.org/mozilla-central/rev/caa9e261c08c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Comment on attachment 715098 [details] [diff] [review]
v4

Approving for v1-train uplift, low risk and helps with network data usage.
Attachment #715098 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+

Updated

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