Bug fix: Translate: leak of a ComboboxModel

chrome/browser/ui/views/translate/translate_bubble_view.cc

Index: chrome/browser/ui/views/translate/translate_bubble_view.cc
diff --git a/chrome/browser/ui/views/translate/translate_bubble_view.cc b/chrome/browser/ui/views/translate/translate_bubble_view.cc
index b72959369e3761ce6b56130fe1514920109f715d..2cf26ceb1071737a424b841b9a9efb8a345d3f35 100644
--- a/chrome/browser/ui/views/translate/translate_bubble_view.cc
+++ b/chrome/browser/ui/views/translate/translate_bubble_view.cc
@@ -6,7 +6,6 @@
 
 #include <algorithm>
 #include <string>
-#include <vector>
 
 #include "base/i18n/string_compare.h"
 #include "base/memory/singleton.h"
@@ -19,6 +18,7 @@
 #include "chrome/browser/translate/translate_service.h"
 #include "chrome/browser/ui/chrome_pages.h"
 #include "chrome/browser/ui/translate/translate_bubble_model_impl.h"
+#include "chrome/browser/ui/translate/translate_denial_combobox_model.h"
 #include "chrome/common/url_constants.h"
 #include "components/translate/core/browser/translate_download_manager.h"
 #include "components/translate/core/browser/translate_manager.h"
@@ -59,47 +59,6 @@ views::Link* CreateLink(views::LinkListener* listener,
   return link;
 }
 
-class TranslateDenialComboboxModel : public ui::ComboboxModel {
- public:
-  enum {
-    INDEX_NOPE = 0,
-    INDEX_NEVER_TRANSLATE_LANGUAGE = 2,
-    INDEX_NEVER_TRANSLATE_SITE = 4,
-  };
-
-  explicit TranslateDenialComboboxModel(
-      const base::string16& original_language_name) {
-    items_.push_back(l10n_util::GetStringUTF16(IDS_TRANSLATE_BUBBLE_DENY));
-    items_.push_back(base::string16());
-    items_.push_back(l10n_util::GetStringFUTF16(
-        IDS_TRANSLATE_BUBBLE_NEVER_TRANSLATE_LANG,
-        original_language_name));
-    items_.push_back(base::string16());
-    items_.push_back(l10n_util::GetStringUTF16(
-        IDS_TRANSLATE_BUBBLE_NEVER_TRANSLATE_SITE));
-  }
-  virtual ~TranslateDenialComboboxModel() {}
-
- private:
-  // Overridden from ui::ComboboxModel:
-  virtual int GetItemCount() const OVERRIDE {
-    return items_.size();
-  }
-  virtual base::string16 GetItemAt(int index) OVERRIDE {
-    return items_[index];
-  }
-  virtual bool IsItemSeparatorAt(int index) OVERRIDE {
-    return items_[index].empty();
-  }
-  virtual int GetDefaultIndex() const OVERRIDE {
-    return 0;
-  }
-
-  std::vector<base::string16> items_;
-
-  DISALLOW_COPY_AND_ASSIGN(TranslateDenialComboboxModel);
-};
-
 }  // namespace
 
 // static
@@ -446,8 +405,9 @@ views::View* TranslateBubbleView::CreateViewBeforeTranslate() {
 
   base::string16 original_language_name =
       model_->GetLanguageNameAt(model_->GetOriginalLanguageIndex());
-  denial_combobox_ = new views::Combobox(
+  denial_combobox_model_.reset(
       new TranslateDenialComboboxModel(original_language_name));
+  denial_combobox_ = new views::Combobox(denial_combobox_model_.get());

[sky, 2014/06/17 16:30:38]

I believe you need to explicitly destroy the combobox before the model,
otherwise ~combobox will try to remove itself as an observer from the model that
has now been destroyed.

[hajimehoshi, 2014/06/18 08:33:37]

|denial_combobox_model_| is initialized only once and destroyed only when the
TranslateBubbleView is destroyed. TranslateBubbleView::CreateView* is called
only at TranslateBubbleView::Init.

In |~TranslateBubbleView|, all children including the combobox is removed and
destructed, so I'm sure that the model is destructed only after the combobox is
destructed.

[sky, 2014/06/18 16:57:19]

I missed the explicit RemoveAlloChildViews(true). nice!

   denial_combobox_->set_id(COMBOBOX_ID_DENIAL);
   denial_combobox_->set_listener(this);
   denial_combobox_->SetStyle(views::Combobox::STYLE_ACTION);