Implement a background downloader using BITS in Windows Chrome.

chrome/browser/component_updater/background_downloader_win.cc

+// Copyright 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/component_updater/background_downloader_win.h"
+#include <atlbase.h>
+#include <atlcom.h>
+#include <functional>
+#include <iomanip>
+#include <vector>
+#include "base/file_util.h"
+#include "base/strings/sys_string_conversions.h"
+#include "base/time/time.h"
+#include "base/win/scoped_co_mem.h"
+#include "chrome/browser/component_updater/component_updater_utils.h"
+#include "content/public/browser/browser_thread.h"
+#include "ui/base/win/atl_module.h"
+#include "url/gurl.h"
+
+using base::win::ScopedCoMem;
+using base::win::ScopedComPtr;
+using content::BrowserThread;
+
+// 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 UI thread, while
+// the BITS specific code runs in a single threaded apartment on the FILE
+// thread.
+// 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.

[cpu_(ooo_6.6-7.5), 2013/12/05 19:32:43]

I haven't looked at the code yet but are we assuming that each component
download url is unique?

[Sorin Jianu, 2013/12/05 22:34:36]

We do, in the sense that we expect different files to be always hosted on
different urls, in other words 1:1 between files and urls. Please tell me more
about your concern, we want to get this assumption correct.

+// The changes arrive either as COM callbacks (due to the threading model, the
+// events always arrive on the FILE thread) or by polling, triggered by a timer,
+// as a last resort. The BITS job contains just one file to download. There
+// could only be one download in progress at a time. If Chrome closes down
+// before the download is complete, the BITS job remains active and finishes in
+// the background, without any intervention. The job can be completed next time
+// the code runs, if the file is still needed, otherwise it will be cleaned up
+// on a periodic basis.
+

[cpu_(ooo_6.6-7.5), 2013/12/05 19:32:43]

can you put here the minimalist command line to list the current bits jobs?

[Sorin Jianu, 2013/12/05 22:34:36]

Done.

