From 7f2855f8e27cca78479481d80a06618568c8d10a Mon Sep 17 00:00:00 2001 From: "jcampan@chromium.org" Date: Tue, 25 Nov 2008 22:27:41 +0000 Subject: Some crashes reported from the field seems to indicate that when storing/restoring the focused view on a tab, the focus manager or focused view is garbage. I have not been able to repro any of these crashers and was not able to discover much from the mini-dumps. This is a simple work-around that just prevents the storing/restoring of focus when closing a tab, to avoid the cases reported in the crasher. Note this is the equivalent of CL 12622 (which is on the release branck), which was reviewed by Ben. BUG=4633 Review URL: http://codereview.chromium.org/12652 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@6000 0039d316-1c4b-4281-b951-d872f2087c98 --- chrome/browser/tab_contents.cc | 6 +++++- chrome/browser/tab_contents.h | 8 ++++++++ chrome/browser/views/frame/browser_view.cc | 9 +++++++-- 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/chrome/browser/tab_contents.cc b/chrome/browser/tab_contents.cc index 9bdca3c..ebd8f2b 100644 --- a/chrome/browser/tab_contents.cc +++ b/chrome/browser/tab_contents.cc @@ -46,7 +46,8 @@ TabContents::TabContents(TabContentsType type) shelf_visible_(false), max_page_id_(-1), blocked_popups_(NULL), - capturing_contents_(false) { + capturing_contents_(false), + is_being_destroyed_(false) { last_focused_view_storage_id_ = views::ViewStorage::GetSharedInstance()->CreateStorageID(); } @@ -75,6 +76,9 @@ void TabContents::CloseContents() { } void TabContents::Destroy() { + DCHECK(!is_being_destroyed_); + is_being_destroyed_ = true; + // First cleanly close all child windows. // TODO(mpcomplete): handle case if MaybeCloseChildWindows() already asked // some of these to close. CloseWindows is async, so it might get called diff --git a/chrome/browser/tab_contents.h b/chrome/browser/tab_contents.h index 2f78a79..dad5f63 100644 --- a/chrome/browser/tab_contents.h +++ b/chrome/browser/tab_contents.h @@ -246,6 +246,11 @@ class TabContents : public PageNavigator, bool is_active() const { return is_active_; } void set_is_active(bool active) { is_active_ = active; } + // Whether the tab is in the process of being destroyed. + // Added as a tentative work-around for focus related bug #4633. This allows + // us not to store focus when a tab is being closed. + bool is_being_destroyed() const { return is_being_destroyed_; } + // Convenience method for notifying the delegate of a navigation state // change. See TabContentsDelegate. void NotifyNavigationStateChanged(unsigned changed_flags); @@ -542,6 +547,9 @@ class TabContents : public PageNavigator, // Delegates for InfoBars associated with this TabContents. std::vector infobar_delegates_; + // See getter above. + bool is_being_destroyed_; + DISALLOW_COPY_AND_ASSIGN(TabContents); }; diff --git a/chrome/browser/views/frame/browser_view.cc b/chrome/browser/views/frame/browser_view.cc index 91b148c..019fdc8 100644 --- a/chrome/browser/views/frame/browser_view.cc +++ b/chrome/browser/views/frame/browser_view.cc @@ -647,7 +647,10 @@ void BrowserView::TabSelectedAt(TabContents* old_contents, bool user_gesture) { DCHECK(old_contents != new_contents); - if (old_contents) + // We do not store the focus when closing the tab to work-around bug 4633. + // Some reports seem to show that the focus manager and/or focused view can + // be garbage at that point, it is not clear why. + if (old_contents && !old_contents->is_being_destroyed()) old_contents->StoreFocus(); // Update various elements that are interested in knowing the current @@ -659,8 +662,10 @@ void BrowserView::TabSelectedAt(TabContents* old_contents, // required to make features like Duplicate Tab, Undo Close Tab, // etc not result in sad tab. new_contents->DidBecomeSelected(); - if (BrowserList::GetLastActive() == browser_) + if (BrowserList::GetLastActive() == browser_ && + !browser_->tabstrip_model()->closing_all()) { new_contents->RestoreFocus(); + } // Update all the UI bits. UpdateTitleBar(); -- cgit v1.1