Fix Rewards reset failing on Windows #6274

emerick · 2020-07-30T20:17:05Z

Resolves brave/brave-browser#10934

Submitter Checklist:

Test Plan:

Clean profile
Enable Rewards
Click on settings icon on brave://rewards page
Choose Reset option and confirm
Verify that Rewards welcome page is shown
Verify that all Rewards-related files are deleted from profile (db, log, etc.)

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.

emerick · 2020-07-30T20:22:47Z

components/brave_rewards/browser/rewards_service_impl.cc

+
+  // Close any open files before deleting them below (required on
+  // Windows)
+  file_task_runner_->DeleteSoon(FROM_HERE, rewards_database_.release());


Closing the database wasn't a great option, as it led to some cascading problems in other parts of the code that weren't expecting or testing for a closed database. Releasing the database seems safer and we handle it properly elsewhere (and it's already being done later in the reset process).

My understanding of DeleteSoon is that it fires the release immediately after unwinding the current function stack and will therefore always run before the task we post to delete the files. Let me know if that doesn't sound like a good approach though.

zenparsing · 2020-07-30T20:33:30Z

components/brave_rewards/browser/rewards_service_impl.cc

+
+  // Close any open files before deleting them below (required on
+  // Windows)
+  file_task_runner_->DeleteSoon(FROM_HERE, rewards_database_.release());


Looks like OnCompleteReset calls StopLedger which does this delete of the rewards database. Does CompleteReset need to call StopLedger before deleting all of the files? Like that would be the last thing that it does instead of the first?

I'm not opposed to something like that, but StopLedger is also asynchronous. I'm a little leery about changing this too deeply, as there could be some unintended side effects (just ran into one with the staging server on Android yesterday in fact).

In the ideal world, it seems like the OnCompleteReset callback should do nothing but call observers and run its success callback. CompleteReset should shutdown ledger and then (when that completes) close and delete files. That approach is definitely a little more complicated than the current solution, but also feels a bit more "correct". What do you think @NejcZdovc?

@emerick sure, adjust PR and let's see how it will work

zenparsing · 2020-07-31T18:15:53Z

components/brave_rewards/browser/rewards_service_impl.cc

 }