+namespace component_updater {
+
+namespace {
+
+// All jobs created by this module have a specific description so they can
+// be found at run-time or by using system administration tools.
+const char16 kJobDescription[] = L"Chrome Component Updater";
+
+// How often the code looks for changes in the BITS job state.
+const int kJobPollingIntervalSec = 10;
+
+// How often the jobs which were started but not completed for any reason
+// are cleaned up. Reasons for jobs to be left behind include browser restarts,
+// system restarts, etc. Also, the check to purge stale jobs only happens
+// at most once a day.
+const int kPurgeStaleJobsAfterDays = 7;
+const int kPurgeStaleJobsIntervalBetweenChecksDays = 1;

[cpu_(ooo_6.6-7.5), 2013/12/05 19:32:43]

These two seem to be good to expose in the configurator, but for a follow up CL.

[Sorin Jianu, 2013/12/05 22:34:36]

will do.

+
+// Returns the status code from a given BITS error.
+int GetHttpStatusFromBitsError(HRESULT error) {
+  // BITS errors are defined in bitsmsg.h. Although not documented, it is
+  // clear that all errors corresponding to http status code have the high
+  // word equal to 0x8019 and the low word equal to the http status code.
+  const int kHttpStatusFirst = 100;    // Continue.
+  const int kHttpStatusLast  = 505;    // Version not supported.
+  bool is_valid = HIWORD(error) == 0x8019 &&
+                  LOWORD(error) >= kHttpStatusFirst &&
+                  LOWORD(error) <= kHttpStatusLast;
+  return is_valid ? LOWORD(error) : 0;
+}
+
+// Returns the files in a BITS job.
+HRESULT GetFilesInJob(IBackgroundCopyJob* job,
+                      std::vector<ScopedComPtr<IBackgroundCopyFile> >* files) {
+  ScopedComPtr<IEnumBackgroundCopyFiles> enum_files;
+  HRESULT hr = job->EnumFiles(enum_files.Receive());
+  if (FAILED(hr))
+    return hr;
+
+  ULONG num_files = 0;
+  hr = enum_files->GetCount(&num_files);
+  if (FAILED(hr))
+    return hr;
+
+  for (ULONG i = 0; i != num_files; ++i) {
+    ScopedComPtr<IBackgroundCopyFile> file;
+    if (enum_files->Next(1, file.Receive(), NULL) == S_OK)
+      files->push_back(file);
+  }
+
+  return S_OK;
+}
+
+// Returns the file name, the url, and some per-file progress information.
+// The function out parameters can be NULL if that data is not requested.
+HRESULT GetJobFileProperties(IBackgroundCopyFile* file,
+                             string16* local_name,
+                             string16* remote_name,
+                             BG_FILE_PROGRESS* progress) {
+  HRESULT hr = S_OK;
+
+  if (local_name) {
+    ScopedCoMem<char16> name;
+    hr = file->GetLocalName(&name);
+    if (FAILED(hr))
+      return hr;
+    local_name->assign(name);
+  }
+
+  if (remote_name) {
+    ScopedCoMem<char16> name;
+    hr = file->GetRemoteName(&name);
+    if (FAILED(hr))
+      return hr;
+    remote_name->assign(name);
+  }
+
+  if (progress) {
+    BG_FILE_PROGRESS bg_file_progress = {};
+    hr = file->GetProgress(&bg_file_progress);
+    if (FAILED(hr))
+      return hr;
+    *progress = bg_file_progress;
+  }
+
+  return hr;
+}
+
+// Finds the jobs matching the given predicate.
+// Returns S_OK if the function has found at least one job, returns S_FALSE if
+// no job was found, and it returns an error otherwise.
+template<class Predicate>
+HRESULT FindBitsJobIf(Predicate pred,
+                      IBackgroundCopyManager* bits_manager,
+                      std::vector<ScopedComPtr<IBackgroundCopyJob> >* jobs) {
+  ScopedComPtr<IEnumBackgroundCopyJobs> enum_jobs;
+  HRESULT hr = bits_manager->EnumJobs(0, enum_jobs.Receive());
+  if (FAILED(hr))
+    return hr;
+
+  ULONG job_count = 0;
+  hr = enum_jobs->GetCount(&job_count);
+  if (FAILED(hr))
+    return hr;
+
+  for (ULONG i = 0; i != job_count; ++i) {
+    ScopedComPtr<IBackgroundCopyJob> current_job;
+    if (enum_jobs->Next(1, current_job.Receive(), NULL) == S_OK &&
+        pred(current_job)) {
+      jobs->push_back(current_job);
+    }

[cpu_(ooo_6.6-7.5), 2013/12/05 21:16:31]

seems we should have here the logic of JobDescriptionEqual()  and thus the code
will be cleaner.

[Sorin Jianu, 2013/12/05 22:34:36]

This function takes any predicate and it iterates over all jobs.

[cpu_(ooo_6.6-7.5), 2013/12/06 00:12:51]

Yes, but all the callers (predicates) filter out the ones without
kJobDescription. Unless we are planning on iterating over other people's bits
jobs?

[Sorin Jianu, 2013/12/06 01:37:36]

Done.

+  }
+
+  return jobs->empty() ? S_FALSE : S_OK;
+}
+
+// Compares the description of a job matches |name|.
+struct JobDescriptionEqual
+    : public std::binary_function<IBackgroundCopyJob*, const string16&, bool> {
+  bool operator()(IBackgroundCopyJob* job, const string16& name) const;
+};
+
+bool JobDescriptionEqual::operator()(IBackgroundCopyJob* job,
+                                     const string16& name) const {
+  ScopedCoMem<char16> description;
+  HRESULT hr = job->GetDescription(&description);
+  return SUCCEEDED(hr) && name.compare(description) == 0;
+}
+
+// Compares the job creation time and returns true if the job creation time
+// is older than |num_days|.
+struct JobCreationOlderThanDays
+    : public std::binary_function<IBackgroundCopyJob*, int, bool> {
+  bool operator()(IBackgroundCopyJob* job, int num_days) const;
+};
+
+bool JobCreationOlderThanDays::operator()(IBackgroundCopyJob* job,
+                                          int num_days) const {
+  if (!JobDescriptionEqual()(job, kJobDescription))
+    return false;
+
+  BG_JOB_TIMES times = {0};
+  HRESULT hr = job->GetTimes(&times);
+  const base::TimeDelta time_delta(base::TimeDelta::FromDays(num_days));
+  const base::Time creation_time(base::Time::FromFileTime(times.CreationTime));

[cpu_(ooo_6.6-7.5), 2013/12/05 21:16:31]

seems incorrect to call 186 regardless of the value of hr. 

[Sorin Jianu, 2013/12/05 22:34:36]

Done.

+  return SUCCEEDED(hr) && (creation_time + time_delta < base::Time::Now());
+}
+
+// Compares the url of a file in a job and returns true if the remote name
+// of any file in a job matches |url|.
+struct JobFileUrlEqual
+    : public std::binary_function<IBackgroundCopyJob*, const string16&, bool> {
+  bool operator()(IBackgroundCopyJob* job, const string16& url) const;
+};
+
+bool JobFileUrlEqual::operator()(IBackgroundCopyJob* job,
+                                 const string16& url) const {

[cpu_(ooo_6.6-7.5), 2013/12/05 21:16:31]

I think this should take a GURL& instead of a string.

[Sorin Jianu, 2013/12/05 22:34:36]

The other predicates take string16 as well, at least for consistency reasons,
string16 works nicely. The only difference would be where the conversion from
GURL to string16 happens.

[cpu_(ooo_6.6-7.5), 2013/12/06 00:12:51]

It is frowned upon to use url params not being GURLs unless forced to, like for
example in printf cases.

If you are attached to this then rename the param to be remote_name.

[Sorin Jianu, 2013/12/06 01:37:36]

Done.

+  if (!JobDescriptionEqual()(job, kJobDescription))
+    return false;
+
+  std::vector<ScopedComPtr<IBackgroundCopyFile> > files;
+  HRESULT hr = GetFilesInJob(job, &files);
+  if (FAILED(hr))
+    return false;
+
+  for (size_t i = 0; i != files.size(); ++i) {
+    ScopedCoMem<char16> name;
+    if (SUCCEEDED(files[i]->GetRemoteName(&name)) && url.compare(name) == 0)
+      return true;
+  }
+
+  return false;
+}
+
+// Sets the proxy authentication credentials for the job.
+HRESULT SetProxyAuthCredentials(IBackgroundCopyJob* job) {
+  ScopedComPtr<IBackgroundCopyJob2> job2;

[cpu_(ooo_6.6-7.5), 2013/12/05 21:16:31]

is IBackgroundCopyJob2 Vista and above?  also who calls this?

[Sorin Jianu, 2013/12/05 22:34:36]

yes. At the moment, nobody is calling it, I will have to add proxy support in a
future CL.

[cpu_(ooo_6.6-7.5), 2013/12/06 00:12:51]

Delete it please.

[Sorin Jianu, 2013/12/06 01:37:36]

Done.

+  HRESULT hr = job2.QueryFrom(job);
+  if (FAILED(hr))
+    return hr;
+
+  BG_AUTH_CREDENTIALS auth_cred = {};
+  auth_cred.Target = BG_AUTH_TARGET_PROXY;
+  auth_cred.Scheme = BG_AUTH_SCHEME_NEGOTIATE;
+

[cpu_(ooo_6.6-7.5), 2013/12/05 21:16:31]

Is there an option for no credentials?

[Sorin Jianu, 2013/12/05 22:34:36]

We can set proxies for a job. If we do a proxy request and the response is 407
then we would have to use this function to set the credentials. I don't know
what happens if we set the credentials proactively for any proxy request. In
general, the usage pattern seems to be: set the proxy, look at the response, if
it is 401 or 407, set the credentials.

[cpu_(ooo_6.6-7.5), 2013/12/06 00:12:51]

My only concern is leaking ntlm password hashes if we connect to an evil bits
server. See the internet about pass-the-hash (ntlm proxy) attacks.

[Sorin Jianu, 2013/12/06 01:37:36]

Understood. Let's talk about proxy authentication after this lands.

+  return job2->SetCredentials(&auth_cred);
+}
+
+HRESULT CancelJob(IBackgroundCopyJob* job) {
+  BG_JOB_STATE job_state = BG_JOB_STATE_ERROR;
+  HRESULT hr = job->GetState(&job_state);
+  if (FAILED(hr))
+    return hr;
+
+  if (job_state != BG_JOB_STATE_CANCELLED &&
+      job_state != BG_JOB_STATE_ACKNOWLEDGED) {
+    hr = job->Cancel();
+    if (FAILED(hr))
+      return hr;
+  }
+
+  return hr;
+}
+
+HRESULT PauseJob(IBackgroundCopyJob* job) {
+  BG_JOB_STATE job_state = BG_JOB_STATE_ERROR;
+  HRESULT hr = job->GetState(&job_state);
+  if (FAILED(hr))
+    return hr;

[cpu_(ooo_6.6-7.5), 2013/12/05 21:16:31]

function also seems unused.

[Sorin Jianu, 2013/12/05 22:34:36]

Indeed, I wrote the functions that I will need soon. Is there an issue with
this? If not used, it will not be linked in.

[cpu_(ooo_6.6-7.5), 2013/12/06 00:12:51]

delete it please, chrome style is not leave unused code.

[Sorin Jianu, 2013/12/06 01:37:36]

Done.

+
+  if (job_state != BG_JOB_STATE_TRANSFERRED &&
+      job_state != BG_JOB_STATE_ACKNOWLEDGED &&
+      job_state != BG_JOB_STATE_CANCELLED) {
+    hr = job->Suspend();
+    if (FAILED(hr))
+      return hr;
+  }
+
+  return hr;
+}
+
+HRESULT ResumeJob(IBackgroundCopyJob* job) {

[cpu_(ooo_6.6-7.5), 2013/12/05 21:16:31]

another unused function? unless you plan to use it in 559

[Sorin Jianu, 2013/12/05 22:34:36]

Done.

[Sorin Jianu, 2013/12/06 01:37:36]

deleted.

+  BG_JOB_STATE job_state = BG_JOB_STATE_ERROR;
+  HRESULT hr = job->GetState(&job_state);
+  if (FAILED(hr))
+    return hr;
+
+  if (job_state != BG_JOB_STATE_SUSPENDED) {
+    hr = job->Resume();
+    if (FAILED(hr))
+      return hr;
+  }
+
+  return hr;
+}
+
+// Creates an instance of the BITS manager.
+HRESULT GetBitsManager(IBackgroundCopyManager** bits_manager) {
+  ScopedComPtr<IBackgroundCopyManager> object;
+  HRESULT hr = object.CreateInstance(__uuidof(BackgroundCopyManager));
+  if (FAILED(hr)) {
+    VLOG(1) << "Failed to instantiate BITS." << std::hex << hr;

[cpu_(ooo_6.6-7.5), 2013/12/05 21:16:31]

todo: add UMA or pings here.

[Sorin Jianu, 2013/12/05 22:34:36]

Done.

+    return hr;
+  }
+  *bits_manager = object.Detach();
+  return S_OK;
+}
+
+// JobObserver receives notifications when a BITS job has been completed,
+// modified, or has encountered an error. This class lives on the FILE thread.
+class JobObserver
+    : public CComObjectRootEx<CComSingleThreadModel>,
+      public IBackgroundCopyCallback {
+ public:
+  typedef  base::Callback<void (void)> JobChangedCallback;

[cpu_(ooo_6.6-7.5), 2013/12/05 21:16:31]

extra space between typedef and base::

[Sorin Jianu, 2013/12/05 22:34:36]

Done.

+
+  JobObserver() {}
+
+  virtual ~JobObserver() {}
+
+  void set_callback(const JobChangedCallback& callback) {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+    callback_ = callback;
+  }
+
+  BEGIN_COM_MAP(JobObserver)
+    COM_INTERFACE_ENTRY(IBackgroundCopyCallback)
+  END_COM_MAP()
+
+  // IBackgroundCopyCallback methods.
+  STDMETHOD(JobTransferred)(IBackgroundCopyJob* job) OVERRIDE {
+    NotifyJobChanged();
+    return S_OK;
+  }
+
+  STDMETHOD(JobError)(IBackgroundCopyJob* job,
+                      IBackgroundCopyError* error) OVERRIDE {
+    NotifyJobChanged();
+    return S_OK;
+  }
+
+  STDMETHOD(JobModification)(IBackgroundCopyJob* job,
+                            DWORD reserved) OVERRIDE {

[cpu_(ooo_6.6-7.5), 2013/12/05 21:16:31]

indent 324

[Sorin Jianu, 2013/12/05 22:34:36]

Done.

+    NotifyJobChanged();
+    return S_OK;
+  }
+
+ private:
+  void NotifyJobChanged() {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+    if (!callback_.is_null())
+      BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, callback_);
+  }
+
+  JobChangedCallback callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(JobObserver);
+};
+
+}  // namespace
+
+BackgroundDownloader::BackgroundDownloader(
+    scoped_ptr<CrxDownloader> successor,
+    net::URLRequestContextGetter* context_getter,
+    scoped_refptr<base::SequencedTaskRunner> task_runner,
+    const DownloadCallback& download_callback)
+    : CrxDownloader(successor.Pass(), download_callback),
+      context_getter_(context_getter),
+      task_runner_(task_runner),
+      is_completed_(false) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+}
+
+BackgroundDownloader::~BackgroundDownloader() {
+}
+
+void BackgroundDownloader::DoStartDownload(const GURL& url) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  BrowserThread::PostTask(
+        BrowserThread::FILE,
+        FROM_HERE,
+        base::Bind(&BackgroundDownloader::BeginDownload,
+                   base::Unretained(this),

[cpu_(ooo_6.6-7.5), 2013/12/05 21:16:31]

can it really be unretained? remind me of the lifetime of chain of
responsibility pattern

[Sorin Jianu, 2013/12/05 22:34:36]

The caller own the head of the chain, who owns the successors and so on.

[cpu_(ooo_6.6-7.5), 2013/12/06 00:12:51]

So the top dog in the chain does not get deleted until the CUS dies?

[Sorin Jianu, 2013/12/06 01:37:36]

The life cycle of the downloader and its successors is determined by the CUS.
When CUS needs a download, it instantiates the downloader and retains the
ownership of the download object until the download completes. See
CrxUpdateService::UpdateComponent and CrxUpdateService::DownloadComplete where
the downloader is created and destroyed.

+                   url));
+}
+
+// Called once when this class is asked to do a download. Creates or opens
+// an existing bits job, hooks up the notifications, and starts the timer.
+void BackgroundDownloader::BeginDownload(const GURL& url) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));

