AU: Reduce instances of signalling the crash reporter.

omaha_request_action.cc

 // Copyright (c) 2010 The Chromium OS 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 "update_engine/omaha_request_action.h"
 
 #include <inttypes.h>
 
 #include <sstream>
 
@@ skipping 323 matching lines @@
 
 // If the transfer was successful, this uses libxml2 to parse the response
 // and fill in the appropriate fields of the output object. Also, notifies
 // the processor that we're done.
 void OmahaRequestAction::TransferComplete(HttpFetcher *fetcher,
                                           bool successful) {
   ScopedActionCompleter completer(processor_, this);
   LOG(INFO) << "Omaha request response: " << string(response_buffer_.begin(),
                                                     response_buffer_.end());
 
-  // Events are best effort transactions -- assume they always succeed.
-  if (IsEvent()) {
-    CHECK(!HasOutputPipe()) << "No output pipe allowed for event requests.";
-    completer.set_code(kActionCodeSuccess);

[petkov, 2011/01/12 01:10:22]

Here you could:

if (event->result ==
OmahaEvent::kResultError && !successful && IsOfficialBuild()) {
  utils::ScheduleCrashReporterUpload();
}

See other comment why this might be a better implementation.

[petkov, 2011/01/12 01:17:08]

Of course I meant:

if (event->result == OmahaEvent::kResultError && successful &&
IsOfficialBuild()) ...

-    return;
-  }
-
   if (!successful) {
     LOG(ERROR) << "Omaha request network transfer failed.";
     int code = GetHTTPResponseCode();
     // Makes sure we send sane error values.
     if (code < 0 || code >= 1000) {
       code = 999;
     }
     completer.set_code(static_cast<ActionExitCode>(
         kActionCodeOmahaRequestHTTPResponseBase + code));
     return;
   }
+
+  // Events are best effort transactions, but it's still okay to fail them,

[petkov, 2011/01/12 01:10:22]

This is somewhat dangerous. The code runs for both real error events and
"normal" events -- e.g., the successful kTypeUpdateComplete at the end of the
attempter action list.

If, for example, you lose Internet connection during the post-install action,
the update will technically succeed but the Event will not go through and the
update attempter will say that the whole update has failed. Maybe we should
extend the comment to make that clear.

A safer way to implement this might be to move the ScheduleCrashReporterUpload
call here. Basically, check for IsEvent first (original condition order), then
inside the if do the upload only if it's a real error event (if event->result ==
OmahaEvent::kResultError) and !successful and IsOfficialBuild.

See the other comment for a possible implementation.

+  // (which happens above) since we can use the failure code to decide if we
+  // should signal the crash reporter about a failure.
+  // If we get here, assume success for the Event.
+  if (IsEvent()) {
+    CHECK(!HasOutputPipe()) << "No output pipe allowed for event requests.";
+    completer.set_code(kActionCodeSuccess);
+    return;
+  }
+
   if (!HasOutputPipe()) {
     // Just set success to whether or not the http transfer succeeded,
     // which must be true at this point in the code.
     completer.set_code(kActionCodeSuccess);
     return;
   }
 
   // parse our response and fill the fields in the output object
   scoped_ptr_malloc<xmlDoc, ScopedPtrXmlDocFree> doc(
       xmlParseMemory(&response_buffer_[0], response_buffer_.size()));
@@ skipping 72 matching lines @@
   output_object.needs_admin =
       XmlGetProperty(updatecheck_node, "needsadmin") == "true";
   output_object.prompt = XmlGetProperty(updatecheck_node, "Prompt") == "true";
   output_object.is_delta =
       XmlGetProperty(updatecheck_node, "IsDelta") == "true";
   output_object.deadline = XmlGetProperty(updatecheck_node, "deadline");
   SetOutputObject(output_object);
 }
 
 };  // namespace chromeos_update_engine