AU: Reduce instances of signalling the crash reporter.

omaha_request_action.cc

Index: omaha_request_action.cc
diff --git a/omaha_request_action.cc b/omaha_request_action.cc
index bb2275d2f78e58389702854bb1348b7ebd18e888..c74ea8d648b420d1eeda41300829a6831f7f01d9 100644
--- a/omaha_request_action.cc
+++ b/omaha_request_action.cc
@@ -341,13 +341,6 @@ void OmahaRequestAction::TransferComplete(HttpFetcher *fetcher,
   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();
@@ -359,6 +352,17 @@ void OmahaRequestAction::TransferComplete(HttpFetcher *fetcher,
         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.