[cpu_(ooo_6.6-7.5), 2013/12/05 21:16:31]

dcheck(timer_ is null) ?

[Sorin Jianu, 2013/12/05 22:34:36]

Done.

Nothing breaks in the code if the timer is not null. It does provide an indirect
indication that some other invariant could be broken. Do you have a specific
concern in mind with the timer object itself?

[cpu_(ooo_6.6-7.5), 2013/12/06 00:12:51]

Not per se.

[Sorin Jianu, 2013/12/06 01:37:36]

Done.

+
+  HRESULT hr = QueueBitsJob(url);
+  if (FAILED(hr)) {
+    if (job_)
+      CancelJob(job_);
+    EndDownload(hr);
+    return;
+  }
+
+  timer_.reset(new base::OneShotTimer<BackgroundDownloader>);
+  timer_->Start(FROM_HERE,
+                base::TimeDelta::FromSeconds(kJobPollingIntervalSec),
+                this,
+                &BackgroundDownloader::OnDownloading);
+}
+
+// Called any time there is a change in the state of the job or when
+// the timer fires.
+void BackgroundDownloader::OnDownloading() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+
+  // Stop the timer in case this function has run due to a BITS event.
+  timer_->Stop();
+
+  BG_JOB_STATE job_state = BG_JOB_STATE_ERROR;
+  HRESULT hr = job_->GetState(&job_state);
+  if (FAILED(hr)) {
+    EndDownload(hr);
+    return;
+  }
+
+  switch (job_state) {
+    case BG_JOB_STATE_TRANSFERRED:
+      OnStateTransferred();
+      return;
+
+    case BG_JOB_STATE_ERROR:
+      OnStateError();
+      return;
+
+    case BG_JOB_STATE_CANCELLED:
+      OnStateCancelled();
+      return;
+
+    case BG_JOB_STATE_ACKNOWLEDGED:
+      OnStateAcknowledged();
+      return;
+
+    // TODO: handle the non-final states, so that the download does not get
+    // stuck if BITS is not able to make progress on a given url.
+    case BG_JOB_STATE_TRANSIENT_ERROR:
+    case BG_JOB_STATE_QUEUED:
+    case BG_JOB_STATE_CONNECTING:
+    case BG_JOB_STATE_TRANSFERRING:
+    case BG_JOB_STATE_SUSPENDED:
+    default:
+      break;
+  }
+

