-
Notifications
You must be signed in to change notification settings - Fork 889
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
Fix crash in brave_ads migration #4558
Conversation
} | ||
|
||
return false; | ||
return GetDB().Execute(sql.c_str()); |
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.
Great work, thank you.
The issue also exists for v1 to v2 so for this PR could you also resolve BundleStateDatabase::MigrateV1toV2
as the final return false;
should also be return true;
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.
Ah I guess I stated at a later migration in the first place to not hit that one. Perhaps that's related to the other crash reports I see in the tracker.
1631be5
to
ab514f4
Compare
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++
@jonathanKingston great work and thank you for taking the time to resolve this issue. As soon as CI passes I will merge to |
@tmancey thanks, I was about to ask what the merge process was :) |
@jonathanKingston CI passed and merged with |
Fixes brave/brave-browser#8132
Submitter Checklist:
npm run lint
)git rebase master
(if needed).git rebase -i
to squash commits (if needed).Test Plan:
Upgrade from 1.5.x to 1.6.8, and confirm
DB: Error migrating database from v3 to v4
does not appear in the console logsReviewer Checklist:
After-merge Checklist:
changes has landed on.