Bug fix: Translate: leak of a ComboboxModel

chrome/browser/ui/views/translate/translate_bubble_view.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/translate/translate_bubble_view.h"
 
 #include <algorithm>
 #include <string>
-#include <vector>
 
 #include "base/i18n/string_compare.h"
 #include "base/memory/singleton.h"
 #include "base/metrics/histogram.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/translate/chrome_translate_client.h"
 #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"
 #include "components/translate/core/browser/translate_ui_delegate.h"
 #include "content/public/browser/web_contents.h"
 #include "grit/generated_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/models/combobox_model.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/views/controls/button/checkbox.h"
@@ skipping 20 matching lines @@
 views::Link* CreateLink(views::LinkListener* listener,
                         int resource_id,
                         int id) {
   views::Link* link = new views::Link(
       l10n_util::GetStringUTF16(resource_id));
   link->set_listener(listener);
   link->set_id(id);
   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
 TranslateBubbleView* TranslateBubbleView::translate_bubble_view_ = NULL;
 
 TranslateBubbleView::~TranslateBubbleView() {
   // A child view could refer to a model which is owned by this class when
   // the child view is destructed. For example, |source_language_combobx_model_|
   // is referred by Combobox's destructor. Before destroying the models,
   // removing the child views is needed.
@@ skipping 326 matching lines @@
     view->SetVisible(view == GetCurrentView());
   }
 }
 
 views::View* TranslateBubbleView::CreateViewBeforeTranslate() {
   views::Label* message_label = new views::Label(
       l10n_util::GetStringUTF16(IDS_TRANSLATE_BUBBLE_BEFORE_TRANSLATE));
 
   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);
 
   views::View* view = new views::View();
   views::GridLayout* layout = new views::GridLayout(view);
   view->SetLayoutManager(layout);
 
   using views::GridLayout;
 
@@ skipping 304 matching lines @@
         model_->ShouldAlwaysTranslate());
   }
 
   base::string16 label;
   if (model_->IsPageTranslatedInCurrentLanguages())
     label = l10n_util::GetStringUTF16(IDS_DONE);
   else
     label = l10n_util::GetStringUTF16(IDS_TRANSLATE_BUBBLE_ACCEPT);
   advanced_done_button_->SetText(label);
 }