Ensure calling Close and set_listener(NULL) on End().

Ensure calling Close and set_listener(NULL) on End().

WebContentsImpl's dtor calls End() but it may not cause
set_listener(NULL) in some case, which may cause a use-after-free.

This CL always reset them at End(). This would mean
DidEndColorChooser() may be called twice, so web_contents_
has to be reset to NULL once DidEndColorChooser() is called.

BUG=338464
R=keishi@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247917

[keishi, 2014-01-28 20:58:21]

lgtm

[Jun Mukai, 2014-01-28 21:00:05]

Peter, could you take a look at this?

[Jun Mukai, 2014-01-29 18:16:39]

sky, could you review this?  This fix would be urgent.

[sky, 2014-01-29 21:38:27]

https://codereview.chromium.org/148223009/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/color_chooser_aura.cc (right):

https://codereview.chromium.org/148223009/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/color_chooser_aura.cc:37: void ColorChooserAura::End() {
I'm not clear on the ownership here. Is there a reason this can't live in the
destructor?

[Jun Mukai, 2014-01-29 21:55:25]

https://codereview.chromium.org/148223009/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/color_chooser_aura.cc (right):

https://codereview.chromium.org/148223009/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/color_chooser_aura.cc:37: void ColorChooserAura::End() {
On 2014/01/29 21:38:27, sky wrote:
> I'm not clear on the ownership here. Is there a reason this can't live in the
> destructor?

WebContentsImpl owns this class, and End() is called in its destructor. See:
http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/web_contents/...

Also, WebContentsImpl::DidEndColorChooser() removes this class.
http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/web_contents/...


I think it would be nicer to call those cleanups regardless of IsVisible(),
rather than adding another hack to ColorChooserAura's destructor.

[sky, 2014-01-29 22:01:59]

Ok, LGTM

[commit-bot: I haz the power, 2014-01-29 22:05:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/148223009/20001

[commit-bot: I haz the power, 2014-01-30 03:04:29]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-30 03:12:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/148223009/20001

[commit-bot: I haz the power, 2014-01-30 10:23:48]

Change committed as 247917