If the homescreen fails to fetch app icons, it will replace existing icons with the placeholder 'rocket' icon

VERIFIED FIXED in Firefox OS v2.2

Status

Firefox OS
Gaia::Homescreen
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: cwiiis, Assigned: kgrandon)

Tracking

({polish})

unspecified
2.2 S12 (15may)
All
Gonk (Firefox OS)
polish

Firefox Tracking Flags

(b2g-v2.1 unaffected, b2g-v2.1S unaffected, b2g-v2.2 verified, b2g-master verified)

Details

(Whiteboard: [systemsfe])

Attachments

(5 attachments)

(Reporter)

Description

3 years ago
If the homescreen decides to update app icons (for bookmark icons) and the icon fetching fails, it will replace whatever's there with the placeholder rocket icon, even if an existing, valid icon was previously there. This icon will not be replaced until restarting the homescreen with a working internet connection.

This is especially evident if you restart the phone while a captive-portal wifi network is available.

As such, nominating for blocking: Not a hugely serious issue, but no obvious work-around for the user, and it makes collapsed icons useless (there's no way to identify them). As you're more likely to collapse a group of bookmarks than anything else, it's quite easy to run into this issue in a way that's quite annoying.

I expect that this would be an easy fix (just don't replace an icon with nothing).
Whiteboard: [systemsfe]
confirmed blocker in team triage
blocking-b2g: 2.2? → 2.2+
Assignee: nobody → kgrandon
(Assignee)

Comment 2

3 years ago
Hmm, it actually seems like this is the intended behavior: https://github.com/mozilla-b2g/gaia/blob/master/shared/elements/gaia_grid/js/items/grid_item.js#L461

I wonder if this is some kind of regression? Looking for a good way to reproduce this now..
(Assignee)

Comment 3

3 years ago
I'm unable to reproduce at my house with what I've tried so far. Adding qawanted to see if they can reproduce and to hopefully get a screenshot/video if they can.
Keywords: qawanted
(Assignee)

Comment 4

3 years ago
Re-nominating as I'm not 100% sure of the STR or end outcome. Recommend getting branch checks and a clear impact before deciding on blocking. I'm sure something here can go wrong, but I'm not sure what.

To see it maybe it needs to be some combination of home screen state along with captive portal?
blocking-b2g: 2.2+ → 2.2?
Keywords: qawanted → steps-wanted
(Reporter)

Comment 5

3 years ago
(In reply to Kevin Grandon :kgrandon from comment #2)
> Hmm, it actually seems like this is the intended behavior:
> https://github.com/mozilla-b2g/gaia/blob/master/shared/elements/gaia_grid/js/
> items/grid_item.js#L461

Perhaps the icon cache is being invalidated before the newly fetched icon itself is validated?

As for STR, the exact steps for someone in London:

1- Go to most any London underground station
2- Add the free underground wifi network to remembered networks
3- Make sure wifi is on
4- Make sure you have at least one bookmark on your homescreen
5- Restart phone

The London underground network has a captive portal that intercepts all http requests and redirects to an ad page, where after pressing a button, everything behaves normally for a certain amount of time.

I expect this can be replicated in various hotels/airports, and also on some mobile networks when running out of credit.
(Reporter)

Comment 6

3 years ago
So, I think what I suggest is correct, a new background image isn't verified before replacing the existing cached image. You can see this here: https://github.com/mozilla-b2g/gaia/blob/master/shared/elements/gaia_grid/js/items/grid_item.js#L338

We should be doing something like is done here: 
https://github.com/mozilla-b2g/gaia/blob/master/shared/elements/gaia_grid/js/items/grid_item.js#L322 before setting decoratedIconBlob.

As it is, I expect we're actually hitting this: https://github.com/mozilla-b2g/gaia/blob/master/shared/elements/gaia_grid/js/items/grid_item.js#L271

If this is intended behaviour, I would suggest that it is terrible intended behaviour :)
(Reporter)

Comment 7

3 years ago
n?me so I can follow up on this when I'm back if necessary.
Flags: needinfo?(chrislord.net)
Created attachment 8588365 [details] [review]
[gaia] KevinGrandon:bug_1150495_cached_icon_overwrite > mozilla-b2g:master
(Assignee)

Comment 9

3 years ago
Thanks for doing that investigation Chris. I think a patch like this is likely to fix it, though I haven't tested it. My next step is going to be to create a HTTP server to see if I can emulate what a captive portal site is doing in order to reproduce this.
See comment 5, although we can't verify the steps in our office.
Flags: needinfo?(ktucker)
Keywords: steps-wanted
Flags: needinfo?(ktucker)
Not blocking until we find out if this is a regression.
Naoki, do you have any ideas how to test this?
blocking-b2g: 2.2? → ---
Flags: needinfo?(nhirata.bugzilla)
(In reply to Gregor Wagner [:gwagner] from comment #11)
> Not blocking until we find out if this is a regression.
> Naoki, do you have any ideas how to test this?

I mean finding out if this is a regression.
(Assignee)

Comment 13

3 years ago
Comment on attachment 8588365 [details] [review]
[gaia] KevinGrandon:bug_1150495_cached_icon_overwrite > mozilla-b2g:master

So I'm not sure if this fixes it, as I haven't tested it. I'm also having trouble getting an integration test into this code path. Chris - up to you if you want to test/land this code. Thanks!
Attachment #8588365 - Flags: review?(chrislord.net)
The captive portal I have set up at home, doesn't show this issue.
I would probably have to try some other place.  Anyone want to send me to london?  XD [ kidding ]

I tried a different approach of using a proxy server as well.  Still no luck.  Once I repro the issue, I'll be able to figure out if this was a regression or not.
I figured out a way to test this.
Create my own web page and change up the favicon.  Duh.
http://people.mozilla.org/~nhirata/web_icon_test/

I think it's triggered from bug 1059470.  

In 2.1, web icons don't seem to update if there is a better on loaded or if it's suppose to update.  It seems to be stored in a database.  Rebooting doesn't seem to do any harm.  Somewhere along the way : Bug 908926 "[browser] icon uri does not update " got closed.

In 2.2, is the same.  the favicon doesn't update on it's own once the bookmark is created.  You have to delete the favicon and then go to the web site and add it in order for the favicon to update.
(side note: people.mozilla.org apparantly has a default favicon setup)

So chances are a page using a proxy server that didn't pull down the favicon got bookmarked and it stayed that way from what I can tell.  So no.  It's not a regression.  It's a bug that got closed.
So much for my trip to london.

Oh hrm.  It looks like there used to be a day expiration before the browser was moved to the system app, apparantly TV is using the browser db and having it separate from the OS?
https://github.com/benfrancis/gaia/blob/726d68d5a78a1a77f4d6191ab092bb1d4b65a587/apps/browser/js/places.js#L112
https://github.com/mozilla-b2g/gaia/blob/f34ce82a840ad3c0aed3bfff18517b3f6a0eb37f/tv_apps/browser/js/browser_db.js#L23

I will have to dig a little bit more in my testing to see what's going on now.

Kevin, should we prevent the places.db from updating the icon as well on error?  Because wouldn't the history be affected as well?
Flags: needinfo?(kgrandon)
(Assignee)

Comment 17

3 years ago
(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #16)
> Kevin, should we prevent the places.db from updating the icon as well on
> error?  Because wouldn't the history be affected as well?

It's possible, though it might already be handling it depending on where it's used. We would need to look into that further, and this is probably a different bug.
Flags: needinfo?(kgrandon)
(Reporter)

Comment 18

3 years ago
Just a note, I've finally gotten round to updating my dogfood phone, so I'll report back after the next time I'm on the tube (this may not be until Monday).
Flags: needinfo?(chrislord.net)
(Reporter)

Comment 19

3 years ago
Comment on attachment 8588365 [details] [review]
[gaia] KevinGrandon:bug_1150495_cached_icon_overwrite > mozilla-b2g:master

I've been running with this patch now and I tried rebooting my phone a couple of times on the tube - I couldn't replicate the issues I was having before, so I guess this is good to go?
Attachment #8588365 - Flags: review?(chrislord.net) → review+
(Assignee)

Comment 20

3 years ago
Thanks for testing it, and for the reveiew. Adding checkin-needed to get this landed.

I tried for a long time to write an integration test for this, but was unfortunately unable to do so - I couldn't find the right condition for this to happen.

We could and should probably write unit test coverage for this, and if I have time I will do so in a follow-up, or someone else can.
Status: NEW → ASSIGNED
Flags: needinfo?(nhirata.bugzilla)
Keywords: checkin-needed

Updated

3 years ago
Keywords: checkin-needed
http://docs.taskcluster.net/tools/task-graph-inspector/#R3GHoyZISXmmJuO7DAPDYA

The pull request failed to pass integration tests. It could not be landed, please try again.
(Assignee)

Comment 22

3 years ago
Rebasing and trying again.
Keywords: checkin-needed

Updated

3 years ago
Keywords: checkin-needed

Updated

3 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Reporter)

Comment 24

3 years ago
I assume we want to get this uplifted to 2.2 as well?
(Assignee)

Comment 25

3 years ago
Comment on attachment 8588365 [details] [review]
[gaia] KevinGrandon:bug_1150495_cached_icon_overwrite > mozilla-b2g:master

(In reply to Chris Lord [:cwiiis] from comment #24)
> I assume we want to get this uplifted to 2.2 as well?

Yeah, I think it's worth it.

For the approver - please see the original description as to why, but here's the for also:

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Likely there since home screen implementation, but tricky to reproduce.
[User impact] if declined: Occasionally poor UX and a frustrating experience when using the home screen (icons will disappear)
[Testing completed]: Clord has been dogfooding for a few weeks and confirm that it works.
[Risk to taking this patch] (and alternatives if risky): Low risk - small patch and contained within a single function.
[String changes made]: None.
Attachment #8588365 - Flags: approval-gaia-v2.2?(bbajaj)

Comment 26

3 years ago
Norry,
Please verify this on master to see whether this is fixed.

Also please qawanted for flame 2.1 to see whether this issue is also happen there as regression issue.
Flags: needinfo?(fan.luo)
Keywords: verifyme
Comment hidden (obsolete)
(Reporter)

Comment 28

3 years ago
(In reply to Norry.L.F from comment #27)

What you describe is not what this bug is about, you may want to file a new bug. This bug is about existing bookmark icons being replaced with the rocket icon when the homescreen restarts on a captive portal network (or other similar situation).

What you describe sounds like bookmark icons not updating at all. If that is indeed happening, it would be good to have that filed as a separate bug so that QA can verify and we can see what broke it (if it wasn't the patch on this bug).
Flags: needinfo?(chrislord.net)

Comment 29

3 years ago
Hi Norry,
Can you check whether you got any website icon at the beginning while the network is good?
Thanks
Flags: needinfo?(fan.luo)
Created attachment 8603995 [details]
verify_v2.1&3.0.mp4

Thanks Chris,Josh.

The bug has been verified successfully on Flame v3.0 with the STR on comment 5&comment 28, and I can't repro it on flame v2.1 either, now the actual result is: the existing bookmark icon shows normally and will not be replaced by the rocket icon after phone restarts under a captive portal network regardless if the valid account has logged or not.

Device: Flame v2.1 build(unaffected)
Build ID               20150510001201
Gaia Revision          3e7bd686ecd852f4dfa4605b45f558e6bd34f02a
Gaia Date              2015-05-07 15:12:34
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/de4ebfdc7db6
Gecko Version          34.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150510.035554
Firmware Date          Sun May 10 03:56:05 EDT 2015
Bootloader             L1TC000118D0

Device: Flame v3.0 build(unaffected)
Build ID               20150510010201
Gaia Revision          5b2a150f6f5d29bddfaac13fcbbf099376f2f275
Gaia Date              2015-05-09 12:34:41
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/77d92f6d7679
Gecko Version          40.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150510.045933
Firmware Date          Sun May 10 04:59:44 EDT 2015
Bootloader             L1TC000118D0
Flags: needinfo?(fan.luo)
status-b2g-v2.1: --- → unaffected
status-b2g-master: --- → verified
Keywords: verifyme
QA Whiteboard: [MGSEI-Triage+]

Updated

3 years ago
status-b2g-v2.1S: --- → unaffected
status-b2g-v2.2: --- → affected

Comment 31

3 years ago
Comment on attachment 8588365 [details] [review]
[gaia] KevinGrandon:bug_1150495_cached_icon_overwrite > mozilla-b2g:master

Approving given this affects user experience in certain scenario.
Attachment #8588365 - Flags: approval-gaia-v2.2?(bajaj.bhavana) → approval-gaia-v2.2+

Comment 32

3 years ago
Norry,
Please also verify after the patch is land on 2.2.
Thanks!
Flags: needinfo?(fan.luo)
Keywords: verifyme
See Also: → bug 1163930
v2.2: https://github.com/mozilla-b2g/gaia/commit/5555c91736da5e4160bb53ed0ab44e565b5163dc
status-b2g-v2.2: affected → fixed
Target Milestone: --- → 2.2 S12 (15may)
Created attachment 8605789 [details]
verified_2.2.mp4

Hi Chris,

I have tried the latest flame v2.2&v3.0 build and Apr 1 or earlier builds, but can't repro this bug.
20150303162505(v2.2)
20150401162503(v2.2)
20150331160205(v3.0)
20150513002507(v2.2)

So I doubt that the wifi I used is not same as your's wifi you used to repro(Maybe I should go to London XD). I attach my STR and video and logcat for your reference, could you check it? Thanks a lot.

STR:
1.Connect a good wifi and add a bookmark icon to homescreen.
2.Connect a captive-portal wifi but not enter the account and password/or enter the account and PWD.
3.Restart phone.
**The bookmark icon shows normally and will not be changed as rocket icon.
See attachment: verified_2.2.mp4, afterreboot_logcat.txt, beforereboot_logcat.txt(the logcat is from build:20150401162503)
Rate:0/5.
Flags: needinfo?(chrislord.net)
Created attachment 8605790 [details]
beforereboot_logcat.txt
Flags: needinfo?(fan.luo)
Created attachment 8605791 [details]
afterreboot_logcat.txt
(Reporter)

Comment 37

3 years ago
Not seen this in a long time, pretty sure it's fixed.
Flags: needinfo?(chrislord.net)

Comment 38

3 years ago
Per comment 34 and cdomment 37,this bug has been fixed on Flame kk v2.2,so change the status to "verified".

Updated

3 years ago
Status: RESOLVED → VERIFIED
status-b2g-v2.2: fixed → verified
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.