Make NativeViewHostAura clipping window non-focusable.

Make NativeViewHostAura clipping window non-focusable.

This CL fixes an issue where the clipping window would get a focus event
when the SadTab was displayed causing the focus to be set to nothing,
disabling all keyboard shortcuts.

This is fixed by making the clipping window nonfocusable.

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

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

[sky, 2014-07-15 04:41:12]

https://codereview.chromium.org/397493005/diff/20001/ui/views/controls/native...
File ui/views/controls/native/native_view_host_aura.cc (right):

https://codereview.chromium.org/397493005/diff/20001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura.cc:19: class
NativeViewHostAura::ClippingWindowDelegate : public aura::WindowDelegate {
Would making NavigateViewHostAura an ActivationDelegate make this work too? I
believe the window for the RWHVA should still be focusable, but you'll need to
verify that.

[calamity, 2014-07-15 06:35:01]

https://codereview.chromium.org/397493005/diff/20001/ui/views/controls/native...
File ui/views/controls/native/native_view_host_aura.cc (right):

https://codereview.chromium.org/397493005/diff/20001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura.cc:19: class
NativeViewHostAura::ClippingWindowDelegate : public aura::WindowDelegate {
On 2014/07/15 04:41:12, sky wrote:
> Would making NavigateViewHostAura an ActivationDelegate make this work too? I
> believe the window for the RWHVA should still be focusable, but you'll need to
> verify that.

No dice =( It seems that base_focus_rules.cc only considers ShouldActivate() for
CanActivateWindow() whereas we need to be false for CanFocusWindow().

Other options are to add another method to ActivationDelegate (ShouldFocus) or
make a FocusDelegate.

[sky, 2014-07-15 16:19:40]

https://codereview.chromium.org/397493005/diff/20001/ui/views/controls/native...
File ui/views/controls/native/native_view_host_aura.cc (right):

https://codereview.chromium.org/397493005/diff/20001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura.cc:19: class
NativeViewHostAura::ClippingWindowDelegate : public aura::WindowDelegate {
On 2014/07/15 06:35:01, calamity wrote:
> On 2014/07/15 04:41:12, sky wrote:
> > Would making NavigateViewHostAura an ActivationDelegate make this work too?
I
> > believe the window for the RWHVA should still be focusable, but you'll need
to
> > verify that.
> 
> No dice =( It seems that base_focus_rules.cc only considers ShouldActivate()
for
> CanActivateWindow() whereas we need to be false for CanFocusWindow().
> 
> Other options are to add another method to ActivationDelegate (ShouldFocus) or
> make a FocusDelegate.

What about making NativeViewHostAura itself the WindowDelegate?

[calamity, 2014-07-16 01:19:20]

On 2014/07/15 16:19:40, sky wrote:
>
https://codereview.chromium.org/397493005/diff/20001/ui/views/controls/native...
> File ui/views/controls/native/native_view_host_aura.cc (right):
> 
>
https://codereview.chromium.org/397493005/diff/20001/ui/views/controls/native...
> ui/views/controls/native/native_view_host_aura.cc:19: class
> NativeViewHostAura::ClippingWindowDelegate : public aura::WindowDelegate {
> On 2014/07/15 06:35:01, calamity wrote:
> > On 2014/07/15 04:41:12, sky wrote:
> > > Would making NavigateViewHostAura an ActivationDelegate make this work
too?
> I
> > > believe the window for the RWHVA should still be focusable, but you'll
need
> to
> > > verify that.
> > 
> > No dice =( It seems that base_focus_rules.cc only considers ShouldActivate()
> for
> > CanActivateWindow() whereas we need to be false for CanFocusWindow().
> > 
> > Other options are to add another method to ActivationDelegate (ShouldFocus)
or
> > make a FocusDelegate.
> 
> What about making NativeViewHostAura itself the WindowDelegate?

I need it to be a WindowObserver too to listen for host_->native_view()'s
destruction so making the NVHA a WindowDelegate would make OnWindowDestroyed()
override the same method name as in WindowObserver. That can work but seems a
bit dodgy...

[sky, 2014-07-16 14:55:42]

*SIGH* LGTM

[calamity, 2014-07-17 03:00:46]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-07-17 03:06:46]

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

[commit-bot: I haz the power, 2014-07-17 07:02:00]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[commit-bot: I haz the power, 2014-07-17 07:10:13]

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

[commit-bot: I haz the power, 2014-07-17 07:10:14]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[calamity, 2014-07-18 00:48:35]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-07-18 00:53:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/calamity@chromium.org/397493005/60001

[commit-bot: I haz the power, 2014-07-18 05:30:03]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[calamity, 2014-07-18 07:15:11]

Yeah. Broke all the tests. Changed a thing to fix them if you'd like to take
another look. Thanks.

[sky, 2014-07-18 15:55:06]

SLGTM

[calamity, 2014-07-18 16:48:36]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-07-18 16:49:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/calamity@chromium.org/397493005/80001

[commit-bot: I haz the power, 2014-07-18 19:08:02]

Change committed as 284177

[waffles, 2014-07-18 21:13:28]

On 2014/07/18 19:08:02, I haz the power (commit-bot) wrote:
> Change committed as 284177

Reverted at 284196,

http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te...

==3213==ERROR: AddressSanitizer: heap-use-after-free on address 0x61400000f718
at pc 0x7daaaba bp 0x7fffb9b79660 sp 0x7fffb9b79658
READ of size 8 at 0x61400000f718 thread T0 (content_browser)
    #0 0x7daaab9 in content::RenderFrameHostManager::GetRenderWidgetHostView()
const content/browser/frame_host/render_frame_host_manager.cc:134
    #1 0x811b255 in CanFocus
content/browser/web_contents/web_contents_view_aura.cc:1419
    #2 0x811b255 in non-virtual thunk to
content::WebContentsViewAura::CanFocus()
content/browser/web_contents/web_contents_view_aura.cc:1424
    #3 0x897b331 in
views::NativeViewHostAura::ClippingWindowDelegate::CanFocus()
ui/views/controls/native/native_view_host_aura.cc:43
    #4 0x2b257f1 in aura::Window::CanFocus() const ui/aura/window.cc:724
    #5 0x8706fa7 in wm::BaseFocusRules::CanFocusWindow(aura::Window*) const
ui/wm/core/base_focus_rules.cc:92
    #6 0x870730b in wm::BaseFocusRules::GetFocusableWindow(aura::Window*) const
ui/wm/core/base_focus_rules.cc:141
    #7 0x8713d6e in
wm::FocusController::WindowLostFocusFromDispositionChange(aura::Window*,
aura::Window*) ui/wm/core/focus_controller.cc:350
    #8 0x2b1a8ea in aura::Window::~Window() ui/aura/window.cc:229
    #9 0x2b1c27d in aura::Window::~Window() ui/aura/window.cc:218
    #10 0x81111a9 in operator() base/memory/scoped_ptr.h:137
    #11 0x81111a9 in reset base/memory/scoped_ptr.h:246
    #12 0x81111a9 in reset base/memory/scoped_ptr.h:367
    #13 0x81111a9 in content::WebContentsViewAura::~WebContentsViewAura()
content/browser/web_contents/web_contents_view_aura.cc:750
    #14 0x811163d in content::WebContentsViewAura::~WebContentsViewAura()
content/browser/web_contents/web_contents_view_aura.cc:741
    #15 0x80c85da in operator() base/memory/scoped_ptr.h:137
    #16 0x80c85da in ~scoped_ptr_impl base/memory/scoped_ptr.h:220
    #17 0x80c85da in ~scoped_ptr base/memory/scoped_ptr.h:432
    #18 0x80c85da in content::WebContentsImpl::~WebContentsImpl()
content/browser/web_contents/web_contents_impl.cc:455
    #19 0x80c98fd in content::WebContentsImpl::~WebContentsImpl()
content/browser/web_contents/web_contents_impl.cc:387
    #20 0xff7bd5 in operator() base/memory/scoped_ptr.h:137
    #21 0xff7bd5 in ~scoped_ptr_impl base/memory/scoped_ptr.h:220
    #22 0xff7bd5 in ~scoped_ptr base/memory/scoped_ptr.h:432
    #23 0xff7bd5 in content::Shell::~Shell() content/shell/browser/shell.cc:93
    #24 0xff7d6d in content::Shell::~Shell() content/shell/browser/shell.cc:77
    #25 0x100a533 in content::(anonymous
namespace)::ShellWindowDelegateView::WindowClosing()
content/shell/browser/shell_views.cc:350
    #26 0x16fadec in views::Widget::OnNativeWidgetDestroying()
ui/views/widget/widget.cc:1089
    #27 0x1766b0e in views::DesktopWindowTreeHostX11::CloseNow()
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:318
    #28 0x16f6389 in views::Widget::CloseNow() ui/views/widget/widget.cc:594
    #29 0xff83a7 in content::Shell::CloseAllWindows()
content/shell/browser/shell.cc:120
    #30 0xfefa3d in content::ContentBrowserTest::RunTestOnMainThreadLoop()
content/public/test/content_browser_test.cc:154
    #31 0x10587bc in Run base/callback.h:401
    #32 0x10587bc in content::ShellBrowserMainParts::PreMainMessageLoopRun()
content/shell/browser/shell_browser_main_parts.cc:168
    #33 0x7c8bbb7 in content::BrowserMainLoop::PreMainMessageLoopRun()
content/browser/browser_main_loop.cc:695
    #34 0x808dbc7 in Run base/callback.h:401
    #35 0x808dbc7 in content::StartupTaskRunner::RunAllTasksNow()
content/browser/startup_task_runner.cc:45
    #36 0x7c87cc0 in content::BrowserMainLoop::CreateStartupTasks()
content/browser/browser_main_loop.cc:595
    #37 0x7c925a3 in
content::BrowserMainRunnerImpl::Initialize(content::MainFunctionParams const&)
content/browser/browser_main_runner.cc:106
    #38 0xff58c8 in ShellBrowserMain(content::MainFunctionParams const&,
scoped_ptr\u003Ccontent::BrowserMainRunner,
base::DefaultDeleter\u003Ccontent::BrowserMainRunner> > const&)
content/shell/browser/shell_browser_main.cc:171
    #39 0xff409e in content::ShellMainDelegate::RunProcess(std::string const&,
content::MainFunctionParams const&) content/shell/app/shell_main_delegate.cc:243
    #40 0x7c6c1fa in content::RunNamedProcessTypeMain(std::string const&,
content::MainFunctionParams const&, content::ContentMainDelegate*)
content/app/content_main_runner.cc:404
    #41 0x7c6d5d4 in content::ContentMainRunnerImpl::Run()
content/app/content_main_runner.cc:764
    #42 0x7c6a71f in content::ContentMain(content::ContentMainParams const&)
content/app/content_main.cc:19
    #43 0x112b6ef in content::BrowserTestBase::SetUp()
content/public/test/browser_test_base.cc:253
    #44 0xfef2b6 in content::ContentBrowserTest::SetUp()
content/public/test/content_browser_test.cc:100
 <TRUNCATED>

0x61400000f718 is located 216 bytes inside of 416-byte region
[0x61400000f640,0x61400000f7e0)
freed by thread T0 (content_browser) here:
    #0 0x48121b in operator delete(void*)
/usr/local/google/work/chromium/src/third_party/llvm/projects/compiler-rt/lib/asan/asan_new_delete.cc:87
    #1 0x82b431f in operator() base/memory/scoped_ptr.h:137
    #2 0x82b431f in ~scoped_ptr_impl base/memory/scoped_ptr.h:220
    #3 0x82b431f in ~scoped_ptr base/memory/scoped_ptr.h:432
    #4 0x82b431f in content::FrameTree::~FrameTree()
content/browser/frame_host/frame_tree.cc:79
    #5 0x80c8484 in content::WebContentsImpl::~WebContentsImpl()
content/browser/web_contents/web_contents_impl.cc:455
    #6 0x80c98fd in content::WebContentsImpl::~WebContentsImpl()
content/browser/web_contents/web_contents_impl.cc:387
    #7 0xff7bd5 in operator() base/memory/scoped_ptr.h:137
    #8 0xff7bd5 in ~scoped_ptr_impl base/memory/scoped_ptr.h:220
    #9 0xff7bd5 in ~scoped_ptr base/memory/scoped_ptr.h:432
    #10 0xff7bd5 in content::Shell::~Shell() content/shell/browser/shell.cc:93
    #11 0xff7d6d in content::Shell::~Shell() content/shell/browser/shell.cc:77
    #12 0x100a533 in content::(anonymous
namespace)::ShellWindowDelegateView::WindowClosing()
content/shell/browser/shell_views.cc:350
    #13 0x16fadec in views::Widget::OnNativeWidgetDestroying()
ui/views/widget/widget.cc:1089
    #14 0x1766b0e in views::DesktopWindowTreeHostX11::CloseNow()
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:318
    #15 0x16f6389 in views::Widget::CloseNow() ui/views/widget/widget.cc:594
    #16 0xff83a7 in content::Shell::CloseAllWindows()
content/shell/browser/shell.cc:120
    #17 0xfefa3d in content::ContentBrowserTest::RunTestOnMainThreadLoop()
content/public/test/content_browser_test.cc:154
    #18 0x10587bc in Run base/callback.h:401
    #19 0x10587bc in content::ShellBrowserMainParts::PreMainMessageLoopRun()
content/shell/browser/shell_browser_main_parts.cc:168
    #20 0x7c8bbb7 in content::BrowserMainLoop::PreMainMessageLoopRun()
content/browser/browser_main_loop.cc:695
    #21 0x808dbc7 in Run base/callback.h:401
    #22 0x808dbc7 in content::StartupTaskRunner::RunAllTasksNow()
content/browser/startup_task_runner.cc:45
    #23 0x7c87cc0 in content::BrowserMainLoop::CreateStartupTasks()
content/browser/browser_main_loop.cc:595
 <TRUNCATED>

previously allocated by thread T0 (content_browser) here:
    #0 0x480cdb in operator new(unsigned long)
/usr/local/google/work/chromium/src/third_party/llvm/projects/compiler-rt/lib/asan/asan_new_delete.cc:55
    #1 0x82b40d4 in content::FrameTree::FrameTree(content::Navigator*,
content::RenderFrameHostDelegate*, content::RenderViewHostDelegate*,
content::RenderWidgetHostDelegate*, content::RenderFrameHostManager::Delegate*)
content/browser/frame_host/frame_tree.cc:75
    #2 0x80c488f in
content::WebContentsImpl::WebContentsImpl(content::BrowserContext*,
content::WebContentsImpl*) content/browser/web_contents/web_contents_impl.cc:379
    #3 0x80c393c in
content::WebContentsImpl::CreateWithOpener(content::WebContents::CreateParams
const&, content::WebContentsImpl*)
content/browser/web_contents/web_contents_impl.cc:461
    #4 0xff8845 in content::Shell::CreateNewWindow(content::BrowserContext*,
GURL const&, content::SiteInstance*, int, gfx::Size const&)
content/shell/browser/shell.cc:161
    #5 0x105837a in content::ShellBrowserMainParts::PreMainMessageLoopRun()
content/shell/browser/shell_browser_main_parts.cc:141
    #6 0x7c8bbb7 in content::BrowserMainLoop::PreMainMessageLoopRun()
<TRUNCATED>

[calamity, 2014-07-22 07:41:02]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-07-22 07:42:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/calamity@chromium.org/397493005/100001

[commit-bot: I haz the power, 2014-07-22 10:59:19]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-07-22 12:37:36]

Change committed as 284670