Closed
Bug 1412832
Opened 7 years ago
Closed 7 years ago
No favicon displayed for baidu.com after adding the engine in Search bar panel
Categories
(Firefox :: Search, defect, P1)
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)
121.17 KB,
image/png
|
Details |
[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.
Reporter | ||
Comment 1•7 years ago
|
||
Regression range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=4dc8f5f9259c1413dcd2755ae01bad8dc8139050&tochange=6d5fe3151e733d6ac818728f44f5985f1aa63f8c
It seems that bug 1352459 introduced this issue.
Depends on: 1352459
Flags: needinfo?(najiang)
Comment 2•7 years ago
|
||
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)
Assignee | ||
Comment 3•7 years ago
|
||
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)
Comment 4•7 years ago
|
||
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.
Assignee | ||
Updated•7 years ago
|
Blocks: 1352459
No longer depends on: 1352459
Keywords: regression
Priority: -- → P1
Whiteboard: [fxsearch]
Comment 5•7 years ago
|
||
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)
Comment 6•7 years ago
|
||
(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.
Assignee | ||
Comment 7•7 years ago
|
||
no, this is due to our changes, we delayed setting mIconURL but AddSearch expects it to be set already.
Assignee | ||
Comment 8•7 years ago
|
||
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.
Depends on: 361923
Assignee | ||
Comment 9•7 years ago
|
||
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 | ||
Updated•7 years ago
|
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Assignee | ||
Updated•7 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Updated•7 years ago
|
status-firefox56:
--- → unaffected
status-firefox-esr52:
--- → unaffected
Target Milestone: --- → Firefox 58
Updated•7 years ago
|
Flags: qe-verify+
QA Contact: camelia.badau
Comment 10•7 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•