Handle WebContentsViewMac being destroyed while a <select> menu is showing.

Handle WebContentsViewMac being destroyed while a <select> menu is showing.

content::PopupMenuHelper currently has protections for the
RenderFrameHostImpl and the RenderWidgetHostViewMac being destroyed in
the nested message loop that is run while showing the native NSMenu.
However, PopupMenuHelper doesn't protect against the WebContentsViewMac
(its owner) being destroyed.

Since r470769, posted tasks may be executed while a menu is fading out.
So it's possible for WebContentsViewMac to be destroyed sooner. Handle
this case.

Adds an integration test. Previously, destroying a tab wouldn't close a
<select> menu open on it, now it does. So, without the fix, the test
gets stuck waiting for the menu to close, but dismissing the menu (e.g.
by clicking anywhere) would reproduce the crash in the linked bug.

BUG=722830
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2888803002
Cr-Commit-Position: refs/heads/master@{#472738}
Committed: https://chromium.googlesource.com/chromium/src/+/271db1a5a41b62a447f29582a50a4bf0e69433c0

[tapted, 2017-05-17 01:04:56]

Description was changed from

==========
cl format


fix crash

BUG=722830
==========

to

==========
cl format


fix crash

BUG=722830
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[tapted, 2017-05-17 01:16:47]

Description was changed from

==========
cl format


fix crash

BUG=722830
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Handle WebContentsViewMac being destroyed while a <select> menu is showing.

content::PopupMenuHelper currently has protections for the
RenderFrameHostImpl and the RenderWidgetHostViewMac being destroyed in
the nested message loop that is run while showing the native NSMenu, but
not the WebContentsViewMac, which owns it.

Since r470769 posted tasks may be executed while a menu is fading out.
So it's possible for WebContentsViewMac to be destroyed sooner. Handle
this case.

BUG=722830
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[tapted, 2017-05-17 01:18:05]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 01:19:03]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-05-17 02:49:47]

Description was changed from

==========
Handle WebContentsViewMac being destroyed while a <select> menu is showing.

content::PopupMenuHelper currently has protections for the
RenderFrameHostImpl and the RenderWidgetHostViewMac being destroyed in
the nested message loop that is run while showing the native NSMenu, but
not the WebContentsViewMac, which owns it.

Since r470769 posted tasks may be executed while a menu is fading out.
So it's possible for WebContentsViewMac to be destroyed sooner. Handle
this case.

BUG=722830
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Handle WebContentsViewMac being destroyed while a <select> menu is showing.

content::PopupMenuHelper currently has protections for the
RenderFrameHostImpl and the RenderWidgetHostViewMac being destroyed in
the nested message loop that is run while showing the native NSMenu.
However,  PopupMenuHelper doesn't protect against the WebContentsViewMac
(its owner) being destroyed.

Since r470769, posted tasks may be executed while a menu is fading out.
So it's possible for WebContentsViewMac to be destroyed sooner. Handle
this case.

BUG=722830
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2017-05-17 03:27:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-17 03:27:46]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-05-17 07:00:44]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 07:01:14]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-05-17 07:03:59]

Description was changed from

==========
Handle WebContentsViewMac being destroyed while a <select> menu is showing.

content::PopupMenuHelper currently has protections for the
RenderFrameHostImpl and the RenderWidgetHostViewMac being destroyed in
the nested message loop that is run while showing the native NSMenu.
However,  PopupMenuHelper doesn't protect against the WebContentsViewMac
(its owner) being destroyed.

Since r470769, posted tasks may be executed while a menu is fading out.
So it's possible for WebContentsViewMac to be destroyed sooner. Handle
this case.

BUG=722830
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Handle WebContentsViewMac being destroyed while a <select> menu is showing.

content::PopupMenuHelper currently has protections for the
RenderFrameHostImpl and the RenderWidgetHostViewMac being destroyed in
the nested message loop that is run while showing the native NSMenu.
However,  PopupMenuHelper doesn't protect against the WebContentsViewMac
(its owner) being destroyed.

Since r470769, posted tasks may be executed while a menu is fading out.
So it's possible for WebContentsViewMac to be destroyed sooner. Handle
this case.

Adds an integration test. Previously, destroying a tab wouldn't close a
<select> menu open on it, now it does. Without the fix, the test gets
stuck waiting for the menu to close, but dismissing the menu (e.g. by
clicking anywhere) then reproduces the crash in the linked bug.

BUG=722830
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[tapted, 2017-05-17 07:04:00]

tapted@chromium.org changed reviewers:
+ avi@chromium.org

[tapted, 2017-05-17 07:09:16]

Description was changed from

==========
Handle WebContentsViewMac being destroyed while a <select> menu is showing.

