Differential updates for components.

chrome/browser/component_updater/component_updater_service.cc

 // Copyright (c) 2012 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/component_updater_service.h"
 
 #include <algorithm>
 #include <set>
 #include <vector>
 
 #include "base/at_exit.h"
 #include "base/bind.h"
+#include "base/compiler_specific.h"
 #include "base/file_util.h"
 #include "base/files/file_path.h"
+#include "base/guid.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
+#include "base/sys_info.h"
 #include "base/timer.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/component_updater/component_patcher.h"
 #include "chrome/browser/component_updater/component_unpacker.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_utility_messages.h"
 #include "chrome/common/chrome_version_info.h"
 #include "chrome/common/extensions/extension.h"
+#include "chrome/common/omaha_query_params/omaha_query_params.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/utility_process_host.h"
 #include "content/public/browser/utility_process_host_client.h"
 #include "googleurl/src/gurl.h"
 #include "net/base/escape.h"
 #include "net/base/load_flags.h"
 #include "net/base/net_errors.h"
 #include "net/url_request/url_fetcher.h"
 #include "net/url_request/url_fetcher_delegate.h"
 #include "net/url_request/url_request_status.h"
 
 using content::BrowserThread;
 using content::UtilityProcessHost;
 using content::UtilityProcessHostClient;
 using extensions::Extension;
 
 // The component updater is designed to live until process shutdown, so
 // base::Bind() calls are not refcounted.
 
 namespace {
+
+typedef ComponentUpdateService::Configurator Config;

[robertshield, 2013/06/18 13:27:07]

This typedef appears to be used only in a couple of places below (whereas the
original type is used more often). I'd suggest getting rid of the typedef, since
it doesn't greatly enhance readability. If you are attached to it, please use it
everywhere. 

[Sorin Jianu, 2013/06/18 17:00:12]

Done.

+
 // Manifest sources, from most important to least important.
 const CrxComponent::UrlSource kManifestSources[] = {
   CrxComponent::BANDAID,
   CrxComponent::CWS_PUBLIC,
-  CrxComponent::CWS_SANDBOX
+  CrxComponent::CWS_SANDBOX,
 };
 
 // Extends an omaha compatible update check url |query| string. Does
 // not mutate the string if it would be longer than |limit| chars.
 bool AddQueryString(const std::string& id,
                     const std::string& version,
+                    const std::string& fingerprint,
                     size_t limit,
                     std::string* query) {
   std::string additional =
-      base::StringPrintf("id=%s&v=%s&uc", id.c_str(), version.c_str());
+      base::StringPrintf("id=%s&v=%s&fp=%s&uc",
+                         id.c_str(), version.c_str(), fingerprint.c_str());
   additional = "x=" + net::EscapeQueryParamValue(additional, true);
   if ((additional.size() + query->size() + 1) > limit)
     return false;
   if (!query->empty())
     query->append(1, '&');
   query->append(additional);
   return true;
 }
 
 // Create the final omaha compatible query. The |extra| is optional and can
@@ skipping 81 matching lines @@
                         net::LOAD_DISABLE_CACHE);
   // TODO(cpu): Define our retry and backoff policy.
   fetcher->SetAutomaticallyRetryOn5xx(false);
   if (save_to_file) {
     fetcher->SaveResponseToTemporaryFile(
         BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE));
   }
   fetcher->Start();
 }
 
-// Returs true if the url request of |fetcher| was succesful.
+// Returns true if the url request of |fetcher| was succesful.
 bool FetchSuccess(const net::URLFetcher& fetcher) {
   return (fetcher.GetStatus().status() == net::URLRequestStatus::SUCCESS) &&
          (fetcher.GetResponseCode() == 200);
 }
 
+// Returns the error code which occured during the fetch. This function
+// is intended to be called for logging purposes only, since it folds different
+// types of errors to fit them in the returned type. The function returns 0 if
+// the fetch was successful. If errors happen, the function could return a
+// network error, an http response code, or the status of the fetch, if the
+// fetch is pending or canceled.
+int GetFetchError(const net::URLFetcher& fetcher) {
+  if (FetchSuccess(fetcher))
+    return 0;
+
+  const net::URLRequestStatus::Status status(fetcher.GetStatus().status());
+  if (status == net::URLRequestStatus::FAILED)
+    return fetcher.GetStatus().error();
+
+  if (status == net::URLRequestStatus::IO_PENDING ||
+      status == net::URLRequestStatus::CANCELED)
+    return status;
+
+  const int response_code(fetcher.GetResponseCode());
+  if (status == net::URLRequestStatus::SUCCESS && response_code != 200)
+    return response_code;
+
+  return -1;
+}
+
+
 // This is the one and only per-item state structure. Designed to be hosted
 // in a std::vector or a std::list. The two main members are |component|
 // which is supplied by the the component updater client and |status| which
 // is modified as the item is processed by the update pipeline. The expected
 // transition graph is:
