Add viewing of error messages from Keystone upon self-update failure.

chrome/browser/mac/keystone_glue.mm

Index: chrome/browser/mac/keystone_glue.mm
diff --git a/chrome/browser/mac/keystone_glue.mm b/chrome/browser/mac/keystone_glue.mm
index c6333e7c65031739873ab630349ace1d5f8c0f3d..254c1a546fe83ac4a1d1254276edff34db3aac8c 100644
--- a/chrome/browser/mac/keystone_glue.mm
+++ b/chrome/browser/mac/keystone_glue.mm
@@ -127,7 +127,9 @@ class PerformBridge : public base::RefCountedThreadSafe<PerformBridge> {
 // Called when an update check or update installation is complete.  Posts the
 // kAutoupdateStatusNotification notification to the default notification
 // center.
-- (void)updateStatus:(AutoupdateStatus)status version:(NSString*)version;
+- (void)updateStatus:(AutoupdateStatus)status
+             version:(NSString*)version
+               error:(NSString *)error;

[Boris Vidolov, 2016/03/08 22:32:51]

Fix the spaces around NSString pointer type. I believe that the Chrome style
guide is "(NSString*)parameter"

[Ryan Myers (chromium), 2016/03/22 22:39:22]

Done.

 
 // Returns the version of the currently-installed application on disk.
 - (NSString*)currentlyInstalledVersion;
@@ -135,7 +137,7 @@ class PerformBridge : public base::RefCountedThreadSafe<PerformBridge> {
 // These three methods are used to determine the version of the application
 // currently installed on disk, compare that to the currently-running version,
 // decide whether any updates have been installed, and call
-// -updateStatus:version:.
+// -updateStatus:version:error:.
 //
 // In order to check the version on disk, the installed application's
 // Info.plist dictionary must be read; in order to see changes as updates are
@@ -153,8 +155,8 @@ class PerformBridge : public base::RefCountedThreadSafe<PerformBridge> {
 // CFBundleShortVersionString key, and performs
 // -determineUpdateStatusForVersion: on the main thread.
 // -determineUpdateStatusForVersion: does the actual comparison of the version
-// on disk with the running version and calls -updateStatus:version: with the
-// results of its analysis.
+// on disk with the running version and calls -updateStatus:version:error: with
+// the results of its analysis.
 - (void)determineUpdateStatusAsync;
 - (void)determineUpdateStatus;
 - (void)determineUpdateStatusForVersion:(NSString*)version;
@@ -208,6 +210,7 @@ class PerformBridge : public base::RefCountedThreadSafe<PerformBridge> {
 NSString* const kAutoupdateStatusNotification = @"AutoupdateStatusNotification";
 NSString* const kAutoupdateStatusStatus = @"status";
 NSString* const kAutoupdateStatusVersion = @"version";
+NSString* const kAutoupdateStatusErrorMessages = @"errormessages";
 
 namespace {
 
@@ -536,12 +539,14 @@ NSString* const kVersionKey = @"KSVersion";
 }
 
 - (void)registerWithKeystone {
-  [self updateStatus:kAutoupdateRegistering version:nil];
+  [self updateStatus:kAutoupdateRegistering version:nil error:nil];
 
   NSDictionary* parameters = [self keystoneParameters];
   BOOL result = [registration_ registerWithParameters:parameters];
   if (!result) {
-    [self updateStatus:kAutoupdateRegisterFailed version:nil];
+    // TODO: If Keystone ever makes a variant of this API with a withError:
+    // parameter, include the error message here in the call to updateStatus:.
+    [self updateStatus:kAutoupdateRegisterFailed version:nil error:nil];
     return;
   }
 
@@ -562,15 +567,22 @@ NSString* const kVersionKey = @"KSVersion";
 
 - (void)registrationComplete:(NSNotification*)notification {
   NSDictionary* userInfo = [notification userInfo];
+
+  // Error messages may be included, depending on the version of Keystone.

[Boris Vidolov, 2016/03/08 22:32:51]

I don't believe that this is correct. It depends on the version of the
registration framework, which is typically similar to the version of this code.
As such, I would omit the comment altogether.

[Ryan Myers (chromium), 2016/03/22 22:39:22]

Done.

+  NSString *errorMessages =

[Boris Vidolov, 2016/03/08 22:32:51]

Fix the position of the '*': should be "NSString* ". Applies to the whole file.

[Ryan Myers (chromium), 2016/03/22 22:39:22]

Done.

+      [userInfo objectForKey:ksr::KSRegistrationUpdateCheckRawErrorMessagesKey];
+
   if ([[userInfo objectForKey:ksr::KSRegistrationStatusKey] boolValue]) {
     if ([self isSystemTicketDoomed]) {

[Boris Vidolov, 2016/03/08 22:32:51]

Even in the happy case there could be warnings, represented in the
"errorMessages". Would you like to log them for diagnostic purposes?

[Ryan Myers (chromium), 2016/03/22 22:39:22]

Done. (Passed them on to updateStatus; version_updater_mac can decide whether or
not to render/log them in a happy case.  See there, where I log if they're
present on status != Failed.)

-      [self updateStatus:kAutoupdateNeedsPromotion version:nil];
+      [self updateStatus:kAutoupdateNeedsPromotion version:nil error:nil];
     } else {
-      [self updateStatus:kAutoupdateRegistered version:nil];
+      [self updateStatus:kAutoupdateRegistered version:nil error:nil];
     }
   } else {
     // Dump registration_?
-    [self updateStatus:kAutoupdateRegisterFailed version:nil];
+    [self updateStatus:kAutoupdateRegisterFailed
+               version:nil
+                 error:errorMessages];
   }
 }
 
@@ -586,11 +598,13 @@ NSString* const kVersionKey = @"KSVersion";
   DCHECK(![self asyncOperationPending]);

[Boris Vidolov, 2016/03/08 22:32:51]

I believe that this should not be a debug check, but just a return if another
check is happening. The condition is hit when the user clicks the refresh button
on the chrome://help page, as it takes some time for the update to be
downloaded. The impact is very interesting when active updates are present: two
update checks start, followed by two installs. Both update checks come back as
"update available", but only the first "install" succeeds in installing it. The
second waits for the first due to Keystone agent lock. The second "install"
finds Chrome "up to date", which confuses the registration framework and the
user sees error 12. In reality there is no error.

[Ryan Myers (chromium), 2016/03/22 22:39:22]

Done.

 
   if (!registration_) {
-    [self updateStatus:kAutoupdateCheckFailed version:nil];
+    [self updateStatus:kAutoupdateCheckFailed
+               version:nil
+                 error:@"keystone_glue: Keystone not available."];

[Boris Vidolov, 2016/03/08 22:32:52]

This should happen only on debug builds and Chrome clones. Still, maybe we
should do something more user-friendly. "Cannot setup automatic updates.
KeystoneRegistration is not available."

'Keystone' is an internal name, but KeystoneRegistration is the name of the
framework, so maybe it is ok to use it.

[Ryan Myers (chromium), 2016/03/22 22:39:22]

Done.

     return;
   }
 
-  [self updateStatus:kAutoupdateChecking version:nil];
+  [self updateStatus:kAutoupdateChecking version:nil error:nil];
 
   // All checks from inside Chrome are considered user-initiated, because they
   // only happen following a user action, such as visiting the about page.
@@ -605,14 +619,18 @@ NSString* const kVersionKey = @"KSVersion";
 - (void)checkForUpdateComplete:(NSNotification*)notification {
   NSDictionary* userInfo = [notification userInfo];
 
+  // Error messages may be included, depending on the version of Keystone.

[Boris Vidolov, 2016/03/08 22:32:51]

Ditto for the version of Keystone in the comment

[Ryan Myers (chromium), 2016/03/22 22:39:22]

Done.

+  NSString *errorMessages =
+      [userInfo objectForKey:ksr::KSRegistrationUpdateCheckRawErrorMessagesKey];
+
   if ([[userInfo objectForKey:ksr::KSRegistrationUpdateCheckErrorKey]
           boolValue]) {
-    [self updateStatus:kAutoupdateCheckFailed version:nil];
+    [self updateStatus:kAutoupdateCheckFailed version:nil error:errorMessages];
   } else if ([[userInfo objectForKey:ksr::KSRegistrationStatusKey] boolValue]) {
     // If an update is known to be available, go straight to
     // -updateStatus:version:.  It doesn't matter what's currently on disk.
     NSString* version = [userInfo objectForKey:ksr::KSRegistrationVersionKey];
-    [self updateStatus:kAutoupdateAvailable version:version];
+    [self updateStatus:kAutoupdateAvailable version:version error:nil];

[Boris Vidolov, 2016/03/08 22:32:52]

Again, maybe we should log the reported errors? It is good to see them in the
Console.

[Ryan Myers (chromium), 2016/03/22 22:39:22]

Done.

   } else {
     // If no updates are available, check what's on disk, because an update
     // may have already been installed.  This check happens on another thread,
@@ -625,11 +643,13 @@ NSString* const kVersionKey = @"KSVersion";
   DCHECK(![self asyncOperationPending]);
 
   if (!registration_) {
-    [self updateStatus:kAutoupdateInstallFailed version:nil];
+    [self updateStatus:kAutoupdateInstallFailed
+               version:nil
+                 error:@"keystone_glue: Keystone not available."];

[Boris Vidolov, 2016/03/08 22:32:52]

Ditto

[Ryan Myers (chromium), 2016/03/22 22:39:22]

Done.

     return;
   }
 
-  [self updateStatus:kAutoupdateInstalling version:nil];
+  [self updateStatus:kAutoupdateInstalling version:nil error:nil];
 
   [registration_ startUpdate];
 
@@ -640,13 +660,17 @@ NSString* const kVersionKey = @"KSVersion";
 - (void)installUpdateComplete:(NSNotification*)notification {
   NSDictionary* userInfo = [notification userInfo];
 
+  // Error messages may be included, depending on the version of Keystone.

[Boris Vidolov, 2016/03/08 22:32:51]

Ditto

[Ryan Myers (chromium), 2016/03/22 22:39:22]

Done.

[Boris Vidolov, 2016/03/23 00:24:13]

Still here...

[Ryan Myers (chromium), 2016/03/23 00:48:23]

Done.

+  NSString *errorMsgs =
+      [userInfo objectForKey:ksr::KSRegistrationUpdateCheckRawErrorMessagesKey];
+
   // http://crbug.com/160308 and b/7517358: when using system Keystone and on
   // a user ticket, KSUpdateCheckSuccessfulKey will be NO even when an update
   // was installed correctly, so don't check it. It should be redudnant when
   // KSUpdateCheckSuccessfullyInstalledKey is checked.
   if (![[userInfo objectForKey:ksr::KSUpdateCheckSuccessfullyInstalledKey]
           intValue]) {
-    [self updateStatus:kAutoupdateInstallFailed version:nil];
+    [self updateStatus:kAutoupdateInstallFailed version:nil error:errorMsgs];
   } else {
     updateSuccessfullyInstalled_ = YES;

[Boris Vidolov, 2016/03/08 22:32:52]

Ditto for logging

[Ryan Myers (chromium), 2016/03/22 22:39:22]

Done.

 
@@ -709,10 +733,12 @@ NSString* const kVersionKey = @"KSVersion";
     }
   }
 
-  [self updateStatus:status version:version];
+  [self updateStatus:status version:version error:nil];
 }
 
-- (void)updateStatus:(AutoupdateStatus)status version:(NSString*)version {
+- (void)updateStatus:(AutoupdateStatus)status
+             version:(NSString*)version
+               error:(NSString *)error {
   NSNumber* statusNumber = [NSNumber numberWithInt:status];
   NSMutableDictionary* dictionary =
       [NSMutableDictionary dictionaryWithObject:statusNumber
@@ -720,6 +746,9 @@ NSString* const kVersionKey = @"KSVersion";
   if (version) {
     [dictionary setObject:version forKey:kAutoupdateStatusVersion];
   }
+  if (error) {
+    [dictionary setObject:version forKey:kAutoupdateStatusErrorMessages];
+  }
 
   NSNotification* notification =
       [NSNotification notificationWithName:kAutoupdateStatusNotification
@@ -873,7 +902,7 @@ NSString* const kVersionKey = @"KSVersion";
 
   synchronousPromotion_ = synchronous;
 
-  [self updateStatus:kAutoupdatePromoting version:nil];
+  [self updateStatus:kAutoupdatePromoting version:nil error:nil];
 
   // TODO(mark): Remove when able!
   //
@@ -922,12 +951,24 @@ NSString* const kVersionKey = @"KSVersion";
   if (status != errAuthorizationSuccess) {
     OSSTATUS_LOG(ERROR, status)
         << "AuthorizationExecuteWithPrivileges preflight";
-    [self updateStatus:kAutoupdatePromoteFailed version:nil];
+
+    NSString *errorMessage =
+        [NSString stringWithFormat:@"Pre-flight script failed to launch: %ld",
+                                   (long)status];
+    [self updateStatus:kAutoupdatePromoteFailed
+               version:nil
+                 error:errorMessage];
     return;
   }
   if (exit_status != 0) {
     LOG(ERROR) << "keystone_promote_preflight status " << exit_status;
-    [self updateStatus:kAutoupdatePromoteFailed version:nil];
+
+    NSString *errorMessage =
+        [NSString stringWithFormat:@"Promotion pre-flight script failed: %d",
+                                     exit_status];
+    [self updateStatus:kAutoupdatePromoteFailed
+               version:nil
+                 error:errorMessage];
     return;
   }
 
@@ -951,7 +992,9 @@ NSString* const kVersionKey = @"KSVersion";
 
   if (![registration_ promoteWithParameters:parameters
                               authorization:authorization_]) {
-    [self updateStatus:kAutoupdatePromoteFailed version:nil];
+    // TODO: If Keystone ever makes a variant of this API with a withError:
+    // parameter, include the error message here in the call to updateStatus:.
+    [self updateStatus:kAutoupdatePromoteFailed version:nil error:nil];
     authorization_.reset();
     return;
   }
@@ -980,7 +1023,7 @@ NSString* const kVersionKey = @"KSVersion";
     }
   } else {
     authorization_.reset();
-    [self updateStatus:kAutoupdatePromoteFailed version:nil];
+    [self updateStatus:kAutoupdatePromoteFailed version:nil error:nil];
   }
 
   if (synchronousPromotion_) {
@@ -1036,7 +1079,7 @@ NSString* const kVersionKey = @"KSVersion";
 - (void)changePermissionsForPromotionComplete {
   authorization_.reset();
 
-  [self updateStatus:kAutoupdatePromoted version:nil];
+  [self updateStatus:kAutoupdatePromoted version:nil error:nil];
 }
 
 - (void)setAppPath:(NSString*)appPath {