1586453 - Bookmarks with invalid favicons don't work

Status: VERIFIED FIXED

(Firefox :: Enterprise Policies, defect, P1)

Description

[Mike Kaply [:mkaply]]

If you have invalid favicons, bookmarks don't work properly:

{
"policies": {
"DisplayBookmarksToolbar": true,
"Bookmarks": [
{
"Title": "Example.com",
"URL": "https://www.example.com",
"Favicon": "https://www.example.com/favicon.ico"
},
{
"Title": "Example Email",
"URL": "https://mail.example.com",
"Favicon": "https://mail.example.com/favicon.ico"
},
{
"Title": "Example Wiki",
"URL": "https://wiki.example.com/intranet/links",
"Favicon": "https://wiki.example.com/img/favicon.png"
}
]
}
}

This is because setAndFetchFaviconForPage never calls the callback in the failing case.

We actually don't need to wait for any of this (it's all async).

Comment 1

[Mike Kaply [:mkaply]]

Attachment: Bug 1586453 - Don't try to wait for favicons in policy

Comment 3

[Oliver Gutierrez]

Duplicate of this bug: 1587091

Yes, the problem is the favicon statement. I just removed it and check that all is working as expected.

Thanks Mike

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e92698988324
Don't try to wait for favicons in policy r=Standard8

Comment 5

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/e92698988324

Comment 6

[Mike Kaply [:mkaply]]

Comment on attachment 9098978 [details]
Bug 1586453 - Don't try to wait for favicons in policy

Beta/Release Uplift Approval Request

• User impact if declined: Some bookmarks policies don't apply and admins don't know why.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Steps detailed in bug
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Policy only, has automated test.
• String changes made/needed:

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Policy specific
• User impact if declined: Some bookmarks policies don't apply and admins don't know why.
• Fix Landed on Version: 72
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Policy only, has automated test.
• String or UUID changes made by this patch:

Comment 7

[Vlad Lucaci, Thunderbird QA (:vlucaci)]

Hello,
Managed to reproduce this issue with 71.0a1 (Build ID:20191004094656) on Windows 10x64.

The behavior on the latest Nightly(72.0a1 (2019-11-20) checked on Ubuntu 18.04 , Win10x64 and macOS 10.14.6 ) has shifted into the following one:

When launching the browser with a fresh profile, the (second and last in .json file) good favicon bookmarks are displayed in the bookmarks toolbar first, with the exception of the(first in .json file) bad favicon which will only be displayed after a browser restart. Is this intended behavior?

Comment 8

[Pascal Chevrel:pascalc]

FYI, I am not evaluating the uplift until we have confirmation that the new behaviour is the intended one.

Comment 9

[Mike Kaply [:mkaply]]

When launching the browser with a fresh profile, the (second and last in .json file) good favicon bookmarks are displayed in the bookmarks toolbar first, with the exception of the(first in .json file) bad favicon which will only be displayed after a browser restart. Is this intended behavior?

That's correct. Places is still waiting for the icon so until restart, it won't put an icon there.

Comment 10

[Vlad Lucaci, Thunderbird QA (:vlucaci)]

Confirming this issue as verified fixed based upon the comment above on the latest Nightly 72.0a1 (2019-11-20) checked on Ubuntu 18.04 , Win10x64 and macOS 10.14.6 .

Comment 11

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

Can this wait for 72/ 68.4esr? I know it's verified in nightly, but it's quite late for a beta uplift.

Comment 12

[Mike Kaply [:mkaply]]

Yes, that's fine. Users have a workaround.

Comment 13

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

Tracking so we remember to re-assess this for uplift for the next ESR.

Comment 14

[Vlad Lucaci, Thunderbird QA (:vlucaci)]

Hello,
Marking 71 as wontfix flag based upon the comments above. Please modify accordingly if I wrongly assumed .

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Is this something you wanted to re-nominate for 68.4esr?

Comment 16

[Mike Kaply [:mkaply]]

yep.

ESR Uplift Approval Request

If this is not a sec:{high,crit} bug, please state case for ESR consideration: Policy specific
User impact if declined: Some bookmarks policies don't apply and admins don't know why.
Fix Landed on Version: 72
Risk to taking this patch: Low
Why is the change risky/not risky? (and alternatives if risky): Policy only, has automated test.
String or UUID changes made by this patch:

For some reason I can't request the approval again.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9098978 [details]
Bug 1586453 - Don't try to wait for favicons in policy

Bookmark policy bugfix. Approved for 68.4esr.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/8f90e06662bd