-//                                error          error         error
-//              +--kNoUpdate<------<-------+------<------+------<------+
-//              |                          |             |             |
-//              V                  yes     |             |             |
-//  kNew --->kChecking-->[update?]----->kCanUpdate-->kDownloading-->kUpdating
-//              ^               |                                      |
-//              |               |no                                    |
-//              |--kUpToDate<---+                                      |
-//              |                                  success             |
-//              +--kUpdated<-------------------------------------------+
+//
+//                                 kNew
+//                                  |
+//                                  V
+//     +----------------------> kChecking -<---------+-----<-------+
+//     |                            |                |             |
+//     |                            V       no       |             |
+//  kNoUpdate                   [update?] ->---- kUpToDate     kUpdated
+//     ^                            |                              ^
+//     |                        yes |                              |
+//     |        diff=false          V                              |
+//     |          +-----------> kCanUpdate                         |
+//     |          |                 |                              |
+//     |          |                 V              no              |
+//     |          |        [differential update?]->----+           |
+//     |          |                 |                  |           |
+//     |          |             yes |                  |           |
+//     |          |   error         V                  |           |
+//     |          +---------<- kDownloadingDiff        |           |
+//     |          |                 |                  |           |
+//     |          |                 |                  |           |
+//     |          |   error         V                  |           |
+//     |          +---------<- kUpdatingDiff ->--------|-----------+ success
+//     |                                               |           |
+//     |              error                            V           |
+//     +----------------------------------------- kDownloading     |
+//     |                                               |           |
+//     |              error                            V           |
+//     +------------------------------------------ kUpdating ->----+ success
 //
 struct CrxUpdateItem {
   enum Status {
     kNew,
     kChecking,
     kCanUpdate,
+    kDownloadingDiff,
     kDownloading,
+    kUpdatingDiff,
     kUpdating,
     kUpdated,
     kUpToDate,
     kNoUpdate,
     kLastStatus
   };
 
   Status status;
+  std::string id;
+  CrxComponent component;
+
+  base::Time last_check;
+
+  // True if the update response includes an update for this component.
+  bool is_update_available;
+
+  // True is a completion ping has been queued for this component. If an update

[cpu_(ooo_6.6-7.5), 2013/06/18 17:02:06]

"True is"?

[Sorin Jianu, 2013/06/18 20:46:39]

Done.

+  // is available for this component, one completion ping must be sent
+  // after the component has reached either the kNoUpdate or kUpdated states.
+  bool ping_queued;

[cpu_(ooo_6.6-7.5), 2013/06/18 17:02:06]

maybe I got it wrong but you don't need this. If it CanPing() and it reached one
of the states, a ping will be made.

[Sorin Jianu, 2013/06/18 20:46:39]

We removed the queuing of pings

+
+  // These members are initialized with their corresponding values from the
+  // update server response.
   GURL crx_url;
-  std::string id;
-  base::Time last_check;
-  CrxComponent component;

[cpu_(ooo_6.6-7.5), 2013/06/18 17:02:06]

In here we differ in philosophy. As you can see I took pains to minimize the per
item state, the new CL makes the per item state everything that you can possibly
care about.

[Sorin Jianu, 2013/06/18 20:46:39]

I am sorry if we are breaking the existing design. CrxUpdateItem is more
stateful since we need to remember some more state information regarding to
delta updates. We are also introducing completion pings which include some of
the state information. 

In general, what is the concern that made you minimize the per item state
maintained in the CrxUpdateItem? I thought that this was the purpose of the
CrxUpdateItem: hold the update state of the object as it runs through the state
machine driven by the UpdateService.

We have agreed that more state is needed and the state has to go somewhere. I am
open to discuss this, we want to do what is right.

+  GURL diff_crx_url;
+  std::string package_hash;
+  std::string diff_package_hash;
+  int size;
+  int diff_size;

[cpu_(ooo_6.6-7.5), 2013/06/18 17:02:06]

why do we need these sizes here? I failed to find where they are used. Also why
are they int? what does it mean to have a negative size?

[Sorin Jianu, 2013/06/18 20:46:39]

These are values that we parsed from the update response. In Omaha we use these
values for both validating the file integrity and for sending back quality of
service data.

We parse these values as ints. It does not mean sizes can be negative but we do
have a limit of how large the components can be. Is there a portable type in
Chromium for file sizes? What do you recommend?

+
+  // The from/to version and fingerprint values.
+  Version previous_version;
   Version next_version;
+  std::string previous_fp;
+  std::string next_fp;
 
-  CrxUpdateItem() : status(kNew) {}
+  // True if the differential update failed for any reason.
+  bool diff_update_failed;
+
+  // The error information for full and differential updates.
+  int error_code;
+  int extra_code1;
+  int diff_error_code;
+  int diff_extra_code1;

[cpu_(ooo_6.6-7.5), 2013/06/18 17:02:06]

sigh

[Sorin Jianu, 2013/06/18 20:46:39]

I understand. The challenge we have here is that the amount of data we want to
return is more than what an integer can fit, so there has be be a way to hold
more error state, unless we don't want to return extended error information.
Which is so important to understand why things break in production.

+
+  CrxUpdateItem()
+      : status(kNew),
+        is_update_available(false),
+        ping_queued(false),
+        size(0),
+        diff_size(0),
+        diff_update_failed(false),
+        error_code(0),
+        extra_code1(0),
+        diff_error_code(0),
+        diff_extra_code1(0) {
+  }
 
   // Function object used to find a specific component.
   class FindById {
    public:
     explicit FindById(const std::string& id) : id_(id) {}
 
     bool operator() (CrxUpdateItem* item) const {
       return (item->id == id_);
     }
    private:
     const std::string& id_;
   };
 };
 
