Implemented completion pings for component updates.

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/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/timer/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/browser/component_updater/component_updater_service_observer.h"
+#include "chrome/browser/component_updater/crx_update_item.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 "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"
 #include "url/gurl.h"
 
+using component_updater::ComponentUpdaterServiceObserver;
 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 {
 
@@ skipping 46 matching lines @@
                              &val)) {
       id.append(1, val + 'a');
     } else {
       id.append(1, 'a');
     }
   }
   DCHECK(Extension::IdIsValid(id));
   return id;
 }
 
-// Returns given a crx id it returns a small number, less than 100, that has a
-// decent chance of being unique among the registered components. It also has
-// the nice property that can be trivially computed by hand.
-static int CrxIdtoUMAId(const std::string& id) {
-  CHECK_GT(id.size(), 2U);
-  return id[0] + id[1] + id[2] - ('a' * 3);
-}
-
 // Helper to do version check for components.
 bool IsVersionNewer(const Version& current, const std::string& proposed) {
   Version proposed_ver(proposed);
   if (!proposed_ver.IsValid())
     return false;
   return (current.CompareTo(proposed_ver) < 0);
 }
 
 // Helper template class that allows our main class to have separate
 // OnURLFetchComplete() callbacks for different types of url requests
@@ skipping 37 matching lines @@
   }
   fetcher->Start();
 }
 
 // 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);
 }
 
-// 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:
-//
-//                                 kNew
-//                                  |
-//                                  V
-//     +----------------------> kChecking -<---------+-----<-------+
-//     |                            |                |             |
-//     |              error         V       no       |             |
-//  kNoUpdate <---------------- [update?] ->---- kUpToDate     kUpdated
-//     ^                            |                              ^
-//     |                        yes |                              |
-//     |        diff=false          V                              |
-//     |          +-----------> kCanUpdate                         |
-//     |          |                 |                              |
-//     |          |                 V              no              |
-//     |          |        [differential update?]->----+           |
-//     |          |                 |                  |           |
-//     |          |             yes |                  |           |
-//     |          |   error         V                  |           |
-//     |          +---------<- kDownloadingDiff        |           |
-//     |          |                 |                  |           |
-//     |          |                 |                  |           |
-//     |          |   error         V                  |           |

[cpu_(ooo_6.6-7.5), 2013/07/03 22:34:34]

why did we move my precious diagram?

[Sorin Jianu, 2013/07/03 23:35:13]

I considered the diagram to be part of the comment describing the CrxUpdateItem,
which also contains the definitions of the states. Where would you suggest this
be moved?

-//     |          +---------<- kUpdatingDiff ->--------|-----------+ success
-//     |                                               |           |
-//     |              error                            V           |
-//     +----------------------------------------- kDownloading     |
-//     |                                               |           |
-//     |              error                            V           |
-//     +------------------------------------------ kUpdating ->----+ success
-//
-struct CrxUpdateItem {
-  enum Status {
-    kNew,
-    kChecking,
-    kCanUpdate,
-    kDownloadingDiff,
-    kDownloading,
-    kUpdatingDiff,
-    kUpdating,
-    kUpdated,
-    kUpToDate,
-    kNoUpdate,
-    kLastStatus
-  };
+// Returns the error code which occured during the fetch.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;
 
-  Status status;
-  std::string id;
-  CrxComponent component;
+  const net::URLRequestStatus::Status status(fetcher.GetStatus().status());
+  if (status == net::URLRequestStatus::FAILED)
+    return fetcher.GetStatus().error();
 
-  base::Time last_check;
+  if (status == net::URLRequestStatus::IO_PENDING ||
+      status == net::URLRequestStatus::CANCELED)
+    return status;
 
-  // These members are initialized with their corresponding values from the
-  // update server response.
-  GURL crx_url;
-  GURL diff_crx_url;
-  int size;
-  int diff_size;
+  const int response_code(fetcher.GetResponseCode());
+  if (status == net::URLRequestStatus::SUCCESS && response_code != 200)
+    return response_code;
 
-  // The from/to version and fingerprint values.
-  Version previous_version;
-  Version next_version;
-  std::string previous_fp;
-  std::string next_fp;
-
-  // True if the differential update failed for any reason.
-  bool diff_update_failed;
-
-  CrxUpdateItem()
-      : status(kNew),
-        size(0),
-        diff_size(0),
-        diff_update_failed(false) {
-  }
-
-  // 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_;
-  };
-};
+  return -1;
+}
 
 // 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 ComponentUpdateService::Configurator& config) {
   return IsDiffUpdateAvailable(update_item) &&
          !update_item->diff_update_failed &&
          config.DeltasEnabled();
 }
 
