Closed
Bug 860681
Opened 12 years ago
Closed 12 years ago
Icons revert to default in 3rd party apps
Categories
(Firefox OS Graveyard :: Gaia::Homescreen, defect, P1)
Tracking
(blocking-b2g:tef+, 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)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
vingtetun
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
Is the bug 828888 the same?
Comment 2•12 years ago
|
||
It seems so but we have preferred to open a new one just in case the root cause is different
Updated•12 years ago
|
blocking-b2g: tef? → tef+
Comment 3•12 years ago
|
||
Do we have STR here? Thanks!
Updated•12 years ago
|
Flags: needinfo?(brg)
Comment 4•12 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
Updated•12 years ago
|
Blocks: b2g-app-updates
Comment 5•12 years ago
|
||
Julien - Any ideas why icons are being lost on the app updates?
Flags: needinfo?(felash)
Keywords: regression
Assignee | ||
Comment 6•12 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.
Flags: needinfo?(felash) → needinfo?(crdlc)
Comment 7•12 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
Flags: needinfo?(crdlc)
Comment 8•12 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
Flags: needinfo?(felash)
Assignee | ||
Comment 9•12 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.
Flags: needinfo?(felash)
Assignee | ||
Comment 10•12 years ago
|
||
Also, the keyword "regression" is quite wrong, I think this bug is there forever.
Reporter | ||
Comment 11•12 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.
Flags: needinfo?(brg)
Updated•12 years ago
|
Assignee: nobody → felash
Comment 12•12 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.
Status: NEW → ASSIGNED
Assignee | ||
Comment 13•12 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...
Assignee | ||
Comment 14•12 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
Comment 15•12 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
Comment 16•12 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
Flags: needinfo?(felash)
Assignee | ||
Comment 17•12 years ago
|
||
Is it possible that our "find best icon" function is not finding an icon at all ?
Flags: needinfo?(felash)
Comment 18•12 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
Updated•12 years ago
|
Whiteboard: See also: 828888 → See also: 828888 [status: no patch, still investigating]
Updated•12 years ago
|
Whiteboard: See also: 828888 [status: no patch, still investigating] → IOT, Spain, See also: 828888 [status: no patch, still investigating]
Updated•12 years ago
|
Whiteboard: IOT, Spain, See also: 828888 [status: no patch, still investigating] → IOT, Spain, See also: 828888 [status: no patch, still investigating][madrid]
Assignee | ||
Comment 19•12 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.
Updated•12 years ago
|
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]
Updated•12 years ago
|
Comment 20•12 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
Attachment #738986 -
Flags: feedback?(brg)
Reporter | ||
Comment 21•12 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 :-)
Flags: needinfo?(khu)
Comment 22•12 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!!
Comment 24•12 years ago
|
||
hi Cristian, would this be considered part of vendor build?
Flags: needinfo?(crdlc)
Comment 25•12 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
Flags: needinfo?(crdlc)
Assignee | ||
Comment 26•12 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
Updated•12 years ago
|
Target Milestone: --- → Madrid (19apr)
Assignee | ||
Comment 27•12 years ago
|
||
found bug 863337 while investigating.
Assignee | ||
Comment 28•12 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.
Assignee | ||
Comment 29•12 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.
Assignee | ||
Comment 30•12 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
Attachment #738986 -
Attachment is obsolete: true
Attachment #738986 -
Flags: feedback?(brg)
Attachment #740436 -
Flags: review?(21)
Comment 31•12 years ago
|
||
Hi, Vivien, could you help to review this? Thanks!
Attachment #740436 -
Flags: review?(21) → review+
Assignee | ||
Comment 32•12 years ago
|
||
master: af3fdcea435e24bb3f760779e734e0c12d299328
I'm resolving the conflict for the branches
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 33•12 years ago
|
||
v1-train: 7ea7918013a012ddf04a1324592b898530488178
status-b2g18:
--- → fixed
Assignee | ||
Comment 34•12 years ago
|
||
v1.0.1: 79f122d568e78bdcc60652d5e695552c4e7d61c1
and the tests for homescreen are green in both branches. Let's keep it that way :-)
status-b2g18-v1.0.1:
--- → fixed
Assignee | ||
Comment 35•12 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.
Comment 36•12 years ago
|
||
Good work, I can see that you mixed both patches, r+ although I am late, you know, I was on holidays.
Comment 37•12 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.
Comment 38•12 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?
Flags: needinfo?(felash)
Assignee | ||
Comment 39•12 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)
Flags: needinfo?(felash)
Comment 40•12 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.
Comment 41•12 years ago
|
||
Putting verifyme to remind myself to do more testing around comment 40.
Keywords: verifyme
Assignee | ||
Comment 42•12 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.
Comment 44•12 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.
Keywords: verifyme
Comment 45•12 years ago
|
||
Marking verified to indicate testing was completed here on 1.01.
Comment 46•12 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
Assignee | ||
Comment 47•12 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.
Comment 48•12 years ago
|
||
I'll file a separate bug about that manifest being incorrect. Good catch though.
Comment 49•12 years ago
|
||
Filed bug 868497 for the manifest issue.
Updated•11 years ago
|
No longer blocks: b2g-apps-v1-next
You need to log in
before you can comment on or make changes to this bug.
Description
•