-
Notifications
You must be signed in to change notification settings - Fork 901
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
Migrate sidebar item pref for built-in items which do not specify built-in-item-type #14813
Conversation
6e62806
to
44635a4
Compare
DCHECK(iter != default_items.end()); | ||
if (iter == default_items.end()) { | ||
// This should not happen, since |LoadSidebarItems| ignores items which | ||
// aren't included in |GetDefaultSidebarItems|. | ||
LOG(ERROR) << "Bad built-in item loaded, cannot match to : " | ||
<< static_cast<int>(added_item.type); | ||
} else { | ||
default_items.erase(iter); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is where the crash was, I would guess. It shouldn't get here anymore because of the fix earlier in the stack, but it's better to keep a check here.
@@ -421,6 +421,64 @@ TEST_F(SidebarServiceTest, MigratePrefSidebarBuiltInItemsNoneHidden) { | |||
EXPECT_EQ(3, index); | |||
} | |||
|
|||
// Verify service has migrated the previous pref format where built-in items | |||
// had url stored and not built-in-item-type. | |||
TEST_F(SidebarServiceTest, MigratePrefSidebarBuiltInItemsNoType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I verified that this new test fails with the version of SidebarService previous to this PR, and passes with this PR.
44b7b7b
to
4bc5b60
Compare
if (!built_in_type_value.has_value()) { | ||
VLOG(1) << "built-in item did not have a type: " | ||
<< item.DebugString(); | ||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These lines solve the issue since we should skip parsing the item if we have bad data. The previous version of this line was missing the continue
. Plus it isn't immediately obvious that:
if (const auto value =
item.FindIntKey(kSidebarItemBuiltInItemTypeKey)
is the same as:
const auto value = item.FindIntKey(kSidebarItemBuiltInItemTypeKey);
if (!value.has_value())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
continue; | ||
} | ||
auto id = | ||
static_cast<SidebarItem::BuiltInItemType>(*built_in_type_value); | ||
auto iter = std::find_if(default_items_to_add.begin(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
base::ranges::find_if
4bc5b60
to
5f33f62
Compare
I have addressed all feedback and rebased on #14800 so that it is ready. We can even merge all together in this PR for most optimized CI and merge and easy uplift. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…lt-in-item-type Avoid crash by not trying to load them, but also migrate these items to include the item type key
5f33f62
to
dd2c242
Compare
Avoid crash by not trying to load them, but also migrate these items to include the item type key, bringing back the function to find built-in item type from url.
This crash was occuring on Nightly for some users who had modified sidebar preferences close to the time that it was initially released and not modified the preferences since.
Resolves brave/brave-browser#24965
Additionally, from #14800:
Resolves https://github.com/brave/internal/issues/899
Resolves https://github.com/brave/internal/issues/900
Resolves https://github.com/brave/internal/issues/901
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
,npm run lint
,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan:
On any profile, open the Preferences file in a code editor, and replace the "brave.sidebar" section with the following: