SIGSEGV in content::RenderWidgetHostViewAura::ApplyEventFilterForPopupExit()

SIGSEGV in content::RenderWidgetHostViewAura::ApplyEventFilterForPopupExit()

The cause is very likely that an event-handler is destroyed inside the
event-handling function (OnMouseEvent). Furthermore, this destruction
is not needed due to state variable "in_shutdown_". It was there for a 
very marginal efficiency gain. Removed the handler destruction.

BUG=230872
R=ben@chromium.org
TEST=manual

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

[sschmitz, 2013-04-12 21:14:32]

Most likely cause of crash.

[Ben Goodger (Google), 2013-04-16 16:23:13]

https://codereview.chromium.org/13932029/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/13932029/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_aura.cc:406: if
(is_fullscreen_ || !event || event->type() != ui::ET_MOUSE_PRESSED ||
Why is event NULL?

[sschmitz, 2013-04-16 16:35:06]

Not sure. What do you suggest if I can't repo the crash?


On Tue, Apr 16, 2013 at 9:23 AM, <ben@chromium.org> wrote:

>
> https://codereview.chromium.**org/13932029/diff/1/content/**
>
browser/renderer_host/render_**widget_host_view_aura.cc<https://codereview.chromium.org/13932029/diff/1/content/browser/renderer_host/render_widget_host_view_aura.cc>
> File content/browser/renderer_host/**render_widget_host_view_aura.**cc
> (right):
>
> https://codereview.chromium.**org/13932029/diff/1/content/**
>
browser/renderer_host/render_**widget_host_view_aura.cc#**newcode406<https://codereview.chromium.org/13932029/diff/1/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode406>
> content/browser/renderer_host/**render_widget_host_view_aura.**cc:406: if
>
> (is_fullscreen_ || !event || event->type() != ui::ET_MOUSE_PRESSED ||
> Why is event NULL?
>
>
https://codereview.chromium.**org/13932029/<https://codereview.chromium.org/1...
>

[sschmitz, 2013-04-17 20:20:36]

On 2013/04/17 18:51:39, sschmitz wrote:

Ben, I think you're right. It's not event==NULL, but rather event handler
deletion while handling the event.

[commit-bot: I haz the power, 2013-04-18 00:00:39]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[sschmitz, 2013-04-18 00:02:57]

On 2013/04/18 00:00:39, I haz the power (commit-bot) wrote:
> No LGTM from a valid reviewer yet. Only full committers are accepted.
> Even if an LGTM may have been provided, it was from a non-committer or
> a lowly provisional committer, _not_ a full super star committer.
> See http://www.chromium.org/getting-involved/become-a-committer
> Note that this has nothing to do with OWNERS files.

Oops... accidental commit click.

[Ben Goodger (Google), 2013-04-18 17:04:51]

https://codereview.chromium.org/13932029/diff/6001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/13932029/diff/6001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:405:
DCHECK(event);
I think you shouldn't need this. We make the assumption that event is non-NULL
everywhere in the code.

[sschmitz, 2013-04-18 17:21:43]

https://codereview.chromium.org/13932029/diff/6001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/13932029/diff/6001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:405:
DCHECK(event);
On 2013/04/18 17:04:51, Ben Goodger (Google) wrote:
> I think you shouldn't need this. We make the assumption that event is non-NULL
> everywhere in the code.

Done.

[Ben Goodger (Google), 2013-04-18 17:29:58]

lgtm

[commit-bot: I haz the power, 2013-04-18 17:50:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sschmitz@chromium.org/13932029/15001

[commit-bot: I haz the power, 2013-04-18 18:00:33]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win7_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-18 18:24:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sschmitz@chromium.org/13932029/15001

[commit-bot: I haz the power, 2013-04-18 23:43:56]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-04-19 00:54:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sschmitz@chromium.org/13932029/15001

[commit-bot: I haz the power, 2013-04-19 03:19:04]

Change committed as 195064