Closed Bug 839071 Opened 9 years ago Closed 9 years ago

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

Categories

(Core Graveyard :: DOM: Apps, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

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

RESOLVED FIXED
mozilla21
blocking-b2g -
Tracking Status
firefox20 --- wontfix
firefox21 --- fixed
b2g18 + fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix

People

(Reporter: julienw, Assigned: ferjm)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
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
Attached 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)
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.
Attachment #712989 - Flags: feedback?(felash)
Duplicate of this bug: 839864
Attached patch v2 (obsolete) — Splinter Review
Attachment #712989 - Attachment is obsolete: true
Attachment #713429 - Flags: review?(fabrice)
Attachment #713429 - Flags: feedback?(felash)
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+
The patch should really do the same changes in the "onlyCheckAppCache" case...
Yes, I have another patch for this.
Attached 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)
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+
Attached 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: 9 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+
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.