860681 - Icons revert to default in 3rd party apps

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Homescreen, defect, P1)

Description

[Beatriz Rodríguez [:brg]]

Attachment: screen shot

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

Comment 1

[Cristian Rodriguez (:crdlc)]

Is the bug 828888 the same?

Comment 2

[Daniel Coloma:dcoloma]

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

Comment 3

[Kai-Chih Hu [:khu]]

Do we have STR here? Thanks!

Comment 4

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

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

Comment 5

[Jason Smith [:jsmith]]

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

Comment 6

[Julien Wajsberg [:julienw]]

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.

Comment 7

[Cristian Rodriguez (:crdlc)]

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

Comment 8

[Cristian Rodriguez (:crdlc)]

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

Comment 9

[Julien Wajsberg [:julienw]]

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.

Comment 10

[Julien Wajsberg [:julienw]]

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

Comment 11

[Beatriz Rodríguez [:brg]]

(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.

Comment 12

[Cristian Rodriguez (:crdlc)]

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.

Comment 13

[Julien Wajsberg [:julienw]]

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...

Comment 14

[Julien Wajsberg [:julienw]]

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

Comment 15

[Cristian Rodriguez (:crdlc)]

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

Comment 16

[Cristian Rodriguez (:crdlc)]

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

Comment 17

[Julien Wajsberg [:julienw]]

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

Comment 18

[Cristian Rodriguez (:crdlc)]

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

Comment 19

[Julien Wajsberg [:julienw]]

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.

Comment 20

[Cristian Rodriguez (:crdlc)]

Attachment: Patch v1

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

Comment 21

[Beatriz Rodríguez [:brg]]

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 :-)

Comment 22

[Cristian Rodriguez (:crdlc)]

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!!

Comment 23

[Kai-Chih Hu [:khu]]

Thank you, Cristian!

Comment 24

[Wayne Chang [:wchang]]

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

Comment 25

[Cristian Rodriguez (:crdlc)]

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

Comment 26

[Julien Wajsberg [:julienw]]

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

Comment 27

[Julien Wajsberg [:julienw]]

found bug 863337 while investigating.

Comment 28

[Julien Wajsberg [:julienw]]

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.

Comment 29

[Julien Wajsberg [:julienw]]

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.

Comment 30

[Julien Wajsberg [:julienw]]

Attachment: patch v2

* 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

Comment 31

[Kai-Chih Hu [:khu]]

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

Comment 32

[Julien Wajsberg [:julienw]]

master: af3fdcea435e24bb3f760779e734e0c12d299328

I'm resolving the conflict for the branches

Comment 33

[Julien Wajsberg [:julienw]]

v1-train: 7ea7918013a012ddf04a1324592b898530488178

Comment 34

[Julien Wajsberg [:julienw]]

v1.0.1: 79f122d568e78bdcc60652d5e695552c4e7d61c1

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

Comment 35

[Julien Wajsberg [:julienw]]

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.

Comment 36

[Cristian Rodriguez (:crdlc)]

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

Comment 37

[Jason Smith [:jsmith]]

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.

Comment 38

[Jason Smith [:jsmith]]

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?

Comment 39

[Julien Wajsberg [:julienw]]

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)

Comment 40

[Jason Smith [:jsmith]]

(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.

Comment 41

[Jason Smith [:jsmith]]

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

Comment 42

[Julien Wajsberg [:julienw]]

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.

Comment 44

[Jason Smith [:jsmith]]

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.

Comment 45

[Jason Smith [:jsmith]]

Marking verified to indicate testing was completed here on 1.01.

Comment 46

[Cristian Rodriguez (:crdlc)]

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

Comment 47

[Julien Wajsberg [:julienw]]

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.

Comment 48

[Jason Smith [:jsmith]]

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

Comment 49

[Jason Smith [:jsmith]]

Filed bug 868497 for the manifest issue.