Differential updates for components.

chrome/installer/setup/setup_util.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.
 //
 // This file declares util functions for setup project.
 
 #include "chrome/installer/setup/setup_util.h"
 
 #include <windows.h>
 
 #include "base/command_line.h"
 #include "base/file_util.h"
 #include "base/files/file_enumerator.h"
 #include "base/files/file_path.h"
 #include "base/logging.h"
 #include "base/process_util.h"
 #include "base/strings/string_util.h"
 #include "base/version.h"
 #include "base/win/windows_version.h"
 #include "chrome/installer/util/copy_tree_work_item.h"
 #include "chrome/installer/util/installation_state.h"
 #include "chrome/installer/util/installer_state.h"
 #include "chrome/installer/util/master_preferences.h"
 #include "chrome/installer/util/util_constants.h"
 #include "chrome/installer/util/work_item.h"
 #include "courgette/courgette.h"
+#include "courgette/third_party/bsdiff.h"
 #include "third_party/bspatch/mbspatch.h"
 
 namespace installer {
 
 namespace {
 
 // Launches |setup_exe| with |command_line|, save --install-archive and its
 // value if present. Returns false if the process failed to launch. Otherwise,
 // waits indefinitely for it to exit and populates |exit_code| as expected. On
 // the off chance that waiting itself fails, |exit_code| is set to
@@ skipping 42 matching lines @@
 
 // Returns true if product |type| cam be meaningfully installed without the
 // --multi-install flag.
 bool SupportsSingleInstall(BrowserDistribution::Type type) {
   return (type == BrowserDistribution::CHROME_BROWSER ||
           type == BrowserDistribution::CHROME_FRAME);
 }
 
 }  // namespace
 
+int CourgettePatchFiles(const base::FilePath& src,
+                        const base::FilePath& patch,
+                        const base::FilePath& dest) {
+  VLOG(1) << "Applying Courgette patch " << patch.value() << " to file "
+          << src.value() << " and generating file " << dest.value();
+
+  if (src.empty() || patch.empty() || dest.empty()) {

[grt (UTC plus 2), 2013/06/19 14:45:36]

nit: no braces for single-line conditionals like this

[Sorin Jianu, 2013/06/19 17:14:45]

Done.

+     return installer::PATCH_INVALID_ARGUMENTS;
+  }
+
+  const courgette::Status patch_status =
+      courgette::ApplyEnsemblePatch(src.value().c_str(),
+                                    patch.value().c_str(),
+                                    dest.value().c_str());
+  const int exit_code = (patch_status != courgette::C_OK) ?
+      static_cast<int>(patch_status) + kCourgetteErrorOffset : 0;
+  if (exit_code) {

[grt (UTC plus 2), 2013/06/19 14:45:36]

  LOG_IF(ERROR, exit_code) ...
(see comment below about always logging)

[Sorin Jianu, 2013/06/19 17:14:45]

Done.

+     VLOG(1) << "Failed to apply patch " << patch.value()
+             << ". err=" << exit_code;
+  }
+
+  return exit_code;
+}
+
+int BsdiffPatchFiles(const base::FilePath& src,
+                     const base::FilePath& patch,
+                     const base::FilePath& dest) {
+  VLOG(1) << "Applying bsdiff patch " << patch.value() << " to file "
+          << src.value() << " and generating file " << dest.value();
+
+  if (src.empty() || patch.empty() || dest.empty()) {

[grt (UTC plus 2), 2013/06/19 14:45:36]

nit: no braces for single-line conditionals like this

[Sorin Jianu, 2013/06/19 17:14:45]

Done.

+     return installer::PATCH_INVALID_ARGUMENTS;
+  }
+
+  const int patch_status = courgette::ApplyBinaryPatch(src, patch, dest);
+  const int exit_code = patch_status ?
+                        patch_status + kBsdiffErrorOffset : 0;
+  if (exit_code) {

[grt (UTC plus 2), 2013/06/19 14:45:36]

  LOG_IF(ERROR, exit_code) ...
(see comment below about always logging)

[Sorin Jianu, 2013/06/19 17:14:45]

Done.

+     VLOG(1) << "Failed to apply patch " << patch.value()
+             << ". err=" << exit_code;
+  }
+
+  return exit_code;
+}
+
 int ApplyDiffPatch(const base::FilePath& src,
                    const base::FilePath& patch,
                    const base::FilePath& dest,
                    const InstallerState* installer_state) {
   VLOG(1) << "Applying patch " << patch.value() << " to file " << src.value()
           << " and generating file " << dest.value();
 
   if (installer_state != NULL)
     installer_state->UpdateStage(installer::ENSEMBLE_PATCHING);
 
@@ skipping 13 matching lines @@
   // we will see.  If we run into them, return an error and stay on the
   // 'ENSEMBLE_PATCHING' update stage.
   if (patch_status == courgette::C_DISASSEMBLY_FAILED ||
       patch_status == courgette::C_STREAM_ERROR) {
     return MEM_ERROR;
   }
 
   if (installer_state != NULL)
     installer_state->UpdateStage(installer::BINARY_PATCHING);
 
-  return ApplyBinaryPatch(src.value().c_str(), patch.value().c_str(),
-                          dest.value().c_str());
+  int binary_patch_status = ApplyBinaryPatch(src.value().c_str(),
+                                             patch.value().c_str(),
+                                             dest.value().c_str());
+
+  VLOG(1) << "Failed to apply patch " << patch.value()

[grt (UTC plus 2), 2013/06/19 14:45:36]

what do you think about surfacing this in the log always, rather than only when
--verbose-logging is specified? consider:
  LOG_IF(WARNING, binary_patch_status != OK)
or even ERROR if this should really never happen.

[Sorin Jianu, 2013/06/19 17:14:45]

This looks like a logging bug, since I was logging the error all the time.
Fixed.

I have to know more about the logging policy in Chrome to give you an informed
answer. For the moment, I will mechanically follow your advice and change the
log statements.

[grt (UTC plus 2), 2013/06/19 20:17:39]

Do you think it's useful to find these log message after the fact (i.e., ask a
user to send in their chrome_installer.log file after they've reported things
not working properly), or only while a component is being updated (by way of
sawbuck)? VLOG(1) is good for the latter, but not for the former. If you want to
collect logs from customers, then I suggest harmonizing on either LOG at WARNING
if the conditions leading to them aren't surprising, or LOG at ERROR if they
truly are errors indicating something is wrong.

[Sorin Jianu, 2013/06/19 21:05:54]

Thank you, please tell me if this is what you have in mind.

+          << " using bsdiff. err=" << binary_patch_status;
+
+  return binary_patch_status;
 }
 
 Version* GetMaxVersionFromArchiveDir(const base::FilePath& chrome_path) {
   VLOG(1) << "Looking for Chrome version folder under " << chrome_path.value();
   Version* version = NULL;
   base::FileEnumerator version_enum(chrome_path, false,
       base::FileEnumerator::DIRECTORIES);
   // TODO(tommi): The version directory really should match the version of
   // setup.exe.  To begin with, we should at least DCHECK that that's true.
 
@@ skipping 228 matching lines @@
 }
 
 ScopedTokenPrivilege::~ScopedTokenPrivilege() {
   if (is_enabled_ && previous_privileges_.PrivilegeCount != 0) {
     ::AdjustTokenPrivileges(token_, FALSE, &previous_privileges_,
                             sizeof(TOKEN_PRIVILEGES), NULL, NULL);
   }
 }
 
 }  // namespace installer