content::PopupMenuHelper currently has protections for the
RenderFrameHostImpl and the RenderWidgetHostViewMac being destroyed in
the nested message loop that is run while showing the native NSMenu.
However,  PopupMenuHelper doesn't protect against the WebContentsViewMac
(its owner) being destroyed.

Since r470769, posted tasks may be executed while a menu is fading out.
So it's possible for WebContentsViewMac to be destroyed sooner. Handle
this case.

Adds an integration test. Previously, destroying a tab wouldn't close a
<select> menu open on it, now it does. Without the fix, the test gets
stuck waiting for the menu to close, but dismissing the menu (e.g. by
clicking anywhere) then reproduces the crash in the linked bug.

BUG=722830
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Handle WebContentsViewMac being destroyed while a <select> menu is showing.

content::PopupMenuHelper currently has protections for the
RenderFrameHostImpl and the RenderWidgetHostViewMac being destroyed in
the nested message loop that is run while showing the native NSMenu.
However,  PopupMenuHelper doesn't protect against the WebContentsViewMac
(its owner) being destroyed.

Since r470769, posted tasks may be executed while a menu is fading out.
So it's possible for WebContentsViewMac to be destroyed sooner. Handle
this case.

Adds an integration test. Previously, destroying a tab wouldn't close a
<select> menu open on it, now it does. So, without the fix, the test
gets stuck waiting for the menu to close, but dismissing the menu (e.g.
by clicking anywhere) would reproduce the crash in the linked bug.

BUG=722830
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[tapted, 2017-05-17 07:09:56]

Description was changed from

==========
Handle WebContentsViewMac being destroyed while a <select> menu is showing.

content::PopupMenuHelper currently has protections for the
RenderFrameHostImpl and the RenderWidgetHostViewMac being destroyed in
the nested message loop that is run while showing the native NSMenu.
However,  PopupMenuHelper doesn't protect against the WebContentsViewMac
(its owner) being destroyed.

Since r470769, posted tasks may be executed while a menu is fading out.
So it's possible for WebContentsViewMac to be destroyed sooner. Handle
this case.

Adds an integration test. Previously, destroying a tab wouldn't close a
<select> menu open on it, now it does. So, without the fix, the test
gets stuck waiting for the menu to close, but dismissing the menu (e.g.
by clicking anywhere) would reproduce the crash in the linked bug.

BUG=722830
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Handle WebContentsViewMac being destroyed while a <select> menu is showing.

content::PopupMenuHelper currently has protections for the
RenderFrameHostImpl and the RenderWidgetHostViewMac being destroyed in
the nested message loop that is run while showing the native NSMenu.
However, PopupMenuHelper doesn't protect against the WebContentsViewMac
(its owner) being destroyed.

Since r470769, posted tasks may be executed while a menu is fading out.
So it's possible for WebContentsViewMac to be destroyed sooner. Handle
this case.

Adds an integration test. Previously, destroying a tab wouldn't close a
<select> menu open on it, now it does. So, without the fix, the test
gets stuck waiting for the menu to close, but dismissing the menu (e.g.
by clicking anywhere) would reproduce the crash in the linked bug.

BUG=722830
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2017-05-17 08:47:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-17 08:47:37]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[tapted, 2017-05-17 12:08:56]

Hi Avi, please take a look

https://codereview.chromium.org/2888803002/diff/40001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/tab_contents/web_contents_view_mac_interactive_uitest.mm
(right):

https://codereview.chromium.org/2888803002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tab_contents/web_contents_view_mac_interactive_uitest.mm:1:
// Copyright 2017 The Chromium Authors. All rights reserved.
I don't really know where this file should live..

The test must be interactive for the native menu goop, but there are no
interactive test targets in src/content  ( https://crbug.com/491535 ). The test
APIs there aren't well suited to this in any case, and we probably don't ever
want the burden of another interactive_ui_tests target.

interactive_ui_tests already has precedent for reaching outside of src/chrome
for test cases. E.g. //ui/base and //ui/views.

There are some non-interactive, render-side-only tests in
src/content/renderer/external_popup_menu_browsertest.cc . There is no coverage
of the browser side there.

So I went with chrome/browser/ui/cocoa/tab_contents/ - that's also a little
weird, e.g. since only content/public is available (and not
web_contents_view_mac.h), but that's not necessary in this case.

https://codereview.chromium.org/2888803002/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_view_mac.h (right):

https://codereview.chromium.org/2888803002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_view_mac.h:113: void TakeFocus(bool
reverse) override;
(reordering the header to match the .mm also gets it closer to the declaration
order in RenderViewHostDelegateView)

[Avi (use Gerrit), 2017-05-17 15:36:00]

lgtm

https://codereview.chromium.org/2888803002/diff/40001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/tab_contents/web_contents_view_mac_interactive_uitest.mm
(right):

https://codereview.chromium.org/2888803002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tab_contents/web_contents_view_mac_interactive_uitest.mm:1:
// Copyright 2017 The Chromium Authors. All rights reserved.
On 2017/05/17 12:08:55, tapted wrote:
> I don't really know where this file should live..
> 
> The test must be interactive for the native menu goop, but there are no
> interactive test targets in src/content  ( https://crbug.com/491535 ). The
test
> APIs there aren't well suited to this in any case, and we probably don't ever
> want the burden of another interactive_ui_tests target.
> 
> interactive_ui_tests already has precedent for reaching outside of src/chrome
> for test cases. E.g. //ui/base and //ui/views.
> 
> There are some non-interactive, render-side-only tests in
> src/content/renderer/external_popup_menu_browsertest.cc . There is no coverage
> of the browser side there.
> 
> So I went with chrome/browser/ui/cocoa/tab_contents/ - that's also a little
> weird, e.g. since only content/public is available (and not
> web_contents_view_mac.h), but that's not necessary in this case.

Ack.

I'm not really sure, either. I'm happy to have it here for now, and figure out a
better location later. Getting ourselves a test is the best thing.

https://codereview.chromium.org/2888803002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/popup_menu_helper_mac.h (right):

https://codereview.chromium.org/2888803002/diff/40001/content/browser/frame_h...
content/browser/frame_host/popup_menu_helper_mac.h:36: };
blank line after

https://codereview.chromium.org/2888803002/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_view_mac.h (right):

https://codereview.chromium.org/2888803002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_view_mac.h:113: void TakeFocus(bool
reverse) override;
On 2017/05/17 12:08:56, tapted wrote:
> (reordering the header to match the .mm also gets it closer to the declaration
> order in RenderViewHostDelegateView)

Acknowledged.

[tapted, 2017-05-18 03:25:56]

https://codereview.chromium.org/2888803002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/popup_menu_helper_mac.h (right):

https://codereview.chromium.org/2888803002/diff/40001/content/browser/frame_h...
content/browser/frame_host/popup_menu_helper_mac.h:36: };
On 2017/05/17 15:35:59, Avi (ping after 24h) wrote:
> blank line after

Done.

[tapted, 2017-05-18 03:26:05]

The CQ bit was checked by tapted@chromium.org

[tapted, 2017-05-18 03:26:06]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org
Link to the patchset: https://codereview.chromium.org/2888803002/#ps60001
(title: "respond to comments")

[commit-bot: I haz the power, 2017-05-18 03:28:26]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 06:20:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-18 06:20:48]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[tapted, 2017-05-18 06:22:29]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2017-05-18 06:23:14]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 08:16:03]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495088549374200,
"parent_rev": "c560f19cffa304227d7e1e9b3cf00acb4cd92cb8", "commit_rev":
"271db1a5a41b62a447f29582a50a4bf0e69433c0"}

[commit-bot: I haz the power, 2017-05-18 08:16:12]

Description was changed from

==========
Handle WebContentsViewMac being destroyed while a <select> menu is showing.

content::PopupMenuHelper currently has protections for the
RenderFrameHostImpl and the RenderWidgetHostViewMac being destroyed in
the nested message loop that is run while showing the native NSMenu.
However, PopupMenuHelper doesn't protect against the WebContentsViewMac
(its owner) being destroyed.

Since r470769, posted tasks may be executed while a menu is fading out.
So it's possible for WebContentsViewMac to be destroyed sooner. Handle
this case.

Adds an integration test. Previously, destroying a tab wouldn't close a
<select> menu open on it, now it does. So, without the fix, the test
gets stuck waiting for the menu to close, but dismissing the menu (e.g.
by clicking anywhere) would reproduce the crash in the linked bug.

BUG=722830
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Handle WebContentsViewMac being destroyed while a <select> menu is showing.

content::PopupMenuHelper currently has protections for the
RenderFrameHostImpl and the RenderWidgetHostViewMac being destroyed in
the nested message loop that is run while showing the native NSMenu.
However, PopupMenuHelper doesn't protect against the WebContentsViewMac
(its owner) being destroyed.

Since r470769, posted tasks may be executed while a menu is fading out.
So it's possible for WebContentsViewMac to be destroyed sooner. Handle
this case.

Adds an integration test. Previously, destroying a tab wouldn't close a
<select> menu open on it, now it does. So, without the fix, the test
gets stuck waiting for the menu to close, but dismissing the menu (e.g.
by clicking anywhere) would reproduce the crash in the linked bug.

BUG=722830
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2888803002
Cr-Commit-Position: refs/heads/master@{#472738}
Committed:
https://chromium.googlesource.com/chromium/src/+/271db1a5a41b62a447f29582a50a...
==========

[commit-bot: I haz the power, 2017-05-18 08:16:13]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/271db1a5a41b62a447f29582a50a...