+// Returns true if a differential update is available for the update item.
+bool IsDiffUpdateAvailable(const CrxUpdateItem* update_item) {
+  return update_item->diff_crx_url.is_valid();
+}
+
+// Returns true if a differential update is available, it has not failed yet,
+// and the configuration allows it.
+bool CanTryDiffUpdate(const CrxUpdateItem* update_item, const Config& config) {
+  return IsDiffUpdateAvailable(update_item) &&
+         !update_item->diff_update_failed &&
+         config.DeltasEnabled();
+}
+
 }  // namespace.
 
-typedef ComponentUpdateService::Configurator Config;
-
 CrxComponent::CrxComponent()
     : installer(NULL),
       source(BANDAID) {
 }
 
 CrxComponent::~CrxComponent() {
 }
 
 //////////////////////////////////////////////////////////////////////////////
 // The one and only implementation of the ComponentUpdateService interface. In
 // charge of running the show. The main method is ProcessPendingItems() which
 // is called periodically to do the upgrades/installs or the update checks.
 // An important consideration here is to be as "low impact" as we can to the
 // rest of the browser, so even if we have many components registered and
 // eligible for update, we only do one thing at a time with pauses in between
 // the tasks. Also when we do network requests there is only one |url_fetcher_|
-// in flight at at a time.
+// in flight at a time.
 // There are no locks in this code, the main structure |work_items_| is mutated
 // only from the UI thread. The unpack and installation is done in the file
 // thread and the network requests are done in the IO thread and in the file
 // thread.
 class CrxUpdateService : public ComponentUpdateService {
  public:
   explicit CrxUpdateService(ComponentUpdateService::Configurator* config);
 
   virtual ~CrxUpdateService();
 
@@ skipping 38 matching lines @@
     CrxUpdateService* service_;
     DISALLOW_COPY_AND_ASSIGN(ManifestParserBridge);
   };
 
   // Context for a update check url request. See DelegateWithContext above.
   struct UpdateContext {
     base::Time start;
     UpdateContext() : start(base::Time::Now()) {}
   };
 
+  // Context for a completion ping. See DelegateWithContext above. The type
+  // is needed for template type deduction and it could be used in the
+  // future to pass arguments to the callback.
+  struct PingContext {
+  };
+
   // Context for a crx download url request. See DelegateWithContext above.
   struct CRXContext {
     ComponentInstaller* installer;
     std::vector<uint8> pk_hash;
     std::string id;
+    std::string fingerprint;
     CRXContext() : installer(NULL) {}
   };
 
   void OnURLFetchComplete(const net::URLFetcher* source,
                           UpdateContext* context);
 
   void OnURLFetchComplete(const net::URLFetcher* source,
                           CRXContext* context);
 
+  void OnURLFetchComplete(const net::URLFetcher* source,
+                          PingContext* context);
+
  private:
   // See ManifestParserBridge.
   void OnParseUpdateManifestSucceeded(
       const UpdateManifest::Results& results);
 
   // See ManifestParserBridge.
-  void OnParseUpdateManifestFailed(
-      const std::string& error_message);
+  void OnParseUpdateManifestFailed(const std::string& error_message);
 
   bool AddItemToUpdateCheck(CrxUpdateItem* item, std::string* query);
 
   void ProcessPendingItems();
 
   void ScheduleNextRun(bool step_delay);
 
   void ParseManifest(const std::string& xml);
 
   void Install(const CRXContext* context, const base::FilePath& crx_path);
 
   void DoneInstalling(const std::string& component_id,
-                      ComponentUnpacker::Error error);
+                      ComponentUnpacker::Error error,
+                      int extended_error);
 
   size_t ChangeItemStatus(CrxUpdateItem::Status from,
                           CrxUpdateItem::Status to);
 
   CrxUpdateItem* FindUpdateItemById(const std::string& id);
 
+  // Returns true if completion pings are enabled.
+  bool CanPing() const;
+
+  void SendPing(const std::string& ping);
+
+  // These functions build a ping request. Building the ping changes the state
+  // of the update item to indicate that a completion ping has been collected
+  // and queued for this item.
+  std::string BuildPing() const;
+  std::string BuildPingApps() const;
+  static std::string BuildPingEvent(CrxUpdateItem* item);
+
   scoped_ptr<Config> config_;
 
+  scoped_ptr<ComponentPatcher> component_patcher_;
+
   scoped_ptr<net::URLFetcher> url_fetcher_;
 
