[Autofill] Fix use-after-free

It is possible for the delegate to get deleted before the
popup controller. Make the popup controller use a weakptr
to reference the delegate to handle this case.

BUG=232475

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

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

14 files changed, 82 insertions, 45 deletions

diff --git a/chrome/browser/autofill/autofill_external_delegate_browsertest.cc b/chrome/browser/autofill/autofill_external_delegate_browsertest.cc
index 8684570..d438a47 100644
--- a/chrome/browser/autofill/autofill_external_delegate_browsertest.cc
+++ b/chrome/browser/autofill/autofill_external_delegate_browsertest.cc
@@ -45,7 +45,7 @@ class MockAutofillManagerDelegate
                     const std::vector<string16>& labels,
                     const std::vector<string16>& icons,
                     const std::vector<int>& identifiers,
-                    AutofillPopupDelegate* delegate));
+                    base::WeakPtr<AutofillPopupDelegate> delegate));
 
   MOCK_METHOD0(HideAutofillPopup, void());
 
diff --git a/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc b/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
index 86ad08c..a9bbd01 100644
--- a/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
+++ b/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
@@ -1077,7 +1077,10 @@ void AutofillDialogControllerImpl::UserEditedOrActivatedInput(
   }
 
   popup_controller_ = AutofillPopupControllerImpl::GetOrCreate(
-      popup_controller_, this, parent_view, content_bounds);
+      popup_controller_,
+      weak_ptr_factory_.GetWeakPtr(),
+      parent_view,
+      content_bounds);
   popup_controller_->Show(popup_values,
                           popup_labels,
                           popup_icons,
diff --git a/chrome/browser/ui/autofill/autofill_popup_controller_browsertest.cc b/chrome/browser/ui/autofill/autofill_popup_controller_browsertest.cc
index 91080f7..28bae2d 100644
--- a/chrome/browser/ui/autofill/autofill_popup_controller_browsertest.cc
+++ b/chrome/browser/ui/autofill/autofill_popup_controller_browsertest.cc
@@ -4,7 +4,6 @@
 
 #include "base/basictypes.h"
 #include "base/memory/scoped_ptr.h"
-#include "chrome/browser/ui/autofill/autofill_popup_controller_impl.h"
 #include "chrome/browser/ui/autofill/autofill_popup_view.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_window.h"
@@ -114,4 +113,16 @@ IN_PROC_BROWSER_TEST_F(AutofillPopupControllerBrowserTest,
 }
 #endif // !defined(OS_MACOSX)
 
+// This test checks that the browser doesn't crash if the delegate is deleted
+// before the popup is hidden.
+IN_PROC_BROWSER_TEST_F(AutofillPopupControllerBrowserTest,
+                       DeleteDelegateBeforePopupHidden){
+  GenerateTestAutofillPopup(autofill_external_delegate_.get());
+
+  // Delete the external delegate here so that is gets deleted before popup is
+  // hidden. This can happen if the web_contents are destroyed before the popup
+  // is hidden. See http://crbug.com/232475
+  autofill_external_delegate_.reset();
+}
+
 }  // namespace autofill
