Fix sequence checker asserts in CrxDownloader.

The code crashes because there is a race condition between
the main sequence and another task runner when the instances
of the CrxDownloader are destroyed.

On the main sequence, the CrxDownloader is being used to
download a file, then the reference to this instance
is released. The CrxDownloader is using a task runner
to run a blocking task, which validate the file it has just
downloaded. This task runner receives a closure which holds a
reference to the CrxDownloader.
The reference is released after the task is run, and therefore,
it results in a race when the dtor of CrxDownloader is being
invoked.

Besides removing the offending sequence checkers of the
destructors of the CrxDownloader classes, this CL changes
where the timer of the BackgroundDownloader is destroyed.
The timer has sequence affinity and it must always
be destroyed on the main sequence.

Bug: 1105289,1105460
Change-Id: I78e002502ce6f5501c78d1e849f51b47bf3dc092
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2297696Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
Commit-Queue: Sorin Jianu <sorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#788330}

components/update_client/background_downloader_win.cc

@@ -37,10 +37,10 @@ using base::win::ScopedCoMem;
 
 // The class BackgroundDownloader in this module is an adapter between
 // the CrxDownloader interface and the BITS service interfaces.
-// The interface exposed on the CrxDownloader code runs on the main thread,
-// while the BITS specific code runs on a separate thread passed in by the
-// client. For every url to download, a BITS job is created, unless there is
-// already an existing job for that url, in which case, the downloader
+// The interface exposed on the CrxDownloader code runs on the main sequence,
+// while the BITS specific code runs in a separate sequence bound to a
+// COM apartment. For every url to download, a BITS job is created, unless
+// there is already an existing job for that url, in which case, the downloader
 // connects to it. Once a job is associated with the url, the code looks for
 // changes in the BITS job state. The checks are triggered by a timer.
 // The BITS job contains just one file to download. There could only be one
@@ -402,25 +402,24 @@ BackgroundDownloader::BackgroundDownloader(
       git_cookie_bits_manager_(0),
       git_cookie_job_(0) {}
 
-BackgroundDownloader::~BackgroundDownloader() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  timer_.reset();
-}
+BackgroundDownloader::~BackgroundDownloader() = default;
 
 void BackgroundDownloader::StartTimer() {
-  timer_.reset(new base::OneShotTimer);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  timer_ = std::make_unique<base::OneShotTimer>();
   timer_->Start(FROM_HERE, base::TimeDelta::FromSeconds(kJobPollingIntervalSec),
                 this, &BackgroundDownloader::OnTimer);
 }
 
 void BackgroundDownloader::OnTimer() {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  timer_ = nullptr;
   com_task_runner_->PostTask(
       FROM_HERE, base::BindOnce(&BackgroundDownloader::OnDownloading, this));
 }
 
 void BackgroundDownloader::DoStartDownload(const GURL& url) {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   com_task_runner_->PostTask(
       FROM_HERE,
       base::BindOnce(&BackgroundDownloader::BeginDownload, this, url));
@@ -580,10 +579,6 @@ void BackgroundDownloader::EndDownload(HRESULT error) {
   main_task_runner()->PostTask(
       FROM_HERE, base::BindOnce(&BackgroundDownloader::OnDownloadComplete, this,
                                 is_handled, result, download_metrics));
-
-  // Once the task is posted to the the main thread, this object may be deleted
-  // by its owner. It is not safe to access members of this object on this task
-  // runner from now on.
 }
 
 // Called when the BITS job has been transferred successfully. Completes the

components/update_client/background_downloader_win.h

@@ -12,8 +12,8 @@
 #include <memory>
 
 #include "base/memory/ref_counted.h"
+#include "base/sequence_checker.h"
 #include "base/strings/string16.h"
-#include "base/threading/thread_checker.h"
 #include "base/time/time.h"
 #include "base/timer/timer.h"
 #include "components/update_client/crx_downloader.h"
@@ -27,13 +27,12 @@ namespace update_client {
 
 // Implements a downloader in terms of the BITS service. The public interface
 // of this class and the CrxDownloader overrides are expected to be called
-// from the main thread. The rest of the class code runs on a sequenced
-// task runner, usually associated with a blocking thread pool. The task runner
-// must initialize COM.
+// from the main sequence. The rest of the class code runs on a sequenced
+// task runner. The task runner must initialize COM.
 //
 // This class manages a COM client for Windows BITS. The client uses polling,
 // triggered by an one-shot timer, to get state updates from BITS. Since the
-// timer has thread afinity, the callbacks from the timer are delegated to
+// timer has sequence afinity, the callbacks from the timer are delegated to
 // a sequenced task runner, which handles all client COM interaction with
 // the BITS service.
 class BackgroundDownloader : public CrxDownloader {
@@ -48,8 +47,7 @@ class BackgroundDownloader : public CrxDownloader {
   // Called asynchronously on the |com_task_runner_| at different stages during
   // the download. |OnDownloading| can be called multiple times.
   // |EndDownload| switches the execution flow from the |com_task_runner_| to
-  // the main thread. Accessing any data members of this object from the
-  // |com_task_runner_| after calling |EndDownload| is unsafe.
+  // the main sequence.
   void BeginDownload(const GURL& url);
   void OnDownloading();
   void EndDownload(HRESULT hr);
@@ -104,13 +102,11 @@ class BackgroundDownloader : public CrxDownloader {
   // Revokes the interface pointers from GIT.
   HRESULT ClearGit();
 
-  // Updates the BITS interface pointers so that they can be used by the
-  // thread calling the function. Call this function to get valid COM interface
-  // pointers when a thread from the thread pool enters the object.
+  // Updates the BITS interface pointers so that the COM functions of these
+  // interfaces can be called in this COM STA apartment.
   HRESULT UpdateInterfacePointers();
 
-  // Resets the BITS interface pointers. Call this function when a thread
-  // from the thread pool leaves the object to release the interface pointers.
+  // Resets the BITS interface pointers.
   void ResetInterfacePointers();
 
   // Returns the number of jobs in the BITS queue which were created by this
@@ -120,13 +116,13 @@ class BackgroundDownloader : public CrxDownloader {
   // Cleans up incompleted jobs that are too old.
   void CleanupStaleJobs();
 
-  // Ensures that we are running on the same thread we created the object on.
-  base::ThreadChecker thread_checker_;
+  // This sequence checker is bound to the main sequence.
+  SEQUENCE_CHECKER(sequence_checker_);
 
   // Executes blocking COM calls to BITS.
   scoped_refptr<base::SequencedTaskRunner> com_task_runner_;
 
-  // The timer has thread affinity. This member is initialized and destroyed
+  // The timer has sequence affinity. This member is created and destroyed
   // on the main task runner.
   std::unique_ptr<base::OneShotTimer> timer_;
 

components/update_client/url_fetcher_downloader.cc

@@ -26,9 +26,7 @@ UrlFetcherDownloader::UrlFetcherDownloader(
     : CrxDownloader(std::move(successor)),
       network_fetcher_factory_(network_fetcher_factory) {}
 
-UrlFetcherDownloader::~UrlFetcherDownloader() {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-}
+UrlFetcherDownloader::~UrlFetcherDownloader() = default;
 
 void UrlFetcherDownloader::DoStartDownload(const GURL& url) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);