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..8e2780decc23ac8084b2d53ff25a94ba190dabf9 100644
--- a/chrome/browser/mac/keystone_glue.mm
+++ b/chrome/browser/mac/keystone_glue.mm
@@ -59,6 +59,11 @@ NSString* SystemBrandFilePath() {
   return [kBrandSystemFile stringByStandardizingPath];
 }
 
+// Non-localized error message to return when we can't initiate any actions

[Mark Mentovai, 2016/03/23 15:52:03]

Why is this not localized?

This should be localized. We have a bunch of infrastructure for localizing
strings.

[Boris Vidolov, 2016/03/23 17:17:24]

If I follow the code correctly, this message should never appear. Why do we need
to waste localization resources for this particular message?

[Mark Mentovai, 2016/03/23 17:22:06]

Boris Vidolov wrote:
If it shouldn’t ever happen, then the logic should be a DCHECK(), we shouldn’t
be firing off this message (localized or not).

[Boris Vidolov, 2016/03/23 17:37:29]

Agree.

[Ryan Myers (chromium), 2016/03/23 22:29:38]

OK, removing and going back to DCHECKs.  (TBH, I'm skeptical that we even need
the DCHECKs, on a second review of the code.)  Done.

+// because Keystone isn't present or won't initialize.
+NSString* kNoRegistrationErrorMessage =
+      @"Cannot start automatic updates - KeystoneRegistration not available.";
+
 // Adaptor for scheduling an Objective-C method call on a |WorkerPool|
 // thread.
 class PerformBridge : public base::RefCountedThreadSafe<PerformBridge> {
@@ -127,7 +132,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;
 
 // Returns the version of the currently-installed application on disk.
 - (NSString*)currentlyInstalledVersion;
@@ -135,7 +142,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 +160,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 +215,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 +544,21 @@ NSString* const kVersionKey = @"KSVersion";
 }
 
 - (void)registerWithKeystone {
-  [self updateStatus:kAutoupdateRegistering version:nil];
+  [self updateStatus:kAutoupdateRegistering version:nil error:nil];
+
+  if (!registration_) {

[Mark Mentovai, 2016/03/23 15:52:04]

This can happen before you enter the “registering” state.

[Boris Vidolov, 2016/03/23 17:17:24]

I don't see how this could happen: see +[KeystoneGlue defaultKeystoneGlue] and
loadKeystoneRegistration. If I read it correctly, registration_ is always set or
defaultKeystoneGlue returns nil, so this method won't be called. All usage of
KeystoneGlue is based on the instance returned by defaultKeystoneGlue method.

[Mark Mentovai, 2016/03/23 17:22:06]

Boris Vidolov wrote:
So then what’s the point of this check at all? Why don’t we get rid of it?

[Boris Vidolov, 2016/03/23 17:37:29]

I am fine with DCHECK for "registration_". Then it should be across the whole
file. Ryan's call.

[Ryan Myers (chromium), 2016/03/23 22:29:38]

Done.

+    [self updateStatus:kAutoupdateRegisterFailed
+               version:nil
+                 error:kNoRegistrationErrorMessage];
+    return;
+  }
 
   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 +579,22 @@ NSString* const kVersionKey = @"KSVersion";
 
 - (void)registrationComplete:(NSNotification*)notification {
   NSDictionary* userInfo = [notification userInfo];
+  NSString* errorMessages =
+      [userInfo objectForKey:ksr::KSRegistrationUpdateCheckRawErrorMessagesKey];

[Mark Mentovai, 2016/03/23 15:52:03]

When you pull things out of a dictionary, use ObjCCast<NSString> to make sure
that you’re really getting an NSString. Otherwise, possible type confusion
looms. ObjCCast will return nil if the thing isn’t an NSString.

You can also use ObjCCastStrict which will CHECK that it’s an NSString or nil,
and abort otherwise.

[Ryan Myers (chromium), 2016/03/23 22:29:38]

Done, both here and elsewhere in the file (i.e. in -brandFilePath where we pull
the bundle plist.)

+
   if ([[userInfo objectForKey:ksr::KSRegistrationStatusKey] boolValue]) {
     if ([self isSystemTicketDoomed]) {
-      [self updateStatus:kAutoupdateNeedsPromotion version:nil];
+      [self updateStatus:kAutoupdateNeedsPromotion
+                 version:nil
+                   error:errorMessages];
     } else {
-      [self updateStatus:kAutoupdateRegistered version:nil];
+      [self updateStatus:kAutoupdateRegistered version:nil error:errorMessages];
     }
   } else {
     // Dump registration_?
-    [self updateStatus:kAutoupdateRegisterFailed version:nil];
+    [self updateStatus:kAutoupdateRegisterFailed
+               version:nil
+                 error:errorMessages];
   }
 }
 
@@ -583,14 +607,20 @@ NSString* const kVersionKey = @"KSVersion";
 }
 
 - (void)checkForUpdate {
-  DCHECK(![self asyncOperationPending]);
+  if ([self asyncOperationPending]) {

[Mark Mentovai, 2016/03/23 15:52:04]

This can come after the registration_ check. Same on line 659.

[Ryan Myers (chromium), 2016/03/23 22:29:38]

Done.

+    // Update check already in process; return without doing anything.
+    VLOG(1) << "checkForUpdate ignored; async operation pending";

[Mark Mentovai, 2016/03/23 15:52:03]

I wasn’t a fan of the three VLOGs that this file already grew, and now things
are getting worse. Do we really need these? This is a form of bloat in my
opinion.

[Ryan Myers (chromium), 2016/03/23 22:29:38]

They're nice to have, but not necessary IMO.  At best, if we have a long-running
async operation in progress, this lets you know why other messages aren't having
an effect, without joining the debugger.  I don't feel strongly about it, so
removed.

+    return;
+  }
 
   if (!registration_) {
-    [self updateStatus:kAutoupdateCheckFailed version:nil];
+    [self updateStatus:kAutoupdateCheckFailed
+               version:nil
+                 error:kNoRegistrationErrorMessage];
     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.
@@ -604,15 +634,19 @@ NSString* const kVersionKey = @"KSVersion";
 
 - (void)checkForUpdateComplete:(NSNotification*)notification {
   NSDictionary* userInfo = [notification userInfo];
+  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:errorMessages];
   } 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,
@@ -622,14 +656,20 @@ NSString* const kVersionKey = @"KSVersion";
 }
 
 - (void)installUpdate {
-  DCHECK(![self asyncOperationPending]);
+  if ([self asyncOperationPending]) {
+    // Update check already in process; return without doing anything.
+    VLOG(1) << "installUpdate ignored; async operation pending";
+    return;
+  }
 
   if (!registration_) {
-    [self updateStatus:kAutoupdateInstallFailed version:nil];
+    [self updateStatus:kAutoupdateInstallFailed
+               version:nil
+                 error:kNoRegistrationErrorMessage];
     return;
   }
 
-  [self updateStatus:kAutoupdateInstalling version:nil];
+  [self updateStatus:kAutoupdateInstalling version:nil error:nil];
 
   [registration_ startUpdate];
 
@@ -639,6 +679,8 @@ NSString* const kVersionKey = @"KSVersion";
 
 - (void)installUpdateComplete:(NSNotification*)notification {
   NSDictionary* userInfo = [notification userInfo];
+  NSString* errorMsgs =

[Mark Mentovai, 2016/03/23 15:52:03]

Don’t abbreviate. Elsewhere, this was named errorMessages. It can have that name
here too.

[Ryan Myers (chromium), 2016/03/23 22:29:38]

Done.

+      [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
@@ -646,7 +688,7 @@ NSString* const kVersionKey = @"KSVersion";
   // KSUpdateCheckSuccessfullyInstalledKey is checked.
   if (![[userInfo objectForKey:ksr::KSUpdateCheckSuccessfullyInstalledKey]
           intValue]) {
-    [self updateStatus:kAutoupdateInstallFailed version:nil];
+    [self updateStatus:kAutoupdateInstallFailed version:nil error:errorMsgs];
   } else {
     updateSuccessfullyInstalled_ = YES;
 
@@ -709,10 +751,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 {

[Mark Mentovai, 2016/03/23 15:52:03]

Chromium style is NSString*, no space in there. Compare to the line above.

[Ryan Myers (chromium), 2016/03/23 22:29:38]

Done.

   NSNumber* statusNumber = [NSNumber numberWithInt:status];
   NSMutableDictionary* dictionary =
       [NSMutableDictionary dictionaryWithObject:statusNumber
@@ -720,6 +764,9 @@ NSString* const kVersionKey = @"KSVersion";
   if (version) {
     [dictionary setObject:version forKey:kAutoupdateStatusVersion];
   }
+  if (error) {
+    [dictionary setObject:version forKey:kAutoupdateStatusErrorMessages];
+  }
 
   NSNotification* notification =
       [NSNotification notificationWithName:kAutoupdateStatusNotification
@@ -838,6 +885,7 @@ NSString* const kVersionKey = @"KSVersion";
     // not lock each other out, it may be possible to arrive here while an
     // asynchronous operation is pending, or even after promotion has already
     // occurred.  Just quietly return without doing anything.
+    VLOG(1) << "promoteTicket ignored; async operation pending";
     return;
   }
 
@@ -862,6 +910,7 @@ NSString* const kVersionKey = @"KSVersion";
   if ([self asyncOperationPending]) {
     // Starting a synchronous operation while an asynchronous one is pending
     // could be trouble.
+    VLOG(1) << "promoteTicketWithAuth ignored; async operation pending";
     return;
   }
   if (!synchronous && ![self wantsPromotion]) {
@@ -873,7 +922,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 +971,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",

[Mark Mentovai, 2016/03/23 15:52:03]

This ought to be localized. Line 987 too.

[Ryan Myers (chromium), 2016/03/23 22:29:38]

Done.

+                                   (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 +1012,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 +1043,7 @@ NSString* const kVersionKey = @"KSVersion";
     }
   } else {
     authorization_.reset();
-    [self updateStatus:kAutoupdatePromoteFailed version:nil];
+    [self updateStatus:kAutoupdatePromoteFailed version:nil error:nil];
   }
 
   if (synchronousPromotion_) {
@@ -1036,7 +1099,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 {