Attempt at fixing crash. I believe this is happening during session

end, which has a different shutdown path and is possible for the model
to be deleted before the view.

I'm also adding checking to XPFrame/VistaFrame to make sure the
BookmarkBarView doesn't have a parent in the destructor. This
shouldn't be possible, but I'm adding the check to be sure;)

BUG=1359852
TEST=make sure you don't see problems with bookmarks

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

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

6 files changed, 44 insertions, 2 deletions

diff --git a/chrome/browser/bookmark_bar_model.cc b/chrome/browser/bookmark_bar_model.cc
index 25612eb..07e2acd 100644
--- a/chrome/browser/bookmark_bar_model.cc
+++ b/chrome/browser/bookmark_bar_model.cc
@@ -104,6 +104,9 @@ BookmarkBarModel::~BookmarkBarModel() {
         this, NOTIFY_HISTORY_LOADED, Source<Profile>(profile_));
   }
 
+  FOR_EACH_OBSERVER(BookmarkBarModelObserver, observers_,
+                    BookmarkModelBeingDeleted(this));
+
   if (store_) {
     // The store maintains a reference back to us. We need to tell it we're gone
     // so that it doesn't try and invoke a method back on us again.
diff --git a/chrome/browser/bookmark_bar_model.h b/chrome/browser/bookmark_bar_model.h
index a15bb67..0bcbdf0 100644
--- a/chrome/browser/bookmark_bar_model.h
+++ b/chrome/browser/bookmark_bar_model.h
@@ -128,6 +128,9 @@ class BookmarkBarModelObserver {
   // Invoked when the model has finished loading.
   virtual void Loaded(BookmarkBarModel* model) = 0;
 
+  // Invoked from the destructor of the BookmarkBarModel.
+  virtual void BookmarkModelBeingDeleted(BookmarkBarModel* model) { }
+
   // Invoked when a node has moved.
   virtual void BookmarkNodeMoved(BookmarkBarModel* model,
                                  BookmarkBarNode* old_parent,
diff --git a/chrome/browser/views/bookmark_bar_view.cc b/chrome/browser/views/bookmark_bar_view.cc
index b5a952f..fb9d579 100644
--- a/chrome/browser/views/bookmark_bar_view.cc
+++ b/chrome/browser/views/bookmark_bar_view.cc
@@ -1118,7 +1118,6 @@ void BookmarkBarView::Init() {
   SetContextMenuController(this);
 
   size_animation_.reset(new SlideAnimation(this));
-  size_animation_->SetSlideDuration(4000);
 }
 
 MenuButton* BookmarkBarView::CreateOtherBookmarkedButton() {
@@ -1166,6 +1165,17 @@ void BookmarkBarView::Loaded(BookmarkBarModel* model) {
   SchedulePaint();
 }
 
+void BookmarkBarView::BookmarkModelBeingDeleted(BookmarkBarModel* model) {
+  // The bookmark model should never be deleted before us. This code exists
+  // to check for regressions in shutdown code and not crash.
+  NOTREACHED();
+
+  // Do minimal cleanup, presumably we'll be deleted shortly.
+  NotifyModelChanged();
+  model_->RemoveObserver(this);
+  model_ = NULL;
+}
+
 void BookmarkBarView::BookmarkNodeMoved(BookmarkBarModel* model,
                                         BookmarkBarNode* old_parent,
                                         int old_index,
diff --git a/chrome/browser/views/bookmark_bar_view.h b/chrome/browser/views/bookmark_bar_view.h
index 507da5d..d48780c 100644
--- a/chrome/browser/views/bookmark_bar_view.h
+++ b/chrome/browser/views/bookmark_bar_view.h
@@ -216,6 +216,9 @@ class BookmarkBarView : public ChromeViews::View,
   // for each of the children of the root node from the model.
   virtual void Loaded(BookmarkBarModel* model);
 
+  // Invoked when the model is being deleted.
+  virtual void BookmarkModelBeingDeleted(BookmarkBarModel* model);
+
   // Invokes added followed by removed.
   virtual void BookmarkNodeMoved(BookmarkBarModel* model,
                                  BookmarkBarNode* old_parent,
diff --git a/chrome/browser/vista_frame.cc b/chrome/browser/vista_frame.cc
index 64bf4f8..b1b0ebb3 100644
--- a/chrome/browser/vista_frame.cc
+++ b/chrome/browser/vista_frame.cc
@@ -1576,6 +1576,18 @@ void VistaFrame::DestroyBrowser() {
   // the tabstrip from the model's observer list because the model was
   // destroyed with browser_.
   if (browser_) {
+    if (bookmark_bar_view_.get() && bookmark_bar_view_->GetParent()) {
+      // The bookmark bar should not be parented by the time we get here.
+      // If you hit this NOTREACHED file a bug with the trace.
+      NOTREACHED();
+      bookmark_bar_view_->GetParent()->RemoveChildView(bookmark_bar_view_.get());
+    }
+
+    // Explicitly delete the BookmarkBarView now. That way we don't have to
+    // worry about the BookmarkBarView potentially outliving the Browser &
+    // Profile.
+    bookmark_bar_view_.reset(NULL);
+
     browser_->tabstrip_model()->RemoveObserver(tabstrip_);
     delete browser_;
     browser_ = NULL;
@@ -1610,4 +1622,3 @@ void VistaFrame::ShelfVisibilityChangedImpl(TabContents* current_tab) {
   if (changed && current_tab)
     Layout();
 }
-
diff --git a/chrome/browser/xp_frame.cc b/chrome/browser/xp_frame.cc
index 872acac..fee785b 100644
--- a/chrome/browser/xp_frame.cc
+++ b/chrome/browser/xp_frame.cc
@@ -2462,6 +2462,18 @@ void XPFrame::DestroyBrowser() {
   // the tabstrip from the model's observer list because the model was
   // destroyed with browser_.
   if (browser_) {
+    if (bookmark_bar_view_.get() && bookmark_bar_view_->GetParent()) {
+      // The bookmark bar should not be parented by the time we get here.
+      // If you hit this NOTREACHED file a bug with the trace.
+      NOTREACHED();
+      bookmark_bar_view_->GetParent()->RemoveChildView(bookmark_bar_view_.get());
+    }
+
+    // Explicitly delete the BookmarkBarView now. That way we don't have to
+    // worry about the BookmarkBarView potentially outliving the Browser &
+    // Profile.
+    bookmark_bar_view_.reset(NULL);
+
     browser_->tabstrip_model()->RemoveObserver(tabstrip_);
     delete browser_;
     browser_ = NULL;