[cpu_(ooo_6.6-7.5), 2013/12/05 21:16:31]

can you use timer_->Reset() instead?

[Sorin Jianu, 2013/12/05 22:34:36]

Done. Please notice I removed the Stop call at the beginning of this function,
is that ok?

[cpu_(ooo_6.6-7.5), 2013/12/06 00:12:51]

Cursory look tells me it is ok.

[Sorin Jianu, 2013/12/06 01:37:36]

I put the call the Stop back since I check as a post condition somewhere that
the timer should not be running when the download ends.

+  timer_->Start(FROM_HERE,
+                base::TimeDelta::FromSeconds(kJobPollingIntervalSec),
+                this,
+                &BackgroundDownloader::OnDownloading);
+}
+
+// Completes the BITS download, picks up the file path of the response, and
+// notifies the CrxDownloader. Usually called one time but it could be called
+// multiple times due to the dual polling/event drive mechanism to receive
+// job state changes.
+void BackgroundDownloader::EndDownload(HRESULT error) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  DCHECK(!timer_->IsRunning());
+
+  timer_.reset();
+
+  if (is_completed_)
+    return;
+
+  base::FilePath response;
+  HRESULT hr = error;
+  if (SUCCEEDED(hr)) {
+    std::vector<ScopedComPtr<IBackgroundCopyFile> > files;
+    GetFilesInJob(job_, &files);
+    DCHECK(files.size() == 1);
+    string16 local_name;
+    BG_FILE_PROGRESS progress = {0};
+    hr = GetJobFileProperties(files[0], &local_name, NULL, &progress);
+    if (SUCCEEDED(hr)) {
+      DCHECK(progress.Completed);
+      response = base::FilePath(local_name);
+    }
+  }
+
+  // Consider the url handled if it has been successfully downloaded or a
+  // 5xx has been received.
+  const bool is_handled = SUCCEEDED(hr) ||
+                          IsHttpServerError(GetHttpStatusFromBitsError(error));
+
+  Result result;
+  result.error = error;
+  result.is_background_download = true;
+  result.response = response;
+  BrowserThread::PostTask(
+        BrowserThread::UI,
+        FROM_HERE,
+        base::Bind(&BackgroundDownloader::OnDownloadComplete,
+                   base::Unretained(this),
+                   is_handled,
+                   result));
+
+  is_completed_ = true;
+
+  CleanupStaleJobs(bits_manager_);
+}
+
+// Called when the BITS job has been transferred successfully. Completes the
+// BITS job by removing it from the BITS queue and making the download
+// available to the caller.
+void BackgroundDownloader::OnStateTransferred() {
+  RemoveJobObserver();
+
+  HRESULT hr = job_->Complete();
+  if (SUCCEEDED(hr) || hr == BG_S_UNABLE_TO_DELETE_FILES)
+    hr = S_OK;
+  else
+    hr = job_->Cancel();
+
+  EndDownload(hr);
+}
+
+// Called when the job has encountered an error and no further progress can
+// be made. Cancels this job and remove it from the BITS queue.
+void BackgroundDownloader::OnStateError() {
+  RemoveJobObserver();
+
+  ScopedComPtr<IBackgroundCopyError> copy_error;
+  HRESULT hr = job_->GetError(copy_error.Receive());
+  if (SUCCEEDED(hr)) {
+    BG_ERROR_CONTEXT error_context = BG_ERROR_CONTEXT_NONE;
+    HRESULT error_code = E_FAIL;
+    hr = copy_error->GetError(&error_context, &error_code);
+    if (SUCCEEDED(hr)) {
+      EndDownload(error_code);
+      return;
+    }
+  }
+
+  hr = job_->Cancel();
+  EndDownload(hr);
+}
+
+// Called when the download was cancelled. Since the observer should have
+// been disconnected by now, this notification must not be seen.
+void BackgroundDownloader::OnStateCancelled() {
+  EndDownload(E_UNEXPECTED);
+}
+
+// Called when the download was completed. Same as above.
+void BackgroundDownloader::OnStateAcknowledged() {
+  EndDownload(E_UNEXPECTED);
+}
+
+// Creates or opens a job for the given url and queues it up. Tries to
+// install a job observer but continues on if an observer can't be set up.
+HRESULT BackgroundDownloader::QueueBitsJob(const GURL& url) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+
+  HRESULT hr = S_OK;
+  if (bits_manager_ == NULL) {
+    hr = GetBitsManager(bits_manager_.Receive());
+    if (FAILED(hr))
+      return hr;
+  }
+
+  hr = CreateOrOpenJob(url);

