1018913 - Customization mode favicon not showing anymore

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[u428464]

At least on Windows 7 the customization favicon is not showing anymore.

Comment 1

[:Gijs (out for now; he/him)]

More info? Nightly only? How long? Are you sure this isn't just the issue where it would not show in all the cases (but show in some) because something weird is up with our icon code?

Comment 2

[u428464]

Seems to be a new issue on Nightly. I noticed it just today. I'll try to find more info.

Comment 3

[u428464]

Ok first bad build is 05.27.2014

Comment 4

[u428464]

I'm wondering if it's not the same issue that was present with Aero at the beginning : i.e. only Windows 7 had no favicon after the first patch landed.

Comment 5

[u428464]

Cant this be nominated for the backlog ?

Comment 6

[:Gijs (out for now; he/him)]

(In reply to Guillaume C. [:ge3k0s] from comment #3)
> Ok first bad build is 05.27.2014

(In reply to Guillaume C. [:ge3k0s] from comment #4)
> I'm wondering if it's not the same issue that was present with Aero at the
> beginning : i.e. only Windows 7 had no favicon after the first patch landed.

Now I'm just confused. Is this regression window still valid?

Comment 7

[u428464]

(In reply to :Gijs Kruitbosch from comment #6)
> (In reply to Guillaume C. [:ge3k0s] from comment #3)
> > Ok first bad build is 05.27.2014
> 
> (In reply to Guillaume C. [:ge3k0s] from comment #4)
> > I'm wondering if it's not the same issue that was present with Aero at the
> > beginning : i.e. only Windows 7 had no favicon after the first patch landed.
> 
> Now I'm just confused. Is this regression window still valid?

05.27.2014 is the first bad build on Windows 7. The other comment was just a theory since it seems to be an issue just on Win 7.

Comment 8

[:Gijs (out for now; he/him)]

(In reply to Guillaume C. [:ge3k0s] from comment #7)
> (In reply to :Gijs Kruitbosch from comment #6)
> > (In reply to Guillaume C. [:ge3k0s] from comment #3)
> > > Ok first bad build is 05.27.2014
> > 
> > (In reply to Guillaume C. [:ge3k0s] from comment #4)
> > > I'm wondering if it's not the same issue that was present with Aero at the
> > > beginning : i.e. only Windows 7 had no favicon after the first patch landed.
> > 
> > Now I'm just confused. Is this regression window still valid?
> 
> 05.27.2014 is the first bad build on Windows 7. The other comment was just a
> theory since it seems to be an issue just on Win 7.

I tried to get a pushlog / proper regression window for this using mozregression on my win7 vm, but I can't reproduce.

What icon is missing? What do you see instead? I'm assuming this is about the icon shown on the tab, rather than the [() Nightly] identity block? How are you opening customize mode?

On the May 27 build I still see an icon there. Are you checking with a clean profile?

Clearing backlog flag while we're not sure what's going on.

Comment 9

[u428464]

The customization favicon (in tab) is missing. Since we don't show the default favicon when there is none I see just the tab title. You're right the favicon is still there in 05.27.2014 build. It's my mistake. First bad build is 05.29.2014 build. Tested with a clean profile.

Comment 10

[u428464]

Ok the favicon is there when you directly type about:customizing in the address bar, but is missing when you click on the menu panel button.

Comment 11

[:Gijs (out for now; he/him)]

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e017c15325ae&tochange=1e712b724d17

Comment 12

[:Gijs (out for now; he/him)]

Which looks like this on TBPL:

https://tbpl.mozilla.org/?fromchange=e017c15325ae&tochange=1e712b724d17

Comment 13

[:Gijs (out for now; he/him)]

And which blames https://tbpl.mozilla.org/?fromchange=e017c15325ae&tochange=1e712b724d17&rev=c8252fd40ba7, which means I put my money on bug 1016942.

Tim, is it possible/plausible that bug 1016942 broke this because we have the whole cached-like-about:newtab thingymajig going on for aboutCustomizing as well?

Comment 14

[:Gijs (out for now; he/him)]

I can reproduce on mac as well.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

I... don't know? Does backing out bug 1016942 fix it?

Comment 16

[:Gijs (out for now; he/him)]

(In reply to Tim Taubert [:ttaubert] from comment #15)
> I... don't know? Does backing out bug 1016942 fix it?

Interestingly... no. I must have made a mistake with the regression hunting because I /thought/ that it was working on:

http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-win32-pgo/1401305412/

( 9506880e4879 )

and broken on:

http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-win32-pgo/1401316203/

( c8252fd40ba7 )


I need to run out right now - Guillaume, do you have time to check if you get the same results? Almost everything else in that push is android-only, so I'm confused as to why I saw what I think I saw...

Comment 17

[u428464]

Both of the builds above are already broken on my PC.

Comment 18

[:Gijs (out for now; he/him)]

(In reply to Guillaume C. [:ge3k0s] from comment #17)
> Both of the builds above are already broken on my PC.

Hrm. New checks suggest https://hg.mozilla.org/mozilla-central/pushloghtml?startID=26845&endID=26846 instead, which is odd because it's an m-i merge... I'll bisect further on m-i in a second, but meanwhile...

Because I'm paranoid now, can you check that http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-win32/1401254569/ is broken and http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-win32/1401244733/ works?

Comment 19

[u428464]

(In reply to :Gijs Kruitbosch from comment #18)
> Because I'm paranoid now, can you check that
> http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-
> central-win32/1401254569/ is broken and
> http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-
> central-win32/1401244733/ works?

This time this is correct. ;-)

Comment 20

[:Gijs (out for now; he/him)]

Sooo... confirmed with a local backout, this was caused by... drumroll... bug 990812!

I expect that there's /something/ wrong with the frame script stuff causing the tab preloader to work... differently? But I don't pretend to understand /why/ :-\

Comment 21

[:Gijs (out for now; he/him)]

Specifically, I backed out https://hg.mozilla.org/integration/mozilla-inbound/rev/f35d007a22a7 .

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

Always me :'(

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1018913-Customization-mode-favicon-not-showing-a.patch

Not sure why exactly that fixes it but I definitely forgot to update the file.

Comment 24

[:Gijs (out for now; he/him)]

Comment on attachment 8439398 [details] [diff] [review]
0001-Bug-1018913-Customization-mode-favicon-not-showing-a.patch

Review of attachment 8439398 [details] [diff] [review]:
-----------------------------------------------------------------

Do we have a bug already for unifying these two files, and if not, can you file one and CC me + mconley? Thanks!

(might be useful to do a diff to find any other things that may have sneaked in if you haven't already...)

Comment 25

[:Gijs (out for now; he/him)]

(In reply to Tim Taubert [:ttaubert] from comment #22)
> Always me :'(

(thanks for picking this up so quickly!)

Marco, I think Tim is fixing this this iteration. ;-)

Comment 26

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Gijs Kruitbosch from comment #24)
> Comment on attachment 8439398 [details] [diff] [review]
> 0001-Bug-1018913-Customization-mode-favicon-not-showing-a.patch
> 
> Review of attachment 8439398 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do we have a bug already for unifying these two files, and if not, can you
> file one and CC me + mconley? Thanks!

We _could_ file that, but I'd prefer following up on bug 1014185 and getting rid of the about:customizing preloading hack entirely.

Comment 27

[Marco Mucci [:MarcoM]]

Added to Iteration 33.1

Comment 28

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Mike Conley (:mconley) from comment #26)
> We _could_ file that, but I'd prefer following up on bug 1014185 and getting
> rid of the about:customizing preloading hack entirely.

I agree, I hope we can get rid of about:newtab preloading too in the future.

Comment 29

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/8c4371b5482e

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8c4371b5482e

Comment 31

[Camelia Badau [:cbadau], Desktop Test Engineering]

Verified fix on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.8.5 using latest Nightly 33.0a1 (buildID: 20140615030204).