mac: Prevent Address Book permissions dialog from erroneously appearing.

This CL fixes a bug where auto-updating Chrome can cause the Address Book
permissions dialog to appear, even though the user has already granted/denied
Chrome access to the user's Address Book.

When the Chrome binary is updated, chrome_autofill_client now passes the
information to personal_data_manager, which will refrain from accessing the
address book, if doing so would cause a blocking dialog to appear.

BUG=381763

Review URL: https://codereview.chromium.org/334653006

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@278254 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/ui/autofill/chrome_autofill_client.cc

@@ -37,6 +37,9 @@ namespace autofill {
 ChromeAutofillClient::ChromeAutofillClient(content::WebContents* web_contents)
     : content::WebContentsObserver(web_contents), web_contents_(web_contents) {
   DCHECK(web_contents);
+#if defined(OS_MACOSX) && !defined(OS_IOS)
+  RegisterForKeystoneNotifications();
+#endif  // defined(OS_MACOSX) && !defined(OS_IOS)
 }
 
 ChromeAutofillClient::~ChromeAutofillClient() {
@@ -45,6 +48,9 @@ ChromeAutofillClient::~ChromeAutofillClient() {
   // this point (in particular, the WebContentsImpl destructor has already
   // finished running and we are now in the base class destructor).
   DCHECK(!popup_controller_);
+#if defined(OS_MACOSX) && !defined(OS_IOS)
+  UnregisterFromKeystoneNotifications();
+#endif  // defined(OS_MACOSX) && !defined(OS_IOS)
 }
 
 void ChromeAutofillClient::TabActivated() {

chrome/browser/ui/autofill/chrome_autofill_client.h

@@ -22,6 +22,7 @@ class WebContents;
 namespace autofill {
 
 class AutofillDialogController;
+class AutofillKeystoneBridgeWrapper;
 class AutofillPopupControllerImpl;
 struct FormData;
 
@@ -81,6 +82,15 @@ class ChromeAutofillClient
   }
 
  private:
+#if defined(OS_MACOSX) && !defined(OS_IOS)
+  // Creates |bridge_wrapper_|, which is responsible for dealing with Keystone
+  // notifications.
+  void RegisterForKeystoneNotifications();
+
+  // Deletes |bridge_wrapper_|.
+  void UnregisterFromKeystoneNotifications();
+#endif  // defined(OS_MACOSX) && !defined(OS_IOS)
+
   explicit ChromeAutofillClient(content::WebContents* web_contents);
   friend class content::WebContentsUserData<ChromeAutofillClient>;
 
@@ -88,6 +98,17 @@ class ChromeAutofillClient
   base::WeakPtr<AutofillDialogController> dialog_controller_;
   base::WeakPtr<AutofillPopupControllerImpl> popup_controller_;
 
+#if defined(OS_MACOSX) && !defined(OS_IOS)
+  // Listens to Keystone notifications and passes relevant ones on to the
+  // PersonalDataManager.
+  //
+  // The class of this member must remain a forward declaration, even in the
+  // .cc implementation file, since the class is defined in a Mac-only
+  // implementation file. This means that the pointer cannot be wrapped in a
+  // scoped_ptr.
+  AutofillKeystoneBridgeWrapper* bridge_wrapper_;
+#endif  // defined(OS_MACOSX) && !defined(OS_IOS)
+
   DISALLOW_COPY_AND_ASSIGN(ChromeAutofillClient);
 };
 

chrome/browser/ui/autofill/chrome_autofill_client_mac.mm

+// Copyright 2014 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.
+
+#include "chrome/browser/ui/autofill/chrome_autofill_client.h"
+
+#import <Cocoa/Cocoa.h>
+
+#include "base/logging.h"
+#include "base/mac/scoped_nsobject.h"
+#import "chrome/browser/mac/keystone_glue.h"
+#include "components/autofill/core/browser/personal_data_manager.h"
+
+// Listens to Keystone notifications and passes relevant ones on to the
+// PersonalDataManager.
+@interface AutofillKeystoneBridge : NSObject {
+ @private
+  // The PersonalDataManager, if it exists, is expected to outlive this object.
+  autofill::PersonalDataManager* personal_data_manager_;
+}
+
+// Designated initializer. The PersonalDataManager, if it exists, is expected
+// to outlive this object. The PersonalDataManager may be NULL in tests.
+- (instancetype)initWithPersonalDataManager:
+    (autofill::PersonalDataManager*)personal_data_manager;
+
+// Receieved a notification from Keystone.
+- (void)handleStatusNotification:(NSNotification*)notification;
+@end
+
+@implementation AutofillKeystoneBridge
+
+- (instancetype)init {
+  NOTREACHED();
+  return nil;
+}
+
+- (instancetype)initWithPersonalDataManager:
+    (autofill::PersonalDataManager*)personal_data_manager {
+  self = [super init];
+  if (self) {
+    personal_data_manager_ = personal_data_manager;
+    NSNotificationCenter* center = [NSNotificationCenter defaultCenter];
+    [center addObserver:self
+               selector:@selector(handleStatusNotification:)
+                   name:kAutoupdateStatusNotification
+                 object:nil];
+  }
+  return self;
+}
+
+- (void)dealloc {
+  [[NSNotificationCenter defaultCenter] removeObserver:self];
+  [super dealloc];
+}
+
+- (void)handleStatusNotification:(NSNotification*)notification {
+  if (!personal_data_manager_)
+    return;
+
+  NSNumber* statusNumber =
+      [[notification userInfo] objectForKey:kAutoupdateStatusStatus];
+  DCHECK(statusNumber);
+  DCHECK([statusNumber isKindOfClass:[NSNumber class]]);
+  AutoupdateStatus status =
+      static_cast<AutoupdateStatus>([statusNumber intValue]);
+
+  switch (status) {
+    case kAutoupdateInstalling:
+    case kAutoupdateInstalled: {
+      personal_data_manager_->BinaryChanging();
+      return;
+    }
+    default:
+      return;
+  }
+}
+
+@end
+
+namespace autofill {
+
+class AutofillKeystoneBridgeWrapper {
+ public:
+  explicit AutofillKeystoneBridgeWrapper(
+      PersonalDataManager* personal_data_manager) {
+    bridge_.reset([[AutofillKeystoneBridge alloc]
+        initWithPersonalDataManager:personal_data_manager]);
+  }
+  ~AutofillKeystoneBridgeWrapper() {}
+
+ private:
+  base::scoped_nsobject<AutofillKeystoneBridge> bridge_;
+
+  DISALLOW_COPY_AND_ASSIGN(AutofillKeystoneBridgeWrapper);
+};
+
+void ChromeAutofillClient::RegisterForKeystoneNotifications() {
+  bridge_wrapper_ = new AutofillKeystoneBridgeWrapper(GetPersonalDataManager());
+}
+
+void ChromeAutofillClient::UnregisterFromKeystoneNotifications() {
+  delete bridge_wrapper_;
+}
+
+}  // namespace autofill

chrome/chrome_browser_ui.gypi

@@ -408,6 +408,7 @@
         'browser/ui/autofill/autofill_popup_view_delegate.h',
         'browser/ui/autofill/chrome_autofill_client.cc',
         'browser/ui/autofill/chrome_autofill_client.h',
+        'browser/ui/autofill/chrome_autofill_client_mac.mm',
         'browser/ui/autofill/country_combobox_model.cc',
         'browser/ui/autofill/country_combobox_model.h',
         'browser/ui/autofill/data_model_wrapper.cc',

components/autofill/core/browser/personal_data_manager.h

@@ -210,6 +210,11 @@ class PersonalDataManager : public KeyedService,
   // Whether an autofill suggestion should be displayed to prompt the user to
   // grant Chrome access to the user's address book.
   bool ShouldShowAccessAddressBookSuggestion(AutofillType type);
+
+  // The Chrome binary is in the process of being changed, or has been changed.
+  // Future attempts to access the Address Book might incorrectly present a
+  // blocking dialog.
+  void BinaryChanging();
 #endif  // defined(OS_MACOSX) && !defined(OS_IOS)
 
  protected:

components/autofill/core/browser/personal_data_manager_mac.mm

@@ -31,6 +31,27 @@
 namespace autofill {
 namespace {
 
+// There is an uncommon sequence of events that causes the Address Book
+// permissions dialog to appear more than once for a given install of Chrome.
+//  1. Chrome has previously presented the Address Book permissions dialog.
+//  2. Chrome is launched.
+//  3. Chrome performs an auto-update, and changes its binary.
+//  4. Chrome attempts to access the Address Book for the first time since (2).
+// This sequence of events is rare because Chrome attempts to acess the Address
+// Book when the user focuses most form fields, so (4) generally occurs before
+// (3). For more details, see http://crbug.com/381763.
+//
+// When this sequence of events does occur, Chrome should not attempt to access
+// the Address Book unless the user explicitly asks Chrome to do so. The
+// jarring nature of the permissions dialog is worse than the potential benefit
+// of pulling information from the Address Book.
+//
+// Set to true after the Address Book is accessed for the first time.
+static bool g_accessed_address_book = false;
+
+// Set to true after the Chrome binary has been changed.
+static bool g_binary_changed = false;
+
 const char kAddressBookOrigin[] = "OS X Address Book";
 
 // Whether Chrome has prompted the user for permission to access the user's
@@ -45,6 +66,13 @@ bool ShouldUseAddressBook(PrefService* pref_service) {
   return pref_service->GetBoolean(prefs::kAutofillUseMacAddressBook);
 }
 
+// Records a UMA metric indicating whether an attempt to access the Address
+// Book was skipped because doing so would cause the Address Book permissions
+// prompt to incorrectly appear.
+void RecordAccessSkipped(bool skipped) {
+  UMA_HISTOGRAM_BOOLEAN("Autofill.AddressBook.AccessSkipped", skipped);
+}
+
 ABAddressBook* GetAddressBook(PrefService* pref_service) {
   bool first_access = !HasPromptedForAccessToAddressBook(pref_service);
 
@@ -62,6 +90,7 @@ ABAddressBook* GetAddressBook(PrefService* pref_service) {
                           addressBook != nil);
   }
 
+  g_accessed_address_book = true;
   pref_service->SetBoolean(prefs::kAutofillMacAddressBookQueried, true);
   return addressBook;
 }
@@ -121,6 +150,14 @@ void AuxiliaryProfilesImpl::GetAddressBookMeCard(const std::string& app_locale,
   if (!ShouldUseAddressBook(pref_service))
     return;
 
+  // See the comment at the definition of g_accessed_address_book for an
+  // explanation of this logic.
+  if (g_binary_changed && !g_accessed_address_book) {
+    RecordAccessSkipped(true);
+    return;
+  }
+  RecordAccessSkipped(false);
+
   ABAddressBook* addressBook = GetAddressBook(pref_service);
 
   ABPerson* me = [addressBook me];
@@ -343,4 +380,8 @@ bool PersonalDataManager::ShouldShowAccessAddressBookSuggestion(
   return false;
 }
 
+void PersonalDataManager::BinaryChanging() {
+  g_binary_changed = true;
+}
+
 }  // namespace autofill

tools/metrics/histograms/histograms.xml

@@ -1574,6 +1574,16 @@ Therefore, the affected-histogram name has to have at least one dot in it.
   </summary>
 </histogram>
 
+<histogram name="Autofill.AddressBook.AccessSkipped" enum="BooleanSkipped">
+  <owner>erikchen@chromium.org</owner>
+  <summary>
+    Whether an attempt to access the Mac AddressBook was skipped because doing
+    so would incorrectly cause the appearance of the permissions dialog. This
+    happens when Chrome auto-update changes the binary on disk before the first
+    AddressBook access attempt.
+  </summary>
+</histogram>
+
 <histogram name="Autofill.AddressBookAvailable" enum="BooleanAvailable">
   <owner>isherman@chromium.org</owner>
   <summary>
@@ -33866,6 +33876,11 @@ Therefore, the affected-histogram name has to have at least one dot in it.
   <int value="1" label="Selected"/>
 </enum>
 
+<enum name="BooleanSkipped" type="int">
+  <int value="0" label="Not skipped"/>
+  <int value="1" label="Skipped"/>
+</enum>
+
 <enum name="BooleanStale" type="int">
   <int value="0" label="Fresh"/>
   <int value="1" label="Stale"/>