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

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

chrome/browser/browsing_data/browsing_data_remover.cc

@@ -380,6 +380,7 @@ void BrowsingDataRemover::RemoveImpl(
     const BrowsingDataFilterBuilder& filter_builder,
     int origin_type_mask) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  waiting_for_synchronous_clear_operations_ = true;
 
   // crbug.com/140910: Many places were calling this with base::Time() as
   // delete_end, even though they should've used base::Time::Max().
@@ -1040,6 +1041,7 @@ void BrowsingDataRemover::RemoveImpl(
   }
 
   // Notify in case all actions taken were synchronous.
+  waiting_for_synchronous_clear_operations_ = false;
   NotifyIfDone();
 
   UMA_HISTOGRAM_ENUMERATION(
@@ -1086,7 +1088,8 @@ void BrowsingDataRemover::ClearSettingsForOneTypeWithPredicate(
 }
 
 bool BrowsingDataRemover::AllDone() {
-  return !waiting_for_clear_autofill_origin_urls_ &&
+  return !waiting_for_synchronous_clear_operations_ &&
+         !waiting_for_clear_autofill_origin_urls_ &&
          !waiting_for_clear_cache_ &&
          !waiting_for_clear_flash_content_licenses_ &&
          !waiting_for_clear_channel_ids_ && !waiting_for_clear_cookies_count_ &&

chrome/browser/browsing_data/browsing_data_remover.h

@@ -453,6 +453,7 @@ class BrowsingDataRemover : public KeyedService
   uint32_t deauthorize_flash_content_licenses_request_id_ = 0;
   // True if we're waiting for various data to be deleted.
   // These may only be accessed from UI thread in order to avoid races!
+  bool waiting_for_synchronous_clear_operations_ = false;
   bool waiting_for_clear_autofill_origin_urls_ = false;
   bool waiting_for_clear_cache_ = false;
   bool waiting_for_clear_channel_ids_ = false;