-}  // namespace.
+}  // namespace
+
+CrxUpdateItem::CrxUpdateItem()
+    : status(kNew),
+      diff_update_failed(false),
+      error_category(0),
+      error_code(0),
+      extra_code1(0),
+      diff_error_category(0),
+      diff_error_code(0),
+      diff_extra_code1(0) {
+}
+
+CrxUpdateItem::~CrxUpdateItem() {
+}
 
 CrxComponent::CrxComponent()
     : installer(NULL) {
 }
 
 CrxComponent::~CrxComponent() {
 }
 
 //////////////////////////////////////////////////////////////////////////////
 // The one and only implementation of the ComponentUpdateService interface. In
@@ skipping 71 matching lines @@
     CRXContext() : installer(NULL) {}
   };
 
   void OnURLFetchComplete(const net::URLFetcher* source,
                           UpdateContext* context);
 
   void OnURLFetchComplete(const net::URLFetcher* source,
                           CRXContext* context);
 
  private:
+  enum ErrorCategory {
+    kErrorNone = 0,
+    kNetworkError,
+    kUnpackError,
+    kInstallError,
+  };
+
   // See ManifestParserBridge.
-  void OnParseUpdateManifestSucceeded(
-      const UpdateManifest::Results& results);
+  void OnParseUpdateManifestSucceeded(const UpdateManifest::Results& results);
 
   // See ManifestParserBridge.
   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,
                       int extended_error);
 
   size_t ChangeItemStatus(CrxUpdateItem::Status from,
                           CrxUpdateItem::Status to);
 
   CrxUpdateItem* FindUpdateItemById(const std::string& id);
 
   scoped_ptr<ComponentUpdateService::Configurator> config_;
 
   scoped_ptr<ComponentPatcher> component_patcher_;
 
   scoped_ptr<net::URLFetcher> url_fetcher_;
 
+  scoped_ptr<ComponentUpdaterServiceObserver> observer_;

[cpu_(ooo_6.6-7.5), 2013/07/03 22:34:34]

I like this approach, but it needs to be a bit more general, use the observer
framework from chrome, we are going to need more than one observer, for example
see
https://code.google.com/p/chromium/issues/detail?id=257193

Check chromium/src/base/observer_list.h

[Sorin Jianu, 2013/07/03 23:35:13]

Do you suggest I use ObserverList directly instead of a scoped_ptr or is it a
more sophisticated abstraction I need to use, such as ScopedObserver.

[cpu_(ooo_6.6-7.5), 2013/07/04 00:02:49]

Yeah use an observer list, with FOR_EACH_OBSERVER and all the toppings.

On 2013/07/03 23:35:13, Sorin Jianu wrote:

[Sorin Jianu, 2013/07/09 21:40:09]

Done.

+
   // A collection of every work item.
   typedef std::vector<CrxUpdateItem*> UpdateItems;
   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_;
 
   const Version chrome_version_;
   const std::string prod_id_;
-
   bool running_;
 
   DISALLOW_COPY_AND_ASSIGN(CrxUpdateService);
 };
 
 //////////////////////////////////////////////////////////////////////////////
 
 CrxUpdateService::CrxUpdateService(ComponentUpdateService::Configurator* config)
     : config_(config),
       component_patcher_(config->CreateComponentPatcher()),
+      observer_(new ComponentUpdaterServiceObserver(config)),
       chrome_version_(chrome::VersionInfo().Version()),
       prod_id_(chrome::OmahaQueryParams::GetProdIdString(
           chrome::OmahaQueryParams::CHROME)),
       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.
