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

windows appears to sometimes move the unique_ptr and send an empty Pu… #527

Merged
merged 1 commit into from
Sep 29, 2018

Conversation

bridiver
Copy link
Collaborator

…blisherInfo to SavePublisherInfoOnFileTaskRunner. Explicitly make a copy before binding the callback

fix brave/brave-browser#1326

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.
  • Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone

Test Plan:

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

…blisherInfo to SavePublisherInfoOnFileTaskRunner. Explicitly make a copy before binding the callback

fix brave/brave-browser#1326
@bridiver bridiver self-assigned this Sep 29, 2018
@@ -575,9 +575,10 @@ void RewardsServiceImpl::OnPublisherStateSaved(
void RewardsServiceImpl::SavePublisherInfo(
std::unique_ptr<ledger::PublisherInfo> publisher_info,
ledger::PublisherInfoCallback callback) {
ledger::PublisherInfo info_copy = *publisher_info;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not sure why this fixes it because it shouldn't actually change anything, but I can't repro after this change. Maybe a compiler bug?

@bbondy
Copy link
Member

bbondy commented Sep 29, 2018

As per slack you mentioned it works after this fix but not before and you could repro, so merging. But I don't understand why this should help.

@bbondy bbondy merged commit 1c64265 into master Sep 29, 2018
bbondy added a commit that referenced this pull request Sep 29, 2018
windows appears to sometimes move the unique_ptr and send an empty Pu…
bbondy added a commit that referenced this pull request Sep 29, 2018
windows appears to sometimes move the unique_ptr and send an empty Pu…
@bbondy
Copy link
Member

bbondy commented Sep 29, 2018

master: 1c64265
0.56.x: d93cdda
0.55.x: a5a2b4e

@bridiver
Copy link
Collaborator Author

I don't understand either, but it was verified by @ryanml so maybe a win clang bug?

@NejcZdovc NejcZdovc deleted the issue/1326 branch October 1, 2018 02:58
@bbondy bbondy added this to the 0.55.x - Release milestone Jan 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

youtube crash after claiming the token grant
2 participants