Attempt at fixing possible crash in ModalHTMLDialogDelegate. It's

possible to get more than one NOTIFY_WEB_CONTENTS_DISCONNECTED. The
first time ModalHTMLDialogDelegate gets a
NOTIFY_WEB_CONTENTS_DISCONNECTED it sets the contents_ to NULL. This
is problematic because the destructor than removes the observer using
a source of NULL. Instead we should remove the observer immediately,
then NULL out the contents_.

BUG=4129
TEST=covered by QEMU

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

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

2 files changed, 20 insertions, 9 deletions

diff --git a/chrome/browser/modal_html_dialog_delegate.cc b/chrome/browser/modal_html_dialog_delegate.cc
index a4aec61..46489cc 100644
--- a/chrome/browser/modal_html_dialog_delegate.cc
+++ b/chrome/browser/modal_html_dialog_delegate.cc
@@ -25,9 +25,7 @@ ModalHtmlDialogDelegate::ModalHtmlDialogDelegate(
 }
 
 ModalHtmlDialogDelegate::~ModalHtmlDialogDelegate() {
-  NotificationService::current()->
-      RemoveObserver(this, NOTIFY_WEB_CONTENTS_DISCONNECTED,
-      Source<WebContents>(contents_));
+  RemoveObserver();
 }
 
 void ModalHtmlDialogDelegate::Observe(NotificationType type,
@@ -35,7 +33,7 @@ void ModalHtmlDialogDelegate::Observe(NotificationType type,
                                       const NotificationDetails& details) {
   DCHECK(type == NOTIFY_WEB_CONTENTS_DISCONNECTED);
   DCHECK(Source<WebContents>(source).ptr() == contents_);
-  contents_ = NULL;  // No longer safe to access.
+  RemoveObserver();
 }
 
 bool ModalHtmlDialogDelegate::IsModal() const {
@@ -66,3 +64,12 @@ void ModalHtmlDialogDelegate::OnDialogClosed(const std::string& json_retval) {
   delete this;
 }
 
+void ModalHtmlDialogDelegate::RemoveObserver() {
+  if (!contents_)
+    return;
+
+  NotificationService::current()->
+      RemoveObserver(this, NOTIFY_WEB_CONTENTS_DISCONNECTED,
+      Source<WebContents>(contents_));
+  contents_ = NULL;  // No longer safe to access.
+}
diff --git a/chrome/browser/modal_html_dialog_delegate.h b/chrome/browser/modal_html_dialog_delegate.h
index 25afd706..34dd0ef 100644
--- a/chrome/browser/modal_html_dialog_delegate.h
+++ b/chrome/browser/modal_html_dialog_delegate.h
@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef CHROME_BROWSER_MODAL_HTML_DIALOG_DELEGATE_H__
-#define CHROME_BROWSER_MODAL_HTML_DIALOG_DELEGATE_H__
+#ifndef CHROME_BROWSER_MODAL_HTML_DIALOG_DELEGATE_H_
+#define CHROME_BROWSER_MODAL_HTML_DIALOG_DELEGATE_H_
 
 #include <vector>
 
@@ -37,6 +37,11 @@ class ModalHtmlDialogDelegate
   virtual void OnDialogClosed(const std::string& json_retval);
 
  private:
+  // Invoked from the destructor or when we receive notification the web
+  // contents has been disconnnected. Removes the observer from the WebContents
+  // and NULLs out contents_.
+  void RemoveObserver();
+
   // The WebContents that opened the dialog.
   WebContents* contents_;
 
@@ -47,8 +52,7 @@ class ModalHtmlDialogDelegate
   // plugin using this |sync_result| pointer so we store it between calls.
   IPC::Message* sync_response_;
 
-  DISALLOW_EVIL_CONSTRUCTORS(ModalHtmlDialogDelegate);
+  DISALLOW_COPY_AND_ASSIGN(ModalHtmlDialogDelegate);
 };
 
-#endif  // CHROME_BROWSER_MODAL_HTML_DIALOG_DELEGATE_H__
-
+#endif  // CHROME_BROWSER_MODAL_HTML_DIALOG_DELEGATE_H_