Revert "Fix sequence checker asserts in CrxDownloader."

This reverts commit 1d114582.

Reason for revert: it is breaking the Android tests https://ci.chromium.org/p/chromium/builders/ci/Android%20FYI%20Release%20%28Nexus%209%29/27588

Original change's description:
> 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/+/2297696
> Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
> Commit-Queue: Sorin Jianu <sorin@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#788330}

TBR=sorin@chromium.org,waffles@chromium.org

Change-Id: I87c30dbc1542c14e18c69c0806f138508f99bf9a
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1105289, 1105460
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2298439Reviewed-by: Sorin Jianu <sorin@chromium.org>
Commit-Queue: Sorin Jianu <sorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#788373}

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 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
+// 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
 // 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,24 +402,25 @@ BackgroundDownloader::BackgroundDownloader(
       git_cookie_bits_manager_(0),
       git_cookie_job_(0) {}
 
-BackgroundDownloader::~BackgroundDownloader() = default;
+BackgroundDownloader::~BackgroundDownloader() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  timer_.reset();
+}
 
 void BackgroundDownloader::StartTimer() {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  timer_ = std::make_unique<base::OneShotTimer>();
+  timer_.reset(new base::OneShotTimer);
   timer_->Start(FROM_HERE, base::TimeDelta::FromSeconds(kJobPollingIntervalSec),
                 this, &BackgroundDownloader::OnTimer);
 }
 
 void BackgroundDownloader::OnTimer() {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  timer_ = nullptr;
+  DCHECK(thread_checker_.CalledOnValidThread());
   com_task_runner_->PostTask(
       FROM_HERE, base::BindOnce(&BackgroundDownloader::OnDownloading, this));
 }
 
 void BackgroundDownloader::DoStartDownload(const GURL& url) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DCHECK(thread_checker_.CalledOnValidThread());
   com_task_runner_->PostTask(
       FROM_HERE,
       base::BindOnce(&BackgroundDownloader::BeginDownload, this, url));
@@ -579,6 +580,10 @@ 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,12 +27,13 @@ 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 sequence. The rest of the class code runs on a sequenced
-// task runner. The task runner must initialize COM.
+// 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.
 //
 // 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 sequence afinity, the callbacks from the timer are delegated to
+// timer has thread 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 {
@@ -47,7 +48,8 @@ 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 sequence.
+  // the main thread. Accessing any data members of this object from the
+  // |com_task_runner_| after calling |EndDownload| is unsafe.
   void BeginDownload(const GURL& url);
   void OnDownloading();
   void EndDownload(HRESULT hr);
@@ -102,11 +104,13 @@ class BackgroundDownloader : public CrxDownloader {
   // Revokes the interface pointers from GIT.
   HRESULT ClearGit();
 
-  // Updates the BITS interface pointers so that the COM functions of these
-  // interfaces can be called in this COM STA apartment.
+  // 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.
   HRESULT UpdateInterfacePointers();
 
-  // Resets the BITS interface pointers.
+  // Resets the BITS interface pointers. Call this function when a thread
+  // from the thread pool leaves the object to release the interface pointers.
   void ResetInterfacePointers();
 
   // Returns the number of jobs in the BITS queue which were created by this
@@ -116,13 +120,13 @@ class BackgroundDownloader : public CrxDownloader {
   // Cleans up incompleted jobs that are too old.
   void CleanupStaleJobs();
 
-  // This sequence checker is bound to the main sequence.
-  SEQUENCE_CHECKER(sequence_checker_);
+  // Ensures that we are running on the same thread we created the object on.
+  base::ThreadChecker thread_checker_;
 
   // Executes blocking COM calls to BITS.
   scoped_refptr<base::SequencedTaskRunner> com_task_runner_;
 
-  // The timer has sequence affinity. This member is created and destroyed
+  // The timer has thread affinity. This member is initialized and destroyed
   // on the main task runner.
   std::unique_ptr<base::OneShotTimer> timer_;
 

components/update_client/url_fetcher_downloader.cc

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