GitLab

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

Review-Url: https://codereview.chromium.org/2173443002
Cr-Commit-Position: refs/heads/master@{#407154}