-
Notifications
You must be signed in to change notification settings - Fork 879
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
Fixes Brave Ads crash when parsing country code from subdivision targeting code #6812
Conversation
5d33a61
to
cd6afa2
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
@@ -57,6 +57,10 @@ bool SubdivisionTargetingFrequencyCap::DoesRespectCap( | |||
const std::string subdivision_targeting_code = | |||
ads_->get_subdivision_targeting()->GetAdsSubdivisionTargetingCode(); | |||
|
|||
if (subdivision_targeting_code.empty()) { | |||
return false; |
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.
In general, should DoesRespectCap
return false if the data is malformed? My impression, without insight in previous discussions, is that we should 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.
If subdivision targeting has not been initialized we need to return false so that the ad is not shown outside of the intended region which is a part of the cap. Thanks
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!
Resolves brave/brave-browser#12030
Submitter Checklist:
npm run lint
,npm run gn_check
)git rebase master
(if needed).git rebase -i
to squash commits (if needed).QA/Yes
orQA/No
) to the associated issuerelease-notes/include
orrelease-notes/exclude
) to the associated issueTest Plan:
GET /v5/getstate
server endpoint to 304 using a tool such as Charles ProxyReviewer Checklist:
After-merge Checklist:
changes has landed on.