Fix memory smash, take 2.

Fix freed-memory accesses of members of a destroyed object while unwinding the callstack.  This avoids introducing a different memory corruption issue at shutdown by using WeakPtr to hold the object that the OS can delete out from under us.  It also clarifies and simplifies the shutdown sequence by moving the AutocompleteController::Stop() call into the controller's destructor, and avoiding notifying observers of changes; this actually altogether sidesteps the problematic codepath for which I needed the WeakPtr, but I'm not sure whether the OS still might be able to destroy the object at another time, so I'm leaving the WeakPtr in.

BUG=41274
TEST=Run Chrome with full page heap on.  Clicking an item in the omnibox dropdown should not crash.
Review URL: http://codereview.chromium.org/1657002

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

8 files changed, 46 insertions, 21 deletions

diff --git a/chrome/browser/autocomplete/autocomplete.cc b/chrome/browser/autocomplete/autocomplete.cc
index 1278a9b..ba37681 100644
--- a/chrome/browser/autocomplete/autocomplete.cc
+++ b/chrome/browser/autocomplete/autocomplete.cc
@@ -729,6 +729,15 @@ AutocompleteController::AutocompleteController(Profile* profile)
 }
 
 AutocompleteController::~AutocompleteController() {
+  // The providers may have tasks outstanding that hold refs to them.  We need
+  // to ensure they won't call us back if they outlive us.  (Practically,
+  // calling Stop() should also cancel those tasks and make it so that we hold
+  // the only refs.)  We also don't want to bother notifying anyone of our
+  // result changes here, because the notification observer is in the midst of
+  // shutdown too, so we don't ask Stop() to clear |result_| (and notify).
+  result_.Reset();  // Not really necessary.
+  Stop(false);
+
   for (ACProviders::iterator i(providers_.begin()); i != providers_.end(); ++i)
     (*i)->Release();
 
diff --git a/chrome/browser/autocomplete/autocomplete_popup_model.cc b/chrome/browser/autocomplete/autocomplete_popup_model.cc
index f769937..5939b64 100644
--- a/chrome/browser/autocomplete/autocomplete_popup_model.cc
+++ b/chrome/browser/autocomplete/autocomplete_popup_model.cc
@@ -31,7 +31,6 @@ AutocompletePopupModel::AutocompletePopupModel(
 }
 
 AutocompletePopupModel::~AutocompletePopupModel() {
-  StopAutocomplete();
 }
 
 void AutocompletePopupModel::SetProfile(Profile* profile) {
diff --git a/chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc b/chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc
index 7f974a8..8a571c9 100644
--- a/chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc
+++ b/chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc
@@ -517,6 +517,12 @@ AutocompletePopupContentsView::AutocompletePopupContentsView(
   set_border(bubble_border);
 }
 
+AutocompletePopupContentsView::~AutocompletePopupContentsView() {
+  // We don't need to do anything with |popup_| here.  The OS either has already
+  // closed the window, in which case it's been deleted, or it will soon, in
+  // which case there's nothing we need to do.
+}
+
 gfx::Rect AutocompletePopupContentsView::GetPopupBounds() const {
   if (!size_animation_.IsAnimating())
     return target_bounds_;
@@ -549,7 +555,11 @@ void AutocompletePopupContentsView::UpdatePopupAppearance() {
     // No matches, close any existing popup.
     if (popup_ != NULL) {
       size_animation_.Stop();
-      popup_->CloseNow();
+      // NOTE: Do NOT use CloseNow() here, as we may be deep in a callstack
+      // triggered by the popup receiving a message (e.g. LBUTTONUP), and
+      // destroying the popup would cause us to read garbage when we unwind back
+      // to that level.
+      popup_->Close();  // This will eventually delete the popup.
       popup_.reset();
     }
     return;
@@ -590,7 +600,7 @@ void AutocompletePopupContentsView::UpdatePopupAppearance() {
 
   if (popup_ == NULL) {
     // If the popup is currently closed, we need to create it.
-    popup_.reset(new AutocompletePopupClass(edit_view_, this));
+    popup_ = (new AutocompletePopupClass(edit_view_, this))->AsWeakPtr();
   } else {
     // Animate the popup shrinking, but don't animate growing larger since that
     // would make the popup feel less responsive.
diff --git a/chrome/browser/views/autocomplete/autocomplete_popup_contents_view.h b/chrome/browser/views/autocomplete/autocomplete_popup_contents_view.h
index 31237c4..d814206 100644
--- a/chrome/browser/views/autocomplete/autocomplete_popup_contents_view.h
+++ b/chrome/browser/views/autocomplete/autocomplete_popup_contents_view.h
@@ -46,7 +46,7 @@ class AutocompletePopupContentsView : public views::View,
                                 AutocompleteEditModel* edit_model,
                                 Profile* profile,
                                 const views::View* location_bar);
-  virtual ~AutocompletePopupContentsView() {}
+  virtual ~AutocompletePopupContentsView();
 
   // Returns the bounds the popup should be shown at. This is the display bounds
   // and includes offsets for the dropshadow which this view's border renders.
@@ -113,8 +113,11 @@ class AutocompletePopupContentsView : public views::View,
   // match at the specified point.
   size_t GetIndexForPoint(const gfx::Point& point);
 
-  // The popup that contains this view.
-  scoped_ptr<AutocompletePopupClass> popup_;
+  // The popup that contains this view.  We create this, but it deletes itself
+  // when its window is destroyed.  This is a WeakPtr because it's possible for
+  // the OS to destroy the window and thus delete this object before we're
+  // deleted, or without our knowledge.
+  base::WeakPtr<AutocompletePopupClass> popup_;
 
   // The provider of our result set.
   scoped_ptr<AutocompletePopupModel> model_;
diff --git a/chrome/browser/views/autocomplete/autocomplete_popup_gtk.cc b/chrome/browser/views/autocomplete/autocomplete_popup_gtk.cc
index b0bf07b..4bb7f09 100644
--- a/chrome/browser/views/autocomplete/autocomplete_popup_gtk.cc
+++ b/chrome/browser/views/autocomplete/autocomplete_popup_gtk.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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.
 
@@ -17,8 +17,7 @@ AutocompletePopupGtk::AutocompletePopupGtk(
     AutocompleteEditView* edit_view,
     AutocompletePopupContentsView* contents)
     : WidgetGtk(WidgetGtk::TYPE_POPUP) {
-  // Create the popup.  // Owned by |contents|.
-  set_delete_on_destroy(false);
+  // Create the popup.
   MakeTransparent();
   WidgetGtk::Init(gtk_widget_get_parent(edit_view->GetNativeView()),
                   contents->GetPopupBounds());
diff --git a/chrome/browser/views/autocomplete/autocomplete_popup_gtk.h b/chrome/browser/views/autocomplete/autocomplete_popup_gtk.h
index 5c9ee46..1117311 100644
--- a/chrome/browser/views/autocomplete/autocomplete_popup_gtk.h
+++ b/chrome/browser/views/autocomplete/autocomplete_popup_gtk.h
@@ -1,16 +1,19 @@
-// Copyright (c) 2009 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.
+// Copyright (c) 2010 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.
 
 #ifndef CHROME_BROWSER_VIEWS_AUTOCOMPLETE_AUTOCOMPLETE_POPUP_GTK_H_
 #define CHROME_BROWSER_VIEWS_AUTOCOMPLETE_AUTOCOMPLETE_POPUP_GTK_H_
 
+#include "base/weak_ptr.h"
 #include "views/widget/widget_gtk.h"
 
 class AutocompleteEditView;
 class AutocompletePopupContentsView;
 
-class AutocompletePopupGtk : public views::WidgetGtk {
+class AutocompletePopupGtk
+    : public views::WidgetGtk,
+      public base::SupportsWeakPtr<AutocompletePopupGtk> {
  public:
   // Creates the popup and shows it. |edit_view| is the edit that created us.
   AutocompletePopupGtk(AutocompleteEditView* edit_view,
diff --git a/chrome/browser/views/autocomplete/autocomplete_popup_win.cc b/chrome/browser/views/autocomplete/autocomplete_popup_win.cc
index 35231fe..4cb7ddc 100644
--- a/chrome/browser/views/autocomplete/autocomplete_popup_win.cc
+++ b/chrome/browser/views/autocomplete/autocomplete_popup_win.cc
@@ -1,6 +1,6 @@
-// Copyright (c) 2009 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.
+// Copyright (c) 2010 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/views/autocomplete/autocomplete_popup_win.h"
 
@@ -17,7 +17,6 @@ AutocompletePopupWin::AutocompletePopupWin(
     AutocompleteEditView* edit_view,
     AutocompletePopupContentsView* contents) {
   // Create the popup.
-  set_delete_on_destroy(false);  // Owned by |contents|.
   set_window_style(WS_POPUP | WS_CLIPCHILDREN);
   set_window_ex_style(WS_EX_TOOLWINDOW | WS_EX_LAYERED);
   WidgetWin::Init(GetAncestor(edit_view->GetNativeView(), GA_ROOT),
diff --git a/chrome/browser/views/autocomplete/autocomplete_popup_win.h b/chrome/browser/views/autocomplete/autocomplete_popup_win.h
index cb9e838..affe0a9 100644
--- a/chrome/browser/views/autocomplete/autocomplete_popup_win.h
+++ b/chrome/browser/views/autocomplete/autocomplete_popup_win.h
@@ -1,16 +1,19 @@
-// Copyright (c) 2009 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.
+// Copyright (c) 2010 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.
 
 #ifndef CHROME_BROWSER_VIEWS_AUTOCOMPLETE_AUTOCOMPLETE_POPUP_WIN_H_
 #define CHROME_BROWSER_VIEWS_AUTOCOMPLETE_AUTOCOMPLETE_POPUP_WIN_H_
 
+#include "base/weak_ptr.h"
 #include "views/widget/widget_win.h"
 
 class AutocompleteEditView;
 class AutocompletePopupContentsView;
 
-class AutocompletePopupWin : public views::WidgetWin {
+class AutocompletePopupWin
+    : public views::WidgetWin,
+      public base::SupportsWeakPtr<AutocompletePopupWin> {
  public:
   // Creates the popup and shows it. |edit_view| is the edit that created us.
   AutocompletePopupWin(AutocompleteEditView* edit_view,