1222340 - websites pinned as a site don't have a logo anymore

Status: VERIFIED FIXED

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

Description

[Mihai Barbat]

User-Agent: Mozilla/5.0 (Mobile; rv:45.0) Gecko/45.0 Firefox/45.0

I am using a Flame phone running 20151105150203 OTA

Comment 1

[Mihai Barbat]

Attachment: logshake-about_memory-206.json.gz

Comment 2

[Mihai Barbat]

Attachment: cache-recovery-last_log.log

Comment 3

[Mihai Barbat]

Attachment: cache-recovery-last_install.log

Comment 4

[Mihai Barbat]

Attachment: system-b2g-application.ini

Comment 5

[Mihai Barbat]

Attachment: proc-vmstat.log

Comment 6

[Mihai Barbat]

Attachment: proc-vmallocinfo.log

Comment 7

[Mihai Barbat]

Attachment: proc-version.log

Comment 8

[Mihai Barbat]

Attachment: proc-uptime.log

Comment 9

[Mihai Barbat]

Attachment: proc-meminfo.log

Comment 10

[Mihai Barbat]

Attachment: proc-kmsg.log

Comment 11

[Mihai Barbat]

Attachment: proc-cmdline.log

Comment 12

[Mihai Barbat]

Attachment: dev-log-radio.log

Comment 13

[Mihai Barbat]

Attachment: dev-log-system.log

Comment 14

[Mihai Barbat]

Attachment: dev-log-main.log

Comment 15

[Mihai Barbat]

Attachment: properties.log

Comment 16

[Gregor Wagner [:gwagner]]

Same here. My bookmarks I had for some time now lost their logos. This might have happened some time ago already.

Comment 17

[Chris Lord [:cwiiis]]

Any STR anybody?

I can't remember if we rely on the IconsHelper cache or our own cache for site icons, gmarty?

Comment 18

[Gregor Wagner [:gwagner]]

I guess have a bookmark created a while ago. Maybe >6 months. I can see it with cnn.com, dict.leo.org, derstandard.at

Comment 19

[Chris Lord [:cwiiis]]

(In reply to Gregor Wagner [:gwagner] from comment #18)
> I guess have a bookmark created a while ago. Maybe >6 months. I can see it
> with cnn.com, dict.leo.org, derstandard.at

aaah, this sounds like the old bookmark.icon string property handling has been removed... gmarty, any comment?

Comment 20

[Mihai Barbat]

issue still happens with the latest OTA from this morning

Comment 21

[Chris Lord [:cwiiis]]

I'm seeing this for a couple of pinned sites that I wasn't before - I expect this was triggered by an IconsHelper change and is a regression.

Bisecting this would be tricky though, I'll take a look at this later to see if there's anything obvious.

Comment 22

[Chris Lord [:cwiiis]]

Updated my phone to master with the patch from bug 1224136, one of my icons reappeared, the other disappeared entirely. Will see what I can do...

Comment 23

[Chris Lord [:cwiiis]]

This is a regression partially caused by 1221195 - ends up we do need that permission to fetch icons :(

On top of this, failing bookmark icon loading doesn't result in getting the rocket icon, possibly only after bug 1224136.

I'll fix both of these in this bug.

Comment 24

[GitHub Autolander Bot]

Attachment: [gaia] Cwiiis:bug1222340-homescreen-fix-bookmark-icon-fetch > mozilla-b2g:master

Comment 25

[Chris Lord [:cwiiis]]

Comment on attachment 8687965 [details] [review]
[gaia] Cwiiis:bug1222340-homescreen-fix-bookmark-icon-fetch > mozilla-b2g:master

Fix the two bugs here - re-add systemXHR so bookmark icon fetches work, handle bookmark icon fetch failing so the rocket icon displays and add unit tests to make sure that bookmark icon fetch failing is handled correctly.

Comment 27

[Chris Lord [:cwiiis]]

Rebased and merged: https://github.com/mozilla-b2g/gaia/commit/5d2eaa28ac77209ad305509610b732a908495f6d

Comment 28

[Chris Lord [:cwiiis]]

Comment on attachment 8687965 [details] [review]
[gaia] Cwiiis:bug1222340-homescreen-fix-bookmark-icon-fetch > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined: Icons are not downloaded for pinned sites or for the icons in the center of pinned pages.
[Testing completed]: Manual testing completed. Further related unit tests added.
[Risk to taking this patch] (and alternatives if risky): Small risk of other bookmark icon-fetching related issues appearing. Requires bug 1224136.
[String changes made]: None.

Comment 29

[Mihai Barbat]

Awesome. Will this be available in the OTA that will normally be released in 2 hours or so?

Comment 30

[Chris Lord [:cwiiis]]

(In reply to Mihai Barbat from comment #29)
> Awesome. Will this be available in the OTA that will normally be released in
> 2 hours or so?

I'm not sure how the OTA gets made, sorry - if you build from master, you'll have these changes. If there's a Nightly OTA, I'd be surprised if this wasn't at least in tomorrow's.

Comment 32

[Mahendra Potharaju [:mahe]]

Comment on attachment 8687965 [details] [review]
[gaia] Cwiiis:bug1222340-homescreen-fix-bookmark-icon-fetch > mozilla-b2g:master

Approved for 2.5 uplift.

Thanks

Comment 33

[Mihai Barbat]

I confirm this is fixed with OTA 20151118150206

Comment 34

[Carsten Book [:Tomcat]]

landed on 2.5 as https://github.com/mozilla-b2g/gaia/commit/6fbec2cbe340cac027ac382d29c0050bdd4e2a7b

Comment 35

[Boris Zbarsky [:bzbarsky]]

Just for future reference, using Promise.resolve as a function, not a method, as this changeset attempts to do, is not valid per spec.  It kinda works in Gecko right now, but that's a Gecko bug that we're trying to fix, except people keep adding more busted uses...

Comment 36

[yifan [:yifan][:yliao]]

The patch seems to break the unit test on v2.5 branch. Could you please help or find someone who could help on this issue? Thanks.

Comment 37

[yifan [:yifan][:yliao]]

To be more specific the test fails is homescreen/test/unit/app_test.js .

Comment 38

[Michael Henretty [:mikehenrty][:mhenretty]]

Guillaume, can you see why this patch fails the above homescreen unit test on 2.5? If it's not exposing a real bug, go ahead and just disable that test, r=me a=test-only.

Comment 39

[Michael Henretty [:mikehenrty][:mhenretty]]

YiFan, is this failing in automation anywhere? I'm not seeing Gu on the 2.5 branch in treeherder:

https://treeherder.mozilla.org/#/jobs?repo=mozilla-b2g44_v2_5

Comment 40

[GitHub Autolander Bot]

Attachment: [gaia] gmarty:Bug-1222340-v2.5-fix-homescreen-unit-tests > mozilla-b2g:v2.5

Comment 41

[Guillaume Marty [:gmarty]]

Hi Yifan, I worked on a fix but there are still many unrelated failures on the v2.5 branch (See https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=022effae135b97c0af46d003654908ef47faf567).
Do you want me to land the fix still?

Comment 42

[yifan [:yifan][:yliao]]

Thank you! I've notify the owners about the causes of those failures. Yes please land the fix for the homescreen test.

Comment 43

[Michael Henretty [:mikehenrty][:mhenretty]]

v2.5: https://github.com/mozilla-b2g/gaia/commit/612adce6e702c125ad3b770c453d700947d7da27