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

Fixes application crashes when calculating the winning page classification category #4770

Merged
merged 1 commit into from
Feb 28, 2020

Conversation

tmancey
Copy link
Collaborator

@tmancey tmancey commented Feb 27, 2020

Resolves brave/brave-browser#7866

Submitter Checklist:

Test Plan:

See brave/brave-browser#7866

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

Copy link
Contributor

@emerick emerick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@SergeyZhukovsky SergeyZhukovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comments

@@ -872,6 +879,7 @@ std::vector<std::string> AdsImpl::GetWinningCategories() {

for (const auto& page_score : page_score_history) {
DCHECK(page_score.size() == count);
DCHECK(user_model_);
Copy link
Member

@SergeyZhukovsky SergeyZhukovsky Feb 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure we don't need a check on a null pointer for user_model_? As that is the reason of the crash. Maybe we should fail with empty winning_categories?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've discussed this on a call, the place should never be reached when the pointer is null. Let's
apply the fix and reopen the issue if we still see it.

Copy link
Member

@SergeyZhukovsky SergeyZhukovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++

@tmancey
Copy link
Collaborator Author

tmancey commented Feb 28, 2020

CI failed on Linux, restarting CI for Linux

@tmancey tmancey added CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-windows labels Feb 28, 2020
@tmancey
Copy link
Collaborator Author

tmancey commented Feb 28, 2020

CI failing on Linux for unrelated issues, merging

@tmancey tmancey merged commit 27c8f29 into master Feb 28, 2020
@tmancey tmancey deleted the issues/7866 branch February 28, 2020 10:56
@tmancey tmancey removed CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-windows PR/Pending Review labels Feb 28, 2020
@NejcZdovc
Copy link
Contributor

checked CI with @tmancey, linux looks unrelated

@kjozwiak
Copy link
Member

kjozwiak commented Mar 2, 2020

Reproduced the original issue on macOS 10.15.3 x64 using the following build:

Brave 1.7.26 Chromium: 80.0.3987.122 (Official Build) nightly (64-bit)
Revision cf72c4c4f7db75bc3da689cd76513962d31c7b52-refs/branch-heads/3987@{#943}
OS macOS Version 10.15.3 (Build 19D76)

Using the STR from #4770 (comment), ran into the following crash/error:

[0302/112312.477864:WARNING:process_memory_mac.cc(93)] mach_vm_read(0x7ffee9edb000, 0x2000): (os/kern) invalid address (1)
[0302/112312.560187:WARNING:system_snapshot_mac.cc(42)] sysctlbyname kern.nx: No such file or directory (2)

Verification PASSED on macOS 10.15.3 x64 using the following build:

Brave 1.7.32 Chromium: 80.0.3987.122 (Official Build) nightly (64-bit)
Revision cf72c4c4f7db75bc3da689cd76513962d31c7b52-refs/branch-heads/3987@{#943}
OS macOS Version 10.15.3 (Build 19D76)

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

Successfully merging this pull request may close these issues.

Application crashes when calculating the winning page classification category
5 participants