Fix a race condition in BrowsingDataRemover.

Fix a race condition in BrowsingDataRemover.

BrowsingDataRemover::RemoveImpl runs on the UI thread. It schedules several
operations on other threads and flips a boolean flag to "true" for each of
them. Each operation responds with a callback on the UI thread flipping its
flag back to "false" and call NotifyIfDone(), a method that checks if all
flags are already false, in which case it reports that the deletion was
completed.

In addition, NotifyIfDone() is also called at the end of
BrowsingDataRemover::RemoveImpl - this is in case that no tasks on other
threads were scheduled.

Consider the following simplified scenario with arbitrary task "X":

BrowsingDataRemover::RemoveImpl() {
  DCHECK_CURRENTLY_ON(BrowserThread::UI);
  waiting_for_task_x_ = true;
  ScheduleTaskX(&OnXDone);

  // ...

  NotifyIfDone();
}

BrowsingDataRemover::OnXDone() {
  DCHECK_CURRENTLY_ON(BrowserThread::UI);
  waiting_for_task_x_ = false;
  NotifyIfDone();
}

While this pattern should only be used if X is on a different thread,
consider what would happen if X was on the UI thread and yielded to the UI
thread. NotifyIfDone() in OnXDOne() would find all flags false and notify
about the deletion being complete. Immediately after, NotifyIfDone() in
RemoveImpl() would do the same. The observer would receive two deletion
notifications.

This is not hypothetical - clearing with remove_mask == REMOVE_COOKIES
will run OnClearedDomainReliabilityMonitor() before RemoveImpl() is finished.

This CL fixes that by considering the execution of RemoveImpl() as a task
of its own, having its own boolean flag to indicate that the body of
RemoveImpl() has finished.

This was discovered during the implementation of a task scheduler for
BrowsingDataRemover in a different CL and will be covered against regression
by the tests of that CL. It was not caught by our tests before because
BrowsingDataRemoverCompletionObserver that is commonly used in the unittests
always unregisters itself after the first callback and thus never spotted
the second callback.

BUG=630327
Committed: https://crrev.com/2d114cdf706c786e5c9c801bd19d92d7c51aaa9e
Cr-Commit-Position: refs/heads/master@{#407154}

[msramek, 2016-07-21 17:26:15]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-21 17:26:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msramek, 2016-07-21 17:38:29]

msramek@chromium.org changed reviewers:
+ bauerb@chromium.org

[msramek, 2016-07-21 17:38:30]

Hi Bernhard,

Please have a look!

(I wonder if this could be related to the mysterious crash reports that we got
earlier this year...)

Thanks,
Martin

[commit-bot: I haz the power, 2016-07-21 18:12:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-21 18:12:53]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-07-22 14:17:03]

LGTM, thanks!

I'm not quite sure if this would cause a crash though, because I don't think we
have that many listeners that stick around longer?

[msramek, 2016-07-22 14:23:20]

Thanks!

Hmm, true. I got my hopes up... but I'll still watch if any similar reports
appear :)

[msramek, 2016-07-22 14:23:29]

The CQ bit was checked by msramek@chromium.org

[commit-bot: I haz the power, 2016-07-22 14:23:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-22 14:27:18]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-07-22 14:29:51]

Description was changed from

==========
Fix a race condition in BrowsingDataRemover.

BrowsingDataRemover::RemoveImpl runs on the UI thread. It schedules several
operations on other threads and flips a boolean flag to "true" for each of
them. Each operation responds with a callback on the UI thread flipping its
flag back to "false" and call NotifyIfDone(), a method that checks if all
flags are already false, in which case it reports that the deletion was
completed.

In addition, NotifyIfDone() is also called at the end of
BrowsingDataRemover::RemoveImpl - this is in case that no tasks on other
threads were scheduled.

Consider the following simplified scenario with arbitrary task "X":

BrowsingDataRemover::RemoveImpl() {
  DCHECK_CURRENTLY_ON(BrowserThread::UI);
  waiting_for_task_x_ = true;
  ScheduleTaskX(&OnXDone);

  // ...

  NotifyIfDone();
}

BrowsingDataRemover::OnXDone() {
  DCHECK_CURRENTLY_ON(BrowserThread::UI);
  waiting_for_task_x_ = false;
  NotifyIfDone();
}

While this pattern should only be used if X is on a different thread,
consider what would happen if X was on the UI thread and yielded to the UI
thread. NotifyIfDone() in OnXDOne() would find all flags false and notify
about the deletion being complete. Immediately after, NotifyIfDone() in
RemoveImpl() would do the same. The observer would receive two deletion
notifications.

This is not hypothetical - clearing with remove_mask == REMOVE_COOKIES
will run OnClearedDomainReliabilityMonitor() before RemoveImpl() is finished.

This CL fixes that by considering the execution of RemoveImpl() as a task
of its own, having its own boolean flag to indicate that the body of
RemoveImpl() has finished.

This was discovered during the implementation of a task scheduler for
BrowsingDataRemover in a different CL and will be covered against regression
by the tests of that CL. It was not caught by our tests before because
BrowsingDataRemoverCompletionObserver that is commonly used in the unittests
always unregisters itself after the first callback and thus never spotted
the second callback.

BUG=630327
==========

to

==========
Fix a race condition in BrowsingDataRemover.

BrowsingDataRemover::RemoveImpl runs on the UI thread. It schedules several
operations on other threads and flips a boolean flag to "true" for each of
them. Each operation responds with a callback on the UI thread flipping its
flag back to "false" and call NotifyIfDone(), a method that checks if all
flags are already false, in which case it reports that the deletion was
completed.

In addition, NotifyIfDone() is also called at the end of
BrowsingDataRemover::RemoveImpl - this is in case that no tasks on other
threads were scheduled.

Consider the following simplified scenario with arbitrary task "X":

BrowsingDataRemover::RemoveImpl() {
  DCHECK_CURRENTLY_ON(BrowserThread::UI);
  waiting_for_task_x_ = true;
  ScheduleTaskX(&OnXDone);

  // ...

  NotifyIfDone();
}

BrowsingDataRemover::OnXDone() {
  DCHECK_CURRENTLY_ON(BrowserThread::UI);
  waiting_for_task_x_ = false;
  NotifyIfDone();
}

While this pattern should only be used if X is on a different thread,
consider what would happen if X was on the UI thread and yielded to the UI
thread. NotifyIfDone() in OnXDOne() would find all flags false and notify
about the deletion being complete. Immediately after, NotifyIfDone() in
RemoveImpl() would do the same. The observer would receive two deletion
notifications.

This is not hypothetical - clearing with remove_mask == REMOVE_COOKIES
will run OnClearedDomainReliabilityMonitor() before RemoveImpl() is finished.

This CL fixes that by considering the execution of RemoveImpl() as a task
of its own, having its own boolean flag to indicate that the body of
RemoveImpl() has finished.

This was discovered during the implementation of a task scheduler for
BrowsingDataRemover in a different CL and will be covered against regression
by the tests of that CL. It was not caught by our tests before because
BrowsingDataRemoverCompletionObserver that is commonly used in the unittests
always unregisters itself after the first callback and thus never spotted
the second callback.

BUG=630327

Committed: https://crrev.com/2d114cdf706c786e5c9c801bd19d92d7c51aaa9e
Cr-Commit-Position: refs/heads/master@{#407154}
==========

[commit-bot: I haz the power, 2016-07-22 14:29:52]

Patchset 1 (id:??) landed as
https://crrev.com/2d114cdf706c786e5c9c801bd19d92d7c51aaa9e
Cr-Commit-Position: refs/heads/master@{#407154}