829522 - Check for updates never finds app update notifications until after you restart the phone due to hosted+appcache app check for updates failure

Status: VERIFIED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Julien Wajsberg [:julienw]]

STR:
* have a recent enough gecko+gaia so that bug 828939 is fixed
* settings > device information > check now
* wait

expected:
* "checking for updates" should hide after a moment 

actual:
* it's not

By adding logs in the settings app, I verified that we never get the "app.updateStatus" change as we should.

However, if I reboot the phone, I get the update notification for HERE maps (and Firefox Marketplace BTW) which means that app.downloadAvailable is correctly true.

I verified that removing HERE maps fixes this bug. I suspect this is because of the special "preloaded hosted+appcache app" case.

Comment 1

[Jason Smith [:jsmith]]

Yikes. Dug into this with Julien - apparently the maps app doing a failure in check for updates ends up causing a domino effect and causing a failure to check for updates for *all* apps until the phone is restarted and you check for updates again.

Blocker!

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Giving this to baku, but do talk to Fabrice and Julien to figure out who should do what here.

Comment 3

[Jason Smith [:jsmith]]

I've confirmed Julien's findings.

If I try to check for updates with the maps app as being one of the apps were hunting for updates, i won't find any app updates until after a phone restart.

If I try to check for updates without the maps app as being one of the apps were hunting for updates, I will find app updates within 30 seconds without a phone restart.

Comment 4

[Andrea Marchesini [:baku]]

Can it be that your .userconfig doesn't contain VARIANT=user or VARIANT=userdebug ?

Comment 5

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 6

[Jason Smith [:jsmith]]

jst said in theory the patch looked good, but he wanted fabrice to confirm it. I'm going to test the patch shortly.

Comment 7

[Julien Wajsberg [:julienw]]

Comment on attachment 701155 [details] [diff] [review]
patch

sorry I don't follow the logic of this patch at all.

gecko.updateStatus is for system updates.

Comment 8

[Julien Wajsberg [:julienw]]

(In reply to Andrea Marchesini (:baku) from comment #4)
> Can it be that your .userconfig doesn't contain VARIANT=user or
> VARIANT=userdebug ?

no everything works great as soon as we remove the Maps app.

Comment 9

[Jason Smith [:jsmith]]

Comment on attachment 701155 [details] [diff] [review]
patch

Testing this on the phone with the build with this patch, this didn't work for me. I believe Fabrice indicated that the root cause of this bug might actually be on the Gaia side.

Comment 10

[Jason Smith [:jsmith]]

(In reply to Julien Wajsberg [:julienw] from comment #8)
> (In reply to Andrea Marchesini (:baku) from comment #4)
> > Can it be that your .userconfig doesn't contain VARIANT=user or
> > VARIANT=userdebug ?
> 
> no everything works great as soon as we remove the Maps app.

Right. I showed this Andrea directly and he confirmed the issue I was seeing with a fresh flashed build with the old maps app preloading appcache.

Julien - Any ideas on how this could be a potentially be a Gaia issue?

Comment 11

[Julien Wajsberg [:julienw]]

Well, I'm quite sure this is _not_ a Gaia issue, as we don't get the apps.updateStatus change at all. (we get the gecko.updateStatus change though).

I'll work on creating an hosted+appcache app on Monday to try and reproduce this with something else than the Maps app (that is now packaged).

Comment 12

[Julien Wajsberg [:julienw]]

stealing this for today, I'll do some test to try and see what's going on.

Comment 13

[Julien Wajsberg [:julienw]]

Now it doesn't do that with Maps anymore, I think that's because it was converted to a Packaged app.

However I see this with the Slithering app from the marketplace. The Slithering app is a hosted+appcache app.

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 701155 [details] [diff] [review]
patch

Pretty sure this isn't what we'll be doing here per discussion with Fabrice, so obsoleting this patch.

Comment 16

[Julien Wajsberg [:julienw]]

Attachment: debug patch

Comment 17

[Julien Wajsberg [:julienw]]

with this patch, I saw that |updateObserver.observe| is never called back by |updateSvc.checkForUpdate|.

Handing this over to fabrice now.

Comment 18

[[:fabrice] Fabrice Desré]

Attachment: patch

So, we were doing a couple of things wrong:
- trying to load an appcache with the url of the app manifest.
- not setting downloadAvailable properly (using the topic name instead of the event name).

Comment 19

[Gregor Wagner [:gwagner]]

Comment on attachment 701991 [details] [diff] [review]
patch

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

::: dom/apps/src/Webapps.jsm
@@ +21,5 @@
>  Cu.import("resource://gre/modules/SystemMessagePermissionsChecker.jsm");
>  Cu.import("resource://gre/modules/AppDownloadManager.jsm");
>  
>  function debug(aMsg) {
> +  dump("-*-*- Webapps.jsm : " + aMsg + "\n");

Undo!
I will let you decide which debug statements you want to keep in the remaining code.

Comment 20

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1222416519a8
https://hg.mozilla.org/releases/mozilla-b2g18/rev/c0a016137c8d

Comment 21

[Julien Wajsberg [:julienw]]

Fabrice, why didn't I see |updateObserver.observe| being called ? If this is because the AppCache URL was bad then this might be Bug 830044, right ?

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1222416519a8

Comment 23

[Jason Smith [:jsmith]]

Verified on 1/17.