+  scoped_ptr<net::URLFetcher> ping_sender_;
+
+  // A collection of every work item.
   typedef std::vector<CrxUpdateItem*> UpdateItems;
-  // A collection of every work item.
   UpdateItems work_items_;
 
   // A particular set of items from work_items_, which should be checked ASAP.
   std::set<CrxUpdateItem*> requested_work_items_;
 
   base::OneShotTimer<CrxUpdateService> timer_;
 
-  Version chrome_version_;
+  const Version chrome_version_;
+  const std::string prod_id_;
+  const std::string os_type_;
+  const std::string os_version_;
 
   bool running_;
 
   DISALLOW_COPY_AND_ASSIGN(CrxUpdateService);
 };
 
 //////////////////////////////////////////////////////////////////////////////
 
-CrxUpdateService::CrxUpdateService(
-    ComponentUpdateService::Configurator* config)
+CrxUpdateService::CrxUpdateService(ComponentUpdateService::Configurator* config)
     : config_(config),
+      component_patcher_(config->CreateComponentPatcher()),
       chrome_version_(chrome::VersionInfo().Version()),
+      prod_id_(chrome::OmahaQueryParams::GetProdIdString(
+          chrome::OmahaQueryParams::CHROME)),
+      os_type_(chrome::VersionInfo().OSType()),

[cpu_(ooo_6.6-7.5), 2013/06/18 17:02:06]

is this expensive to query, enough that we want to have in the object state?

You will find that one of my fixations is to minimize object state.

I am guessing that a lot of the new state is going to go somewhere once we
settle the item o line 986

[Sorin Jianu, 2013/06/18 20:46:39]

I agree that the state has to go somewhere or it has to be queried as needed.

Querying these members is not expensive, at least for |prod_id_| and |os_type_|.
My thinking is that since these members are used together, they are maintained
together as immutable members in the service.

+      os_version_(base::SysInfo().OperatingSystemVersion()),
       running_(false) {
 }
 
 CrxUpdateService::~CrxUpdateService() {
   // Because we are a singleton, at this point only the UI thread should be
   // alive, this simplifies the management of the work that could be in
   // flight in other threads.
   Stop();
   STLDeleteElements(&work_items_);
 }
@@ skipping 32 matching lines @@
 void CrxUpdateService::ScheduleNextRun(bool step_delay) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(url_fetcher_.get() == NULL);
   CHECK(!timer_.IsRunning());
   // It could be the case that Stop() had been called while a url request
   // or unpacking was in flight, if so we arrive here but |running_| is
   // false. In that case do not loop again.
   if (!running_)
     return;
 
+  if (!step_delay && CanPing()) {
+    const std::string ping(BuildPing());
+    if (!ping.empty())
+      SendPing(ping);
+  }
+
   // Keep the delay short if in the middle of an update (step_delay),
   // or there are new requested_work_items_ that have not been processed yet.
   int64 delay = (step_delay || requested_work_items_.size() > 0)
       ? config_->StepDelay() : config_->NextCheckDelay();
 
   if (!step_delay) {
     content::NotificationService::current()->Notify(
         chrome::NOTIFICATION_COMPONENT_UPDATER_SLEEPING,
         content::Source<ComponentUpdateService>(this),
         content::NotificationService::NoDetails());
@@ skipping 12 matching lines @@
   CrxUpdateItem::FindById finder(id);
   UpdateItems::iterator it = std::find_if(work_items_.begin(),
                                           work_items_.end(),
                                           finder);
   if (it == work_items_.end())
     return NULL;
   return (*it);
 }
 
 // Changes all the components in |work_items_| that have |from| status to
 // |to| statatus and returns how many have been changed.

[cpu_(ooo_6.6-7.5), 2013/06/18 17:02:06]

typo here :)

[Sorin Jianu, 2013/06/18 20:46:39]

What is the typo? I am sorry, we might have fixed this already, I can't see what
is wrong.

[waffles, 2013/06/18 20:50:41]

Sorry for the confusion, I fixed that one. :)

[Sorin Jianu, 2013/06/19 00:29:10]

Done.

 size_t CrxUpdateService::ChangeItemStatus(CrxUpdateItem::Status from,
                                           CrxUpdateItem::Status to) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   size_t count = 0;
   for (UpdateItems::iterator it = work_items_.begin();
        it != work_items_.end(); ++it) {
     CrxUpdateItem* item = *it;
     if (item->status != from)
       continue;
     item->status = to;
@@ skipping 20 matching lines @@
   CrxUpdateItem* uit;
   uit = FindUpdateItemById(id);
   if (uit) {
     uit->component = component;
     return kReplaced;
   }
 
   uit = new CrxUpdateItem;
   uit->id.swap(id);
   uit->component = component;
+
   work_items_.push_back(uit);
   // If this is the first component registered we call Start to
   // schedule the first timer.
   if (running_ && (work_items_.size() == 1))
     Start();
 
   return kOk;
 }
 
 // Sets a component to be checked for updates.