-void RewardsServiceImpl::OnStopLedger(const ledger::Result result) {
+void RewardsServiceImpl::OnStopLedgerInternal(


In order to minimize confusion between OnStopLedgerInternal and OnStopLedger we might want to name things a bit differently. Maybe the new one should be something along the lines of:

OnStopLedgerForCompleteReset

OnCompleteResetStopLedger

OnCompleteResetLedgerStopped

etc.

Great suggestion, I went with the first one - thanks!

NejcZdovc · 2020-07-31T19:02:31Z

components/brave_rewards/browser/rewards_service_impl.cc

@@ -1458,7 +1458,7 @@ void RewardsServiceImpl::SetRewardsMainEnabled(bool enabled) {

  if (!enabled) {
    RecordRewardsDisabledForSomeMetrics();
-    StopLedger();
+    StopLedger(base::BindOnce([](ledger::Result) {}));


you can just do StopLedger(base::DoNothing())

Really helpful! Fixed.

NejcZdovc

Just tried it on macOS and it crashes

[1650:775:0731/212314.492165:FATAL:thread_restrictions.cc(78)] Check failed: !g_blocking_disallowed.Get().Get(). Function marked as blocking was called from a scope that disallows blocking! If this task is running inside the ThreadPool, it needs to have MayBlock() in its TaskTraits. Otherwise, consider making this blocking work asynchronous or, as a last resort, you may use ScopedAllowBlocking (see its documentation for best practices).
g_blocking_disallowed currently set to true by
0   libbase.dylib                       0x00000001042dd119 base::debug::CollectStackTrace(void**, unsigned long) + 9
1   libbase.dylib                       0x00000001041a3e33 base::debug::StackTrace::StackTrace() + 19
2   libbase.dylib                       0x0000000104295c12 base::DisallowUnresponsiveTasks() + 50
3   libcontent.dylib                    0x0000000111407b52 content::BrowserMainLoop::PreMainMessageLoopRun() + 98
4   libcontent.dylib                    0x0000000111aea307 content::StartupTaskRunner::RunAllTasksNow() + 119
5   libcontent.dylib                    0x0000000111406a28 content::BrowserMainLoop::CreateStartupTasks() + 744
6   libcontent.dylib                    0x0000000111409b14 content::BrowserMainRunnerImpl::Initialize(content::MainFunctionParams const&) + 100
7   libcontent.dylib                    0x0000000111404d1b content::BrowserMain(content::MainFunctionParams const&) + 251
8   libcontent.dylib                    0x000000011206379c content::ContentMainRunnerImpl::RunServiceManager(content::MainFunctionParams&, bool) + 1036
9   libcontent.dylib                    0x0000000112063363 content::ContentMainRunnerImpl::Run(bool) + 467
10  libembedder.dylib                   0x0000000104149389 service_manager::Main(service_manager::MainParams const&) + 3321
11  libcontent.dylib                    0x0000000112062668 content::ContentMain(content::ContentMainParams const&) + 120
12  libchrome_dll.dylib                 0x00000001050f4d75 ChromeMain + 277
13  Brave Browser Development           0x0000000103a0d69f main + 287
14  libdyld.dylib                       0x00007fff68508cc9 start + 1

0   libbase.dylib                       0x00000001042dd119 base::debug::CollectStackTrace(void**, unsigned long) + 9
1   libbase.dylib                       0x00000001041a3e33 base::debug::StackTrace::StackTrace() + 19
2   libbase.dylib                       0x00000001041c5185 logging::LogMessage::~LogMessage() + 261
3   libbase.dylib                       0x00000001041c5e8e logging::LogMessage::~LogMessage() + 14
4   libbase.dylib                       0x0000000104294cd5 base::internal::AssertBlockingAllowed() + 149
5   libbase.dylib                       0x000000010428b459 base::ScopedBlockingCall::ScopedBlockingCall(base::Location const&, base::BlockingType) + 121
6   libbase.dylib                       0x00000001042e1108 base::File::Close() + 152
7   libchrome_dll.dylib                 0x0000000105aa7fe7 brave_rewards::RewardsServiceImpl::OnStopLedgerForCompleteReset(base::OnceCallback<void (bool)>, ledger::mojom::Result) + 279
8   libchrome_dll.dylib                 0x0000000105ac47db void base::internal::InvokeHelper<true, void>::MakeItSo<void (brave_rewards::RewardsServiceImpl::*)(base::OnceCallback<void (bool)>, ledger::mojom::Result), base::WeakPtr<brave_rewards::RewardsServiceImpl>, base::OnceCallback<void (bool)>, ledger::mojom::Result>(void (brave_rewards::RewardsServiceImpl::*&&)(base::OnceCallback<void (bool)>, ledger::mojom::Result), base::WeakPtr<brave_rewards::RewardsServiceImpl>&&, base::OnceCallback<void (bool)>&&, ledger::mojom::Result&&) + 203
9   libchrome_dll.dylib                 0x0000000105ac4703 base::internal::Invoker<base::internal::BindState<void (brave_rewards::RewardsServiceImpl::*)(base::OnceCallback<void (bool)>, ledger::mojom::Result), base::WeakPtr<brave_rewards::RewardsServiceImpl>, base::OnceCallback<void (bool)> >, void (ledger::mojom::Result)>::RunOnce(base::internal::BindStateBase*, ledger::mojom::Result) + 35
10  libchrome_dll.dylib                 0x0000000105aa7eb2 brave_rewards::RewardsServiceImpl::OnStopLedger(base::OnceCallback<void (ledger::mojom::Result)>, ledger::mojom::Result) + 994
11  libchrome_dll.dylib                 0x0000000105ac11fb void base::internal::InvokeHelper<true, void>::MakeItSo<void (brave_rewards::RewardsServiceImpl::*)(base::OnceCallback<void (ledger::mojom::Result)>, ledger::mojom::Result), base::WeakPtr<brave_rewards::RewardsServiceImpl>, base::OnceCallback<void (ledger::mojom::Result)>, ledger::mojom::Result>(void (brave_rewards::RewardsServiceImpl::*&&)(base::OnceCallback<void (ledger::mojom::Result)>, ledger::mojom::Result), base::WeakPtr<brave_rewards::RewardsServiceImpl>&&, base::OnceCallback<void (ledger::mojom::Result)>&&, ledger::mojom::Result&&) + 203
12  libchrome_dll.dylib                 0x0000000105ac1123 base::internal::Invoker<base::internal::BindState<void (brave_rewards::RewardsServiceImpl::*)(base::OnceCallback<void (ledger::mojom::Result)>, ledger::mojom::Result), base::WeakPtr<brave_rewards::RewardsServiceImpl>, base::OnceCallback<void (ledger::mojom::Result)> >, void (ledger::mojom::Result)>::RunOnce(base::internal::BindStateBase*, ledger::mojom::Result) + 35
13  libchrome_dll.dylib                 0x0000000105b15d21 bat_ledger::mojom::BatLedger_Shutdown_ForwardToCallback::Accept(mojo::Message*) + 257
14  libbindings.dylib                   0x0000000104819656 mojo::InterfaceEndpointClient::HandleValidatedMessage(mojo::Message*) + 1462
15  libbindings.dylib                   0x000000010481d71b mojo::MessageDispatcher::Accept(mojo::Message*) + 251
16  libbindings.dylib                   0x000000010481a9b1 mojo::InterfaceEndpointClient::HandleIncomingMessage(mojo::Message*) + 97
17  libbindings.dylib                   0x0000000104823848 mojo::internal::MultiplexRouter::ProcessIncomingMessage(mojo::internal::MultiplexRouter::MessageWrapper*, mojo::internal::MultiplexRouter::ClientCallBehavior, base::SequencedTaskRunner*) + 1400
18  libbindings.dylib                   0x0000000104822d10 mojo::internal::MultiplexRouter::Accept(mojo::Message*) + 400
19  libbindings.dylib                   0x000000010481d71b mojo::MessageDispatcher::Accept(mojo::Message*) + 251
20  libbindings.dylib                   0x0000000104811dd4 mojo::Connector::DispatchMessage(mojo::Message) + 452
21  libbindings.dylib                   0x00000001048128b7 mojo::Connector::ReadAllAvailableMessages() + 279

emerick · 2020-07-31T20:08:41Z

Fixed crash, I had been testing in release mode which doesn't crash on blocking calls. Great catch @NejcZdovc.

GeetaSarvadnya · 2020-08-03T18:29:55Z

Verification passed on


Brave | 1.14.12 Chromium: 84.0.4147.105 (Official Build) nightly (64-bit)
-- | --
Revision | a6b12dfad6663f13a7e16e9a42a6a4975374096b-refs/branch-heads/4147@{#943}
OS | Windows 10 OS Version 1903 (Build 18362.959)

Verified the test plan mentioned in the PR
Verified the description from the issue Add a setting to be able to completely reset rewards brave-browser#10064
Encountered Temporary work-around for P3A (if there is a hang) #10940 and Improved Brave Wallets Keyboard Accessibility #10937
Logged Next payment date is null in Ads panel brave-browser#11062

emerick added the feature/rewards label Jul 30, 2020

emerick requested review from zenparsing and NejcZdovc July 30, 2020 20:17

emerick self-assigned this Jul 30, 2020

emerick commented Jul 30, 2020

View reviewed changes

zenparsing reviewed Jul 30, 2020

View reviewed changes

emerick force-pushed the fix-rewards-reset-windows branch 3 times, most recently from 3a53f29 to 707c40f Compare July 31, 2020 17:54

emerick requested a review from zenparsing July 31, 2020 17:54

zenparsing reviewed Jul 31, 2020

View reviewed changes

emerick force-pushed the fix-rewards-reset-windows branch 2 times, most recently from 0cd68a6 to b8af1e7 Compare July 31, 2020 18:57

NejcZdovc reviewed Jul 31, 2020

View reviewed changes

emerick force-pushed the fix-rewards-reset-windows branch from b8af1e7 to 0635cfe Compare July 31, 2020 19:06

emerick requested review from zenparsing and NejcZdovc July 31, 2020 19:07

zenparsing approved these changes Jul 31, 2020

View reviewed changes

NejcZdovc suggested changes Jul 31, 2020

View reviewed changes

emerick force-pushed the fix-rewards-reset-windows branch from 0635cfe to 8e3ce4e Compare July 31, 2020 20:07

emerick requested a review from NejcZdovc July 31, 2020 20:15

Fix Rewards reset failing on Windows

eb52c3a

emerick force-pushed the fix-rewards-reset-windows branch from 8e3ce4e to eb52c3a Compare July 31, 2020 23:44

NejcZdovc approved these changes Aug 3, 2020

View reviewed changes

NejcZdovc added this to the 1.14.x - Nightly milestone Aug 3, 2020

NejcZdovc merged commit db8bb27 into master Aug 3, 2020

NejcZdovc deleted the fix-rewards-reset-windows branch August 3, 2020 05:49

This was referenced Aug 3, 2020

Fixes storage not persist after reset (uplift to 1.13.x) #6299

Merged

Fixes storage not persist after reset (uplift to 1.12.x) #6300

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Rewards reset failing on Windows #6274

Fix Rewards reset failing on Windows #6274

emerick commented Jul 30, 2020 •

edited

Loading

emerick Jul 30, 2020

zenparsing Jul 30, 2020

emerick Jul 30, 2020

emerick Jul 30, 2020 •

edited

Loading

NejcZdovc Jul 31, 2020

zenparsing Jul 31, 2020

emerick Jul 31, 2020

NejcZdovc Jul 31, 2020

emerick Jul 31, 2020

NejcZdovc left a comment •

edited

Loading

emerick commented Jul 31, 2020

GeetaSarvadnya commented Aug 3, 2020

Fix Rewards reset failing on Windows #6274

Fix Rewards reset failing on Windows #6274

Conversation

emerick commented Jul 30, 2020 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

After-merge Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emerick Jul 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NejcZdovc left a comment • edited Loading

Choose a reason for hiding this comment

emerick commented Jul 31, 2020

GeetaSarvadnya commented Aug 3, 2020

emerick commented Jul 30, 2020 •

edited

Loading

emerick Jul 30, 2020 •

edited

Loading

NejcZdovc left a comment •

edited

Loading