Closed Bug 860681 Opened 7 years ago Closed 7 years ago

Icons revert to default in 3rd party apps

Categories

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

x86
macOS
defect

Tracking

(blocking-b2g:tef+, b2g18 fixed, b2g18-v1.0.1 verified)

RESOLVED FIXED
1.0.1 Madrid (19apr)
blocking-b2g tef+
Tracking Status
b2g18 --- fixed
b2g18-v1.0.1 --- verified

People

(Reporter: brg, Assigned: julienw)

References

Details

(Keywords: regression, Whiteboard: IOT, Spain, See also: 828888 [status: no patch, still investigating][madrid] [apps watch list])

Attachments

(2 files, 1 obsolete file)

Attached image 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
Is the bug 828888 the same?
It seems so but we have preferred to open a new one just in case the root cause is different
See Also: → 828888
blocking-b2g: tef? → tef+
Do we have STR here? Thanks!
Flags: needinfo?(brg)
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
Julien - Any ideas why icons are being lost on the app updates?
Flags: needinfo?(felash)
Keywords: regression
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.
Flags: needinfo?(felash) → needinfo?(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
Flags: needinfo?(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
Flags: needinfo?(felash)
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.
Flags: needinfo?(felash)
Also, the keyword "regression" is quite wrong, I think this bug is there forever.
(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.
Flags: needinfo?(brg)
Assignee: nobody → felash
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.
Status: NEW → ASSIGNED
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...
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
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
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
Flags: needinfo?(felash)
Is it possible that our "find best icon" function is not finding an icon at all ?
Flags: needinfo?(felash)
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
Whiteboard: See also: 828888 → See also: 828888 [status: no patch, still investigating]
Whiteboard: See also: 828888 [status: no patch, still investigating] → IOT, Spain, See also: 828888 [status: no patch, still investigating]
Whiteboard: IOT, Spain, See also: 828888 [status: no patch, still investigating] → IOT, Spain, See also: 828888 [status: no patch, still investigating][madrid]
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.
Whiteboard: IOT, Spain, See also: 828888 [status: no patch, still investigating][madrid] → IOT, Spain, See also: 828888 [status: no patch, still investigating][madrid] [apps watch list]
Blocks: b2g-apps-v1-next
No longer blocks: b2g-app-updates
Attached patch Patch v1 (obsolete) — Splinter Review
Hi Bea, could ZTE people apply this patch to their builds? Then I could know if the patch works fine or not. Thanks
Attachment #738986 - Flags: feedback?(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 :-)
Flags: needinfo?(khu)
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!!
Thank you, Cristian!
Flags: needinfo?(khu)
hi Cristian, would this be considered part of vendor build?
Flags: needinfo?(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
Flags: needinfo?(crdlc)
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
Target Milestone: --- → Madrid (19apr)
found bug 863337 while investigating.
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.
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.
Attached patch patch v2Splinter Review
* 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
Attachment #738986 - Attachment is obsolete: true
Attachment #738986 - Flags: feedback?(brg)
Attachment #740436 - Flags: review?(21)
Hi, Vivien, could you help to review this? Thanks!
master: af3fdcea435e24bb3f760779e734e0c12d299328

I'm resolving the conflict for the branches
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
v1-train: 7ea7918013a012ddf04a1324592b898530488178
v1.0.1: 79f122d568e78bdcc60652d5e695552c4e7d61c1

and the tests for homescreen are green in both branches. Let's keep it that way :-)
Keywords: verifyme
QA Contact: jsmith
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.
Good work, I can see that you mixed both patches, r+ although I am late, you know, I was on holidays.
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.
Keywords: verifyme
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?
Flags: needinfo?(felash)
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)
Flags: needinfo?(felash)
(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.
Putting verifyme to remind myself to do more testing around comment 40.
Keywords: verifyme
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.
Duplicate of this bug: 867915
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.
Keywords: verifyme
Depends on: 868258
Marking verified to indicate testing was completed here on 1.01.
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
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.
I'll file a separate bug about that manifest being incorrect. Good catch though.
Filed bug 868497 for the manifest issue.
No longer blocks: b2g-apps-v1-next
You need to log in before you can comment on or make changes to this bug.