Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(5)

Issue 2888803002: Handle WebContentsViewMac being destroyed while a <select> menu is showing. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 months ago by tapted
Modified:
3 months ago
CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org, mac-reviews_chromium.org, chrome-apps-syd-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : self review #

Patch Set 3 : Epic test #

Total comments: 6

Patch Set 4 : respond to comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+178 lines, -46 lines) Patch
A chrome/browser/ui/cocoa/tab_contents/web_contents_view_mac_interactive_uitest.mm View 1 2 1 chunk +79 lines, -0 lines 0 comments Download
M chrome/test/BUILD.gn View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
A chrome/test/data/select.html View 1 2 1 chunk +13 lines, -0 lines 0 comments Download
M content/browser/browser_plugin/browser_plugin_popup_menu_helper_mac.h View 2 chunks +6 lines, -1 line 0 comments Download
M content/browser/browser_plugin/browser_plugin_popup_menu_helper_mac.mm View 1 chunk +6 lines, -1 line 0 comments Download
M content/browser/frame_host/popup_menu_helper_mac.h View 1 2 3 3 chunks +15 lines, -2 lines 0 comments Download
M content/browser/frame_host/popup_menu_helper_mac.mm View 1 5 chunks +36 lines, -28 lines 0 comments Download
M content/browser/web_contents/web_contents_view_mac.h View 5 chunks +16 lines, -12 lines 0 comments Download
M content/browser/web_contents/web_contents_view_mac.mm View 2 chunks +6 lines, -2 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 28 (21 generated)
tapted
Hi Avi, please take a look https://codereview.chromium.org/2888803002/diff/40001/chrome/browser/ui/cocoa/tab_contents/web_contents_view_mac_interactive_uitest.mm 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/tab_contents/web_contents_view_mac_interactive_uitest.mm#newcode1 chrome/browser/ui/cocoa/tab_contents/web_contents_view_mac_interactive_uitest.mm:1: // Copyright 2017 ...
3 months ago (2017-05-17 12:08:56 UTC) #16
Avi (ping after 24h)
lgtm https://codereview.chromium.org/2888803002/diff/40001/chrome/browser/ui/cocoa/tab_contents/web_contents_view_mac_interactive_uitest.mm 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/tab_contents/web_contents_view_mac_interactive_uitest.mm#newcode1 chrome/browser/ui/cocoa/tab_contents/web_contents_view_mac_interactive_uitest.mm:1: // Copyright 2017 The Chromium Authors. All rights ...
3 months ago (2017-05-17 15:36:00 UTC) #17
tapted
https://codereview.chromium.org/2888803002/diff/40001/content/browser/frame_host/popup_menu_helper_mac.h File content/browser/frame_host/popup_menu_helper_mac.h (right): https://codereview.chromium.org/2888803002/diff/40001/content/browser/frame_host/popup_menu_helper_mac.h#newcode36 content/browser/frame_host/popup_menu_helper_mac.h:36: }; On 2017/05/17 15:35:59, Avi (ping after 24h) wrote: ...
3 months ago (2017-05-18 03:25:56 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2888803002/60001
3 months ago (2017-05-18 03:28:26 UTC) #21
commit-bot: I haz the power
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_rel_ng/builds/430136)
3 months ago (2017-05-18 06:20:48 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2888803002/60001
3 months ago (2017-05-18 06:23:14 UTC) #25
commit-bot: I haz the power
3 months ago (2017-05-18 08:16:13 UTC) #28
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/271db1a5a41b62a447f29582a50a...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b