[cpu_(ooo_6.6-7.5), 2013/12/05 21:16:31]

I rather have CreateOrOpenJob just return the job (not set the job_) member and
only
after 556 be set as the member or even possibly on the caller of QueueBitsJob

[Sorin Jianu, 2013/12/05 22:34:36]

I know what you mean. I tried to do the code that way and have CreateOrOpenJob
be a factory method.  I had an issue with the fact that until we can queue the
job up, we have to created it, intialize it, and set an observer. These require
a job_manager, a job, and a job observer be passed over between several
functions as function arguments. The code looked unglier than what it is now. 
These functions are also only called from QueueBitsJob. QueueBitsJob either
succeeds, or not, in which case, the job is cancelled, and we are done with the
request.

I am not pushing back on the idea, I am just articulating the thinking process.

[cpu_(ooo_6.6-7.5), 2013/12/06 00:12:51]

I see. I think there is a happy medium point, it can be kept as a member so you
don't have to pass the job manager, etc.

[Sorin Jianu, 2013/12/06 01:37:36]

Thank you!

+  if (FAILED(hr))
+    return hr;
+
+  if (hr == S_OK) {
+    hr = InitializeNewJob(url);
+    if (FAILED(hr))

[cpu_(ooo_6.6-7.5), 2013/12/05 21:16:31]

here you need to delete the job.

[Sorin Jianu, 2013/12/05 22:34:36]

job_ is refcounted. When the function runs again it will reset this interface
pointer. Or it will be destroyed in the destructor.

[cpu_(ooo_6.6-7.5), 2013/12/06 00:12:51]

it seems weird to have job_ point to a valid, yet broken bits job. I think we
should strive to have whenever job_ is not null, it is a viable, healthy job.

[Sorin Jianu, 2013/12/06 01:37:36]

Done. I moved resetting the job pointer @322 where the job is being cancelled as
well. Any job that errors out after it is created and before it could start has
to be cancelled, so it does not fill up the BITS queue.
BackgroundDownloader::BeginDownload is the function that queues the job up, or
cleans it up by cancelling it and now, resetting the interface pointer too.

+      return hr;
+  }
+
+  InstallJobObserver();
+
+  return job_->Resume();
+}
+
+HRESULT BackgroundDownloader::CreateOrOpenJob(const GURL& url) {
+  std::vector<ScopedComPtr<IBackgroundCopyJob> > jobs;
+  HRESULT hr = FindBitsJobIf(
+      std::bind2nd(JobFileUrlEqual(), base::SysUTF8ToWide(url.spec())),
+      bits_manager_,
+      &jobs);
+  if (SUCCEEDED(hr) && !jobs.empty()) {
+    job_ = jobs.front();
+    return S_FALSE;
+  }
+
+  GUID guid = {0};
+  ScopedComPtr<IBackgroundCopyJob> job;
+  hr = bits_manager_->CreateJob(L"",

[cpu_(ooo_6.6-7.5), 2013/12/05 21:16:31]

L"" ? 

[Sorin Jianu, 2013/12/05 22:34:36]

I need a wide string literal, is the problem with the literal or the harcoded
value?

[cpu_(ooo_6.6-7.5), 2013/12/06 00:12:51]

The value, should it be something that helps finding them?

[Sorin Jianu, 2013/12/06 01:37:36]

Oh, that parameter is the job display name, which gets set one function call
later, as the file name to download. I changed the code to initialize it with
the description name, in case it is never set
by accident. 

+                                BG_JOB_TYPE_DOWNLOAD,
+                                &guid,
+                                job.Receive());
+  if (FAILED(hr))
+    return hr;
+
+  job_ = job;
+  return S_OK;
+}
+
+HRESULT BackgroundDownloader::InitializeNewJob(const GURL& url) {
+  const string16 filename(base::SysUTF8ToWide(url.ExtractFileName()));
+
+  base::FilePath tempdir;
+  if (!file_util::CreateNewTempDirectory(
+      FILE_PATH_LITERAL("chrome_component_updates_"),
+      &tempdir))

[cpu_(ooo_6.6-7.5), 2013/12/05 21:16:31]

bikeshed purple: "chrome_BITS_"

[Sorin Jianu, 2013/12/05 22:34:36]

Done.

+    return E_FAIL;
+
+  HRESULT hr = job_->AddFile(
+      base::SysUTF8ToWide(base::StringPiece(url.spec())).c_str(),

[cpu_(ooo_6.6-7.5), 2013/12/05 21:16:31]

you need stringpiece there?

[Sorin Jianu, 2013/12/05 22:34:36]

I don't. Thank you for pointing it out. 

I noticed SysUTF8ToWide only takes a StringPiece argument but I did not occur to
me that StringPiece has an implicit conversion from string.

+      tempdir.Append(filename).AsUTF16Unsafe().c_str());
+  if (FAILED(hr))
+    return hr;
+
+  hr = job_->SetDisplayName(filename.c_str());
+  if (FAILED(hr))
+    return hr;
+
+  hr = job_->SetDescription(kJobDescription);
+  if (FAILED(hr))
+    return hr;
+
+  hr = job_->SetPriority(BG_JOB_PRIORITY_NORMAL);
+  if (FAILED(hr))
+    return hr;
+
+  return S_OK;
+}
+
+HRESULT BackgroundDownloader::InstallJobObserver() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+
+  // Make sure ATL is initialized in this module.
+  ui::win::CreateATLModuleIfNeeded();
+
+  CComObject<JobObserver>* job_observer = NULL;
+  HRESULT hr(CComObject<JobObserver>::CreateInstance(&job_observer));
+  if (FAILED(hr))
+    return hr;
+
+  job_observer->set_callback(
+      base::Bind(&BackgroundDownloader::OnDownloading,
+                 base::Unretained(this)));
+
+  job_observer->AddRef();
+  job_observer_.Attach(job_observer);
+
+  hr = job_->SetNotifyInterface(job_observer_);
+  if (FAILED(hr))
+    return hr;
+
+  hr = job_->SetNotifyFlags(BG_NOTIFY_FILE_TRANSFERRED |
+                            BG_NOTIFY_JOB_TRANSFERRED |
+                            BG_NOTIFY_JOB_ERROR);
+  if (FAILED(hr))
+    return hr;
+
+  return S_OK;
+}
+
+HRESULT BackgroundDownloader::RemoveJobObserver() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+
+  if (!job_ || !job_observer_)
+    return S_OK;
+
+  HRESULT hr = job_->SetNotifyFlags(0);
+  if (FAILED(hr))
+    return hr;
+
+  hr = job_->SetNotifyInterface(NULL);
+  if (FAILED(hr))
+    return hr;
+
+  static_cast<JobObserver*>(job_observer_.get())->set_callback(
+      JobObserver::JobChangedCallback());
+  job_observer_ = NULL;
+
+  return S_OK;
+}
+
+// Cleans up incompleted jobs that are too old.
+HRESULT BackgroundDownloader::CleanupStaleJobs(
+    base::win::ScopedComPtr<IBackgroundCopyManager> bits_manager) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+
+  if (!bits_manager)
+    return E_FAIL;
+
+  static base::Time last_sweep;
+
+  const base::TimeDelta time_delta(base::TimeDelta::FromDays(
+      kPurgeStaleJobsIntervalBetweenChecksDays));
+  const base::Time current_time(base::Time::Now());
+  if (last_sweep + time_delta > current_time)
+    return S_OK;
+
+  last_sweep = current_time;
+
+  std::vector<ScopedComPtr<IBackgroundCopyJob> > jobs;
+  HRESULT hr = FindBitsJobIf(
+      std::bind2nd(JobCreationOlderThanDays(), kPurgeStaleJobsAfterDays),
+      bits_manager,
+      &jobs);
+  if (FAILED(hr))
+    return hr;
+
+  for (size_t i = 0; i != jobs.size(); ++i) {
+    CancelJob(jobs[i]);
+  }
+
+  return S_OK;
+}
+
+}  // namespace component_updater
+