Fix memory leak for extension uninstall dialog.

chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc

 // Copyright (c) 2012 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 <memory>
 
 #include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
@@ skipping 26 matching lines @@
 class ExtensionUninstallDialogViews
     : public extensions::ExtensionUninstallDialog {
  public:
   ExtensionUninstallDialogViews(
       Profile* profile,
       gfx::NativeWindow parent,
       extensions::ExtensionUninstallDialog::Delegate* delegate);
   ~ExtensionUninstallDialogViews() override;
 
   // Called when the ExtensionUninstallDialogDelegate has been destroyed to make
-  // sure we invalidate pointers.
-  void DialogDelegateDestroyed() { view_ = NULL; }
+  // sure we invalidate pointers. This object will also be freed.
+  void DialogDelegateDestroyed();
 
   // Forwards the accept and cancels to the delegate.
   void DialogAccepted(bool handle_report_abuse);
   void DialogCanceled();
 
  private:
   void Show() override;
 
   ExtensionUninstallDialogDelegateView* view_;
 
@@ skipping 53 matching lines @@
 
 ExtensionUninstallDialogViews::ExtensionUninstallDialogViews(
     Profile* profile,
     gfx::NativeWindow parent,
     extensions::ExtensionUninstallDialog::Delegate* delegate)
     : extensions::ExtensionUninstallDialog(profile, delegate),
       view_(NULL),
       parent_(parent) {
   if (parent_)
     parent_window_tracker_ = NativeWindowTracker::Create(parent_);
+  LOG(ERROR) << this << " Dialog Created";
 }
 
 ExtensionUninstallDialogViews::~ExtensionUninstallDialogViews() {
   // Close the widget (the views framework will delete view_).
   if (view_) {
     view_->DialogDestroyed();
     view_->GetWidget()->CloseNow();
   }
+  LOG(ERROR) << this << " Dialog Died";
 }
 
 void ExtensionUninstallDialogViews::Show() {
   if (parent_ && parent_window_tracker_->WasNativeWindowClosed()) {
     OnDialogClosed(CLOSE_ACTION_CANCELED);
     return;
   }
 
   view_ = new ExtensionUninstallDialogDelegateView(
       this, triggering_extension() != nullptr, &icon());
   constrained_window::CreateBrowserModalDialogViews(view_, parent_)->Show();
 }
 
+void ExtensionUninstallDialogViews::DialogDelegateDestroyed() {
+  view_ = NULL;
+  OnDialogClosed(CLOSE_ACTION_CANCELED);

[Devlin, 2016/11/05 06:26:18]

It seems like there's a potential that this could be called twice now, since the
dialog view is eventually destructed after e.g. hitting accept.  In the tests,
this won't necessarily fail, but it would be bad in production (where we have
lifetime issues and real consequences calling multiple times).  We should a)
guard this here and b) maybe put in a DCHECK
ExtensionUninstallDialog::OnDialogClosed to make sure we don't call twice.

[lgcheng, 2016/11/10 21:39:51]

Thanks for pointing out the potential issue. But after re-examine more on
details of implementation, I think it's safe since in the deconstructor of
ExtensionUninstallDialogDelegateView there is a check   
if (dialog_)
    dialog_->DialogDelegateDestroyed();

If the view is closed due to accept or cancel,
ExtensionUninstallDialogViews::DialogAccepted or
ExtensionUninstallDialogViews::DialogCanceled() will be called, in which
view_->DialogDestroyed() { dialog_ = NULL} is called and guarantees  
dialog_->DialogDelegateDestroyed() will not be called in this situation.

But I agree to add debug mode checks for this.

+}
+
 void ExtensionUninstallDialogViews::DialogAccepted(bool report_abuse_checked) {
   // The widget gets destroyed when the dialog is accepted.
   view_->DialogDestroyed();
   view_ = nullptr;
   OnDialogClosed(report_abuse_checked ?
       CLOSE_ACTION_UNINSTALL_AND_REPORT_ABUSE : CLOSE_ACTION_UNINSTALL);
 }
 
 void ExtensionUninstallDialogViews::DialogCanceled() {
   // The widget gets destroyed when the dialog is canceled.
@@ skipping 16 matching lines @@
   icon_ = new views::ImageView();
   icon_->SetImageSize(size);
   icon_->SetImage(*image);
   AddChildView(icon_);
 
   heading_ = new views::Label(base::UTF8ToUTF16(dialog_->GetHeadingText()));
   heading_->SetMultiLine(true);
   heading_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
   heading_->SetAllowCharacterBreak(true);
   AddChildView(heading_);
+  LOG(ERROR) << this << " Dialog View Created";
 }
 
 ExtensionUninstallDialogDelegateView::~ExtensionUninstallDialogDelegateView() {
   // If we're here, 2 things could have happened. Either the user closed the
   // dialog nicely and one of the installed/canceled methods has been called
   // (in which case dialog_ will be null), *or* neither of them have been
   // called and we are being forced closed by our parent widget. In this case,
   // we need to make sure to notify dialog_ not to call us again, since we're
   // about to be freed by the Widget framework.
   if (dialog_)
     dialog_->DialogDelegateDestroyed();
+  LOG(ERROR) << this << " Dialog View Died";
 }
 
 views::View* ExtensionUninstallDialogDelegateView::CreateExtraView() {
   if (dialog_->ShouldShowReportAbuseCheckbox()) {
     report_abuse_checkbox_ = new views::Checkbox(
         l10n_util::GetStringUTF16(IDS_EXTENSION_PROMPT_UNINSTALL_REPORT_ABUSE));
   }
   return report_abuse_checkbox_;
 }
 
@@ skipping 69 matching lines @@
 
 }  // namespace
 
 // static
 extensions::ExtensionUninstallDialog*
 extensions::ExtensionUninstallDialog::Create(Profile* profile,
                                              gfx::NativeWindow parent,
                                              Delegate* delegate) {
   return new ExtensionUninstallDialogViews(profile, parent, delegate);
 }