705516 - Missing Favicons in the Application Menu Bar

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Mehmet]

Attachment: 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

[ChrisX]

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

[ChrisX]

Attachment: screenshot bookmarks toolbar

Comment 3

[ChrisX]

Attachment: screenshot bookmarks menu

Comment 4

[Cameron Kaiser [:spectre]]

History favicons show up, but only the second time the menu is opened.

Comment 5

[Cameron Kaiser [:spectre]]

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.

Comment 6

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 7

[Mehmet]

(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 ...

Comment 8

[Mehmet]

(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

Comment 9

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

> and restarting

Quitting and restarting Firefox.

Comment 11

[Mehmet]

(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.

Comment 12

[Steven Michaud [:smichaud] (Retired)]

If this turns out to be Mac-specific, it should probably be moved to Core : Widget: Cocoa.

Comment 13

[Joe Drew (not getting mail)]

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.

Comment 14

[Cameron Kaiser [:spectre]]

Changing the pref also fixes it in TenFourFox. Is it worth a patch to disable it for -beta, or try to find a fix?

Comment 15

[Joe Drew (not getting mail)]

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.

Comment 16

[Joe Drew (not getting mail)]

(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.

Comment 17

[Cameron Kaiser [:spectre]]

Attachment: request decoding of icons in Mac menu items

Yup, that works!

Patch for posterity since it does appear to affect Firefox too.

Comment 18

[Steven Michaud [:smichaud] (Retired)]

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;").

Comment 19

[Cameron Kaiser [:spectre]]

Attachment: request decoding of icons in Mac menu items (v2)

D'oh. I blame my cat.

Comment 20

[Steven Michaud [:smichaud] (Retired)]

(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?

Comment 21

[Joe Drew (not getting mail)]

(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 22

[Steven Michaud [:smichaud] (Retired)]

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

[ChrisX]

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.

Comment 24

[Cameron Kaiser [:spectre]]

Attachment: request decoding of icons in Mac menu items (v3)

Yup, it works without it.

Comment 25

[Cameron Kaiser [:spectre]]

(I left the check in OnStartContainer(), though, just in case)

Comment 26

[Steven Michaud [:smichaud] (Retired)]

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

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

Comment 27

[Cameron Kaiser [:spectre]]

Attachment: request decoding of icons in Mac menu items (v4)

Okay.

Comment 28

[Cameron Kaiser [:spectre]]

Thanks, Steven and Joe!

Check-in requested.

Comment 29

[Joe Drew (not getting mail)]

http://hg.mozilla.org/mozilla-central/rev/6ee5c4c7beb6

Comment 30

[Cameron Kaiser [:spectre]]

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.

Comment 31

[Alex Keybl [:akeybl]]

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.

Comment 32

[Steven Michaud [:smichaud] (Retired)]

I'll do the aurora landing in the next hour or so.

Comment 33

[Steven Michaud [:smichaud] (Retired)]

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

Comment 35

[Dão Gottwald [:dao]]

(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?

Comment 36

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Any chance to get an automated test for? If not we definitely should have a manual test for it.

Comment 39

[Mihaela Velimiroviciu (:mihaelav)]

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.