-// The componet to add is |crxit| and the |query| string is modified with the
+// The component to add is |crxit| and the |query| string is modified with the
 // required omaha compatible query. Returns false when the query strings
 // is longer than specified by UrlSizeLimit().
 bool CrxUpdateService::AddItemToUpdateCheck(CrxUpdateItem* item,
                                             std::string* query) {
   if (!AddQueryString(item->id,
                       item->component.version.GetString(),
+                      item->component.fingerprint,
                       config_->UrlSizeLimit(), query))
     return false;
+
   item->status = CrxUpdateItem::kChecking;
   item->last_check = base::Time::Now();
+  item->is_update_available = false;
+  item->ping_queued = false;
+  item->previous_version = item->component.version;
+  item->next_version = Version();
+  item->previous_fp = item->component.fingerprint;
+  item->next_fp = "";
+  item->diff_update_failed = false;
+  item->error_code = 0;
+  item->extra_code1 = 0;
+  item->diff_error_code = 0;
+  item->diff_extra_code1 = 0;
   return true;
 }
 
 // Start the process of checking for an update, for a particular component
 // that was previously registered.
 ComponentUpdateService::Status CrxUpdateService::CheckForUpdateSoon(
     const CrxComponent& component) {
   if (component.pk_hash.empty() ||
       !component.version.IsValid() ||
       !component.installer)
     return kError;
 
   std::string id =
     HexStringToID(StringToLowerASCII(base::HexEncode(&component.pk_hash[0],
                                      component.pk_hash.size()/2)));
 
   CrxUpdateItem* uit;
   uit = FindUpdateItemById(id);
   if (!uit)
     return kError;
 
   // Check if the request is too soon.
   base::TimeDelta delta = base::Time::Now() - uit->last_check;
-  if (delta < base::TimeDelta::FromSeconds(config_->OnDemandDelay())) {
+  if (delta < base::TimeDelta::FromSeconds(config_->OnDemandDelay()))
     return kError;
-  }
 
   switch (uit->status) {
     // If the item is already in the process of being updated, there is
     // no point in this call, so return kInProgress.
     case CrxUpdateItem::kChecking:
     case CrxUpdateItem::kCanUpdate:
+    case CrxUpdateItem::kDownloadingDiff:
     case CrxUpdateItem::kDownloading:
+    case CrxUpdateItem::kUpdatingDiff:
     case CrxUpdateItem::kUpdating:
       return kInProgress;
     // Otherwise the item was already checked a while back (or it is new),
     // set its status to kNew to give it a slightly higher priority.
     case CrxUpdateItem::kNew:
     case CrxUpdateItem::kUpdated:
     case CrxUpdateItem::kUpToDate:
     case CrxUpdateItem::kNoUpdate:
       uit->status = CrxUpdateItem::kNew;
       requested_work_items_.insert(uit);
@@ skipping 18 matching lines @@
 void CrxUpdateService::ProcessPendingItems() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   // First check for ready upgrades and do one. The first
   // step is to fetch the crx package.
   for (UpdateItems::const_iterator it = work_items_.begin();
        it != work_items_.end(); ++it) {
     CrxUpdateItem* item = *it;
     if (item->status != CrxUpdateItem::kCanUpdate)
       continue;
     // Found component to update, start the process.
-    item->status = CrxUpdateItem::kDownloading;
     CRXContext* context = new CRXContext;
     context->pk_hash = item->component.pk_hash;
     context->id = item->id;
     context->installer = item->component.installer;
+    context->fingerprint = item->next_fp;
+    GURL package_url;
+    if (CanTryDiffUpdate(item, *config_)) {
+      package_url = item->diff_crx_url;
+      item->status = CrxUpdateItem::kDownloadingDiff;
+    } else {
+      package_url = item->crx_url;
+      item->status = CrxUpdateItem::kDownloading;
+    }
     url_fetcher_.reset(net::URLFetcher::Create(
-        0, item->crx_url, net::URLFetcher::GET,
+        0, package_url, net::URLFetcher::GET,
         MakeContextDelegate(this, context)));
     StartFetch(url_fetcher_.get(), config_->RequestContext(), true);
     return;
   }
 
   for (size_t ix = 0; ix != arraysize(kManifestSources); ++ix) {
     const CrxComponent::UrlSource manifest_source = kManifestSources[ix];
 
     std::string query;
     // If no pending upgrades, we check if there are new components we have not
@@ skipping 90 matching lines @@
   delete context;
 }
 
 // Parsing the manifest is either done right now for tests or in a sandboxed
 // process for the production environment. This mitigates the case where an
 // attacker was able to feed us a malicious xml string.
 void CrxUpdateService::ParseManifest(const std::string& xml) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   if (config_->InProcess()) {
     UpdateManifest manifest;
-    if (!manifest.Parse(xml)) {
+    if (!manifest.Parse(xml))
        CrxUpdateService::OnParseUpdateManifestFailed(manifest.errors());
-    } else {
+    else
        CrxUpdateService::OnParseUpdateManifestSucceeded(manifest.results());
-    }
   } else {
     UtilityProcessHost* host =
         UtilityProcessHost::Create(new ManifestParserBridge(this),
                                    base::MessageLoopProxy::current().get());
     host->EnableZygote();
     host->Send(new ChromeUtilityMsg_ParseUpdateManifest(xml));
   }
 }
 
 // A valid Omaha update check has arrived, from only the list of components that
@@ skipping 27 matching lines @@
     }
     if (!it->browser_min_version.empty()) {
       if (IsVersionNewer(chrome_version_, it->browser_min_version)) {
         // Does not apply for this chrome version.
         crx->status = CrxUpdateItem::kNoUpdate;
         continue;
       }
     }
     // All test passed. Queue an upgrade for this component and fire the
     // notifications.
