Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Debug crash when changing theme color (due to VPN button) #21190

Closed
Tracked by #15804
bsclifton opened this issue Feb 18, 2022 · 2 comments · Fixed by brave/brave-core#12351
Closed
Tracked by #15804

Debug crash when changing theme color (due to VPN button) #21190

bsclifton opened this issue Feb 18, 2022 · 2 comments · Fixed by brave/brave-core#12351

Comments

@bsclifton
Copy link
Member

bsclifton commented Feb 18, 2022

Description

There is a debug crash after changing theme.

I think we need to add a value in https://github.com/brave/brave-core/blob/master/browser/themes/theme_properties.cc representing the color used for VPN button. It's looking for ID 10029 and not finding it.

Steps to Reproduce

  1. New profile
  2. Set brave://flags/#skus-sdk to ENABLED
  3. Set brave://flags/#brave-vpn to ENABLED
  4. Restart browser
  5. Visit brave://settings/manageProfile
  6. Change your theme color

Actual result:

The console shows:

[11952:56368:0217/233156.255:FATAL:theme_properties.cc(133)] Check failed: false. This color should only be queried through ThemeService.
Backtrace:
        base::debug::CollectStackTrace [0x00007FF9A8D86B72+18] (C:\bb2\src\base\debug\stack_trace_win.cc:305)
        base::debug::StackTrace::StackTrace [0x00007FF9A8C58142+18] (C:\bb2\src\base\debug\stack_trace.cc:197)
        logging::LogMessage::~LogMessage [0x00007FF9A8C789BA+138] (C:\bb2\src\base\logging.cc:588)
        logging::LogMessage::~LogMessage [0x00007FF9A8C79D10+16] (C:\bb2\src\base\logging.cc:581)
        `anonymous namespace'::GetLightModeColor [0x00007FF98D94402B+551] (C:\bb2\src\chrome\browser\themes\theme_properties.cc:133)
        ThemeProperties::GetDefaultColor [0x00007FF98D943C46+128] (C:\bb2\src\chrome\browser\themes\theme_properties.cc:309)
        ThemeHelper::GetColor [0x00007FF98CE76C18+352] (C:\bb2\src\chrome\browser\themes\theme_helper.cc:297)
        ThemeService::BrowserThemeProvider::GetColor [0x00007FF98CE70DA4+148] (C:\bb2\src\chrome\browser\themes\theme_service.cc:250)
        BraveVPNButton::UpdateColorsAndInsets [0x00007FF98B69390B+283] (C:\bb2\src\brave\browser\ui\views\toolbar\brave_vpn_button.cc:102)
        ToolbarButton::OnThemeChanged [0x00007FF98C9EF208+24] (C:\bb2\src\chrome\browser\ui\views\toolbar\toolbar_button.cc:448)

If you comment out the DCHECK or run a release build, it shows the border as red:
image

Expected result:

It should have a nice contrasted border, similar to defaults on dark mode and light mode
image
image

Reproduces how often:

100%

simonhong added a commit to brave/brave-core that referenced this issue Feb 22, 2022
fix brave/brave-browser#21190

So far BRAVE_THEME_PROPERTIES_LAST is not the last brave theme color id.
Due to this, vpn button's color id was higher than BRAVE_THEME_PROPERTIES_LAST.
This made BraveThemeProperties::IsBraveThemeProperties() gives false for vpn button's color id.
@simonhong simonhong added this to the 1.37.x - Nightly milestone Feb 22, 2022
@stephendonner
Copy link

Verified PASSED using

Brave 1.37.65 Chromium: 99.0.4844.35 (Official Build) nightly (64-bit)
Revision f60a827ddb87f1c403e07713751a5551d5856ac0-refs/branch-heads/4844@{#579}
OS Windows 10 Version 21H2 (Build 19044.1566)

Steps:

  1. installed 1.37.65
  2. launched Brave
  3. set brave://flags/#skus-sdk to Enabled
  4. set brave://flags/#brave-vpn to Enabled
  5. clicked on the Relaunch button
  6. opened brave://settings/manageProfile
  7. changed themes a few times, restarted, etc.

Confirmed the theme switches took, and the VPN button's border was its normal color.

example example
21190-2 21190-3

Filed #21242.

@bsclifton
Copy link
Member Author

bsclifton commented Mar 3, 2022

Marking as QA/No because it needs to be a debug build in order to crash. Situations with a Release build would fail too - where it would showing a RED border around the VPN button (see picture in original post). We could test for that actually - but unnecessary IMO

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment