Retry c7e292ea58a76fe2 - Redirect all mouse input to Alt+Tab window

Retry c7e292ea58a76fe2 - Redirect all mouse input to Alt+Tab window

Redirect all mouse input to Alt+Tab window when it's visible,
effectively disabling the mouse. Capture is reset when the widget is
destroyed.

original review: https://codereview.chromium.org/2284763002/

BUG=641171
Committed: https://crrev.com/5a1a9aaec662cf0c8bcb24444c180e3fe1456085
Cr-Commit-Position: refs/heads/master@{#415761}

[Evan Stade, 2016-08-30 15:16:57]

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

[commit-bot: I haz the power, 2016-08-30 15:17:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Evan Stade, 2016-08-30 15:52:06]

estade@chromium.org changed reviewers:
+ sky@chromium.org

[Evan Stade, 2016-08-30 15:52:07]

Should ~Widget be removed from the public API? There's a note to that effect
about CloseNow(), and it seems like the same should apply to the destructor.

[Evan Stade, 2016-08-30 15:52:10]

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

[commit-bot: I haz the power, 2016-08-30 15:52:40]

Dry run: 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, 2016-08-30 16:34:46]

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

[commit-bot: I haz the power, 2016-08-30 16:34:46]

Dry run: This issue passed the CQ dry run.

[sky, 2016-08-30 17:13:58]

I looked at the bot and saw a stack like this:

#0 0x7f59a716c91e base::debug::StackTrace::StackTrace()
#1 0x7f59a716c45f base::debug::(anonymous namespace)::StackDumpSignalHandler()
#2 0x7f59923e3cb0 <unknown>
#3 0x7f59a73d3908 tcmalloc::Abort()
#4 0x7f59a73db79b LogPrintf()
#5 0x7f59a73db63b RAW_VLOG()
#6 0x7f59a7402ed5 MallocBlock::CheckForCorruptedBuffer()
#7 0x7f59a7402bdb MallocBlock::CheckForDanglingWrites()
#8 0x7f59a7400a5f MallocBlock::ProcessFreeQueue()
#9 0x7f59a7404884 MallocBlock::Deallocate()
#10 0x7f59a73fd335 DebugDeallocate()
#11 0x7f59a742b063 tc_free
#12 0x7f59a73d3869 (anonymous namespace)::TCFree()
#13 0x7f59a73d3167 ShimCppDelete
#14 0x000000546b20 __gnu_cxx::new_allocator<>::deallocate()
#15 0x000000546aef std::_Rb_tree<>::_M_put_node()
#16 0x000000558e29 std::_Rb_tree<>::_M_destroy_node()
#17 0x000000558dd8 std::_Rb_tree<>::_M_erase()
#18 0x000000558dbb std::_Rb_tree<>::_M_erase()
#19 0x000000558dbb std::_Rb_tree<>::_M_erase()
#20 0x000000558dbb std::_Rb_tree<>::_M_erase()
#21 0x7f599e8c91b5 std::_Rb_tree<>::~_Rb_tree()
#22 0x7f599e8c9185 std::__cxx1998::set<>::~set()
#23 0x7f599e8c875f std::__debug::set<>::~set()
#24 0x7f599e8c75be gl::GLVersionInfo::GLVersionInfo()
#25 0x7f599e8a7131
_ZN4base10MakeUniqueIN2gl13GLVersionInfoEJPKcS4_S4_EEENS_8internal16MakeUniqueResultIT_E6ScalarEDpOT0_
#26 0x7f599e8a5e86 gl::GLContext::GetVersionInfo()
#27 0x7f599e8d18ff gl::GPUTimingImpl::GPUTimingImpl()
#28 0x7f599e8d2b9b gl::GPUTiming::CreateGPUTiming()
#29 0x7f599e8a6a1b gl::GLContextReal::CreateGPUTimingClient()
#30 0x7f599f04d0fd gpu::gles2::GPUTracer::GPUTracer()
#31 0x7f599efd7de9 gpu::gles2::GLES2DecoderImpl::Initialize()
#32 0x7f599f05b4de gpu::InProcessCommandBuffer::InitializeOnGpuThread()

Which doesn't look related to your patch. What was the crash you're hitting? My
guess is you're deleting everything from OnCaptureLost, which most likely widget
(and related classes) doesn't deal with well. So, I think you're fix is right,
but you need to change the ownership.

Should ~Widget be private? It can't be because of WIDGET_OWNS_NATIVE_WIDGET.
Arguably WIDGET_OWNS_NATIVE_WIDGET shouldn't be allowed, but it makes tests
easier...

https://codereview.chromium.org/2296763002/diff/40001/ash/common/wm/window_cy...
File ash/common/wm/window_cycle_list.cc (right):

https://codereview.chromium.org/2296763002/diff/40001/ash/common/wm/window_cy...
ash/common/wm/window_cycle_list.cc:609: params.ownership =
views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
Because you're configuring the Widget like this it means you should now be
leaking the widget.

[Evan Stade, 2016-08-31 00:51:10]

On 2016/08/30 17:13:58, sky wrote:
> I looked at the bot and saw a stack like this:
> 
> #0 0x7f59a716c91e base::debug::StackTrace::StackTrace()
> #1 0x7f59a716c45f base::debug::(anonymous namespace)::StackDumpSignalHandler()
> #2 0x7f59923e3cb0 <unknown>
> #3 0x7f59a73d3908 tcmalloc::Abort()
> #4 0x7f59a73db79b LogPrintf()
> #5 0x7f59a73db63b RAW_VLOG()
> #6 0x7f59a7402ed5 MallocBlock::CheckForCorruptedBuffer()
> #7 0x7f59a7402bdb MallocBlock::CheckForDanglingWrites()
> #8 0x7f59a7400a5f MallocBlock::ProcessFreeQueue()
> #9 0x7f59a7404884 MallocBlock::Deallocate()
> #10 0x7f59a73fd335 DebugDeallocate()
> #11 0x7f59a742b063 tc_free
> #12 0x7f59a73d3869 (anonymous namespace)::TCFree()
> #13 0x7f59a73d3167 ShimCppDelete
> #14 0x000000546b20 __gnu_cxx::new_allocator<>::deallocate()
> #15 0x000000546aef std::_Rb_tree<>::_M_put_node()
> #16 0x000000558e29 std::_Rb_tree<>::_M_destroy_node()
> #17 0x000000558dd8 std::_Rb_tree<>::_M_erase()
> #18 0x000000558dbb std::_Rb_tree<>::_M_erase()
> #19 0x000000558dbb std::_Rb_tree<>::_M_erase()
> #20 0x000000558dbb std::_Rb_tree<>::_M_erase()
> #21 0x7f599e8c91b5 std::_Rb_tree<>::~_Rb_tree()
> #22 0x7f599e8c9185 std::__cxx1998::set<>::~set()
> #23 0x7f599e8c875f std::__debug::set<>::~set()
> #24 0x7f599e8c75be gl::GLVersionInfo::GLVersionInfo()
> #25 0x7f599e8a7131
>
_ZN4base10MakeUniqueIN2gl13GLVersionInfoEJPKcS4_S4_EEENS_8internal16MakeUniqueResultIT_E6ScalarEDpOT0_
> #26 0x7f599e8a5e86 gl::GLContext::GetVersionInfo()
> #27 0x7f599e8d18ff gl::GPUTimingImpl::GPUTimingImpl()
> #28 0x7f599e8d2b9b gl::GPUTiming::CreateGPUTiming()
> #29 0x7f599e8a6a1b gl::GLContextReal::CreateGPUTimingClient()
> #30 0x7f599f04d0fd gpu::gles2::GPUTracer::GPUTracer()
> #31 0x7f599efd7de9 gpu::gles2::GLES2DecoderImpl::Initialize()
> #32 0x7f599f05b4de gpu::InProcessCommandBuffer::InitializeOnGpuThread()
> 
> Which doesn't look related to your patch. What was the crash you're hitting?
My
> guess is you're deleting everything from OnCaptureLost, which most likely
widget
> (and related classes) doesn't deal with well. So, I think you're fix is right,
> but you need to change the ownership.

yea, the stack for the asan bots was more elucidating although now it's hard to
dig up. I'm pretty sure it was crashing on this line:
https://cs.chromium.org/chromium/src/ui/views/widget/widget.cc?rcl=1472584967...

https://codereview.chromium.org/2296763002/diff/40001/ash/common/wm/window_cy...
File ash/common/wm/window_cycle_list.cc (right):

https://codereview.chromium.org/2296763002/diff/40001/ash/common/wm/window_cy...
ash/common/wm/window_cycle_list.cc:609: params.ownership =
views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
On 2016/08/30 17:13:58, sky wrote:
> Because you're configuring the Widget like this it means you should now be
> leaking the widget.

Done.

[sky, 2016-08-31 15:59:05]

LGTM

[Evan Stade, 2016-08-31 20:49:30]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-08-31 20:50:09]

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, 2016-08-31 21:30:25]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-31 21:31:51]

Description was changed from

==========
Retry c7e292ea58a76fe2 - Redirect all mouse input to Alt+Tab window

Redirect all mouse input to Alt+Tab window when it's visible,
effectively disabling the mouse. Capture is reset when the widget is
destroyed.

original review: https://codereview.chromium.org/2284763002/

BUG=641171
==========

to

==========
Retry c7e292ea58a76fe2 - Redirect all mouse input to Alt+Tab window

Redirect all mouse input to Alt+Tab window when it's visible,
effectively disabling the mouse. Capture is reset when the widget is
destroyed.

original review: https://codereview.chromium.org/2284763002/

BUG=641171

Committed: https://crrev.com/5a1a9aaec662cf0c8bcb24444c180e3fe1456085
Cr-Commit-Position: refs/heads/master@{#415761}
==========

[commit-bot: I haz the power, 2016-08-31 21:31:52]

Patchset 4 (id:??) landed as
https://crrev.com/5a1a9aaec662cf0c8bcb24444c180e3fe1456085
Cr-Commit-Position: refs/heads/master@{#415761}