Missing Favicons in the Application Menu Bar

VERIFIED FIXED in Firefox 10

Status

()

Core
Widget: Cocoa
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: Mehmet, Assigned: spectre)

Tracking

({regression, relnote, verified-beta})

9 Branch
mozilla11
x86
Mac OS X
regression, relnote, verified-beta
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?
in-litmus ?

Firefox Tracking Flags

(firefox9+ affected, firefox10 verified)

Details

(Whiteboard: [qa!])

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
Created attachment 577127 [details]
missing_favicons.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:9.0) Gecko/20100101 Firefox/9.0
Build ID: 20111122192043

Steps to reproduce:

OS: Mac OS 10.6.8
Firefox: Version 9 Beta

STR:
1. Open the Bookmarks menu in the Application menu


Actual results:

The Bookmarks have no favicons. (Even after visiting the pages.)


Expected results:

There should be favicons.

Comment 1

6 years ago
Confirmed with TenFourFox 9beta1 with Mac OS X 10.5.8. 

Addtl. Information: Icons in the Bookmarks Toolbar are displayed. Placeholder icons (dotted rounded square) and folder icons are also displayed. Site icons in the Bookmarks Menu are missing.

Comment 2

6 years ago
Created attachment 580396 [details]
screenshot bookmarks toolbar

Comment 3

6 years ago
Created attachment 580397 [details]
screenshot bookmarks menu

Updated

6 years ago
Attachment #580396 - Attachment description: screenshot bookmarks menu → screenshot bookmarks toolbar

Updated

6 years ago
Attachment #580397 - Attachment description: screenshot bookmarks toolbar → screenshot bookmarks menu
(Assignee)

Comment 4

6 years ago
History favicons show up, but only the second time the menu is opened.
(Assignee)

Comment 5

6 years ago
Steven, do you have any ideas on this one? I'm suspecting something in toolkit/themes/pinstripe; I don't see any widget changes that would have caused this.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Off the top of my head I have no idea what could have caused this.

Someone please provide a regression range (using mozilla-central nightlies) -- that'd make it a lot easier to figure out.
(Reporter)

Comment 7

