Skip to content

Commit

Permalink
Make CrxDownloader a ref-counted thread-safe type.
Browse files Browse the repository at this point in the history 
This CL is trying to reland a combination of
https://chromium-review.googlesource.com/c/chromium/src/+/2293461
and https://chromium-review.googlesource.com/c/chromium/src/+/2297696
with an additional fix to make NetworkFetcherFactory a thread-safe
type.

The original CLs were reverted since the code had a race condition
on the destruction of CrxDownloader instances, which triggered
various sequence checkers when the objects were destroyed by
a sequence other than main.

Bug: 1105924
Change-Id: I84f071518f930964ec08611bb14b7fb2aa243fb3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2300599
Commit-Queue: Sorin Jianu <sorin@chromium.org>
Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
Cr-Commit-Position: refs/heads/master@{#788829}
  • Loading branch information
@sorinj
sorinj authored and Commit Bot committed Jul 16, 2020
1 parent 9e80a88 commit 907a161
Show file tree
Hide file tree
Showing 11 changed files with 253 additions and 233 deletions.
55 changes: 22 additions & 33 deletions components/update_client/background_downloader_win.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
#include "base/files/file_util.h"
#include "base/location.h"
#include "base/logging.h"
#include "base/macros.h"
#include "base/metrics/histogram_macros.h"
#include "base/sequenced_task_runner.h"
#include "base/strings/string_piece.h"
Expand All @@ -38,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
Expand Down Expand Up @@ -396,36 +395,34 @@ void CleanupJob(const ComPtr<IBackgroundCopyJob>& job) {
} // namespace

BackgroundDownloader::BackgroundDownloader(
std::unique_ptr<CrxDownloader> successor)
scoped_refptr<CrxDownloader> successor)
: CrxDownloader(std::move(successor)),
com_task_runner_(base::ThreadPool::CreateCOMSTATaskRunner(
kTaskTraitsBackgroundDownloader)),
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,
base::Unretained(this)));
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,
base::Unretained(this), url));
FROM_HERE,
base::BindOnce(&BackgroundDownloader::BeginDownload, this, url));
}

// Called one time when this class is asked to do a download.
Expand All @@ -444,9 +441,8 @@ void BackgroundDownloader::BeginDownload(const GURL& url) {
VLOG(1) << "Starting BITS download for: " << url.spec();

ResetInterfacePointers();
main_task_runner()->PostTask(FROM_HERE,
base::BindOnce(&BackgroundDownloader::StartTimer,
base::Unretained(this)));
main_task_runner()->PostTask(
FROM_HERE, base::BindOnce(&BackgroundDownloader::StartTimer, this));
}

// Creates or opens an existing BITS job to download the |url|, and handles
Expand Down Expand Up @@ -535,9 +531,8 @@ void BackgroundDownloader::OnDownloading() {
return;

ResetInterfacePointers();
main_task_runner()->PostTask(FROM_HERE,
base::BindOnce(&BackgroundDownloader::StartTimer,
base::Unretained(this)));
main_task_runner()->PostTask(
FROM_HERE, base::BindOnce(&BackgroundDownloader::StartTimer, this));
}

// Completes the BITS download, picks up the file path of the response, and
Expand Down Expand Up @@ -582,13 +577,8 @@ void BackgroundDownloader::EndDownload(HRESULT error) {
if (!result.error)
result.response = response_;
main_task_runner()->PostTask(
FROM_HERE, base::BindOnce(&BackgroundDownloader::OnDownloadComplete,
base::Unretained(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.
FROM_HERE, base::BindOnce(&BackgroundDownloader::OnDownloadComplete, this,
is_handled, result, download_metrics));
}

// Called when the BITS job has been transferred successfully. Completes the
Expand Down Expand Up @@ -666,9 +656,8 @@ bool BackgroundDownloader::OnStateTransferring() {
GetJobByteCount(job_, &downloaded_bytes, &total_bytes);

main_task_runner()->PostTask(
FROM_HERE,
base::BindOnce(&BackgroundDownloader::OnDownloadProgress,
base::Unretained(this), downloaded_bytes, total_bytes));
FROM_HERE, base::BindOnce(&BackgroundDownloader::OnDownloadProgress, this,
downloaded_bytes, total_bytes));
return false;
}

Expand Down
33 changes: 13 additions & 20 deletions components/update_client/background_downloader_win.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,9 @@

#include <memory>

#include "base/macros.h"
#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"
Expand All @@ -28,29 +27,27 @@ 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 {
public:
explicit BackgroundDownloader(std::unique_ptr<CrxDownloader> successor);
~BackgroundDownloader() override;
explicit BackgroundDownloader(scoped_refptr<CrxDownloader> successor);

private:
// Overrides for CrxDownloader.
~BackgroundDownloader() override;
void DoStartDownload(const GURL& url) override;

// 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);
Expand Down Expand Up @@ -105,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
Expand All @@ -121,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_;

Expand All @@ -148,8 +143,6 @@ class BackgroundDownloader : public CrxDownloader {

// Contains the path of the downloaded file if the download was successful.
base::FilePath response_;

DISALLOW_COPY_AND_ASSIGN(BackgroundDownloader);
};

} // namespace update_client
Expand Down
4 changes: 2 additions & 2 deletions components/update_client/component.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -759,7 +759,7 @@ void Component::StateDownloadingDiff::DownloadComplete(
for (const auto& download_metrics : crx_downloader_->download_metrics())
component.AppendEvent(component.MakeEventDownloadMetrics(download_metrics));

crx_downloader_.reset();
crx_downloader_ = nullptr;

if (download_result.error) {
DCHECK(download_result.response.empty());
Expand Down Expand Up @@ -832,7 +832,7 @@ void Component::StateDownloading::DownloadComplete(
for (const auto& download_metrics : crx_downloader_->download_metrics())
component.AppendEvent(component.MakeEventDownloadMetrics(download_metrics));

crx_downloader_.reset();
crx_downloader_ = nullptr;

if (download_result.error) {
DCHECK(download_result.response.empty());
Expand Down

0 comments on commit 907a161

Please sign in to comment.