diff --git a/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc b/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc
index 9814ec4..f58d37b 100644
--- a/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc
+++ b/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc
@@ -70,7 +70,7 @@ const DataResource kDataResources[] = {
 // static
 WeakPtr<AutofillPopupControllerImpl> AutofillPopupControllerImpl::GetOrCreate(
     WeakPtr<AutofillPopupControllerImpl> previous,
-    AutofillPopupDelegate* delegate,
+    WeakPtr<AutofillPopupDelegate> delegate,
     gfx::NativeView container_view,
     const gfx::RectF& element_bounds) {
   DCHECK(!previous || previous->delegate_ == delegate);
@@ -90,7 +90,7 @@ WeakPtr<AutofillPopupControllerImpl> AutofillPopupControllerImpl::GetOrCreate(
 }
 
 AutofillPopupControllerImpl::AutofillPopupControllerImpl(
-    AutofillPopupDelegate* delegate,
+    base::WeakPtr<AutofillPopupDelegate> delegate,
     gfx::NativeView container_view,
     const gfx::RectF& element_bounds)
     : view_(NULL),
@@ -174,7 +174,8 @@ void AutofillPopupControllerImpl::Show(
 void AutofillPopupControllerImpl::Hide() {
   SetSelectedLine(kNoSelection);
 
-  delegate_->OnPopupHidden(this);
+  if (delegate_)
+    delegate_->OnPopupHidden(this);
 
   if (view_)
     view_->Hide();
diff --git a/chrome/browser/ui/autofill/autofill_popup_controller_impl.h b/chrome/browser/ui/autofill/autofill_popup_controller_impl.h
index fc58505..e4e4052 100644
--- a/chrome/browser/ui/autofill/autofill_popup_controller_impl.h
+++ b/chrome/browser/ui/autofill/autofill_popup_controller_impl.h
@@ -38,7 +38,7 @@ class AutofillPopupControllerImpl : public AutofillPopupController,
   // this call.
   static base::WeakPtr<AutofillPopupControllerImpl> GetOrCreate(
       base::WeakPtr<AutofillPopupControllerImpl> previous,
-      AutofillPopupDelegate* delegate,
+      base::WeakPtr<AutofillPopupDelegate> delegate,
       gfx::NativeView container_view,
       const gfx::RectF& element_bounds);
 
@@ -63,7 +63,7 @@ class AutofillPopupControllerImpl : public AutofillPopupController,
   FRIEND_TEST_ALL_PREFIXES(AutofillExternalDelegateBrowserTest,
                            CloseWidgetAndNoLeaking);
 
-  AutofillPopupControllerImpl(AutofillPopupDelegate* delegate,
+  AutofillPopupControllerImpl(base::WeakPtr<AutofillPopupDelegate> delegate,
                               gfx::NativeView container_view,
                               const gfx::RectF& element_bounds);
   virtual ~AutofillPopupControllerImpl();
@@ -168,7 +168,7 @@ class AutofillPopupControllerImpl : public AutofillPopupController,
       int popup_required_height) const;
 
   AutofillPopupView* view_;  // Weak reference.
-  AutofillPopupDelegate* delegate_;  // Weak reference.
+  base::WeakPtr<AutofillPopupDelegate> delegate_;
   gfx::NativeView container_view_;  // Weak reference.
 
   // The bounds of the text element that is the focus of the Autofill.
diff --git a/chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc b/chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc
index 6d69417..4dcc7d3 100644
--- a/chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc
+++ b/chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc
@@ -40,6 +40,10 @@ class MockAutofillExternalDelegate : public AutofillExternalDelegate {
   virtual void RemoveSuggestion(const string16& value, int identifier) OVERRIDE
       {}
   virtual void ClearPreviewedForm() OVERRIDE {}
+
+  base::WeakPtr<AutofillExternalDelegate> GetWeakPtr() {
+    return AutofillExternalDelegate::GetWeakPtr();
+  }
 };
 
 class MockAutofillManagerDelegate
@@ -59,7 +63,7 @@ class MockAutofillManagerDelegate
 class TestAutofillPopupController : public AutofillPopupControllerImpl {
  public:
   explicit TestAutofillPopupController(
-      AutofillExternalDelegate* external_delegate,
+      base::WeakPtr<AutofillExternalDelegate> external_delegate,
       const gfx::RectF& element_bounds)
       : AutofillPopupControllerImpl(external_delegate, NULL, element_bounds) {}
   virtual ~TestAutofillPopupController() {}
@@ -155,7 +159,7 @@ class AutofillPopupControllerUnitTest : public ChromeRenderViewHostTestHarness {
 
     autofill_popup_controller_ =
         new testing::NiceMock<TestAutofillPopupController>(
-            external_delegate_.get(), gfx::Rect());
+            external_delegate_->GetWeakPtr(), gfx::Rect());
   }
 
   virtual void TearDown() OVERRIDE {
@@ -328,23 +332,27 @@ TEST_F(AutofillPopupControllerUnitTest, GetOrCreate) {
 
   WeakPtr<AutofillPopupControllerImpl> controller =
       AutofillPopupControllerImpl::GetOrCreate(
-          WeakPtr<AutofillPopupControllerImpl>(), &delegate, NULL, gfx::Rect());
+          WeakPtr<AutofillPopupControllerImpl>(), delegate.GetWeakPtr(), NULL,
+          gfx::Rect());
   EXPECT_TRUE(controller);
 
   controller->Hide();
 
   controller = AutofillPopupControllerImpl::GetOrCreate(
-      WeakPtr<AutofillPopupControllerImpl>(), &delegate, NULL, gfx::Rect());
+      WeakPtr<AutofillPopupControllerImpl>(), delegate.GetWeakPtr(), NULL,
+      gfx::Rect());
   EXPECT_TRUE(controller);
 
   WeakPtr<AutofillPopupControllerImpl> controller2 =
-      AutofillPopupControllerImpl::GetOrCreate(controller, &delegate, NULL,
+      AutofillPopupControllerImpl::GetOrCreate(controller,
+                                               delegate.GetWeakPtr(),
+                                               NULL,
                                                gfx::Rect());
   EXPECT_EQ(controller.get(), controller2.get());
   controller->Hide();
 
   testing::NiceMock<TestAutofillPopupController>* test_controller =
-      new testing::NiceMock<TestAutofillPopupController>(&delegate,
+      new testing::NiceMock<TestAutofillPopupController>(delegate.GetWeakPtr(),
                                                          gfx::Rect());
   EXPECT_CALL(*test_controller, Hide());
 
@@ -352,7 +360,7 @@ TEST_F(AutofillPopupControllerUnitTest, GetOrCreate) {
   AutofillPopupControllerImpl* controller3 =
       AutofillPopupControllerImpl::GetOrCreate(
           test_controller->GetWeakPtr(),
-          &delegate,
+          delegate.GetWeakPtr(),
           NULL,
           bounds);
   EXPECT_EQ(
@@ -458,7 +466,8 @@ TEST_F(AutofillPopupControllerUnitTest, GrowPopupInSpace) {
     NiceMock<MockAutofillExternalDelegate> external_delegate(
         web_contents(), AutofillManager::FromWebContents(web_contents()));
     TestAutofillPopupController* autofill_popup_controller =
-        new TestAutofillPopupController(&external_delegate, element_bounds[i]);
+        new TestAutofillPopupController(external_delegate.GetWeakPtr(),
+                                        element_bounds[i]);
 
     autofill_popup_controller->set_display(display);
     autofill_popup_controller->Show(names, names, names, autofill_ids);
diff --git a/chrome/browser/ui/autofill/tab_autofill_manager_delegate.cc b/chrome/browser/ui/autofill/tab_autofill_manager_delegate.cc
index 3e8b8e4..d7098f3 100644
--- a/chrome/browser/ui/autofill/tab_autofill_manager_delegate.cc
+++ b/chrome/browser/ui/autofill/tab_autofill_manager_delegate.cc
@@ -187,7 +187,7 @@ void TabAutofillManagerDelegate::ShowAutofillPopup(
     const std::vector<string16>& labels,
     const std::vector<string16>& icons,
     const std::vector<int>& identifiers,
-    AutofillPopupDelegate* delegate) {
+    base::WeakPtr<AutofillPopupDelegate> delegate) {
   // Convert element_bounds to be in screen space.
   gfx::Rect client_area;
   web_contents_->GetView()->GetContainerBounds(&client_area);
diff --git a/chrome/browser/ui/autofill/tab_autofill_manager_delegate.h b/chrome/browser/ui/autofill/tab_autofill_manager_delegate.h
index dffd617..cab01cb 100644
--- a/chrome/browser/ui/autofill/tab_autofill_manager_delegate.h
+++ b/chrome/browser/ui/autofill/tab_autofill_manager_delegate.h
@@ -66,12 +66,13 @@ class TabAutofillManagerDelegate
       DialogType dialog_type,
       const base::Callback<void(const FormStructure*,
                                 const std::string&)>& callback) OVERRIDE;
-  virtual void ShowAutofillPopup(const gfx::RectF& element_bounds,
-                                 const std::vector<string16>& values,
-                                 const std::vector<string16>& labels,
-                                 const std::vector<string16>& icons,
-                                 const std::vector<int>& identifiers,
-                                 AutofillPopupDelegate* delegate) OVERRIDE;
+  virtual void ShowAutofillPopup(
+      const gfx::RectF& element_bounds,
+      const std::vector<string16>& values,
+      const std::vector<string16>& labels,
+      const std::vector<string16>& icons,
+      const std::vector<int>& identifiers,
+      base::WeakPtr<AutofillPopupDelegate> delegate) OVERRIDE;
   virtual void HideAutofillPopup() OVERRIDE;
   virtual void UpdateProgressBar(double value) OVERRIDE;
 
diff --git a/components/autofill/browser/autofill_external_delegate.cc b/components/autofill/browser/autofill_external_delegate.cc
index 9985d31..7034a89 100644
--- a/components/autofill/browser/autofill_external_delegate.cc
+++ b/components/autofill/browser/autofill_external_delegate.cc
@@ -50,7 +50,8 @@ AutofillExternalDelegate::AutofillExternalDelegate(
       display_warning_if_disabled_(false),
       has_autofill_suggestion_(false),
       has_shown_autofill_popup_for_current_edit_(false),
-      registered_keyboard_listener_with_(NULL) {
+      registered_keyboard_listener_with_(NULL),
+      weak_ptr_factory_(this) {
   DCHECK(autofill_manager);
 
   registrar_.Add(this,
@@ -134,7 +135,7 @@ void AutofillExternalDelegate::OnSuggestionsReturned(
   // Send to display.
   if (autofill_query_field_.is_focusable) {
     autofill_manager_->delegate()->ShowAutofillPopup(
-        element_bounds_, values, labels, icons, ids, this);
+        element_bounds_, values, labels, icons, ids, GetWeakPtr());
   }
 }
 
@@ -154,7 +155,7 @@ void AutofillExternalDelegate::OnShowPasswordSuggestions(
   std::vector<int> password_ids(suggestions.size(),
                                 WebAutofillClient::MenuItemIDPasswordEntry);
   autofill_manager_->delegate()->ShowAutofillPopup(
-      element_bounds_, suggestions, empty, empty, password_ids, this);
+      element_bounds_, suggestions, empty, empty, password_ids, GetWeakPtr());
 }
 
 void AutofillExternalDelegate::SetCurrentDataListValues(
@@ -258,6 +259,10 @@ void AutofillExternalDelegate::AddPasswordFormMapping(
   password_autofill_manager_.AddPasswordFormMapping(form, fill_data);
 }
 
+base::WeakPtr<AutofillExternalDelegate> AutofillExternalDelegate::GetWeakPtr() {
+  return weak_ptr_factory_.GetWeakPtr();
+}
+
 void AutofillExternalDelegate::FillAutofillFormData(int unique_id,
                                                     bool is_preview) {
   // If the selected element is a warning we don't want to do anything.
diff --git a/components/autofill/browser/autofill_external_delegate.h b/components/autofill/browser/autofill_external_delegate.h
index 57a4b8e..5ea7b37 100644
--- a/components/autofill/browser/autofill_external_delegate.h
+++ b/components/autofill/browser/autofill_external_delegate.h
@@ -113,6 +113,8 @@ class AutofillExternalDelegate
 
   content::WebContents* web_contents() { return web_contents_; }
 
+  base::WeakPtr<AutofillExternalDelegate> GetWeakPtr();
+
  private:
   // Fills the form with the Autofill data corresponding to |unique_id|.
   // If |is_preview| is true then this is just a preview to show the user what
@@ -187,6 +189,8 @@ class AutofillExternalDelegate
   std::vector<base::string16> data_list_icons_;
   std::vector<int> data_list_unique_ids_;
 
+  base::WeakPtrFactory<AutofillExternalDelegate> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(AutofillExternalDelegate);
 };
 
diff --git a/components/autofill/browser/autofill_external_delegate_unittest.cc b/components/autofill/browser/autofill_external_delegate_unittest.cc
index c0d40e9..848a8a0 100644
--- a/components/autofill/browser/autofill_external_delegate_unittest.cc
+++ b/components/autofill/browser/autofill_external_delegate_unittest.cc
@@ -58,7 +58,7 @@ class MockAutofillManagerDelegate
                     const std::vector<base::string16>& labels,
                     const std::vector<base::string16>& icons,
                     const std::vector<int>& identifiers,
-                    AutofillPopupDelegate* delegate));
+                    base::WeakPtr<AutofillPopupDelegate> delegate));
 
   MOCK_METHOD0(HideAutofillPopup, void());
 
@@ -151,7 +151,7 @@ TEST_F(AutofillExternalDelegateUnitTest, TestExternalDelegateVirtualCalls) {
                       static_cast<int>(WebAutofillClient::MenuItemIDSeparator),
                       static_cast<int>(
                           WebAutofillClient::MenuItemIDAutofillOptions)),
-                  external_delegate_.get()));
+                  _));
 
   // This should call ShowAutofillPopup.
   std::vector<base::string16> autofill_item;
@@ -200,7 +200,7 @@ TEST_F(AutofillExternalDelegateUnitTest, ExternalDelegateDataList) {
                       static_cast<int>(WebAutofillClient::MenuItemIDSeparator),
                       static_cast<int>(
                           WebAutofillClient::MenuItemIDAutofillOptions)),
-                      external_delegate_.get()));
+                  _));
 
   // This should call ShowAutofillPopup.
   std::vector<base::string16> autofill_item;