@@ skipping 142 matching lines @@
                       query))
     return false;
 
   item->status = CrxUpdateItem::kChecking;
   item->last_check = base::Time::Now();
   item->previous_version = item->component.version;
   item->next_version = Version();
   item->previous_fp = item->component.fingerprint;
   item->next_fp.clear();
   item->diff_update_failed = false;
+  item->error_category = 0;

[cpu_(ooo_6.6-7.5), 2013/07/03 22:34:34]

the return of the 'just in case' members :)

This was the source of the issues before, I want you send that stuff to the
observer, I don't think we need to maintain it here. Basically if I went to
mexico I want that to be in my passport, not intrinsically part of me.

Can we take a UpdateStatus struct with all this stuff in OnEvent, for example?

[Sorin Jianu, 2013/07/03 23:35:13]

Are you suggesting I define a new structure called UpdateStatus to include the
error information? If yes, who owns this structure and how do we reconcile it
with the fact that CrxUpdateItem is "the only per-item state structure". I hope
it is clear that some of these members hold the error state, for instance, if
delta errors occur, we'll remember them so we can send them in a ping
afterwards. I would also want to avoid duplicating members between data
structures. I am trying to understand why do we need both a CrxUpdateItem and an
UpdateStatus.

[cpu_(ooo_6.6-7.5), 2013/07/04 00:02:49]

Yeah.

 If yes, who owns this structure and how do we reconcile it
Can we just call OnEvent() when the error occurs and move the burden of
remembering this on the ping code?

 I would also want to avoid duplicating members between data
UpdateStatus is just struct to transfer stuff. It is not important, the core of
my idea is to call OnEvent() when stuff happens and have the ping code mantain
whatever list / state it needs to report.

[Sorin Jianu, 2013/07/09 21:40:09]

This is not "just in case members'. It provides the needed disambiguation
between overlapping ranges of errors originating in different modules of the
code, when we analyse errors on the server side.

[Sorin Jianu, 2013/07/09 21:40:09]

I think I understand what you are saying. The model you are suggesting works
well for UMA stats: stuff happens, UMA records it, accumulates it, and then
sends it up.
The issue here is that pings work in a slightly different way.

I would really want to avoid maintaining the state data in a different place,
have redundant data since the UpdateStatus will have to include many of the
members in the CrxUpdateItem anyway, plus I will have to store something similar
in the observer too, mapped from the component id.

Debugging is also simpler is everything is in the CrxUpdateItem. 

I think the original comment is right on: "This is the one and only per-item
state structure.". Moving stuff out means I have to have another per item state
structure someplace else, which I am trying to avoid.

Since you said this is not that important, I wonder if we can settle it like
this.

+  item->error_code = 0;
+  item->extra_code1 = 0;
+  item->diff_error_category = 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)
@@ skipping 195 matching lines @@
   size_t update_pending = 0;
   std::vector<UpdateManifest::Result>::const_iterator it;
   for (it = results.list.begin(); it != results.list.end(); ++it) {
     CrxUpdateItem* crx = FindUpdateItemById(it->extension_id);
     if (!crx)
       continue;
 
     if (crx->status != CrxUpdateItem::kChecking)
       continue;  // Not updating this component now.
 
-    config_->OnEvent(Configurator::kManifestCheck, CrxIdtoUMAId(crx->id));
-
     if (it->version.empty()) {
       // No version means no update available.
       crx->status = CrxUpdateItem::kNoUpdate;
       continue;
     }
     if (!IsVersionNewer(crx->component.version, it->version)) {
       // Our component is up to date.
       crx->status = CrxUpdateItem::kUpToDate;
       continue;
     }
     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->crx_url = it->crx_url;
-    crx->size = it->size;
     crx->diff_crx_url = it->diff_crx_url;
-    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.
   ChangeItemStatus(CrxUpdateItem::kChecking, CrxUpdateItem::kUpToDate);
 
   // If there are updates pending we do a short wait.
   ScheduleNextRun(update_pending > 0);
 }
 
 void CrxUpdateService::OnParseUpdateManifestFailed(
     const std::string& error_message) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   size_t count = ChangeItemStatus(CrxUpdateItem::kChecking,
                                   CrxUpdateItem::kNoUpdate);
-  config_->OnEvent(Configurator::kManifestError, static_cast<int>(count));
   DCHECK_GT(count, 0ul);
   ScheduleNextRun(false);
 }
 
 // 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);
   DCHECK(crx->status == CrxUpdateItem::kDownloadingDiff ||
          crx->status == CrxUpdateItem::kDownloading);
 
   if (source->FileErrorOccurred(&error_code) || !FetchSuccess(*source)) {
     if (crx->status == CrxUpdateItem::kDownloadingDiff) {
+      crx->diff_error_category = kNetworkError;
+      crx->diff_error_code = GetFetchError(*source);
       crx->diff_update_failed = true;
       size_t count = ChangeItemStatus(CrxUpdateItem::kDownloadingDiff,
                                       CrxUpdateItem::kCanUpdate);
       DCHECK_EQ(count, 1ul);
+      url_fetcher_.reset();
+
       ScheduleNextRun(true);
       return;
     }
+    crx->error_category = kNetworkError;
+    crx->error_code = GetFetchError(*source);
     size_t count = ChangeItemStatus(CrxUpdateItem::kDownloading,
                                     CrxUpdateItem::kNoUpdate);
     DCHECK_EQ(count, 1ul);
-    config_->OnEvent(Configurator::kNetworkError, CrxIdtoUMAId(context->id));
     url_fetcher_.reset();
 
+    observer_->OnEvent(component_updater::kEventUpdateComplete, crx);
+
     ScheduleNextRun(false);
   } else {
     base::FilePath temp_crx_path;
     CHECK(source->GetResponseAsFilePath(true, &temp_crx_path));
 
     size_t count = 0;
     if (crx->status == CrxUpdateItem::kDownloadingDiff) {
       count = ChangeItemStatus(CrxUpdateItem::kDownloadingDiff,
                                CrxUpdateItem::kUpdatingDiff);
     } else {
@@ skipping 40 matching lines @@
   BrowserThread::PostDelayedTask(
       BrowserThread::UI,
       FROM_HERE,
       base::Bind(&CrxUpdateService::DoneInstalling, base::Unretained(this),
                  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.
+// schedule the next check. Schedule a short delay before trying the full
+// update when the differential update failed.
 void CrxUpdateService::DoneInstalling(const std::string& component_id,
                                       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_update_failed = true;
-      size_t count = ChangeItemStatus(CrxUpdateItem::kUpdatingDiff,
-                                      CrxUpdateItem::kCanUpdate);
-      DCHECK_EQ(count, 1ul);
-      ScheduleNextRun(true);
-      return;
-    }
+  ErrorCategory error_category = kErrorNone;
+  switch (error) {
+    case ComponentUnpacker::kNone:
+      break;
+    case ComponentUnpacker::kInstallerError:
+      error_category = kInstallError;
+      break;
+    default:
+      error_category = kUnpackError;
+      break;
   }
 
-  item->status = (error == ComponentUnpacker::kNone) ? CrxUpdateItem::kUpdated :
-                                                       CrxUpdateItem::kNoUpdate;
+  CrxUpdateItem* item = FindUpdateItemById(component_id);
+  if (item->status == CrxUpdateItem::kUpdatingDiff &&
+      error != ComponentUnpacker::kNone) {
+    item->diff_error_category = error_category;
+    item->diff_error_code = error;
+    item->diff_extra_code1 = extra_code;
+    item->diff_update_failed = true;
+    size_t count = ChangeItemStatus(CrxUpdateItem::kUpdatingDiff,
+                                    CrxUpdateItem::kCanUpdate);
+    DCHECK_EQ(count, 1ul);
+    ScheduleNextRun(true);
+    return;
+  }
+
+  if (error != ComponentUnpacker::kNone) {
+    item->error_category = error_category;
+    item->error_code = error;
+    item->extra_code1 = extra_code;
+  }
+
+  item->status = (error == ComponentUnpacker::kNone) ?
+      CrxUpdateItem::kUpdated : CrxUpdateItem::kNoUpdate;
+
   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;
-  }
+  observer_->OnEvent(component_updater::kEventUpdateComplete, item);
 
-  config_->OnEvent(event, CrxIdtoUMAId(component_id));
   ScheduleNextRun(false);
 }
 
 // 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);
 }