Fixes bug in bookmark bar view if profile changed. If the profile

changed we didn't remove the old buttons, which meant when you clicked
on a button it might be past the bounds of the model. I don't think we
ever do this, but it's the only thing I could see that could cause the
crash.

Once I've nailed this down I'll nuke the CHECKs.

BUG=44642
TEST=none

Review URL: http://codereview.chromium.org/2822010

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

3 files changed, 79 insertions, 22 deletions

diff --git a/chrome/browser/views/bookmark_bar_view.cc b/chrome/browser/views/bookmark_bar_view.cc
index 9ca4ec5..1723f9f 100644
--- a/chrome/browser/views/bookmark_bar_view.cc
+++ b/chrome/browser/views/bookmark_bar_view.cc
@@ -444,13 +444,16 @@ void BookmarkBarView::SetProfile(Profile* profile) {
   registrar_.Add(this, NotificationType::BOOKMARK_BUBBLE_SHOWN, ns_source);
   registrar_.Add(this, NotificationType::BOOKMARK_BUBBLE_HIDDEN, ns_source);
   registrar_.Add(this, NotificationType::BOOKMARK_BAR_VISIBILITY_PREF_CHANGED,
-                  NotificationService::AllSources());
+                 NotificationService::AllSources());
+
+  // Remove any existing bookmark buttons.
+  while (GetBookmarkButtonCount())
+    delete GetChildViewAt(0);
 
   model_ = profile_->GetBookmarkModel();
   model_->AddObserver(this);
   if (model_->IsLoaded())
     Loaded(model_);
-
   // else case: we'll receive notification back from the BookmarkModel when done
   // loading, then we'll populate the bar.
 }
@@ -948,14 +951,11 @@ MenuButton* BookmarkBarView::CreateOverflowButton() {
   return button;
 }
 
