Beatriz Rodríguez [:brg] Reporter
	Description • 11 years ago

Some applications looses their original icon. 
Bug discovered during certification process in Inari (v1.0.1)
See attachment for more information.

Details of the version:
gaia commit:
f80afc3 Merge pull request #9055 from yurenju/bug-846197-v1.0.1
gecko commit:
8ed9c99 Bug 859552: Fix inari boot image. r=nthomas a=akeybl
RIL version:AU_LINUX_GECKO_ICS_STRAWBERRY_V1.01.00.01.19.046

	Cristian Rodriguez (:crdlc)
	Comment 1 • 11 years ago

Is the bug 828888 the same?

	Daniel Coloma:dcoloma
	Comment 2 • 11 years ago

It seems so but we have preferred to open a new one just in case the root cause is different

	Kai-Chih Hu [:khu]
	Comment 3 • 11 years ago

Do we have STR here? Thanks!

	Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)
	Comment 4 • 11 years ago

did you embed base64 icon in manifest.webapp or just use link? you need to convert icon link to base64 when you need to prelaod apps.

BTW, there is a script can do that.
https://github.com/yurenju/gaia-preload-app

	Jason Smith [:jsmith]
	Comment 5 • 11 years ago

Julien - Any ideas why icons are being lost on the app updates?

	Julien Wajsberg [:julienw] Assignee
	Comment 6 • 11 years ago

I think I've seen that sometimes. An idea is that at the moment when we want to fetch the icon we have no network.

I don't exactly know why we don't transform the icon to their data-url counterpart at install, I think we should. The quick fix is, as Yuren suggested, to use data-url in the manifests instead, but that feels like a workaround, and I wish we worked on this before.

Asking Cristian too, he probably has more information.

	Cristian Rodriguez (:crdlc)
	Comment 7 • 11 years ago

