TooltipWin::Hide runs continously on a timer in every open window |
|||||||
Issue descriptionCame across this by accident when trying to track down a memory leak. The logic around updating "showing_" appears to be broken, and some chain of logic causes the TooltipWin to try to hide itself continuously on a timer, roughly once per second. The stack trace is the following: 00 0000002a`405ed648 00007ffb`2da12cbc chrome_7ffb2b770000!views::corewm::TooltipWin::Hide [c:\b\build\slave\win64-pgo\build\src\ui\views\corewm\tooltip_win.cc @ 145] 01 0000002a`405ed650 00007ffb`2c737cae chrome_7ffb2b770000!views::corewm::TooltipController::UpdateIfRequired+0x3b4 [c:\b\build\slave\win64-pgo\build\src\ui\views\corewm\tooltip_controller.cc @ 342] 02 (Inline Function) --------`-------- chrome_7ffb2b770000!base::Callback<void __cdecl(void),1>::Run+0x5 [c:\b\build\slave\win64-pgo\build\src\base\callback.h @ 388] 03 0000002a`405ed770 00007ffb`2c788f13 chrome_7ffb2b770000!base::Timer::RunScheduledTask+0xde [c:\b\build\slave\win64-pgo\build\src\base\timer\timer.cc @ 213] 04 (Inline Function) --------`-------- chrome_7ffb2b770000!base::Callback<void __cdecl(void),1>::Run+0x7 [c:\b\build\slave\win64-pgo\build\src\base\callback.h @ 388] 05 0000002a`405ed7d0 00007ffb`2c71aaf8 chrome_7ffb2b770000!base::debug::TaskAnnotator::RunTask+0x193 [c:\b\build\slave\win64-pgo\build\src\base\debug\task_annotator.cc @ 56] 06 0000002a`405ed8d0 00007ffb`2c71ae12 chrome_7ffb2b770000!base::MessageLoop::RunTask+0x468 [c:\b\build\slave\win64-pgo\build\src\base\message_loop\message_loop.cc @ 489] 07 0000002a`405eebf0 00007ffb`2c71bce4 chrome_7ffb2b770000!base::MessageLoop::DeferOrRunPendingTask+0x52 [c:\b\build\slave\win64-pgo\build\src\base\message_loop\message_loop.cc @ 500] 08 0000002a`405eec20 00007ffb`2c789822 chrome_7ffb2b770000!base::MessageLoop::DoDelayedWork+0x1b4 [c:\b\build\slave\win64-pgo\build\src\base\message_loop\message_loop.cc @ 660] 09 0000002a`405eecf0 00007ffb`2c7894d4 chrome_7ffb2b770000!base::MessagePumpForUI::DoRunLoop+0x92 [c:\b\build\slave\win64-pgo\build\src\base\message_loop\message_pump_win.cc @ 266] 0a 0000002a`405eed60 00007ffb`2c76a48d chrome_7ffb2b770000!base::MessagePumpWin::Run+0x54 [c:\b\build\slave\win64-pgo\build\src\base\message_loop\message_pump_win.cc @ 142] 0b (Inline Function) --------`-------- chrome_7ffb2b770000!base::MessageLoop::RunHandler+0x15 [c:\b\build\slave\win64-pgo\build\src\base\message_loop\message_loop.cc @ 451] 0c 0000002a`405eedb0 00007ffb`2c694330 chrome_7ffb2b770000!base::RunLoop::Run+0xed [c:\b\build\slave\win64-pgo\build\src\base\run_loop.cc @ 36] 0d 0000002a`405eee00 00007ffb`2b9e37ed chrome_7ffb2b770000!ChromeBrowserMainParts::MainMessageLoopRun+0x138 [c:\b\build\slave\win64-pgo\build\src\chrome\browser\chrome_browser_main.cc @ 2132] 0e (Inline Function) --------`-------- chrome_7ffb2b770000!content::BrowserMainLoop::RunMainMessageLoopParts+0x5f [c:\b\build\slave\win64-pgo\build\src\content\browser\browser_main_loop.cc @ 956] 0f 0000002a`405eee80 00007ffb`2b9dc41f chrome_7ffb2b770000!content::BrowserMainRunnerImpl::Run+0x71 [c:\b\build\slave\win64-pgo\build\src\content\browser\browser_main_runner.cc @ 155] 10 0000002a`405eeed0 00007ffb`2c640053 chrome_7ffb2b770000!content::BrowserMain+0xef [c:\b\build\slave\win64-pgo\build\src\content\browser\browser_main.cc @ 46] 11 (Inline Function) --------`-------- chrome_7ffb2b770000!content::RunNamedProcessTypeMain+0x160 [c:\b\build\slave\win64-pgo\build\src\content\app\content_main_runner.cc @ 418] 12 0000002a`405eef20 00007ffb`2b853897 chrome_7ffb2b770000!content::ContentMainRunnerImpl::Run+0x1ff [c:\b\build\slave\win64-pgo\build\src\content\app\content_main_runner.cc @ 786] 13 (Inline Function) --------`-------- chrome_7ffb2b770000!content::ContentMain+0x81 [c:\b\build\slave\win64-pgo\build\src\content\app\content_main.cc @ 20] 14 0000002a`405ef0d0 00007ff6`84fc72ff chrome_7ffb2b770000!ChromeMain+0x233 [c:\b\build\slave\win64-pgo\build\src\chrome\app\chrome_main.cc @ 85] 15 0000002a`405ef190 00007ff6`84fc2523 chrome!MainDllLoader::Launch+0x3c3 [c:\b\build\slave\win64-pgo\build\src\chrome\app\main_dll_loader_win.cc @ 183] 16 0000002a`405ef2e0 00007ff6`85049d7a chrome!wWinMain+0x457 [c:\b\build\slave\win64-pgo\build\src\chrome\app\chrome_exe_main_win.cc @ 253] 17 (Inline Function) --------`-------- chrome!invoke_main+0x21 [f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl @ 113] 18 0000002a`405ef750 00007ffb`54b78102 chrome!__scrt_common_main_seh+0x11e [f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl @ 255] 19 0000002a`405ef790 00007ffb`5509c5b4 KERNEL32!BaseThreadInitThunk+0x22 1a 0000002a`405ef7c0 00000000`00000000 ntdll!RtlUserThreadStart+0x34 Surely this is not working as intended. This has been verified on both Win7 and Win10, using the current stable build of Chrome 54.0.2840.99.
,
Nov 23 2016
This looks to be by design, but is pretty sloppy. ToolTipController has a repeating 500ms timer that simply calls ToolTipController::TooltipTimerFired, which forwards to TooltipWin::UpdateIfRequired. From what I can see TooltipController is cross-platform and generic, so this incurs a wakeup every 500 ms on every platform that uses Aura (all of them?)
,
Nov 23 2016
Bruce, this seems like it'd be interesting to you.
,
Nov 23 2016
Yep, I'll happily take it. It's a slow spin, but a spin regardless.
,
Jan 5 2017
I will grab it.
,
Jan 6 2017
,
Jan 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6b8312a4903fc6965a64a3835524f1a8f384f26b commit 6b8312a4903fc6965a64a3835524f1a8f384f26b Author: chengx <chengx@chromium.org> Date: Fri Jan 13 16:56:30 2017 Remove unnecessary spin in ToolTipController The motivation for the change is to reduce idle wakeups and therefore reduce idle power. ToolTipController has a repeating 500ms timer that tries to update itself periodically, while most of the time there is no change at all. This CL removes this spin but keeps the same functionality by 1) upgrading OnMouseEvent() in TooltipController; 2) overriding OnWindowPropertyChanged() in TooltipController; 3) overriding OnCursorVisibilityChanged() in TooltipController. After the change, tooltip is updated on demand via observer pattern, rather than unnecessary periodic checking. BUG= 668198 Review-Url: https://codereview.chromium.org/2615993002 Cr-Commit-Position: refs/heads/master@{#443584} [modify] https://crrev.com/6b8312a4903fc6965a64a3835524f1a8f384f26b/ash/tooltips/tooltip_controller_unittest.cc [modify] https://crrev.com/6b8312a4903fc6965a64a3835524f1a8f384f26b/ui/views/corewm/tooltip_controller.cc [modify] https://crrev.com/6b8312a4903fc6965a64a3835524f1a8f384f26b/ui/views/corewm/tooltip_controller.h [modify] https://crrev.com/6b8312a4903fc6965a64a3835524f1a8f384f26b/ui/views/corewm/tooltip_controller_test_helper.cc [modify] https://crrev.com/6b8312a4903fc6965a64a3835524f1a8f384f26b/ui/views/corewm/tooltip_controller_test_helper.h [modify] https://crrev.com/6b8312a4903fc6965a64a3835524f1a8f384f26b/ui/views/corewm/tooltip_controller_unittest.cc [modify] https://crrev.com/6b8312a4903fc6965a64a3835524f1a8f384f26b/ui/wm/public/tooltip_client.cc [modify] https://crrev.com/6b8312a4903fc6965a64a3835524f1a8f384f26b/ui/wm/public/tooltip_client.h
,
Jan 13 2017
,
Jan 18 2017
could this have caused Issue 682141 ?
,
Jan 18 2017
Re: comment #9 Yes, it should be the cause of Issue 682141 . I have reverted the CL and will land a fixed version soon.
,
Jan 19 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/36d50e667f1639241bb8a0798086c764e350047e commit 36d50e667f1639241bb8a0798086c764e350047e Author: chengx <chengx@chromium.org> Date: Thu Jan 19 00:55:04 2017 Revert of Remove unnecessary spin in ToolTipController (patchset #7 id:520001 of https://codereview.chromium.org/2615993002/ ) Reason for revert: Caused the regression 682141. Original issue's description: > Remove unnecessary spin in ToolTipController > > The motivation for the change is to reduce idle wakeups and therefore > reduce idle power. > > ToolTipController has a repeating 500ms timer that tries to update > itself periodically, while most of the time there is no change at all. > > This CL removes this spin but keeps the same functionality by > 1) upgrading OnMouseEvent() in TooltipController; > 2) overriding OnWindowPropertyChanged() in TooltipController; > 3) overriding OnCursorVisibilityChanged() in TooltipController. > > After the change, tooltip is updated on demand via observer pattern, > rather than unnecessary periodic checking. > > BUG= 668198 > > Review-Url: https://codereview.chromium.org/2615993002 > Cr-Commit-Position: refs/heads/master@{#443584} > Committed: https://chromium.googlesource.com/chromium/src/+/6b8312a4903fc6965a64a3835524f1a8f384f26b TBR=brucedawson@chromium.org,sky@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG= 668198 Review-Url: https://codereview.chromium.org/2643973002 Cr-Commit-Position: refs/heads/master@{#444576} [modify] https://crrev.com/36d50e667f1639241bb8a0798086c764e350047e/ash/tooltips/tooltip_controller_unittest.cc [modify] https://crrev.com/36d50e667f1639241bb8a0798086c764e350047e/ui/views/corewm/tooltip_controller.cc [modify] https://crrev.com/36d50e667f1639241bb8a0798086c764e350047e/ui/views/corewm/tooltip_controller.h [modify] https://crrev.com/36d50e667f1639241bb8a0798086c764e350047e/ui/views/corewm/tooltip_controller_test_helper.cc [modify] https://crrev.com/36d50e667f1639241bb8a0798086c764e350047e/ui/views/corewm/tooltip_controller_test_helper.h [modify] https://crrev.com/36d50e667f1639241bb8a0798086c764e350047e/ui/views/corewm/tooltip_controller_unittest.cc [modify] https://crrev.com/36d50e667f1639241bb8a0798086c764e350047e/ui/wm/public/tooltip_client.cc [modify] https://crrev.com/36d50e667f1639241bb8a0798086c764e350047e/ui/wm/public/tooltip_client.h
,
Jan 25 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/122326d5ae2407e11a8b7353fd60dcffba5066fa commit 122326d5ae2407e11a8b7353fd60dcffba5066fa Author: chengx <chengx@chromium.org> Date: Wed Jan 25 01:34:01 2017 Remove infinite spin, make tooltip delay consistent The main motivation for the change is to reduce idle wakeups and therefore reduce idle power. Another motivation is to make the UI better, as currently tooltip shows up with random delay time. ToolTipController has a repeating timer firing 4 times per second that tries to update itself periodically, while most of the time there is no change at all. This CL removes this spin but keeps the same functionality by 1) upgrading OnMouseEvent() in TooltipController; 2) overriding OnWindowPropertyChanged() in TooltipController; 3) overriding OnCursorVisibilityChanged() in TooltipController. After the change, tooltip is updated on demand via observer pattern, rather than unnecessary periodic checking. On UI side, current production version hides tooltip immediately, but shows it up with a delay of a random few hundreds of ms. This is because of the RepeatingTimer implementation. One one hand, the delayed appearance of tooltip is preferred. On the other hand, the random delay time from instant to a few hundreds of ms doesn't look good. We improve this by introducing a timer to delay the tooltip's appearance by 500 ms. This makes the intended delay consistent for all tooltips. BUG= 668198 , 684877 Review-Url: https://codereview.chromium.org/2615993002 Cr-Commit-Position: refs/heads/master@{#443584} Committed: https://chromium.googlesource.com/chromium/src/+/6b8312a4903fc6965a64a3835524f1a8f384f26b patch from issue 2615993002 at patchset 520001 (http://crrev.com/2615993002#ps520001) Review-Url: https://codereview.chromium.org/2652833002 Cr-Commit-Position: refs/heads/master@{#445902} [modify] https://crrev.com/122326d5ae2407e11a8b7353fd60dcffba5066fa/ash/tooltips/tooltip_controller_unittest.cc [modify] https://crrev.com/122326d5ae2407e11a8b7353fd60dcffba5066fa/ui/views/corewm/tooltip_controller.cc [modify] https://crrev.com/122326d5ae2407e11a8b7353fd60dcffba5066fa/ui/views/corewm/tooltip_controller.h [modify] https://crrev.com/122326d5ae2407e11a8b7353fd60dcffba5066fa/ui/views/corewm/tooltip_controller_test_helper.cc [modify] https://crrev.com/122326d5ae2407e11a8b7353fd60dcffba5066fa/ui/views/corewm/tooltip_controller_test_helper.h [modify] https://crrev.com/122326d5ae2407e11a8b7353fd60dcffba5066fa/ui/views/corewm/tooltip_controller_unittest.cc [modify] https://crrev.com/122326d5ae2407e11a8b7353fd60dcffba5066fa/ui/wm/public/tooltip_client.cc [modify] https://crrev.com/122326d5ae2407e11a8b7353fd60dcffba5066fa/ui/wm/public/tooltip_client.h
,
Jan 25 2017
|
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by chrisha@chromium.org
, Nov 23 2016