Fixe use after free during destruction

Problem scenario happened because destroying a tab was triggering
updating another tab. When the other tab updated various BrowserWindow
methods were called when BrowserView was in a bad state (part way
through destruction).

In theory if detaching tried to update state we would end up in the
same situation as this fixes, but that seems less likely.

BUG=236389
TEST=covered by test now
R=avi@chromium.org

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

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

chrome/browser/ui/views/frame/browser_view.cc

@@ -459,11 +459,17 @@ BrowserView::~BrowserView() {
   // Child views maintain PrefMember attributes that point to
   // OffTheRecordProfile's PrefService which gets deleted by ~Browser.
   RemoveAllChildViews(true);
+  toolbar_ = NULL;
 
   // It is possible that we were forced-closed by the native view system and
-  // that tabs remain in the browser. Close any such remaining tabs.
-  while (browser_->tab_strip_model()->count())
-    delete browser_->tab_strip_model()->GetWebContentsAt(0);
+  // that tabs remain in the browser. Close any such remaining tabs. Detach
+  // before destroying in hopes of avoiding less callbacks trying to access
+  // members since destroyed.
+  {
+    ScopedVector<content::WebContents> contents;
+    while (browser_->tab_strip_model()->count())
+      contents.push_back(browser_->tab_strip_model()->DetachWebContentsAt(0));
+  }
 
   // Explicitly set browser_ to NULL.
   browser_.reset();
@@ -963,7 +969,9 @@ void BrowserView::UpdateReloadStopState(bool is_loading, bool force) {
 }
 
 void BrowserView::UpdateToolbar(content::WebContents* contents) {
-  toolbar_->Update(contents);
+  // We may end up here during destruction.
+  if (toolbar_)
+    toolbar_->Update(contents);
 }
 
 void BrowserView::FocusToolbar() {

chrome/browser/ui/views/frame/browser_view_browsertest.cc

+// Copyright 2013 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/views/frame/browser_view.h"
+
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_tabstrip.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "content/public/browser/invalidate_type.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_observer.h"
+
+typedef InProcessBrowserTest BrowserViewTest;
+
+namespace {
+
+// Used to simulate scenario in a crash. When WebContentsDestroyed() is invoked
+// updates the navigation state of another tab.
+class TestWebContentsObserver : public content::WebContentsObserver {
+ public:
+  TestWebContentsObserver(content::WebContents* source,
+                          content::WebContents* other)
+      : content::WebContentsObserver(source),
+        other_(other) {}
+  virtual ~TestWebContentsObserver() {}
+
+  virtual void WebContentsDestroyed(
+      content::WebContents* web_contents) OVERRIDE {
+    other_->NotifyNavigationStateChanged(
+        content::INVALIDATE_TYPE_URL | content::INVALIDATE_TYPE_LOAD);
+  }
+
+ private:
+  content::WebContents* other_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestWebContentsObserver);
+};
+
+}  // namespace
+
+// Verifies don't crash when CloseNow() is invoked with two tabs in a browser.
+// Additionally when one of the tabs is destroyed NotifyNavigationStateChanged()
+// is invoked on the other.
+IN_PROC_BROWSER_TEST_F(BrowserViewTest, CloseWithTabs) {
+  Browser* browser2 =
+      new Browser(Browser::CreateParams(browser()->profile(),
+                                        browser()->host_desktop_type()));
+  chrome::AddBlankTabAt(browser2, -1, true);
+  chrome::AddBlankTabAt(browser2, -1, true);
+  TestWebContentsObserver observer(
+      browser2->tab_strip_model()->GetWebContentsAt(0),
+      browser2->tab_strip_model()->GetWebContentsAt(1));
+  BrowserView::GetBrowserViewForBrowser(browser2)->GetWidget()->CloseNow();
+}

chrome/chrome_tests.gypi

@@ -1639,6 +1639,7 @@
         'browser/ui/views/browser_actions_container_browsertest.cc',
         'browser/ui/views/frame/app_non_client_frame_view_ash_browsertest.cc',
         'browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc',
+        'browser/ui/views/frame/browser_view_browsertest.cc',
         'browser/ui/views/frame/immersive_mode_controller_ash_browsertest.cc',
         'browser/ui/views/location_bar/zoom_bubble_view_browsertest.cc',
         'browser/ui/views/select_file_dialog_extension_browsertest.cc',