Fix Crashes in the New Autofill UI

Move the content::NotificationObserver from AutofillPopup to External Delegate (to avoid calls from popup to delegate if it out lives it).
Check for null pointers in PasswordAutofillManager::FindLoginInfo


BUG=158190,158371


Review URL: https://chromiumcodereview.appspot.com/11337022

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

6 files changed, 87 insertions, 72 deletions

diff --git a/chrome/browser/autofill/autofill_external_delegate.cc b/chrome/browser/autofill/autofill_external_delegate.cc
index 68bad52..e68ea33 100644
--- a/chrome/browser/autofill/autofill_external_delegate.cc
+++ b/chrome/browser/autofill/autofill_external_delegate.cc
@@ -8,6 +8,10 @@
 #include "chrome/browser/autofill/autofill_manager.h"
 #include "chrome/common/autofill_messages.h"
 #include "chrome/common/chrome_constants.h"
+#include "content/public/browser/navigation_controller.h"
+#include "content/public/browser/notification_service.h"
+#include "content/public/browser/notification_source.h"
+#include "content/public/browser/notification_types.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
 #include "grit/chromium_strings.h"
@@ -32,6 +36,16 @@ AutofillExternalDelegate::AutofillExternalDelegate(
       display_warning_if_disabled_(false),
       has_shown_autofill_popup_for_current_edit_(false),
       popup_visible_(false) {
+  registrar_.Add(this,
+                 content::NOTIFICATION_WEB_CONTENTS_VISIBILITY_CHANGED,
+                 content::Source<content::WebContents>(web_contents));
+  if (web_contents) {
+    registrar_.Add(
+        this,
+        content::NOTIFICATION_NAV_ENTRY_COMMITTED,
+        content::Source<content::NavigationController>(
+            &(web_contents->GetController())));
+  }
 }
 
 void AutofillExternalDelegate::SelectAutofillSuggestionAtIndex(int unique_id) {
@@ -221,6 +235,7 @@ void AutofillExternalDelegate::ClearPreviewedForm() {
 void AutofillExternalDelegate::HideAutofillPopup() {
   popup_visible_ = false;
 
+  ClearPreviewedForm();
   HideAutofillPopupInternal();
 }
 
@@ -346,6 +361,21 @@ void AutofillExternalDelegate::InsertDataListValues(
                               data_list_unique_ids_.end());
 }
 
+void AutofillExternalDelegate::Observe(
+    int type,
+    const content::NotificationSource& source,
+    const content::NotificationDetails& details) {
+  if (type == content::NOTIFICATION_WEB_CONTENTS_VISIBILITY_CHANGED) {
+    if (!*content::Details<bool>(details).ptr())
+      HideAutofillPopup();
+  } else if (type == content::NOTIFICATION_NAV_ENTRY_COMMITTED) {
+    HideAutofillPopup();
+  } else {
+    NOTREACHED();
+  }
+}
+
+
 
 #if defined(OS_MACOSX)
 
diff --git a/chrome/browser/autofill/autofill_external_delegate.h b/chrome/browser/autofill/autofill_external_delegate.h
index ca3efd4..e013095 100644
--- a/chrome/browser/autofill/autofill_external_delegate.h
+++ b/chrome/browser/autofill/autofill_external_delegate.h
@@ -13,6 +13,8 @@
 #include "chrome/common/form_data.h"
 #include "chrome/common/form_field_data.h"
 #include "chrome/common/password_form_fill_data.h"
+#include "content/public/browser/notification_registrar.h"
+#include "content/public/browser/notification_observer.h"
 #include "content/public/browser/web_contents_user_data.h"
 
 class AutofillManager;
@@ -32,7 +34,8 @@ class WebContents;
 // Delegate for external processing of Autocomplete and Autofill
 // display and selection.
 class AutofillExternalDelegate
-    : public content::WebContentsUserData<AutofillExternalDelegate> {
+    : public content::WebContentsUserData<AutofillExternalDelegate>,
+      public content::NotificationObserver {
  public:
   // Creates an AutofillExternalDelegate and attaches it to the specified
   // contents; the second argument is an AutofillManager managing Autofill for
@@ -164,6 +167,11 @@ class AutofillExternalDelegate
                             std::vector<string16>* autofill_icons,
                             std::vector<int>* autofill_unique_ids);
 
+  // content::NotificationObserver method override.
+  virtual void Observe(int type,
+                       const content::NotificationSource& source,
+                       const content::NotificationDetails& details) OVERRIDE;
+
   content::WebContents* web_contents_;  // weak; owns me.
   AutofillManager* autofill_manager_;  // weak.
 
@@ -174,6 +182,9 @@ class AutofillExternalDelegate
   // out of date responses.
   int autofill_query_id_;
 
+  // A scoped container for notification registries.
+  content::NotificationRegistrar registrar_;
+
   // The current form and field selected by Autofill.
   FormData autofill_query_form_;
   FormFieldData autofill_query_field_;
diff --git a/chrome/browser/autofill/autofill_popup_unittest.cc b/chrome/browser/autofill/autofill_popup_unittest.cc
index 4078577..9f4e233 100644
--- a/chrome/browser/autofill/autofill_popup_unittest.cc
+++ b/chrome/browser/autofill/autofill_popup_unittest.cc
@@ -25,6 +25,8 @@ class MockAutofillExternalDelegate : public TestAutofillExternalDelegate {
   virtual void RemoveAutocompleteEntry(const string16& value) OVERRIDE {}
   virtual void RemoveAutofillProfileOrCreditCard(int unique_id) OVERRIDE {}
   virtual void ClearPreviewedForm() OVERRIDE {}
+
+  MOCK_METHOD0(HideAutofillPopupInternal, void());
 };
 
 class TestAutofillPopupView : public AutofillPopupView {
@@ -54,8 +56,8 @@ class TestAutofillPopupView : public AutofillPopupView {
   }
 
   MOCK_METHOD1(InvalidateRow, void(size_t));
-  MOCK_METHOD0(HideInternal, void());
-  MOCK_METHOD0(UpdateBoundsAndRedrawPopup, void());
+  MOCK_METHOD0(Hide, void());
+  MOCK_METHOD0(UpdateBoundsAndRedrawPopupInternal, void());
 
  private:
   virtual void ShowInternal() OVERRIDE {}
@@ -70,9 +72,8 @@ class AutofillPopupViewUnitTest : public ::testing::Test {
   }
   virtual ~AutofillPopupViewUnitTest() {}
 
+protected:
   scoped_ptr<TestAutofillPopupView> autofill_popup_view_;
-
- private:
   MockAutofillExternalDelegate external_delegate_;
 };
 
@@ -80,7 +81,7 @@ TEST_F(AutofillPopupViewUnitTest, SetBounds) {
   // Ensure the popup size can be set and causes a redraw.
   gfx::Rect popup_bounds(10, 10, 100, 100);
 
-  EXPECT_CALL(*autofill_popup_view_, UpdateBoundsAndRedrawPopup());
+  EXPECT_CALL(*autofill_popup_view_, UpdateBoundsAndRedrawPopupInternal());
 
   autofill_popup_view_->SetElementBounds(popup_bounds);
 
@@ -160,13 +161,14 @@ TEST_F(AutofillPopupViewUnitTest, RemoveLine) {
 
   // Remove the first entry. The popup should be redrawn since its size has
   // changed.
-  EXPECT_CALL(*autofill_popup_view_, UpdateBoundsAndRedrawPopup());
+  EXPECT_CALL(*autofill_popup_view_, UpdateBoundsAndRedrawPopupInternal());
   autofill_popup_view_->SetSelectedLine(0);
   EXPECT_TRUE(autofill_popup_view_->RemoveSelectedLine());
 
   // Remove the last entry. The popup should then be hidden since there are
   // no Autofill entries left.
-  EXPECT_CALL(*autofill_popup_view_, HideInternal());
+  EXPECT_CALL(external_delegate_, HideAutofillPopupInternal());
+
   autofill_popup_view_->SetSelectedLine(0);
   EXPECT_TRUE(autofill_popup_view_->RemoveSelectedLine());
 }
diff --git a/chrome/browser/autofill/autofill_popup_view.cc b/chrome/browser/autofill/autofill_popup_view.cc
index 476be3c..b2efc6e 100644
--- a/chrome/browser/autofill/autofill_popup_view.cc
+++ b/chrome/browser/autofill/autofill_popup_view.cc
@@ -8,10 +8,6 @@
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/autofill/autofill_external_delegate.h"
 #include "content/public/browser/web_contents.h"
-#include "content/public/browser/navigation_controller.h"
-#include "content/public/browser/notification_service.h"
-#include "content/public/browser/notification_source.h"
-#include "content/public/browser/notification_types.h"
 #include "grit/webkit_resources.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebAutofillClient.h"
 
@@ -72,26 +68,11 @@ AutofillPopupView::AutofillPopupView(
   if (!web_contents)
     return;
 
-  registrar_.Add(this,
-                 content::NOTIFICATION_WEB_CONTENTS_VISIBILITY_CHANGED,
-                 content::Source<content::WebContents>(web_contents));
-  registrar_.Add(
-      this,
-      content::NOTIFICATION_NAV_ENTRY_COMMITTED,
-      content::Source<content::NavigationController>(
-          &(web_contents->GetController())));
-
   label_font_ = value_font_.DeriveFont(kLabelFontSizeDelta);
 }
 
 AutofillPopupView::~AutofillPopupView() {}
 
-void AutofillPopupView::Hide() {
-  HideInternal();
-
-  external_delegate_->ClearPreviewedForm();
-}
-
 void AutofillPopupView::Show(const std::vector<string16>& autofill_values,
                              const std::vector<string16>& autofill_labels,
                              const std::vector<string16>& autofill_icons,
@@ -114,14 +95,19 @@ void AutofillPopupView::Show(const std::vector<string16>& autofill_values,
 
 void AutofillPopupView::SetElementBounds(const gfx::Rect& bounds) {
   element_bounds_ = bounds;
-  UpdateBoundsAndRedrawPopup();
+  UpdateBoundsAndRedrawPopupInternal();
+}
+
+void AutofillPopupView::ClearExternalDelegate() {
+  external_delegate_ = NULL;
+
 }
 
-void AutofillPopupView::UpdatePopupBounds() {
+void AutofillPopupView::UpdateBoundsAndRedrawPopup() {
   element_bounds_.set_width(GetPopupRequiredWidth());
   element_bounds_.set_height(GetPopupRequiredHeight());
 
-  UpdateBoundsAndRedrawPopup();
+  UpdateBoundsAndRedrawPopupInternal();
 }
 
 void AutofillPopupView::SetSelectedPosition(int x, int y) {
@@ -239,12 +225,12 @@ bool AutofillPopupView::RemoveSelectedLine() {
 
   SetSelectedLine(kNoSelection);
 
-  external_delegate_->ClearPreviewedForm();
-
-  if (HasAutofillEntries())
-    UpdatePopupBounds();
-  else
-    Hide();
+  if (HasAutofillEntries()) {
+    external_delegate_->ClearPreviewedForm();
+    UpdateBoundsAndRedrawPopup();
+  } else {
+    external_delegate_->HideAutofillPopup();
+  }
 
   return true;
 }
@@ -369,13 +355,3 @@ bool AutofillPopupView::HasAutofillEntries() {
        autofill_unique_ids_[0] == WebAutofillClient::MenuItemIDDataListEntry);
 }
 
-void AutofillPopupView::Observe(int type,
-                                const content::NotificationSource& source,
-                                const content::NotificationDetails& details) {
-  if (type == content::NOTIFICATION_WEB_CONTENTS_VISIBILITY_CHANGED) {
-    if (!*content::Details<bool>(details).ptr())
-      Hide();
-  } else if (type == content::NOTIFICATION_NAV_ENTRY_COMMITTED) {
-    Hide();
-  }
-}
diff --git a/chrome/browser/autofill/autofill_popup_view.h b/chrome/browser/autofill/autofill_popup_view.h
index 4617af9..e6c1e67 100644
--- a/chrome/browser/autofill/autofill_popup_view.h
+++ b/chrome/browser/autofill/autofill_popup_view.h
@@ -9,8 +9,6 @@
 
 #include "base/compiler_specific.h"
 #include "base/string16.h"
-#include "content/public/browser/notification_registrar.h"
-#include "content/public/browser/notification_observer.h"
 #include "ui/gfx/font.h"
 #include "ui/gfx/rect.h"
 
@@ -20,14 +18,14 @@ class WebContents;
 
 class AutofillExternalDelegate;
 
-class AutofillPopupView : public content::NotificationObserver {
+class AutofillPopupView {
  public:
   explicit AutofillPopupView(content::WebContents* web_contents,
                              AutofillExternalDelegate* external_delegate_);
   virtual ~AutofillPopupView();
 
   // Hide the popup from view. Platform-indepent work.
-  virtual void Hide();
+  virtual void Hide() = 0;
 
   // Display the autofill popup and fill it in with the values passed in.
   // Platform-independent work.
@@ -39,6 +37,10 @@ class AutofillPopupView : public content::NotificationObserver {
   // Update the bounds of the popup element.
   void SetElementBounds(const gfx::Rect& bounds);
 
+  // Sets the current Autofill pointer to NULL, used when the popup can outlive
+  // the delegate.
+  void ClearExternalDelegate();
+
   const gfx::Rect& element_bounds() { return element_bounds_; }
 
  protected:
@@ -46,14 +48,11 @@ class AutofillPopupView : public content::NotificationObserver {
   // Platform-dependent work.
   virtual void ShowInternal() = 0;
 
-  // Hide the popup from view. Platform-dependent work.
-  virtual void HideInternal() = 0;
-
   // Invalidate the given row and redraw it.
   virtual void InvalidateRow(size_t row) = 0;
 
   // Ensure the popup is properly placed at |element_bounds_|.
-  virtual void UpdateBoundsAndRedrawPopup() = 0;
+  virtual void UpdateBoundsAndRedrawPopupInternal() = 0;
 
   AutofillExternalDelegate* external_delegate() { return external_delegate_; }
 
@@ -76,8 +75,8 @@ class AutofillPopupView : public content::NotificationObserver {
   int selected_line() const { return selected_line_; }
   bool delete_icon_selected() const { return delete_icon_selected_; }
 
-  // Recalculate the height and width of the popup.
-  void UpdatePopupBounds();
+  // Recalculate the height and width of the popup and trigger a redraw.
+  void UpdateBoundsAndRedrawPopup();
 
   // Change which line is selected by the user, based on coordinates.
   void SetSelectedPosition(int x, int y);
@@ -158,14 +157,6 @@ class AutofillPopupView : public content::NotificationObserver {
   // Returns true if the popup still has non-options entries to show the user.
   bool HasAutofillEntries();
 
-  // content::NotificationObserver method override.
-  virtual void Observe(int type,
-                       const content::NotificationSource& source,
-                       const content::NotificationDetails& details) OVERRIDE;
-
-  // A scoped container for notification registries.
-  content::NotificationRegistrar registrar_;
-
   AutofillExternalDelegate* external_delegate_;
 
   // The bounds of the text element that is the focus of the Autofill.
diff --git a/chrome/browser/autofill/autofill_popup_view_browsertest.cc b/chrome/browser/autofill/autofill_popup_view_browsertest.cc
index 33f7bfe..cde0505 100644
--- a/chrome/browser/autofill/autofill_popup_view_browsertest.cc
+++ b/chrome/browser/autofill/autofill_popup_view_browsertest.cc
@@ -27,11 +27,14 @@ namespace {
 
 class MockAutofillExternalDelegate : public TestAutofillExternalDelegate {
  public:
-  MockAutofillExternalDelegate() : TestAutofillExternalDelegate(NULL, NULL) {}
+  MockAutofillExternalDelegate(content::WebContents* web_contents) :
+      TestAutofillExternalDelegate(web_contents, NULL) {}
   ~MockAutofillExternalDelegate() {}
 
   virtual void SelectAutofillSuggestionAtIndex(int unique_id)
       OVERRIDE {}
+
+  MOCK_METHOD0(HideAutofillPopupInternal, void());
 };
 
 class TestAutofillPopupView : public AutofillPopupView {
@@ -47,11 +50,9 @@ class TestAutofillPopupView : public AutofillPopupView {
  protected:
   virtual void ShowInternal() OVERRIDE {}
 
-  virtual void HideInternal() OVERRIDE {}
-
   virtual void InvalidateRow(size_t row) OVERRIDE {}
 
-  virtual void UpdateBoundsAndRedrawPopup() OVERRIDE {}
+  virtual void UpdateBoundsAndRedrawPopupInternal() OVERRIDE {}
 };
 
 }  // namespace
@@ -65,20 +66,23 @@ class AutofillPopupViewBrowserTest : public InProcessBrowserTest {
     web_contents_ = chrome::GetActiveWebContents(browser());
     ASSERT_TRUE(web_contents_ != NULL);
 
+    autofill_external_delegate_.reset(new MockAutofillExternalDelegate(
+        web_contents_));
     autofill_popup_view_.reset(new TestAutofillPopupView(
         web_contents_,
-        &autofill_external_delegate_));
+        autofill_external_delegate_.get()));
   }
 
  protected:
   content::WebContents* web_contents_;
   scoped_ptr<TestAutofillPopupView> autofill_popup_view_;
-  MockAutofillExternalDelegate autofill_external_delegate_;
+  scoped_ptr<MockAutofillExternalDelegate> autofill_external_delegate_;
 };
 
 IN_PROC_BROWSER_TEST_F(AutofillPopupViewBrowserTest,
                        SwitchTabAndHideAutofillPopup) {
-  EXPECT_CALL(*autofill_popup_view_, Hide()).Times(AtLeast(1));
+  EXPECT_CALL(*autofill_external_delegate_,
+              HideAutofillPopupInternal()).Times(AtLeast(1));
 
   content::WindowedNotificationObserver observer(
       content::NOTIFICATION_WEB_CONTENTS_VISIBILITY_CHANGED,
@@ -92,7 +96,8 @@ IN_PROC_BROWSER_TEST_F(AutofillPopupViewBrowserTest,
 
 IN_PROC_BROWSER_TEST_F(AutofillPopupViewBrowserTest,
                        TestPageNavigationHidingAutofillPopup) {
-  EXPECT_CALL(*autofill_popup_view_, Hide()).Times(AtLeast(1));
+  EXPECT_CALL(*autofill_external_delegate_,
+              HideAutofillPopupInternal()).Times(AtLeast(1));
 
   content::WindowedNotificationObserver observer(
       content::NOTIFICATION_NAV_ENTRY_COMMITTED,