46 bytes, text/x-github-pull-request
|Details | Review | Splinter Review|
8.68 MB, video/mp4
6.97 MB, video/mp4
146.65 KB, text/plain
773.08 KB, text/plain
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).
confirmed blocker in team triage
blocking-b2g: 2.2? → 2.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 I wonder if this is some kind of regression? Looking for a good way to reproduce this now..
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.
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?
(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.
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 :)
n?me so I can follow up on this when I'm back if necessary.
Created attachment 8588365 [details] [review] [gaia] KevinGrandon:bug_1150495_cached_icon_overwrite > mozilla-b2g:master
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.
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? → ---
(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.
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?
(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.
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).
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+
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
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.
Rebasing and trying again.
Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/1922c412ba18dc6f5ba025bb9cf8fa1f68a9c0dc
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
I assume we want to get this uplifted to 2.2 as well?
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)
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.
(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).
Hi Norry, Can you check whether you got any website icon at the beginning while the network is good? Thanks
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
status-b2g-v2.1: --- → unaffected
status-b2g-master: --- → verified
status-b2g-v2.1S: --- → unaffected
status-b2g-v2.2: --- → affected
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+
Norry, Please also verify after the patch is land on 2.2. Thanks!
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.
Not seen this in a long time, pretty sure it's fixed.
Per comment 34 and cdomment 37,this bug has been fixed on Flame kk v2.2,so change the status to "verified".
Status: RESOLVED → VERIFIED
status-b2g-v2.2: fixed → verified
You need to log in before you can comment on or make changes to this bug.