629869 - Upgrading Firefox while using third-party theme gives unthemed scrollbars on first run after upgrade

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[mcdavis941 (sporadically reading bugmail)]

This is a follow-up to Bug 621475, which addressed the same issue for other element types.

Unlike that bug, this one is only about scrollbars.  Other element types seem to be handled correctly.

Issue: the first time you launch Firefox after upgrading from 4.0b10pre to 4.0b11pre, if you're using a third-party theme, you see most browser chrome styled by the theme, but theme styling doesn't apply to the scrollbars.  See the screenshot.

STR:
1 - Make sure you have both Fx4.0b10pre and Fx4.0b11pre installed
2 - Start Fx4.0b10pre with a fresh profile
3 - Go to about:config, set extensions.checkCompatibility.4.0b=false (may or
may not be needed based on whatever addons you're testing with)
4 - Install a theme (not a persona), select that theme, restart with it
4.1 - https://addons.mozilla.org/en-US/firefox/addon/14313/versions/ (along
with several others) will show this bug
5 - Notice everything looking relatively normal with the theme
6 - Exit Firefox
7 - Start Fx4.0b11pre (a version jump from b10pre to b11pre, representing an app
upgrade) with the same profile used in step 2
8 - Notice the checks for updates to installed addons (because it's a browser
upgrade) after which the app finishes launching normally
9 - Look at the main browser UI and notice scrollbars not using
the theme's style.

Expected Result:
- Theme applies to UI as usual.
Actual Result:
- Theme is only partially applied, resulting in a mix of themed and unthemed
elements in browser chrome.
- This error only shows up the first time Firefox is launched after an upgrade.
 After that, on every subsequent launch, it works normally and as expected.

- Vista 64
- Mozilla/5.0 (Windows NT 6.0; WOW64; rv:2.0b10pre) Gecko/20110125
Firefox/4.0b10pre
- Mozilla/5.0 (Windows NT 6.0; WOW64; rv:2.0b11pre) Gecko/20110127 Firefox/4.0b11pre

Comment 1

[mcdavis941 (sporadically reading bugmail)]

Attachment: screenshot

Comment 2

[mcdavis941 (sporadically reading bugmail)]

The behavior is the same (unthemed scrollbars on first run) after the upgrade from b11pre to the current b12pre.

Comment 3

[mcdavis941 (sporadically reading bugmail)]

Attachment: screenshot after upgrade to 4.0b13pre

It's still happening on the upgrade from b12pre to b13pre.

Comment 4

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

Confirmed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b13pre) Gecko/20110226 Firefox/4.0b13pre ID:20110226030401

Dave or Blair, anything we can do for Firefox 4? Especially for users with dark themes this is kinda odd behavior in the primary ui after an upgrade.

Comment 5

[Dave Townsend [:mossop]]

Attachment: patch rev 1

A couple of additional things need flushing after we've loaded any UI. The skin cache must be flushed to ensure the scrollbar stylesheet is reloaded (http://mxr.mozilla.org/mozilla-central/source/layout/style/nsLayoutStylesheetCache.cpp#64) and the string bundle service drops any loaded strings (http://mxr.mozilla.org/mozilla-central/source/intl/strres/src/nsStringBundle.cpp#604). It's possible that only chrome-flush-caches is needed but there is at least one place where chrome-flush-skin-caches does something additional (http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#8159) so I'd rather flush them both to be on the safe side here.

Not a lot we can automatically test here, I could write an automated test to verify that the notifications are being sent but not sure there is a lot of value in that right now. As is this is simple enough that we ought to be able to land it before RC.

Comment 6

[Robert Strong (they/them - no direct email)]

Comment on attachment 515956 [details] [diff] [review]
patch rev 1

It looks like flushCaches won't always be true when appChanged is true. Will this only happen when flushCaches is set to true? I'm specifically worried about the cases below where a) the theme could be global and b) PREF_EM_SHOW_MISMATCH_UI returns false.

    if (aAppChanged && !this.allAppGlobal) {
      // Should we show a UI or just pass the list via a pref?
      if (Prefs.getBoolPref(PREF_EM_SHOW_MISMATCH_UI, true)) {
        this.showMismatchWindow();
        flushCaches = true;
      }
      else if (this.startupChanges.appDisabled.length > 0) {
        // Remember the list of add-ons that were disabled this startup so
        // the application can notify the user however it wants to
        Services.prefs.setCharPref(PREF_EM_DISABLED_ADDONS_LIST,
                                   this.startupChanges.appDisabled.join(","));
      }
    }

Comment 7

[Robert Strong (they/them - no direct email)]

Comment on attachment 515956 [details] [diff] [review]
patch rev 1

Talked with Mossop on irc and he answered the question.

Please add a comment regarding this is needed because the early ui causes us to cache of parts of the default theme which then need to be flushed. Might be a good thing to only notify when the current theme isn't the default theme as well.

Comment 8

[Robert Strong (they/them - no direct email)]

ignore the default theme suggestions... please add to the comment regarding strings

Comment 9

[Dave Townsend [:mossop]]

Attachment: patch for checkin

Added the comments. This is an extremely low risk patch that fixes this display issue with custom themes. I think it is very safe to take for Firefox 4.

Comment 10

[Dave Townsend [:mossop]]

Landed, not sure if this is worth a litmus test, what do you think henrik?

http://hg.mozilla.org/mozilla-central/rev/a72d0f50429b

Comment 11

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

We can bundle that test with a major update test very easily. So I would keep it.

Comment 12

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

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0) Gecko/20100101 Firefox/4.0