+    crx->is_update_available = true;
     crx->crx_url = it->crx_url;
+    crx->package_hash = it->package_hash;
+    crx->size = it->size;
+    crx->diff_crx_url = it->diff_crx_url;
+    crx->diff_package_hash = it->diff_package_hash;
+    crx->diff_size = it->diff_size;
     crx->status = CrxUpdateItem::kCanUpdate;
     crx->next_version = Version(it->version);
+    crx->next_fp = it->package_fingerprint;
     ++update_pending;
 
     content::NotificationService::current()->Notify(
         chrome::NOTIFICATION_COMPONENT_UPDATE_FOUND,
         content::Source<std::string>(&crx->id),
         content::NotificationService::NoDetails());
   }
 
   // All the components that are not mentioned in the manifest we
   // consider them up to date.
@@ skipping 14 matching lines @@
 }
 
 // Called when the CRX package has been downloaded to a temporary location.
 // Here we fire the notifications and schedule the component-specific installer
 // to be called in the file thread.
 void CrxUpdateService::OnURLFetchComplete(const net::URLFetcher* source,
                                           CRXContext* context) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   int error_code = net::OK;
 
+  CrxUpdateItem* crx = FindUpdateItemById(context->id);
+
   if (source->FileErrorOccurred(&error_code) || !FetchSuccess(*source)) {
-    size_t count = ChangeItemStatus(CrxUpdateItem::kDownloading,
-                                    CrxUpdateItem::kNoUpdate);
-    DCHECK_EQ(count, 1ul);
+    if (crx->status == CrxUpdateItem::kDownloadingDiff) {
+      crx->diff_error_code = GetFetchError(*source);
+      crx->diff_update_failed = true;
+      crx->status = CrxUpdateItem::kCanUpdate;
+      BrowserThread::PostTask(
+          BrowserThread::UI,
+          FROM_HERE,
+          base::Bind(&CrxUpdateService::ProcessPendingItems,
+                     base::Unretained(this)));
+      return;
+    }
+    crx->error_code = GetFetchError(*source);
+    crx->status = CrxUpdateItem::kNoUpdate;

[cpu_(ooo_6.6-7.5), 2013/06/18 17:02:06]

Use ChangeItemStatus

[Sorin Jianu, 2013/06/18 20:46:39]

ChangeItemStatus changes the state of all items (from, to). We only want to
change the state for this item.

[cpu_(ooo_6.6-7.5), 2013/06/18 22:02:36]

Correct. The way it is used like in line 802 is because there can be only one
component in kDownloading, this is important for the operation of the component
updater of having only one component in flight. It is sort of a self discipline
to use when it can be used.

[Sorin Jianu, 2013/06/19 00:29:10]

Done by using ChangeItemStatus throughout this function.

     config_->OnEvent(Configurator::kNetworkError, CrxIdtoUMAId(context->id));

[cpu_(ooo_6.6-7.5), 2013/06/18 17:02:06]

and here we get to the fun part, as you can see there is an existing error
reporting framework. I don't see a compelling reason to have two of them.

The choice of moving the error reporting out of the core and into the
configurator was intentional. How errors are processed and reported is not
'core' to this code.

I am willing to let go of UMA, even though that is how chrome manages reporting
and use your reporting on the view that is omaha folks monitoring it, but in
turn I want all of it out into the configurator. 

Ideally the only code you will see here is config_=>OnEvent(....).

[Sorin Jianu, 2013/06/18 20:46:39]

Let's discuss this and see what is reasonable to do. Let's see what we want to
do and what we agree on so far:
* we want to send completion pings for all users to monitor the quality of the
update service
* we agree that Omaha will do this and we can do outside UMA, just as we do it
for Chrome updates.
* we agree that the error handling and reporting should be decoupled from the
rest of the code.

We are happy to follow your advice and move the error handling and the pinging
logic out in the configurator. The state that needs to be sent to the server
must be encapsulated in some object. Right now that object is the CrxUpdateItem.
At this time, I see three options:
1. Introduce a dependency in ChromeConfigurator on the CrxUpdateItem so that an
item can be passed as an argument to the OnEvent.
2. Pass the state information in some other kind of object which becomes an
argument to OnEvent.
2. Pass the state information as standalone parameters to OnEvent, as in:
config_->OnEvent(..., id, diff_error_code, error_code, version,
previous_version,....)
Which way would you recommend we go? 

[cpu_(ooo_6.6-7.5), 2013/06/18 22:18:25]

Neat to have agreement here.

I like 2a or 2b. If it was me I would try 2b and see how it looks, I would try
that first because then in the configurator I can maintain my own vector of
UpdateItemStatus or whatever name. 

Basically lets nuke the existing UMA reporting and lets do your with the least
coupling that we can do.

I would also be interesting to have this pinging decoupled from the update
scheduler, like say only once every 8 hours or so bulk reporting of the whole
thing.

[Sorin Jianu, 2013/06/19 00:29:10]

I am mailing the comments and uploading the fixes I have so far except this one,
which I am working on it at the moment.

     url_fetcher_.reset();
     ScheduleNextRun(false);
   } else {
     base::FilePath temp_crx_path;
     CHECK(source->GetResponseAsFilePath(true, &temp_crx_path));
-    size_t count = ChangeItemStatus(CrxUpdateItem::kDownloading,
-                                    CrxUpdateItem::kUpdating);
-    DCHECK_EQ(count, 1ul);
+    crx->status = (crx->status == CrxUpdateItem::kDownloadingDiff) ?
+        CrxUpdateItem::kUpdatingDiff : CrxUpdateItem::kUpdating;
+
     url_fetcher_.reset();
 
     content::NotificationService::current()->Notify(
         chrome::NOTIFICATION_COMPONENT_UPDATE_READY,
         content::Source<std::string>(&context->id),
         content::NotificationService::NoDetails());
 
     // Why unretained? See comment at top of file.
     BrowserThread::PostDelayedTask(
         BrowserThread::FILE,
         FROM_HERE,
         base::Bind(&CrxUpdateService::Install,
                    base::Unretained(this),
                    context,
                    temp_crx_path),
         base::TimeDelta::FromMilliseconds(config_->StepDelay()));
   }
 }
 
 // Install consists of digital signature verification, unpacking and then
 // calling the component specific installer. All that is handled by the
 // |unpacker|. If there is an error this function is in charge of deleting
 // the files created.
 void CrxUpdateService::Install(const CRXContext* context,
                                const base::FilePath& crx_path) {
   // This function owns the |crx_path| and the |context| object.
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  ComponentUnpacker
-      unpacker(context->pk_hash, crx_path, context->installer);
-  if (!file_util::Delete(crx_path, false)) {
+  ComponentUnpacker unpacker(context->pk_hash,
+                             crx_path,
+                             context->fingerprint,
+                             component_patcher_.get(),
+                             context->installer);
+  if (!file_util::Delete(crx_path, false))
     NOTREACHED() << crx_path.value();
-  }
   // Why unretained? See comment at top of file.
   BrowserThread::PostDelayedTask(
       BrowserThread::UI,
       FROM_HERE,
       base::Bind(&CrxUpdateService::DoneInstalling, base::Unretained(this),
-                 context->id, unpacker.error()),
+                 context->id, unpacker.error(), unpacker.extended_error()),
       base::TimeDelta::FromMilliseconds(config_->StepDelay()));
   delete context;
 }
 
 // Installation has been completed. Adjust the component status and
 // schedule the next check.
 void CrxUpdateService::DoneInstalling(const std::string& component_id,
-                                      ComponentUnpacker::Error error) {
+                                      ComponentUnpacker::Error error,
+                                      int extra_code) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   CrxUpdateItem* item = FindUpdateItemById(component_id);
+  if (item->status == CrxUpdateItem::kUpdatingDiff) {
+    if (error != ComponentUnpacker::kNone) {
+      item->diff_error_code = error;
+      item->diff_extra_code1 = extra_code;
+      item->diff_update_failed = true;
+      item->status = CrxUpdateItem::kCanUpdate;
+      BrowserThread::PostTask(
+          BrowserThread::UI,
+          FROM_HERE,
+          base::Bind(&CrxUpdateService::ProcessPendingItems,
+                     base::Unretained(this)));
+      return;
+    }
+  }
+
+  if (error != ComponentUnpacker::kNone) {
+    item->error_code = error;
+    item->extra_code1 = extra_code;
+  }
+
   item->status = (error == ComponentUnpacker::kNone) ? CrxUpdateItem::kUpdated :
                                                        CrxUpdateItem::kNoUpdate;