@@ -222,7 +222,7 @@ TEST_F(AutofillExternalDelegateUnitTest, ExternalDelegateDataList) {
                   testing::ElementsAre(
                       static_cast<int>(
                           WebAutofillClient::MenuItemIDDataListEntry)),
-                  external_delegate_.get()));
+                  _));
 
   autofill_item = std::vector<base::string16>();
   autofill_ids = std::vector<int>();
@@ -291,7 +291,7 @@ TEST_F(AutofillExternalDelegateUnitTest, ExternalDelegatePasswordSuggestions) {
                   testing::ElementsAre(
                       static_cast<int>(
                            WebAutofillClient::MenuItemIDPasswordEntry)),
-                  external_delegate_.get()));
+                  _));
 
   external_delegate_->OnShowPasswordSuggestions(suggestions,
                                                 field,
diff --git a/components/autofill/browser/autofill_manager_delegate.h b/components/autofill/browser/autofill_manager_delegate.h
index abcb421..a58025d 100644
--- a/components/autofill/browser/autofill_manager_delegate.h
+++ b/components/autofill/browser/autofill_manager_delegate.h
@@ -8,6 +8,7 @@
 #include <vector>
 
 #include "base/callback_forward.h"
+#include "base/memory/weak_ptr.h"
 #include "base/string16.h"
 
 namespace content {
@@ -128,12 +129,13 @@ class AutofillManagerDelegate {
   // Shows an Autofill popup with the given |values|, |labels|, |icons|, and
   // |identifiers| for the element at |element_bounds|. |delegate| will be
   // notified of popup events.
-  virtual void ShowAutofillPopup(const gfx::RectF& element_bounds,
-                                 const std::vector<base::string16>& values,
-                                 const std::vector<base::string16>& labels,
-                                 const std::vector<base::string16>& icons,
-                                 const std::vector<int>& identifiers,
-                                 AutofillPopupDelegate* delegate) = 0;
+  virtual void ShowAutofillPopup(
+      const gfx::RectF& element_bounds,
+      const std::vector<base::string16>& values,
+      const std::vector<base::string16>& labels,
+      const std::vector<base::string16>& icons,
+      const std::vector<int>& identifiers,
+      base::WeakPtr<AutofillPopupDelegate> delegate) = 0;
 
   // Hide the Autofill popup if one is currently showing.
   virtual void HideAutofillPopup() = 0;
diff --git a/components/autofill/browser/test_autofill_manager_delegate.cc b/components/autofill/browser/test_autofill_manager_delegate.cc
index 6b6f911..6992107 100644
--- a/components/autofill/browser/test_autofill_manager_delegate.cc
+++ b/components/autofill/browser/test_autofill_manager_delegate.cc
@@ -68,7 +68,7 @@ void TestAutofillManagerDelegate::ShowAutofillPopup(
     const std::vector<base::string16>& labels,
     const std::vector<base::string16>& icons,
     const std::vector<int>& identifiers,
-    AutofillPopupDelegate* delegate) {}
+    base::WeakPtr<AutofillPopupDelegate> delegate) {}
 
 void TestAutofillManagerDelegate::HideAutofillPopup() {}
 
diff --git a/components/autofill/browser/test_autofill_manager_delegate.h b/components/autofill/browser/test_autofill_manager_delegate.h
index f88c7cc..03ad783 100644
--- a/components/autofill/browser/test_autofill_manager_delegate.h
+++ b/components/autofill/browser/test_autofill_manager_delegate.h
@@ -47,12 +47,13 @@ class TestAutofillManagerDelegate : public AutofillManagerDelegate {
       DialogType dialog_type,
       const base::Callback<void(const FormStructure*,
                                 const std::string&)>& callback) OVERRIDE;
-  virtual void ShowAutofillPopup(const gfx::RectF& element_bounds,
-                                 const std::vector<base::string16>& values,
-                                 const std::vector<base::string16>& labels,
-                                 const std::vector<base::string16>& icons,
-                                 const std::vector<int>& identifiers,
-                                 AutofillPopupDelegate* delegate) OVERRIDE;
+  virtual void ShowAutofillPopup(
+      const gfx::RectF& element_bounds,
+      const std::vector<base::string16>& values,
+      const std::vector<base::string16>& labels,
+      const std::vector<base::string16>& icons,
+      const std::vector<int>& identifiers,
+      base::WeakPtr<AutofillPopupDelegate> delegate) OVERRIDE;
   virtual void HideAutofillPopup() OVERRIDE;
   virtual void UpdateProgressBar(double value) OVERRIDE;