There is a race condition when the HtmlDialogView is closed which causes a cr...

There is a race condition when the HtmlDialogView is closed which causes a crash while dereferencing an invalid
delegate_ (HtmlDialogUIDelegate*) member. I could not reproduce this consistently though. The inference is
as below:-

1. When the dialog is closed the HtmlDialogView::OnDialogClosed member function is invoked which calls the
   OnDialogClosed function on the delegate. This in turn causes the delegate to be destroyed.
2. It then sets the delegate to NULL and attempts to close the window.
3. Before the Close method is dispatched if the view attempts to Paint it causes a crash in the
   HtmlDialogView::GetWindowTitle function because of dereferencing a NULL delegate_.

Fix is to add corresponding NULL checks in the relevant functions.

This fixes http://b/issue?id=2138035, which was reported with ChromeFrame.

Bug=2138035
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=26953

[jcampan, 2009-09-23 17:57:10]

LGTM

[kuchhal, 2009-09-24 01:04:58]

I think this would fix
http://code.google.com/p/chromium/issues/detail?id=22778 and
http://code.google.com/p/chromium/issues/detail?id=22184 too.

On Wed, Sep 23, 2009 at 10:53 AM, <ananta@chromium.org> wrote:

>
> Reviewers: jcampan,
>
> Description:
> There is a race condition when the HtmlDialogView is closed which causes a
> crash
> while dereferencing an invalid
> delegate_ (HtmlDialogUIDelegate*) member. I could not reproduce this
> consistently though. The inference is
> as below:-
>
> 1. When the dialog is closed the HtmlDialogView::OnDialogClosed member
> function
> is invoked which calls the
>   OnDialogClosed function on the delegate. This in turn causes the delegate
> to
> be destroyed.
> 2. It then sets the delegate to NULL and attempts to close the window.
> 3. Before the Close method is dispatched if the view attempts to Paint it
> causes
> a crash in the
>   HtmlDialogView::GetWindowTitle function because of dereferencing a NULL
> delegate_.
>
> Fix is to add corresponding NULL checks in the relevant functions.
>
> This fixes http://b/issue?id=2138035, which was reported with ChromeFrame.
>
> Bug=2138035
>
>
>
> Please review this at http://codereview.chromium.org/220011
>
> SVN Base: svn://chrome-svn/chrome/trunk/src/
>
> Affected files:
>  M     chrome/browser/views/html_dialog_view.cc
>
>
> Index: chrome/browser/views/html_dialog_view.cc
> ===================================================================
> --- chrome/browser/views/html_dialog_view.cc    (revision 26676)
> +++ chrome/browser/views/html_dialog_view.cc    (working copy)
> @@ -12,7 +12,7 @@
>
>  namespace browser {
>
> -// Declared in browser_dialogs.h so that others don't need to depend on
> our .h.
> +// Declared in browser_dialogs.h so that others don't need to depend on
> our .h.
>  void ShowHtmlDialogView(gfx::NativeWindow parent, Browser* browser,
>                         HtmlDialogUIDelegate* delegate) {
>   HtmlDialogView* html_view = new HtmlDialogView(browser, delegate);
> @@ -43,7 +43,8 @@
>
>  gfx::Size HtmlDialogView::GetPreferredSize() {
>   gfx::Size out;
> -  delegate_->GetDialogSize(&out);
> +  if (delegate_)
> +    delegate_->GetDialogSize(&out);
>   return out;
>  }
>
> @@ -55,11 +56,17 @@
>  }
>
>  bool HtmlDialogView::IsModal() const {
> -  return delegate_->IsDialogModal();
> +  if (delegate_)
> +    return delegate_->IsDialogModal();
> +  else
> +    return false;
>  }
>
>  std::wstring HtmlDialogView::GetWindowTitle() const {
> -  return delegate_->GetDialogTitle();
> +  if (delegate_)
> +    return delegate_->GetDialogTitle();
> +  else
> +    return std::wstring();
>  }
>
>  void HtmlDialogView::WindowClosing() {
> @@ -90,25 +97,34 @@
>  }
>
>  GURL HtmlDialogView::GetDialogContentURL() const {
> -  return delegate_->GetDialogContentURL();
> +  if (delegate_)
> +    return delegate_->GetDialogContentURL();
> +  else
> +    return GURL();
>  }
>
>  void HtmlDialogView::GetDOMMessageHandlers(
>     std::vector<DOMMessageHandler*>* handlers) const {
> -  delegate_->GetDOMMessageHandlers(handlers);
> +  if (delegate_)
> +    delegate_->GetDOMMessageHandlers(handlers);
>  }
>
>  void HtmlDialogView::GetDialogSize(gfx::Size* size) const {
> -  delegate_->GetDialogSize(size);
> +  if (delegate_)
> +    delegate_->GetDialogSize(size);
>  }
>
>  std::string HtmlDialogView::GetDialogArgs() const {
> -  return delegate_->GetDialogArgs();
> +  if (delegate_)
> +    return delegate_->GetDialogArgs();
> +  else
> +    return std::string();
>  }
>
>  void HtmlDialogView::OnDialogClosed(const std::string& json_retval) {
> -  delegate_->OnDialogClosed(json_retval);
> +  HtmlDialogUIDelegate* dialog_delegate = delegate_;
>   delegate_ = NULL;  // We will not communicate further with the delegate.
> +  dialog_delegate->OnDialogClosed(json_retval);
>   window()->Close();
>  }
>
>
>
>