6 years ago
(In reply to Steven Michaud from comment #6)
> Someone please provide a regression range (using mozilla-central nightlies)
> -- that'd make it a lot easier to figure out.

Checking ...
(Reporter)

Comment 8

6 years ago
(In reply to Mehmet Sahin from comment #7)
> (In reply to Steven Michaud from comment #6)
> > Someone please provide a regression range (using mozilla-central nightlies)
> > -- that'd make it a lot easier to figure out.
> 
> Checking ...

Regression range:

It works fine in 9.0a1 (2011-09-22) mozilla central

It is broken in 9.0a1 (2011-09-23) mozilla central


Hope it helps ...
Regards
Mehmet
Thanks, Mehmet!

I can reproduce this bug, but only once.

In other words, the first time a bookmarks menu is displayed, it sometimes is lacking its favicons.  The second time I display it they're no longer missing.

I also think I've found the patch that triggered this problem -- the one for bug 573583, which turned on decode-on-draw by default.

Whoever can reproduce this, try again after setting image.mem.decodeondraw to false (in about:config) and restarting.  I find doing this makes the problem go away.

I'd also like to know whether this bug happens on other platforms, like Windows and Linux.
Blocks: 573583
> and restarting

Quitting and restarting Firefox.
(Reporter)

Comment 11

6 years ago
(In reply to Steven Michaud from comment #9)

> Whoever can reproduce this, try again after setting image.mem.decodeondraw
> to false (in about:config) and restarting.  I find doing this makes the
> problem go away.

I can confirm that setting 'image.mem.decodeondraw' to 'false' fixes this issue.
Component: General → ImageLib
Product: Firefox → Core
QA Contact: general → imagelib
If this turns out to be Mac-specific, it should probably be moved to Core : Widget: Cocoa.
nsMenuItemIconX doesn't handle icons being discarded. It presumes that, once it's loaded, it's done, which is not the case since we enabled discarding for all images.

Instead of using mLoadedIcon, you could perhaps test whether aFrame == 0. Then, if we are called again for the first frame, we can handle that.

It's possible that something else is happening here, and i"m misunderstanding, but in any case this is almost certainly a Cocoa widget problem.
Component: ImageLib → Widget: Cocoa
QA Contact: imagelib → cocoa
(Assignee)

Comment 14

6 years ago
Changing the pref also fixes it in TenFourFox. Is it worth a patch to disable it for -beta, or try to find a fix?
Upon further thought, I think what's happening is that we're actually not requesting a decode of these icons at all (that is what decode-on-draw means, fundamentally - don't decode unless requested). So if the image is already decoded, we're fine, but if it hasn't yet been decoded, either because it's been discarded or never displayed, we'll never get the icon.

Fixing this is pretty simple: in nsMenuItemIconX::LoadIcon(), around nsMenuItemIconX.mm:346, insert a request for the image to be decoded:

mIconRequest->RequestDecode();

If the image is already decoded, this is a no-op. Otherwise, it causes the image to be decoded.

I'm pretty certain comment 13 is wrong.
(In reply to Cameron Kaiser from comment #14)
> Changing the pref also fixes it in TenFourFox. Is it worth a patch to
> disable it for -beta, or try to find a fix?

I don't recommend disabling decode-on-draw, especially not this late. It'll be a memory regression when loading lots of pages in the background.

We've also started building the final beta of Firefox 9, so we're out of luck when it comes to Firefox. Cameron, I suspect TenFourFox will find great joy in the fix I outline in comment 15, though.
(Assignee)

Comment 17

6 years ago
Created attachment 581164 [details] [diff] [review]
request decoding of icons in Mac menu items

Yup, that works!

Patch for posterity since it does appear to affect Firefox too.
Attachment #581164 - Flags: review?(smichaud)
Comment on attachment 581164 [details] [diff] [review]
request decoding of icons in Mac menu items

This is no good, and might result in crashes.

You need to move the call to RequestDecode() to just before "return NS_OK" (and just after "if (NS_FAILED(rv)) return rv;").
Attachment #581164 - Flags: review?(smichaud) → review-
(Assignee)

Comment 19

6 years ago
Created attachment 581463 [details] [diff] [review]
request decoding of icons in Mac menu items (v2)

D'oh. I blame my cat.
Attachment #581164 - Attachment is obsolete: true
Attachment #581463 - Flags: review?(smichaud)
(In reply to comment #15)

> Upon further thought, I think what's happening is that we're
> actually not requesting a decode of these icons at all

This isn't true.  RequestDecode() is called on all of these icons in
nsMenuItemIconX::OnStartContainer().  

But nsMenuItemIconX::OnStartContainer() is called after
nsMenuItemIconX::LoadIcon(), and it may be that the icons won't get
decoded in time unless RequestDecode() is called (or also called) from
nsMenuItemIconX::LoadIcon().

In trying to debug this, I noticed that	when the bug happens (and a
favicon doesn't get drawn), nsMenuItemIconX::OnStopFrame() (where the
actual drawing takes place) doesn't get called.  Why these calls
should depend on the timing of calls to RequestDecode() I don't know,
but it appears that they do.

Joe, I suspect it's alright (and maybe even desirable) to call
RequestDecode() from both nsMenuItemIconX::LoadIcon() and
nsMenuItemIconX::OnStartContainer().  Do you have an opinion on this?	

On a side note, I noticed that on no other platform (besides OS X) is
RequestDecode() called from widget code.  Do you know if there was a
reason for this?
(In reply to Steven Michaud from comment #20)
> In trying to debug this, I noticed that	when the bug happens (and a
> favicon doesn't get drawn), nsMenuItemIconX::OnStopFrame() (where the
> actual drawing takes place) doesn't get called.  Why these calls
> should depend on the timing of calls to RequestDecode() I don't know,
> but it appears that they do.

I'd have to debug to know for sure.

> Joe, I suspect it's alright (and maybe even desirable) to call
> RequestDecode() from both nsMenuItemIconX::LoadIcon() and
> nsMenuItemIconX::OnStartContainer().  Do you have an opinion on this?	

You shouldn't need a call in OnStartContainer if you have one in LoadIcon.

> On a side note, I noticed that on no other platform (besides OS X) is
> RequestDecode() called from widget code.  Do you know if there was a
> reason for this?

None off the top of my head, no.
Comment on attachment 581463 [details] [diff] [review]
request decoding of icons in Mac menu items (v2)

Cameron, please test getting rid of the call to RequestDecode() from nsMenuItemIconX::OnStartContainer().  If you don't have any problems (I didn't), please post another patch that does this.  (You should just clean out OnStartContainer(), so that it only returns NS_OK.)

Comment 23

6 years ago
A bit late, but: image.mem.decodeondraw = false reliably fixes the problem for me on all machines (OS X 10.4./10.5, G3/G4) with TenFourFox 9 beta 1.
(Assignee)

Comment 24

6 years ago
Created attachment 581510 [details] [diff] [review]
request decoding of icons in Mac menu items (v3)

Yup, it works without it.
Attachment #581463 - Attachment is obsolete: true
Attachment #581463 - Flags: review?(smichaud)
Attachment #581510 - Flags: review?(smichaud)
(Assignee)

Comment 25

6 years ago
(I left the check in OnStartContainer(), though, just in case)
> (I left the check in OnStartContainer(), though, just in case)

Please just get rid of it.  It no longer serves any purpose.
(Assignee)

Comment 27

6 years ago
Created attachment 581532 [details] [diff] [review]
request decoding of icons in Mac menu items (v4)

Okay.
Attachment #581510 - Attachment is obsolete: true
Attachment #581510 - Flags: review?(smichaud)
Attachment #581532 - Flags: review?(smichaud)
Attachment #581532 - Flags: review?(smichaud) → review+
(Assignee)

Comment 28

6 years ago
Thanks, Steven and Joe!

Check-in requested.
Status: NEW → ASSIGNED
Keywords: checkin-needed
Assignee: nobody → spectre
http://hg.mozilla.org/mozilla-central/rev/6ee5c4c7beb6
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
(Assignee)

Comment 30

6 years ago
Comment on attachment 581532 [details] [diff] [review]
request decoding of icons in Mac menu items (v4)

It seems it's too late for beta, but asking for aurora since this should be a very minor fix to deal with an annoying possible regression.
Attachment #581532 - Flags: approval-mozilla-aurora?
Summary: [Mac OS 10.6.8] Missing Favicons in the Application Menu Bar → Missing Favicons in the Application Menu Bar
Comment on attachment 581532 [details] [diff] [review]
request decoding of icons in Mac menu items (v4)

[Triage Comment]
Approved for Aurora. Please land on Monday 12/19 or earlier in order to make the cut-over to Beta.

Also including Cheng, as this sounds a lot like a support driver we've had a lot of issues with.
Attachment #581532 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I'll do the aurora landing in the next hour or so.
Comment on attachment 581532 [details] [diff] [review]
request decoding of icons in Mac menu items (v4)

Landed on aurora:
http://hg.mozilla.org/releases/mozilla-aurora/rev/de25fe8ee416
status-firefox10: --- → fixed

Updated

6 years ago
Blocks: 712496

Updated

6 years ago
Duplicate of this bug: 712496
(In reply to Cameron Kaiser from comment #30)
> Comment on attachment 581532 [details] [diff] [review]
> request decoding of icons in Mac menu items (v4)
> 
> It seems it's too late for beta, but asking for aurora since this should be
> a very minor fix to deal with an annoying possible regression.

Next time, please request tracking?
status-firefox9: --- → affected
tracking-firefox9: --- → ?
Any chance to get an automated test for? If not we definitely should have a manual test for it.
Flags: in-testsuite?
Flags: in-litmus?
Keywords: regression
Whiteboard: [qa+]
Keywords: relnote
Duplicate of this bug: 712740

Updated

6 years ago
Duplicate of this bug: 713331

Updated

6 years ago
tracking-firefox9: ? → +
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:10.0) Gecko/20100101 Firefox/10.0

Verified the fix based on comment #0 and comment #9.
Steps:
1. Launch Firefox
2. Go to Bookmarks>Recently Bookmarked
Result: All favicons are displayed at first try.
Status: RESOLVED → VERIFIED
status-firefox10: fixed → verified
Keywords: verified-beta
Whiteboard: [qa+] → [qa!]
Duplicate of this bug: 716159
You need to log in before you can comment on or make changes to this bug.