Sorry :( I don't have more information because I didn't implement that part. Although reading a bit the code, I agree with Julien. I guess that the cause could be that at the moment when we want to fetch the icon we haven't network. Do you have logs? Perhaps, could you read something similar to this [1]?

[1] Got an exception when trying to load icon "' + icon + '", falling back to default icon. Exception is:', e

	Cristian Rodriguez (:crdlc)
	Comment 8 • 11 years ago

I was talking with my colleague David from Telefonica and he explains to me that the problem is the following: 

   "You go to marketplace and install Twitter, Wikipedia or whatever, icons appear on the grid perfectly and works fine. But after that, you lose the data connection and maybe in a couple of hours, or more or less, it is very difficult to reproduce, you can lose the icons"

I gonna say a stupid idea but let's go :) Imagine, I reboot the device or maybe the homescreen app dies, so the homescreen is loaded again and it creates the grid. Well, for each app, two callbacks are added (ondownloadapplied and ondownloaderror) and could happen that the platform would send some of them and homescreen tries to update icons without network and fails... In this point of the code [1] we set both callbacks... I guess that we shouldn't receive these callbacks from gecko but here we could have a problem in my honest opinion if something fails on platform

[1] https://github.com/mozilla-b2g/gaia/blob/v1.0.1/apps/homescreen/js/grid.js#L831

It is a crazy idea but I do not have a better one :(

What do you think about it Julien? Thanks a lot my friend

	Julien Wajsberg [:julienw] Assignee
	Comment 9 • 11 years ago

I actually had another thought: what happens when booting, for apps that have an icon with a HTTP URL, if we don't have a network ?

I'd like to test that but I won't be able to test this today as I'll leave soon, but tomorrow definitely if you want.

	Julien Wajsberg [:julienw] Assignee
	Comment 10 • 11 years ago

Also, the keyword "regression" is quite wrong, I think this bug is there forever.

	Beatriz Rodríguez [:brg] Reporter
	Comment 11 • 11 years ago

(In reply to Yuren Ju [:yurenju] from comment #4)
> did you embed base64 icon in manifest.webapp or just use link? you need to
> convert icon link to base64 when you need to prelaod apps.
> 
> BTW, there is a script can do that.
> https://github.com/yurenju/gaia-preload-app

I am using the build as it is coming from the partner. So I cannot help here.

(In reply to Cristian Rodriguez (a tope!) from comment #7)
> Sorry :( I don't have more information because I didn't implement that part.
> Although reading a bit the code, I agree with Julien. I guess that the cause
> could be that at the moment when we want to fetch the icon we haven't
> network. Do you have logs? Perhaps, could you read something similar to this
> [1]?
> 
> [1] Got an exception when trying to load icon "' + icon + '", falling back
> to default icon. Exception is:', e

We(Isabel and me) can try to get a log, but we need to know how :-) Please Cristian, lets try to synch tomorrow if you are available and teach us about how to do it.

	Cristian Rodriguez (:crdlc)
	Comment 12 • 11 years ago

Julien, I guess that the blob is saved on indexedDB so when you reboot the device you can recover it from there. I don't usually have connection on my mobile phone and I restart it thirty times a day and I have all icons.

	Julien Wajsberg [:julienw] Assignee
	Comment 13 • 11 years ago

ok, so, I can't reproduce with what I was suggesting.

The only thing I could try is install the same app, enable lots of logs, and wait...

	Julien Wajsberg [:julienw] Assignee
	Comment 14 • 11 years ago

probably unrelated, but I just saw that the manifest for the twitter webapp [1] changes for each request, and therefore trigger a reinstall at each check for updates.

[1] https://mobile.twitter.com/cache/twitter.webapp

	Cristian Rodriguez (:crdlc)
	Comment 15 • 11 years ago

We are trying to install a new homescreen with a lot of logs in order to know what happens when it fails and reverts icons. Thanks

	Cristian Rodriguez (:crdlc)
	Comment 16 • 11 years ago

I don't understand anything :) I mean, Beatriz showed me a ZTE device reverting an icon for an app called 'Movistar Recomienda' and it defines their icons like data:image/png;base64 

https://raw.github.com/telefonicaid/firefoxos-gaia-spain/master/external-apps/movistarrecomienda/manifest.webapp

Any idea? suggestion? I am lost

	Julien Wajsberg [:julienw] Assignee
	Comment 17 • 11 years ago

Is it possible that our "find best icon" function is not finding an icon at all ?

	Cristian Rodriguez (:crdlc)
	Comment 18 • 11 years ago

It is very weird :) If you do make install-gaia, all icons are ok. I haven't reproduce yet because I guess that we need more time to see icons reverted. Although, if the first time the "find best icon" works fine, I dont't understand why it should fail with the same manifest file. I am getting insane... In a couple of hours, I will try to keep you posted

	Julien Wajsberg [:julienw] Assignee
	Comment 19 • 11 years ago

Here are our current findings :

* this happens only with apps that were pushed through the "external-apps" mechanism
* this happens whether they're hosted or packaged
* this happens also when the icon is defined as a data/url (but maybe it is not when updated with a new manifest)
* some of the app in the telefonica customization repository have wrong metadata. I'm not saying that is the problem, but this might be. I filed Bug 862418 to be stricter in the builds, so that the customization will be done without errors.
* the only phone with the problem that I could find was in "production mode" and I couldn't investigate it. We can't reproduce the problem with an "engineering mode" phone so far.

	Cristian Rodriguez (:crdlc)
	Comment 20 • 11 years ago

Hi Bea, could ZTE people apply this patch to their builds? Then I could know if the patch works fine or not. Thanks

	Beatriz Rodríguez [:brg] Reporter
	Comment 21 • 11 years ago

Kevin, could you please manage if our partner could include this patch in their next release to check if the issue is fixed. It could be great if we can have the new build tomorrow :-)

	Cristian Rodriguez (:crdlc)
	Comment 22 • 11 years ago

Perfect, I was talking with David and someone from ZTE very kind and they will add this patch to new builds in order to investigate if it'll fixes our ugly problem. Thanks!!

	Kai-Chih Hu [:khu]
	Comment 23 • 11 years ago

Thank you, Cristian!

	Wayne Chang [:wchang]
	Comment 24 • 11 years ago

hi Cristian, would this be considered part of vendor build?

	Cristian Rodriguez (:crdlc)
	Comment 25 • 11 years ago

No, maybe I explained wrong, this patch will apply to Gaia with any vendor! But we detected the problem there, but Yulien reproduced the bug on unagi as well

	Julien Wajsberg [:julienw] Assignee
	Comment 26 • 11 years ago

So yes, I managed to exactly reproduce the bug.

STR:
* have a bunch of external hosted apps, you can copy some from [1] and use |make production| to push them to a device
* set up wifi
* check for update
* disable wifi (and 3G if you have it)
* reboot
=> the hosted apps have their icon reverted


Strangely I don't have the same behaviour with an app that I installed through the marketplace or the browser. Also, when we have the network but the icon request has a 404, we also correctly get the old icon. So I'll investigate a little more about this too.

[1] https://github.com/telefonicaid/firefoxos-gaia-spain/tree/master/external-apps

	Julien Wajsberg [:julienw] Assignee
	Comment 27 • 11 years ago

found bug 863337 while investigating.

	Julien Wajsberg [:julienw] Assignee
	Comment 28 • 11 years ago

I think this bug will happen for other apps than external apps, this happens as soon as the icon changes in an update. The icon is not changed until the next reboot (bug 863337), at the next reboot we try to fetch it, and if we don't have a network, we fail. I still don't exactly understand why we don't keep the old icon though.

So, if we fix bug 863337, this bug will be very less likely to happen, because we're supposed to have the network while we update anyway.

There are so many cases to handle here :
* navigator.onLine === false
* if the request fails, should we try again later ? Next reboot ? Next "check for update" cycle ?

So I'd suggest fixing Bug 863337 for tef+ as it is probably easier to do now, smaller patch etc, and put this bug in leo+ instead.

	Julien Wajsberg [:julienw] Assignee
	Comment 29 • 11 years ago

After a good night of sleep, I now think I can fix this bug by ensuring we never reverting to the default icon if we ever had a good one.

Will try this today.

	Julien Wajsberg [:julienw] Assignee
	Comment 30 • 11 years ago

* hopefully finally fix reliably the "ensure panning" test by mocking
  `requestAnimationFrame`
* call `markDirtyState` in all situations ensure that our state is saved in
  IndexedDB
* add tests to ensure the state is saved in these situations. Had to move the
  initialization of some objects to `doInit` so that we can correctly reset the
  object state.
* don't try to load a default icon if we already have an icon
---
 apps/homescreen/js/grid.js                        |   14 +++--
 apps/homescreen/js/page.js                        |   17 +++++-
 apps/homescreen/test/unit/grid_test.js            |   62 ++++++++++++++++++++-
 apps/homescreen/test/unit/mock_hidden_apps.js     |    4 ++
 apps/homescreen/test/unit/mock_home_state.js      |   14 ++++-
 apps/homescreen/test/unit/mock_icon.js            |   21 +++++++
 apps/homescreen/test/unit/mock_manifest_helper.js |    5 ++
 apps/homescreen/test/unit/mock_page.js            |    6 ++
 8 files changed, 132 insertions(+), 11 deletions(-)
 create mode 100644 apps/homescreen/test/unit/mock_hidden_apps.js
 create mode 100644 apps/homescreen/test/unit/mock_icon.js
 create mode 100644 apps/homescreen/test/unit/mock_manifest_helper.js

see also PR https://github.com/mozilla-b2g/gaia/pull/9347

	Kai-Chih Hu [:khu]
	Comment 31 • 11 years ago

Hi, Vivien, could you help to review this? Thanks!

	Julien Wajsberg [:julienw] Assignee
	Comment 32 • 11 years ago

master: af3fdcea435e24bb3f760779e734e0c12d299328

I'm resolving the conflict for the branches

	Julien Wajsberg [:julienw] Assignee
	Comment 33 • 11 years ago

v1-train: 7ea7918013a012ddf04a1324592b898530488178

	Julien Wajsberg [:julienw] Assignee
	Comment 34 • 11 years ago

v1.0.1: 79f122d568e78bdcc60652d5e695552c4e7d61c1

and the tests for homescreen are green in both branches. Let's keep it that way :-)

	Julien Wajsberg [:julienw] Assignee
	Comment 35 • 11 years ago

FTR what I fixed here is that we should never revert to a default icon when we already had a good icon.

However the questions that I ask in comment 28 still hold and I'll file a new bug about that. I think I've seen other problems in the code as well so I'd like to check that before filing this new bug.

	Cristian Rodriguez (:crdlc)
	Comment 36 • 11 years ago

Good work, I can see that you mixed both patches, r+ although I am late, you know, I was on holidays.

	Jason Smith [:jsmith]
	Comment 37 • 11 years ago

Well, I've confirmed I can reproduce the original bug on the 4/23 build with a full blown partner customization with 20 some apps. Now to see if it's fixed on a 4/24 build or later with a full blown partner customization.

	Jason Smith [:jsmith]
	Comment 38 • 11 years ago

On my buri build for 4/26 with a full partner customization apps, I didn't reproduce this bug with this patch, so that's good. However - with inari, I was able to reproduce this bug with two of the apps in the partner customization with this patch (I can send you the details if need be of what apps they are over email).

Julien - Any ideas why?

	Julien Wajsberg [:julienw] Assignee
	Comment 39 • 11 years ago

No idea, other that you may have an older build, or the apps never had a non-default icon to start with. I can check if you send me the examples by email (or if they are on an external repository just put the url here)

	Jason Smith [:jsmith]
	Comment 40 • 11 years ago

(In reply to Julien Wajsberg [:julienw] from comment #39)
> No idea, other that you may have an older build, or the apps never had a
> non-default icon to start with. I can check if you send me the examples by
> email (or if they are on an external repository just put the url here)

External apps repository that had the bug:

https://github.com/telefonicaid/firefoxos-gaia-spain/tree/master/external-apps

Apps that ended up with no icon:

https://github.com/telefonicaid/firefoxos-gaia-spain/tree/master/external-apps/recomienda

https://github.com/telefonicaid/firefoxos-gaia-spain/tree/master/external-apps/tuenti

I'm going to try to retest with some of these partner customizations if I can reproduce again.

	Jason Smith [:jsmith]
	Comment 41 • 11 years ago

Putting verifyme to remind myself to do more testing around comment 40.

	Julien Wajsberg [:julienw] Assignee
	Comment 42 • 11 years ago

I see no reason why those would not work. I mean, their configuration is still borked, but the icons should not revert if they were correct in the first time. I think.

	Jason Smith [:jsmith]
	Comment 44 • 11 years ago

I did a long study with multiple phones with multiple partner customizations with builds later than 4/24 - this is certainly harder to reproduce now, but it's actually still possible. I'm filing a followup here.

	Jason Smith [:jsmith]
	Comment 45 • 11 years ago

Marking verified to indicate testing was completed here on 1.01.

	Cristian Rodriguez (:crdlc)
	Comment 46 • 11 years ago

Hi, maybe, I am saying some stupid thing, but reading this manifest:

http://www.movistarrecomienda.es/app/manifest.webapp

I can view that the "orientation" property belongs to "permissions" object. I don't guess that it is the problem, but is it good defined?

Thanks

	Julien Wajsberg [:julienw] Assignee
	Comment 47 • 11 years ago

Cristian: yep you're right, I saw that too, and it could actually make the update fail (we have an error in the console at least). We still should not revert the icon but this might be the reason.

	Jason Smith [:jsmith]
	Comment 48 • 11 years ago

I'll file a separate bug about that manifest being incorrect. Good catch though.

	Jason Smith [:jsmith]
	Comment 49 • 11 years ago

Filed bug 868497 for the manifest issue.