-  if (item->status == CrxUpdateItem::kUpdated)
+  if (item->status == CrxUpdateItem::kUpdated) {
     item->component.version = item->next_version;
+    item->component.fingerprint = item->next_fp;
+  }
 
   Configurator::Events event;
   switch (error) {
     case ComponentUnpacker::kNone:
       event = Configurator::kComponentUpdated;
       break;
     case ComponentUnpacker::kInstallerError:
       event = Configurator::kInstallerError;
       break;
     default:
       event = Configurator::kUnpackError;
       break;
   }
 
   config_->OnEvent(event, CrxIdtoUMAId(component_id));
   ScheduleNextRun(false);
 }
 
+bool CrxUpdateService::CanPing() const {
+  return config_->PingUrl().is_valid();
+}
+
+void CrxUpdateService::SendPing(const std::string& ping) {
+  VLOG(3) << "Sending ping. " << ping;

[cpu_(ooo_6.6-7.5), 2013/06/18 17:02:06]

please remove all the VLOGing, instrument your local build if you want to printf
debug.

[Sorin Jianu, 2013/06/18 20:46:39]

Done.

+  const std::string ping_url(config_->PingUrl().spec());
+  ping_sender_.reset(net::URLFetcher::Create(
+      0, GURL(ping_url), net::URLFetcher::POST,
+      MakeContextDelegate(this, new PingContext())));
+  ping_sender_->SetUploadData(std::string("application/xml"), ping);
+  StartFetch(ping_sender_.get(), config_->RequestContext(), false);
+}
+
+void CrxUpdateService::OnURLFetchComplete(const net::URLFetcher* source,
+                                          PingContext* context) {
+  VLOG(3) << "Sending component update ping returned "
+          << FetchSuccess(*source);
+  ping_sender_.reset();
+}
+
+// Builds a ping message for the update items that have completed. Returns
+// an empty string if there are no completed items.
+std::string CrxUpdateService::BuildPing() const {
+  const std::string apps(BuildPingApps());
+  if (apps.empty())
+    return std::string();
+
+  const char response_format[] =
+      "<o:gupdate xmlns:o=\"http://www.google.com/update2/request\" "
+      "protocol=\"2.0\" version=\"%s-%s\" requestid=\"{%s}\"> "
+      "<o:os platform=\"%s\" version=\"%s\"/> "
+      "%s"
+      "</o:gupdate>";
+  const std::string response_envelope(
+      base::StringPrintf(response_format,
+                         prod_id_.c_str(),
+                         chrome_version_.GetString().c_str(),
+                         base::GenerateGUID().c_str(),
+                         os_type_.c_str(),
+                         os_version_.c_str(),
+                         apps.c_str()));
+  return response_envelope;
+}
+
+// Returns a string containing the sequence of app elements inside the
+// ping message.
+std::string CrxUpdateService::BuildPingApps() const {
+  const char app_format[] = "<o:app appid=\"%s\" version=\"%s\">%s</o:app>";
+
+  std::string ping_apps;
+  for (UpdateItems::const_iterator it = work_items_.begin();
+       it != work_items_.end(); ++it) {
+    CrxUpdateItem* item = *it;
+    // Only ping once if an update was available and the item is completed.
+    if (item->is_update_available && !item->ping_queued &&
+        (item->status == CrxUpdateItem::kNoUpdate ||
+         item->status == CrxUpdateItem::kUpdated)) {
+      const std::string version = item->component.version.GetString().c_str();
+      ping_apps += base::StringPrintf(app_format,
+                                      item->id.c_str(),
+                                      version.c_str(),
+                                      BuildPingEvent(item).c_str());
+    }
+  }
+  return ping_apps;
+}
+
+// Returns a string representing one ping event for an update item.
+std::string CrxUpdateService::BuildPingEvent(CrxUpdateItem* item) {
+  DCHECK(item->status == CrxUpdateItem::kNoUpdate ||
+         item->status == CrxUpdateItem::kUpdated);
+  DCHECK(item->is_update_available);
+
+  using base::StringAppendF;
+
+  std::string ping_event("<o:event eventtype=\"3\"");
+  const int event_result = item->status == CrxUpdateItem::kUpdated;
+  StringAppendF(&ping_event, " eventresult=\"%d\"", event_result);
+  StringAppendF(&ping_event, " previousversion=\"%s\"",
+                item->previous_version.GetString().c_str());
+  StringAppendF(&ping_event, " nextversion=\"%s\"",
+                item->next_version.GetString().c_str());
+  if (item->error_code)
+    StringAppendF(&ping_event, " errorcode=\"%d\"", item->error_code);
+  if (item->extra_code1)
+    StringAppendF(&ping_event, " extracode1=\"%d\"", item->extra_code1);
+  if (IsDiffUpdateAvailable(item))
+    StringAppendF(&ping_event, " diffresult=\"%d\"", !item->diff_update_failed);
+  if (item->diff_error_code)
+    StringAppendF(&ping_event, " differrorcode=\"%d\"", item->diff_error_code);
+  if (item->diff_extra_code1) {
+    StringAppendF(&ping_event,
+                  " diffextracode1=\"%d\"",
+                  item->diff_extra_code1);
+  }
+  if (!item->previous_fp.empty())
+    StringAppendF(&ping_event, " previousfp=\"%s\"", item->previous_fp.c_str());
+  if (!item->next_fp.empty())
+    StringAppendF(&ping_event, " nextfp=\"%s\"", item->next_fp.c_str());
+  StringAppendF(&ping_event, "/>");
+  item->ping_queued = true;
+  return ping_event;
+}
+
+
 // The component update factory. Using the component updater as a singleton
 // is the job of the browser process.
 ComponentUpdateService* ComponentUpdateServiceFactory(
     ComponentUpdateService::Configurator* config) {
   DCHECK(config);
   return new CrxUpdateService(config);
 }