1365509 - Default theme chrome is not registered after restart when a LWT is enabled

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

[Tracking Requested - why for this release]: Regression


STR:
1. Hover these things to see issue:
- tab close button
- new tab button
- address bar
2. Go to about:addons to see that: Extensions, Appearance, Dictionaries, Experiments; - icons are missing


"Speedy" Regression window (mozilla-central)
Good:
https://ftp.mozilla.org/pub/firefox/nightly/2017/05/2017-05-15-03-02-05-mozilla-central/

Bad:
https://ftp.mozilla.org/pub/firefox/nightly/2017/05/2017-05-15-03-02-05-mozilla-central/

Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e66dedabe582ba7b394aee4f89ed70fe389b3c46&tochange=b8e9b674033bcd1f3a4c59b9d0ee7619c1a17cc5

Comment 1

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: tab close button.png

Comment 2

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: new tab button.png

Comment 3

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: address bar.png

Comment 4

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: about;addons.png

Comment 5

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

(In reply to Virtual_ManPL [:Virtual] - (please needinfo? me - so I will see your comment/reply/question/etc.) from comment #0)
> - address bar
That issue is duplicate of bug #1365275,
so probably this bug is also caused by bug #1352366, so I'm blocking it.

Comment 6

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: pressed tab close button.png

Pressed close button is also affected

Comment 7

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: hovered background tab.png

Hovered background tab is also affected.

Comment 8

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: big tab close button.png

Comment 9

[Alice0775 White]

I can reproduce some problem  on Windows10 Nightly55 when Light Weight theme is installed.

Regression window:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b2b2746d51ea149bff83ea3e97efdff05bdd9a92&tochange=b8e9b674033bcd1f3a4c59b9d0ee7619c1a17cc5

Regressed by: b8e9b674033b	Kris Maglione — Bug 1365256: Don't register chrome for disabled, restartful add-ons. r=aswan a=RyanVM CLOSED TREE

@:kmag, :aswan, :RyanVM

I think that a bunch of patches should be backed out. And should not touch any codes if there are no automate tests!
Could you look this?

Comment 10

[Dale Harvey (:daleharvey)]

Yup 1365275 is a dupe of the url bar issues, fix in review

Comment 11

[Dão Gottwald [:dao]]

Can't really track this since it appears to be a collection of various unrelated issues.

Comment 12

[Alice0775 White]

[Tracking Requested - why for this release]:

I can also reproduce on Win7 Nightly55.

The problem of Icon Addon manager, NewTab hover styling, Tab Close Icon hover styling are same regression window:

Regression window:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b2b2746d51ea149bff83ea3e97efdff05bdd9a92&tochange=b8e9b674033bcd1f3a4c59b9d0ee7619c1a17cc5

Regressed by: b8e9b674033b	Kris Maglione — Bug 1365256: Don't register chrome for disabled, restartful add-ons. r=aswan a=RyanVM CLOSED TREE

Comment 13

[Ryan VanderMeulen [:RyanVM]]

I asked about backing out the addon manager patches yesterday, but there's a one-way data migration involved, so there's a lot of risk in trying to go backwards at this point too.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

And I'm also not crazy about lumping a bunch of different issues into one bug, but I guess we'll see where it goes.

Comment 15

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

FYI - Some part of this bug issues is tracked in bug #1365275 (fallout of bug #1352366),
so let's track here issues only related to fallout of bug #1365256 (please see comment #12 to see which ones).

Comment 16

[randomreporter]

I can't reproduce any of these bugs in a new profile with any of 3 built-in themes. Windows 8.1

Comment 17

[Kris Maglione [:kmag]]

Are all of these screenshots in safe mode or only some of them?

(In reply to Alice0775 White from comment #9)
> I think that a bunch of patches should be backed out.

These patches can't be backed out.

> And should not touch any codes if there are no automate tests!

There are many, many automated tests. If anything, there are too many. They're
just not very good.

Comment 18

[Alice0775 White]

(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #17)
> Are all of these screenshots in safe mode or only some of them?
> 

You should install a Lightweight Theme and enable it.

On Windows10 , Icon in addons manager are missing.
On Windows7 ,  Icon in addons manager are missing,  Tab Close button, NewTab, Background tab hover styling are broken.

Comment 19

[Alice0775 White]

And you should restart browser after enable the Lightweight Theme.

Comment 20

[Kris Maglione [:kmag]]

That doesn't answer my question. Some of the attached screenshots are clearly in safe mode. I'd like to know if there are any that aren't.

If not, it would help to have screenshots of the behavior in a non-safe-mode session.

Comment 21

[Alice0775 White]

Attachment: Screenshot, windows10 and windows7

All of the screenshots are in NON SAFE MODE.

Comment 22

[Kris Maglione [:kmag]]

attachment 8868418 [details] is very clearly in safe mode.

If these ones are not, that tells me what I need to know. Thanks.

Comment 23

[krupa raj [:krupa--use this to needinfo]]

I cannot reproduce this issue on macosx on today's nightly. 

Peter DeHaan also checked on his win10 laptop and he wasn't able to reproduce this issue either.

windows 10: https://www.dropbox.com/s/t9i69eg30a6u1gz/windows10.jpg?dl=0
macosx: https://www.dropbox.com/s/nqekz6y1231ngkt/Screenshot%202017-05-17%2012.00.24.png?dl=0

Comment 24

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

@  Kris Maglione [:kmag] - All of my screenshoots were taken in Safe Mode (to see if it's not GFX related issue),
but the issue is also reproducible in non Safe Mode (see Alice0775 White reply in Comment #21,
except 1 screenshot in top right of screen, which should have normal close buttons, instead of this big one),
and looks like that Safe Mode uncovered another issue, reported already by  Alice0775 White , which is bug #1365706.

Comment 25

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

(In reply to krupa raj [:kraj] from comment #23)
> I cannot reproduce this issue on macosx on today's nightly. 
> 
> Peter DeHaan also checked on his win10 laptop and he wasn't able to
> reproduce this issue either.
> 
> windows 10: https://www.dropbox.com/s/t9i69eg30a6u1gz/windows10.jpg?dl=0
> macosx:
> https://www.dropbox.com/s/nqekz6y1231ngkt/Screenshot%202017-05-17%2012.00.24.
> png?dl=0

Oddly, looks like he was able to reproduce some part of the issue, just see this big close button on Windows and compare it how it looks on Mac.

Comment 26

[Alice0775 White]

I can also reproduce the icon problem in addon manager on Ubuntu16.04 x64 with new profice+Lightweight Theme.

So, this is not Windows specific.

Comment 27

[Kris Maglione [:kmag]]

OK, apparently we're not loading chrome for the default theme when a persona is enabled, but I'm not sure why this only has obvious effects on some systems.

I suppose there was yet another special case in the extensions.ini code that we missed in the migration.

Comment 28

[Kris Maglione [:kmag]]

Attachment: Bug 1365509: Special case handling of enabled state for XUL themes.

Legacy XUL themes are treated specially for chrome registration when a
lightweight theme is enabled. In those cases, all XUL themes are marked as
disabled for the purposes of UI, but chrome for the default theme still needs
to be loaded. So we need to treate the `selectedTheme` as enabled for chrome
registration purposes regardless of its database disabled state.

Review commit: https://reviewboard.mozilla.org/r/140354/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/140354/

Comment 29

[Kris Maglione [:kmag]]

Attachment: Bug 1365509: Don't trigger a sync load of the add-ons DB at startup when a built-in LWT is enabled.

Review commit: https://reviewboard.mozilla.org/r/140356/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/140356/

Comment 30

[Robert Helmer [:rhelmer]]

Comment on attachment 8868751 [details]
Bug 1365509: Special case handling of enabled state for XUL themes.

https://reviewboard.mozilla.org/r/140354/#review144202

Comment 32

[Robert Helmer [:rhelmer]]

Comment on attachment 8868752 [details]
Bug 1365509: Don't trigger a sync load of the add-ons DB at startup when a built-in LWT is enabled.

https://reviewboard.mozilla.org/r/140356/#review144212

::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:827
(Diff revision 1)
>        // jank-tastic! Must synchronously load DB if the theme switches from
>        // an XPI theme to a lightweight theme before the DB has loaded,
>        // because we're called from sync XPIProvider.addonChanged
> -      logger.warn("Synchronous load of XPI database due to getAddonsByType([" +
> -        aTypes.join(", ") + "])");
> +      logger.warn(`Synchronous load of XPI database due to ` +
> +                  `getAddonsByType([${aTypes.join(", ")}]) ` +
> +                  `Stack: ${Error().stack}`);

Do you think it's useful to log the stack in the warning?

Comment 33

[Kris Maglione [:kmag]]

Comment on attachment 8868752 [details]
Bug 1365509: Don't trigger a sync load of the add-ons DB at startup when a built-in LWT is enabled.

https://reviewboard.mozilla.org/r/140356/#review144212

> Do you think it's useful to log the stack in the warning?

Yes. I had to manually add the stack log to figure out what was triggering it. It would have been much easier if it had been there from teh start.

Comment 34

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/861266a11cd2f2127e2fc0e86fa20120659ef3fc
Bug 1365509: Special case handling of enabled state for XUL themes. r=rhelmer

https://hg.mozilla.org/integration/mozilla-inbound/rev/396de980a0d585f31e06799dfba4fab2656adc3b
Bug 1365509: Don't trigger a sync load of the add-ons DB at startup when a built-in LWT is enabled. r=rhelmer

Comment 35

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f03ab34a6051ead9d3b3d3f75c482ffde72ce31
Bug 1365509: Follow-up: Sync theme XPIState again after pref flip. r=bustage

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/861266a11cd2
https://hg.mozilla.org/mozilla-central/rev/396de980a0d5
https://hg.mozilla.org/mozilla-central/rev/8f03ab34a605

Comment 37

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

I'm confirming that this issue is fixed, starting from Mozilla Firefox Nightly 55.0a1 (2017-05-20).