Closed Bug 1412832 Opened 2 years ago Closed 2 years ago

No favicon displayed for baidu.com after adding the engine in Search bar panel

Categories

(Firefox :: Search, defect, P1)

57 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 58
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 --- wontfix
firefox58 --- verified

People

(Reporter: cbadau, Assigned: mak)

References

Details

(Keywords: regression, Whiteboard: [fxsearch])

Attachments

(1 file)

Attached image issue.png
[Affected versions]:
- Firefox 57 Beta 12
- latest Nightly 58.0a1

[Affected platforms]:
- Windows 7 x64
- Windows 10 x64
- macOS 10.13
- Ubuntu 14.04 x64

[Steps to reproduce]:
1. Launch Firefox and go to http://www.baidu.com/
2. Add the search engine.

[Expected result]:
- A favicon is displayed in the Search bar panel.

[Actual result]:
- No favicon displayed - magnifying glass, visible in the Search bar panel. Please see the attachment "issue.png"

[Regression range]:
- The issue is NOT reproducible on 56.0.2. I will get back with the regression range as soon as possible.
Not sure how rich icon collection would affect the search box.

mak do you have any idea on this? Is it similar to bug 1403504?
Flags: needinfo?(najiang) → needinfo?(mak77)
I think it's because of this:

http://searchfox.org/mozilla-central/rev/1ebd2eff44617df3b82eea7d2f3ca1b60cc591a0/browser/base/content/browser.js#3713

Basically the code assumes by the time we handle a "Link:AddSearch" message, mIconURL has been populated, but that's no more the case since we delayed icon handling.
I'm still not sure how to fix this, we could delay "Link:AddSearch" to after icons have been analyzed and mIconURL has been set, or make _rebuildAddEngineList async... Maybe worth asking Florian what he thinks.
Flags: needinfo?(mak77) → needinfo?(florian)
So I can reproduce this issue on today's nightly.

Looks like Firefox successfully collected the icon and persisted it in favions.sqlite, but the search box didn't show it due to the delay that mak described above.
Blocks: 1352459
No longer depends on: 1352459
Keywords: regression
Priority: -- → P1
Whiteboard: [fxsearch]
Last time we had a search icon issue with baidu.com, it was because they use an SVG icon (see bug 1240729), is there any chance this could be the problem again?
Flags: needinfo?(florian)
(In reply to Florian Quèze [:florian] [:flo] from comment #5)
> Last time we had a search icon issue with baidu.com, it was because they use
> an SVG icon (see bug 1240729), is there any chance this could be the problem
> again?

I don't think so. In my case, it's just a regular ico file in the favicons.sqlite.
no, this is due to our changes, we delayed setting mIconURL but AddSearch expects it to be set already.
We looked at the problem on IRC, and Florian pointed out Bug 361923.
There are 2 bugs, but only one is likely affecting most users.

1. if the user should open the panel really quickly after a page load, due to the delay we added in bug 1352459 on <link> handling, mIconURL may not be set, and we could end up without an icon (comment 3). This is unlikely to hit users though.

2. If the openSearch xml file doesn't specify an icon, we use the page favicon. In this case the page has 2 favicons, and looks like now we pick the ico file, that is larger than 10KB (16KB indeed). Bug 361923 says we just discard icons larger than 10KB.
http://searchfox.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#99

Based on the fact users of baidu are likely to have it as one of the default engines (and that has no problem), and in the past we shipped multiple versions with a similar bug and was not considered critical, we are prone to consider this a wontfix for 57.

I'll verify that Bug 361923 is indeed our problem here, end eventually we can bump the limit a little bit here, or try to resample the icon.
This will be fixed by bug 361923.
I don't think it's worth to keep a bug open for the timing bug; while valid in theory, it seems unlikely to happen in practice.
Thus, I will mark this as fixed once the other bug is.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Flags: qe-verify+
QA Contact: camelia.badau
I have reproduced this issue using Firefox 58.0a1 (build ID = 20171030103605) on Win 8.1 x64.
I can confirm this issue is fixed, I verified using Firefox 58.0b5 on Win 8.1 x64, Ubuntu 14.04 x64 and Mac OS X 10.12.6.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.