-int BookmarkBarView::GetBookmarkButtonCount() {
-  // We contain at least four non-bookmark button views: other bookmarks,
-  // bookmarks separator, chevrons (for overflow), the instruction label and
-  // the sync error button.
-  return GetChildViewCount() - 5;
-}
-
 void BookmarkBarView::Loaded(BookmarkModel* model) {
+  volatile int button_count = GetBookmarkButtonCount();
+  CHECK(button_count == 0);  // If non-zero it means Load was invoked more than
+                             // once, or we didn't properly clear things. Either
+                             // of which shouldn't happen
   const BookmarkNode* node = model_->GetBookmarkBarNode();
   DCHECK(node && model_->other_node());
   // Create a button for each of the children on the bookmark bar.
@@ -966,6 +966,8 @@ void BookmarkBarView::Loaded(BookmarkModel* model) {
 
   Layout();
   SchedulePaint();
+
+  CheckIntegrity();
 }
 
 void BookmarkBarView::BookmarkModelBeingDeleted(BookmarkModel* model) {
@@ -1557,6 +1559,13 @@ void BookmarkBarView::StartThrobbing(const BookmarkNode* node,
     throbbing_view_->StartThrobbing(std::numeric_limits<int>::max());
 }
 
+int BookmarkBarView::GetBookmarkButtonCount() {
+  // We contain at least four non-bookmark button views: other bookmarks,
+  // bookmarks separator, chevrons (for overflow), the instruction label and
+  // the sync error button.
+  return GetChildViewCount() - 5;
+}
+
 void BookmarkBarView::StopThrobbing(bool immediate) {
   if (!throbbing_view_)
     return;
diff --git a/chrome/browser/views/bookmark_bar_view.h b/chrome/browser/views/bookmark_bar_view.h
index 1aa8d1b..bb532fb 100644
--- a/chrome/browser/views/bookmark_bar_view.h
+++ b/chrome/browser/views/bookmark_bar_view.h
@@ -50,6 +50,14 @@ class BookmarkBarView : public DetachableToolbarView,
   friend class ShowFolderMenuTask;
 
  public:
+  // Constants used in Browser View, as well as here.
+  // How inset the bookmarks bar is when displayed on the new tab page.
+  static const int kNewtabHorizontalPadding;
+  static const int kNewtabVerticalPadding;
+
+  // Maximum size of buttons on the bookmark bar.
+  static const int kMaxButtonWidth;
+
   // Interface implemented by controllers/views that need to be notified any
   // time the model changes, typically to cancel an operation that is showing
   // data from the model such as a menu. This isn't intended as a general
@@ -213,22 +221,19 @@ class BookmarkBarView : public DetachableToolbarView,
   // BookmarkBarInstructionsView::Delegate.
   virtual void ShowImportDialog();
 
-  // Maximum size of buttons on the bookmark bar.
-  static const int kMaxButtonWidth;
-
   // If a button is currently throbbing, it is stopped. If immediate is true
   // the throb stops immediately, otherwise it stops after a couple more
   // throbs.
   void StopThrobbing(bool immediate);
 
+  // Returns the number of buttons corresponding to starred urls/groups. This
+  // is equivalent to the number of children the bookmark bar node from the
+  // bookmark bar model has.
+  int GetBookmarkButtonCount();
+
   // If true we're running tests. This short circuits a couple of animations.
   static bool testing_;
 
-  // Constants used in Browser View, as well as here.
-  // How inset the bookmarks bar is when displayed on the new tab page.
-  static const int kNewtabHorizontalPadding;
-  static const int kNewtabVerticalPadding;
-
  private:
   class ButtonSeparatorView;
   struct DropInfo;
@@ -272,11 +277,6 @@ class BookmarkBarView : public DetachableToolbarView,
   // Creates the button used when not all bookmark buttons fit.
   views::MenuButton* CreateOverflowButton();
 
-  // Returns the number of buttons corresponding to starred urls/groups. This
-  // is equivalent to the number of children the bookmark bar node from the
-  // bookmark bar model has.
-  int GetBookmarkButtonCount();
-
   // Invoked when the bookmark bar model has finished loading. Creates a button
   // for each of the children of the root node from the model.
   virtual void Loaded(BookmarkModel* model);
diff --git a/chrome/browser/views/bookmark_bar_view_unittest.cc b/chrome/browser/views/bookmark_bar_view_unittest.cc
new file mode 100644
index 0000000..61249b6
--- /dev/null
+++ b/chrome/browser/views/bookmark_bar_view_unittest.cc
@@ -0,0 +1,48 @@
+// 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/bookmarks/bookmark_model.h"
+#include "chrome/browser/chrome_thread.h"
+#include "chrome/browser/views/bookmark_bar_view.h"
+#include "chrome/test/browser_with_test_window_test.h"
+#include "chrome/test/testing_profile.h"
+
+class BookmarkBarViewTest : public BrowserWithTestWindowTest {
+ public:
+  BookmarkBarViewTest()
+      : ui_thread_(ChromeThread::UI, message_loop()),
+        file_thread_(ChromeThread::FILE, message_loop()) {}
+
+ private:
+  ChromeThread ui_thread_;
+  ChromeThread file_thread_;
+
+  DISALLOW_COPY_AND_ASSIGN(BookmarkBarViewTest);
+};
+
+TEST_F(BookmarkBarViewTest, SwitchProfile) {
+  profile()->CreateBookmarkModel(true);
+  profile()->BlockUntilBookmarkModelLoaded();
+
+  profile()->GetBookmarkModel()->AddURL(
+      profile()->GetBookmarkModel()->GetBookmarkBarNode(),
+      0,
+      L"blah",
+      GURL("http://www.google.com"));
+
+  BookmarkBarView bookmark_bar(profile(), browser());
+
+  EXPECT_EQ(1, bookmark_bar.GetBookmarkButtonCount());
+
+  TestingProfile profile2(0);
+  profile2.CreateBookmarkModel(true);
+  profile2.BlockUntilBookmarkModelLoaded();
+
+  bookmark_bar.SetProfile(&profile2);
+
+  EXPECT_EQ(0, bookmark_bar.GetBookmarkButtonCount());
+
+  bookmark_bar.SetProfile(profile());
+  EXPECT_EQ(1, bookmark_bar.GetBookmarkButtonCount());
+}