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

[ads] Fix intermittent crash on database call during shutdown #25438

Merged
merged 1 commit into from
Sep 6, 2024

Conversation

aseren
Copy link
Collaborator

@aseren aseren commented Sep 5, 2024

This is a fix of intermittent crash happening during browser shutdown, which is hard to reproduce. Crash happens only for version 1.71.x.

Resolves brave/brave-browser#40791

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

This is a fix of intermittent crash happening during browser shutdown, which is hard to reproduce. No manual testing is required.

Copy link
Contributor

github-actions bot commented Sep 6, 2024

[puLL-Merge] - brave/brave-core@25438

Description

This PR changes the way pointers to database-related objects are passed around in the Brave Ads codebase. It replaces raw pointers with smart pointers (mojom::DBTransactionInfoPtr, mojom::DBActionInfoPtr, etc.) for better memory management and safety. The changes are consistent across multiple files, updating function signatures and how these objects are accessed.

Possible Issues

  1. There's a potential for introducing subtle bugs if any part of the codebase was relying on the previous pointer behavior.
  2. Performance impact might need to be evaluated, as smart pointers can introduce a small overhead compared to raw pointers.
Changes

Changes

  1. components/brave_ads/core/internal/account/confirmations/queue/confirmation_queue_database_table.cc

    • Updated function signatures to use smart pointers
    • Changed how database objects are accessed (e.g., &*mojom_db_action to mojom_db_action)
  2. components/brave_ads/core/internal/account/confirmations/queue/confirmation_queue_database_table.h

    • Updated function signatures to use smart pointers
  3. components/brave_ads/core/internal/account/deposits/deposits_database_table.cc

    • Similar changes as in the confirmation queue file
  4. components/brave_ads/core/internal/account/deposits/deposits_database_table.h

    • Updated function signatures
  5. components/brave_ads/core/internal/account/transactions/transactions_database_table.cc

    • Updated function signatures and object access
  6. components/brave_ads/core/internal/account/transactions/transactions_database_table.h

    • Updated function signatures
  7. components/brave_ads/core/internal/common/database/database_column_util.cc

    • Updated function signatures and implementations to use smart pointers
  8. components/brave_ads/core/internal/common/database/database_column_util.h

    • Updated function declarations to use smart pointers
  9. components/brave_ads/core/internal/common/database/database_table_util.cc

    • Updated function implementations to use smart pointers
  10. components/brave_ads/core/internal/common/database/database_table_util.h

    • Updated function declarations
  11. components/brave_ads/core/internal/common/database/database_transaction_util.cc

    • Updated function implementations and error checking logic
  12. components/brave_ads/core/internal/common/database/database_transaction_util.h

    • Updated function declarations
  13. Multiple other files in the creatives, history, and user engagement directories

    • Similar changes updating function signatures and smart pointer usage
  14. components/brave_ads/core/internal/database/database.cc

    • Updated the Database class implementation to use smart pointers
  15. components/brave_ads/core/internal/database/database_manager.cc

    • Updated error checking logic to use smart pointers
  16. components/brave_ads/core/internal/database/database_table_interface.cc and .h

    • Updated the TableInterface to use smart pointers
  17. components/brave_ads/core/internal/legacy_migration/database/*.cc

    • Updated migration-related functions to use smart pointers
  18. components/brave_ads/core/public/database/database.h

    • Updated the public Database class interface to use smart pointers

These changes represent a significant refactoring of the database-related code in Brave Ads to use smart pointers consistently. This should lead to improved memory management and potentially fewer memory-related bugs.

@aseren aseren merged commit b3e981d into master Sep 6, 2024
19 checks passed
@aseren aseren deleted the issues/40791 branch September 6, 2024 19:02
@github-actions github-actions bot added this to the 1.71.x - Nightly milestone Sep 6, 2024
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.

[ads] Database calls intermittently fail during browser shutdown
2 participants