Fix crash in WebsiteSettingsPopupView::OnWidgetClosing

Fix crash in WebsiteSettingsPopupView::OnWidgetClosing


BUG=140323
TEST=Chrome not crashing after opening and closing the website settings popup for an internal chrome page (e.g. chrome://settings)

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

[markusheintz_, 2012-08-07 14:09:55]

Michael could you review this CL please. Thanks a lot.

[tfarina, 2012-08-07 15:07:28]

On Tue, Aug 7, 2012 at 11:09 AM,  <markusheintz@chromium.org> wrote:
> Reviewers: msw,
>
> Message:
> Michael could you review this CL please. Thanks a lot.
>
> Description:
> Fix crash in WebsiteSettingsPopupView::OnWidgetClosing
>
>
> BUG=140323
> TEST=Chrome not crashing after opening and closing the website settings
> popup
> for an internal chrome page (e.g. chrome://settings)
>
>
> Please review this at http://codereview.chromium.org/10826185/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>   M chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
>
>
> Index:
> chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
> diff --git
> a/chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
> b/chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
> index
>
524e796ba04fd30e251e6922c1739f66c42dbc99..48aed2def7c3d6e589791355625d07fc146aac09
> 100644
> ---
> a/chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
> +++
> b/chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
> @@ -348,7 +348,8 @@ gfx::Rect WebsiteSettingsPopupView::GetAnchorRect() {
>  }
>
>  void WebsiteSettingsPopupView::OnWidgetClosing(views::Widget* widget) {
> -  presenter_->OnUIClosing();
> +  if (presenter_.get())
Could you document somewhere when this can happen? i.e, when
presenter_ can be NULL.

-- 
Thiago

[msw, 2012-08-07 17:26:30]

I share Thiago's concern, nothing in the code indicates that presenter should
ever be NULL. Can you clarify what's happening?

[markusheintz_, 2012-08-07 18:39:19]

On 2012/08/07 17:26:30, msw wrote:
> I share Thiago's concern, nothing in the code indicates that presenter should
> ever be NULL. Can you clarify what's happening?

Sorry you two are right. I should have added a comment to in the header file.

One more thing. This is only a emergency fix since I'm currently number two on
the top crasher charts.

I don't really like the  "if (internalURL(url))" check in the constructor of the
WebsiteSettingsPopupViews class.  I'd prefer to have a separate popup class
which I can instantiate and  display for internal sites. I think this leads to
cleaner code and I don't have to worry about things like the presenter being
NULL.

[msw, 2012-08-07 20:13:45]

Ah, I missed the enormous split of if()/else in the ctor...
Yeah, that's probably an indication that two classes might be a better approach,
but this LGTM with a nit as a band-aid fix.

http://codereview.chromium.org/10826185/diff/3003/chrome/browser/ui/views/web...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
(right):

http://codereview.chromium.org/10826185/diff/3003/chrome/browser/ui/views/web...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:337:
presenter_->OnSitePermissionChanged(
Perhaps comment that this won't be called (perms can't change) for an internal
chrome:// site?

[markusheintz_, 2012-08-07 20:28:05]

http://codereview.chromium.org/10826185/diff/3003/chrome/browser/ui/views/web...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
(right):

http://codereview.chromium.org/10826185/diff/3003/chrome/browser/ui/views/web...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:337:
presenter_->OnSitePermissionChanged(
On 2012/08/07 20:13:45, msw wrote:
> Perhaps comment that this won't be called (perms can't change) for an internal
> chrome:// site?

Done.

[commit-bot: I haz the power, 2012-08-07 20:32:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/markusheintz@chromium.org/10826185/10002

[commit-bot: I haz the power, 2012-08-07 22:31:46]

Change committed as 150430