Closed MadhaviSeelam closed 2 months ago
Thanks to @deeppandya for helping me troubleshoot and find the root cause!
The root cause is here: https://github.com/brave/brave-core/blob/7384eded6a3bbb3fb386d59c4bb77123e4373838/components/brave_vpn/browser/connection/brave_vpn_region_data_manager.cc#L125-L129
Basically, we are getting a mapping of timezones from Guardian. This has one set of region names. After merging https://github.com/brave/brave-core/pull/25089/, the region names are updated. For example, instead of us-west
the region would be na-usa
.
However, this code was never updated. It's passing the timezone region to the region code. The region parsing code does not find a match (since us-west
will never match na-usa
):
https://github.com/brave/brave-core/blob/7384eded6a3bbb3fb386d59c4bb77123e4373838/components/brave_vpn/browser/connection/brave_vpn_region_data_helper.cc#L27-L45
nullptr
is returned and this is the source of the crash
The above requires 1.71.92
or higher for 1.71.x
verification 👍
Verification PASSED
using
Brave | 1.71.96 Chromium: 129.0.6668.70 (Official Build) beta (64-bit)
-- | --
Revision | 6dc1837bd07e6dacd8b6b7b5d9f27e7bb09b5eb5
OS | Windows 11 Version 23H2 (Build 22631.4169)
step 4 | step 5 | step 6 | step 7 | step 8 | step 12 |
---|---|---|---|---|---|
nullptr
is returned and this is the source of the crash
Appreciate the region naming convention change didn't hit all the places it should have, but is there a robustness case against crashing even with the naming bug? Is the new code crashproof?
@BrendanEich good call- created https://github.com/brave/brave-browser/issues/41310 to address and self-assigned. Outlined two different ways we can try to improve things 😄
@BrendanEich good call- created #41310 to address and self-assigned. Outlined two different ways we can try to improve things 😄
Added below to prevent crash from GetRegionPtrWithNameFromRegionList()
.
// We should not reach here but if service(guardian) api give wrong list,
// it could be crashed if we return null.
// Instead, return first region in the list for safe.
CHECK(!region_list.empty());
return region_list[0].Clone();
Description
Brave crashed when credentials were refreshed on account.bravesoftware.com
Steps to reproduce
Actual result
https://github.com/user-attachments/assets/930eee46-03b2-442e-b96b-337c42be4cc9
Expected result
Brave browser should not crash
Reproduces how often
Easily reproduced
Brave version (brave://version info)
Channel information
Reproducibility
Miscellaneous information
@bsclifton @mattmcalister cc: @brave/qa-team