Do not start the ledger process when attempting to get publisher info #8036
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@zenparsing Needs an issue in the description... |
|
CI failed on unrelated |
zenparsing
added a commit
that referenced
this pull request
Feb 23, 2021
zenparsing
added a commit
that referenced
this pull request
Feb 23, 2021
This was referenced Feb 23, 2021
Merged
|
Verified using Reproduced issue with STR from original issue and Example LogsAlso confirmed I see connections being made in Fiddler Everywhere when using
Using Launched with
Confirmed no connections made to Verification PASSED on Using Endpoints:
Ensured that the above endpoints weren't being contacted by looking at the terminal output/Charles Proxy when launching Brave for the first time on a clean profile. Also ensured that the above endpoints are not being contacted when visiting the following: Ensured that clicking on the Clicking on BAT icon or inline Tip icon[3044:28716:0223/114643.234:VERBOSE1:rewards_service_impl.cc(429)] Starting ledger process [3044:28716:0223/114643.267:VERBOSE1:database_migration.cc(116)] DB: Migrated to version 1 [3044:28716:0223/114643.268:VERBOSE1:database_migration.cc(116)] DB: Migrated to version 2 [3044:28716:0223/114643.268:VERBOSE1:database_migration.cc(116)] DB: Migrated to version 3 [3044:28716:0223/114643.268:VERBOSE1:database_migration.cc(116)] DB: Migrated to version 4 [3044:28716:0223/114643.269:VERBOSE1:database_migration.cc(116)] DB: Migrated to version 5 [3044:28716:0223/114643.269:VERBOSE1:database_migration.cc(116)] DB: Migrated to version 6 [3044:28716:0223/114643.269:VERBOSE1:database_migration.cc(116)] DB: Migrated to version 7 [3044:28716:0223/114643.269:VERBOSE1:database_migration.cc(116)] DB: Migrated to version 8 [3044:28716:0223/114643.269:VERBOSE1:database_migration.cc(116)] DB: Migrated to version 9 [3044:28716:0223/114643.269:VERBOSE1:database_migration.cc(116)] DB: Migrated to version 10 [3044:28716:0223/114643.269:VERBOSE1:database_migration.cc(116)] DB: Migrated to version 11 [3044:28716:0223/114643.270:VERBOSE1:database_migration.cc(116)] DB: Migrated to version 12 [3044:28716:0223/114643.270:VERBOSE1:database_migration.cc(116)] DB: Migrated to version 13 [3044:28716:0223/114643.270:VERBOSE1:database_migration.cc(116)] DB: Migrated to version 14 [3044:28716:0223/114643.270:VERBOSE1:database_migration.cc(116)] DB: Migrated to version 15 [3044:28716:0223/114643.271:VERBOSE1:database_migration.cc(116)] DB: Migrated to version 16 [3044:28716:0223/114643.271:VERBOSE1:database_migration.cc(116)] DB: Migrated to version 17 [3044:28716:0223/114643.271:VERBOSE1:database_migration.cc(116)] DB: Migrated to version 18 [3044:28716:0223/114643.271:VERBOSE1:database_migration.cc(116)] DB: Migrated to version 19 [3044:28716:0223/114643.271:VERBOSE1:database_migration.cc(116)] DB: Migrated to version 20 [3044:28716:0223/114643.271:VERBOSE1:database_migration.cc(116)] DB: Migrated to version 21 [3044:28716:0223/114643.271:VERBOSE1:database_migration.cc(116)] DB: Migrated to version 22 [3044:28716:0223/114643.271:VERBOSE1:database_migration.cc(116)] DB: Migrated to version 23 [3044:28716:0223/114643.272:VERBOSE1:database_migration.cc(116)] DB: Migrated to version 24 [3044:28716:0223/114643.272:VERBOSE1:database_migration.cc(116)] DB: Migrated to version 25 [3044:28716:0223/114643.272:VERBOSE1:database_migration.cc(116)] DB: Migrated to version 26 [3044:28716:0223/114643.272:VERBOSE1:database_migration.cc(116)] DB: Migrated to version 27 [3044:28716:0223/114643.272:VERBOSE1:database_migration.cc(116)] DB: Migrated to version 28 [3044:28716:0223/114643.272:VERBOSE1:database_migration.cc(116)] DB: Migrated to version 29 [3044:28716:0223/114643.312:VERBOSE1:state_migration.cc(50)] Fresh install, state version set to 8 [3044:28716:0223/114643.313:VERBOSE1:publisher_prefix_list_updater.cc(58)] Scheduling publisher prefix list update in 0 seconds [3044:28716:0223/114643.313:VERBOSE1:contribution.cc(216)] Last reconcile timer set for 2.592e+06 s [3044:28716:0223/114643.314:VERBOSE1:contribution.cc(89)] Queue timer set for 11 s [3044:28716:0223/114643.314:VERBOSE1:promotion.cc(95)] Migrating corrupted promotions [3044:28716:0223/114643.314:VERBOSE5:ledger_impl.cc(133)] [ REQUEST ] > URL: https://api.rewards.brave.com/v1/parameters > Method: UrlMethod::GET [3044:28716:0223/114643.314:VERBOSE1:recovery.cc(22)] Running empty balance check... [3044:28716:0223/114643.314:VERBOSE1:publisher_prefix_list_updater.cc(65)] Fetching publisher prefix list [3044:28716:0223/114643.315:VERBOSE5:ledger_impl.cc(133)] [ REQUEST ] > URL: https://rewards.brave.com/publishers/prefix-list > Method: UrlMethod::GET [3044:28716:0223/114643.315:VERBOSE5:ledger_impl.cc(133)] [ REQUEST ] > URL: https://grant.rewards.brave.com/v1/promotions?migrate=true&platform=windows > Method: UrlMethod::GET [3044:28716:0223/114643.326:VERBOSE1:promotion.cc(597)] Promotion is empty [3044:28716:0223/114643.326:VERBOSE1:database_server_publisher_banner.cc(142)] Server publisher banner not found [3044:28716:0223/114643.327:VERBOSE1:recovery_empty_balance.cc(110)] Creds batch list is emtpy [3044:28716:0223/114643.328:VERBOSE5:ledger_impl.cc(133)] [ REQUEST ] > URL: https://pcdn.brave.com/publishers/prefixes/fb67 > Method: UrlMethod::GET [3044:28716:0223/114643.465:VERBOSE6:logging_util.cc(136)] [ RESPONSE - OnRequest ] > Url: https://grant.rewards.brave.com/v1/promotions?migrate=true&platform=windows > Result: Success > HTTP Code: 200 > Body: {"promotions":[]}[3044:28716:0223/114643.659:VERBOSE6:logging_util.cc(136)]
[3044:28716:0223/114643.659:VERBOSE1:api_parameters.cc(97)] Params timer set for 11871 s
|
bridiver
reviewed
Apr 22, 2021
| if (profile_->GetPrefs()->GetBoolean(prefs::kAutoContributeEnabled)) | ||
| return true; | ||
|
|
||
| auto* ads_service = brave_ads::AdsServiceFactory::GetForProfile(profile_); |
There was a problem hiding this comment.
This is definitely not the right thing to do here and the previous IsRewardsEnabled was intentionally removed. We should always be using ads::prefs::kEnabled for this checks and really this method should go away and prefs::kEnabled should be set to true/false based on the value of the other prefs as needed. cc @tmancey because ads_service->IsEnabled() should also be removed
There was a problem hiding this comment.
this is also the root cause of a crash in the tests because the timing of it triggers AdsService to be added twice to the dependency graph
There was a problem hiding this comment.
bridiver
reviewed
Apr 22, 2021
| @@ -1931,6 +1931,11 @@ void RewardsServiceImpl::GetPublisherInfo( | |||
| const std::string& publisher_key, | |||
| GetPublisherInfoCallback callback) { | |||
| if (!Connected()) { | |||
There was a problem hiding this comment.
unrelated to the problem mentioned below, it seems like it would make more sense to have MaybeStartProcess which checks !Connected() and checks to make sure rewards is enabled
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves brave/brave-browser#14307
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_tests,npm run lint,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan: