Implemented completion pings for component updates.

chrome/browser/component_updater/component_updater_service.cc

Index: chrome/browser/component_updater/component_updater_service.cc
diff --git a/chrome/browser/component_updater/component_updater_service.cc b/chrome/browser/component_updater/component_updater_service.cc
index 7b3d68337ccc9e3c4924413d31ec08eb4b6f16b0..766206a2769a4a65da1596f33a59a15952421c07 100644
--- a/chrome/browser/component_updater/component_updater_service.cc
+++ b/chrome/browser/component_updater/component_updater_service.cc
@@ -24,6 +24,8 @@
 #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"
@@ -41,6 +43,7 @@
 #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;
@@ -107,14 +110,6 @@ static std::string HexStringToID(const std::string& hexstr) {
   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);
@@ -172,97 +167,28 @@ bool FetchSuccess(const net::URLFetcher& fetcher) {
          (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;
-
-  base::Time last_check;
-
-  // 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;
-
-  // 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) {
-  }
+  const net::URLRequestStatus::Status status(fetcher.GetStatus().status());
+  if (status == net::URLRequestStatus::FAILED)
+    return fetcher.GetStatus().error();
 
-  // Function object used to find a specific component.
-  class FindById {
-   public:
-    explicit FindById(const std::string& id) : id_(id) {}
+  if (status == net::URLRequestStatus::IO_PENDING ||
+      status == net::URLRequestStatus::CANCELED)
+    return status;
 
-    bool operator() (CrxUpdateItem* item) const {
-      return (item->id == id_);
-    }
-   private:
-    const std::string& id_;
-  };
-};
+  const int response_code(fetcher.GetResponseCode());
+  if (status == net::URLRequestStatus::SUCCESS && response_code != 200)
+    return response_code;
+
+  return -1;
+}
 
 // Returns true if a differential update is available for the update item.
 bool IsDiffUpdateAvailable(const CrxUpdateItem* update_item) {
@@ -278,7 +204,21 @@ bool CanTryDiffUpdate(const CrxUpdateItem* update_item,
          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) {
@@ -370,9 +310,15 @@ class CrxUpdateService : public ComponentUpdateService {
                           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);
@@ -402,6 +348,8 @@ class CrxUpdateService : public ComponentUpdateService {
 
   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_;
@@ -413,7 +361,6 @@ class CrxUpdateService : public ComponentUpdateService {
 
   const Version chrome_version_;
   const std::string prod_id_;
-
   bool running_;
 
   DISALLOW_COPY_AND_ASSIGN(CrxUpdateService);
@@ -424,6 +371,7 @@ class CrxUpdateService : public ComponentUpdateService {
 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)),
@@ -586,6 +534,12 @@ bool CrxUpdateService::AddItemToUpdateCheck(CrxUpdateItem* item,
   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;
 }
 
@@ -801,8 +755,6 @@ void CrxUpdateService::OnParseUpdateManifestSucceeded(
     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;
@@ -823,9 +775,7 @@ void CrxUpdateService::OnParseUpdateManifestSucceeded(
     // 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;
@@ -850,7 +800,6 @@ void CrxUpdateService::OnParseUpdateManifestFailed(
   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);
 }
@@ -869,19 +818,26 @@ void CrxUpdateService::OnURLFetchComplete(const net::URLFetcher* source,
 
   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;
@@ -942,45 +898,55 @@ void CrxUpdateService::Install(const CRXContext* 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;
-    }
-  }
-
-  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;
+  ErrorCategory error_category = kErrorNone;
   switch (error) {
     case ComponentUnpacker::kNone:
-      event = Configurator::kComponentUpdated;
       break;
     case ComponentUnpacker::kInstallerError:
-      event = Configurator::kInstallerError;
+      error_category = kInstallError;
       break;
     default:
-      event = Configurator::kUnpackError;
+      error_category = kUnpackError;
       break;
   }
 
-  config_->OnEvent(event, CrxIdtoUMAId(component_id));
+  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;
+  }
+
+  observer_->OnEvent(component_updater::kEventUpdateComplete, item);
+
   ScheduleNextRun(false);
 }