New issue
Advanced search Search tips

Issue 899112 link

Starred by 1 user
Status: Fixed
Owner:
robliao@chromium.org
Closed: Oct 27
Cc:
malaykeshav@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

Closing a Window with an AnimatedImageView Causes a Use After Free During AnimatedImageView::RemovedFromWidget

Project Member Reported by robliao@chromium.org, Oct 26 
When a Window is getting destroyed, the compositor is first deleted during views::DesktopWindowTreeHostWin::HandleDestroying, and then view is destroyed at views::DesktopWindowTreeHostWin::HandleDestroyed, triggering a use after free in 
views::AnimatedImageView::RemovedFromWidget

Use after free at:
0:000> kn20
 # ChildEBP RetAddr  
00 00b9d6b8 5afbcb2d compositor!std::vector<base::internal::UncheckedObserverAdapter,std::allocator<base::internal::UncheckedObserverAdapter> >::end+0x37 [D:\src\third_party\depot_tools\win_toolchain\vs_files\3bc0ec615cf20ee342f3bc29bc991b5ad66d8d2c\VC\Tools\MSVC\14.14.26428\include\vector @ 1703] 
01 00b9d724 5afbcac0 compositor!base::ObserverList<ui::CompositorAnimationObserver,0,1,base::internal::UncheckedObserverAdapter>::HasObserver+0x5d [D:\src\base\observer_list.h @ 302] 
02 00b9d738 5f51f01e compositor!ui::Compositor::HasAnimationObserver+0x20 [D:\src\ui\compositor\compositor.cc @ 582] 
03 00b9d74c 5f6620bc views!views::AnimatedImageView::RemovedFromWidget+0x3e [D:\src\ui\views\controls\animated_image_view.cc @ 123] 
04 00b9d7d8 5f662019 views!views::View::PropagateRemoveNotifications+0x17c [D:\src\ui\views\view.cc @ 2072] 
05 00b9d864 5f662019 views!views::View::PropagateRemoveNotifications+0xd9 [D:\src\ui\views\view.cc @ 2061] 
06 00b9d8f0 5f662019 views!views::View::PropagateRemoveNotifications+0xd9 [D:\src\ui\views\view.cc @ 2061] 
07 00b9d97c 5f662019 views!views::View::PropagateRemoveNotifications+0xd9 [D:\src\ui\views\view.cc @ 2061] 
08 00b9da08 5f662019 views!views::View::PropagateRemoveNotifications+0xd9 [D:\src\ui\views\view.cc @ 2061] 
09 00b9da94 5f6541e6 views!views::View::PropagateRemoveNotifications+0xd9 [D:\src\ui\views\view.cc @ 2061] 
0a 00b9dd10 5f652b03 views!views::View::DoRemoveChildView+0x2f6 [D:\src\ui\views\view.cc @ 2035] 
0b 00b9dd3c 5f64d1ee views!views::View::RemoveChildView+0x43 [D:\src\ui\views\view.cc @ 296] 
0c 00b9dde0 5f67390b views!views::View::~View+0x7e [D:\src\ui\views\view.cc @ 155] 
0d 00b9ddf0 715ac6fb views!views::WidgetDelegateView::~WidgetDelegateView+0x1b [D:\src\ui\views\widget\widget_delegate.cc @ 198] 
0e 00b9de18 715ab615 views_examples_lib!views::examples::ExamplesWindowContents::~ExamplesWindowContents+0x9b [D:\src\ui\views\examples\examples_window.cc @ 174] 
0f 00b9de38 5f673952 views_examples_lib!views::examples::ExamplesWindowContents::~ExamplesWindowContents+0x25 [D:\src\ui\views\examples\examples_window.cc @ 174] 
10 00b9de54 5f670ed7 views!views::WidgetDelegateView::DeleteDelegate+0x32 [D:\src\ui\views\widget\widget_delegate.cc @ 201] 
11 00b9dea4 5f6fbc7b views!views::Widget::OnNativeWidgetDestroyed+0xb7 [D:\src\ui\views\widget\widget.cc @ 1111] 
12 00b9df28 5f70d36d views!views::DesktopNativeWidgetAura::OnHostClosed+0x35b [D:\src\ui\views\widget\desktop_aura\desktop_native_widget_aura.cc @ 331] 
13 00b9df38 5f68ca85 views!views::DesktopWindowTreeHostWin::HandleDestroyed+0x1d [D:\src\ui\views\widget\desktop_aura\desktop_window_tree_host_win.cc @ 821] 
14 00b9dfa8 688ba668 views!views::HWNDMessageHandler::OnWndProc+0x1f5 [D:\src\ui\views\win\hwnd_message_handler.cc @ 996] 
15 00b9e0b8 688b9f67 gfx!gfx::WindowImpl::WndProc+0x178 [D:\src\ui\gfx\win\window_impl.cc @ 303] 


Compositor Destroyed at 
0f22ab22 verifier!AVrfDebugPageHeapFree+0x000000c2
    77405348 ntdll!RtlDebugFreeHeap+0x0000003c
    7735ad86 ntdll!RtlpFreeHeap+0x000000d6
    7735ac3d ntdll!RtlFreeHeap+0x000007cd
    0ff5ad3b vrfcore!VfCoreRtlFreeHeap+0x0000002b
    100083d6 vfbasics!AVrfpRtlFreeHeap+0x00000116
    6c64dcf7 ucrtbased!_free_base+0x00000027 [minkernel\crts\ucrt\src\appcrt\heap\free_base.cpp @ 105]
    6c64af0b ucrtbased!free_dbg_nolock+0x0000047b [minkernel\crts\ucrt\src\appcrt\heap\debug_heap.cpp @ 1001]
    6c64d6dc ucrtbased!_free_dbg+0x0000007c [minkernel\crts\ucrt\src\appcrt\heap\debug_heap.cpp @ 1030]
    5b02571e compositor!operator delete+0x0000000e [f:\dd\vctools\crt\vcstartup\src\heap\delete_scalar.cpp @ 34]
    5afe1b5c compositor!ui::Compositor::~Compositor+0x0000003c [D:\src\ui\compositor\compositor.cc @ 266]
    5effed45 aura!std::default_delete<ui::Compositor>::operator()+0x00000035 [D:\src\third_party\depot_tools\win_toolchain\vs_files\3bc0ec615cf20ee342f3bc29bc991b5ad66d8d2c\VC\Tools\MSVC\14.14.26428\include\memory @ 2055]
    5ef73017 aura!std::unique_ptr<ui::Compositor,std::default_delete<ui::Compositor> >::reset+0x00000057 [D:\src\third_party\depot_tools\win_toolchain\vs_files\3bc0ec615cf20ee342f3bc29bc991b5ad66d8d2c\VC\Tools\MSVC\14.14.26428\include\memory @ 2304]
    5ef58d26 aura!aura::WindowTreeHost::DestroyCompositor+0x00000066 [D:\src\ui\aura\window_tree_host.cc @ 312]
    5f70d343 views!views::DesktopWindowTreeHostWin::HandleDestroying+0x00000053 [D:\src\ui\views\widget\desktop_aura\desktop_window_tree_host_win.cc @ 817]
    5f683688 views!views::HWNDMessageHandler::OnDestroy+0x00000068 [D:\src\ui\views\win\hwnd_message_handler.cc @ 1613]
    5f6800c9 views!views::HWNDMessageHandler::_ProcessWindowMessage+0x00001b49 [D:\src\ui\views\win\hwnd_message_handler.h @ 397]
    5f68c984 views!views::HWNDMessageHandler::OnWndProc+0x000000f4 [D:\src\ui\views\win\hwnd_message_handler.cc @ 976]
    688ba668 gfx!gfx::WindowImpl::WndProc+0x00000178 [D:\src\ui\gfx\win\window_impl.cc @ 303]

Comment 1 by robliao@chromium.org, Oct 26

Cc: malaykeshav@chromium.org
Owner: robliao@chromium.org
Working on a quick fix.

Comment 2 by malaykeshav@chromium.org, Oct 26 

I would assume there is a call to OnCompositingShuttingDown() if the compositor is shutting down and destroyed if the compositor is deleted.

https://cs.chromium.org/chromium/src/ui/views/controls/animated_image_view.cc?dr=CSs&g=0&l=134

Comment 3 by robliao@chromium.org, Oct 26 

If we never call Play(), we never call AddAnimationObserver(), and as a result, we won't get the OnCompositingShuttingDown() call.

Comment 4 by malaykeshav@chromium.org, Oct 26 

That would result in the failure of this check for compositor and it would never enter the conditional where the failure occurs:
https://cs.chromium.org/chromium/src/ui/views/controls/animated_image_view.cc?dr=CSs&g=0&l=121

Comment 5 by malaykeshav@chromium.org, Oct 26 

Ah nvm

Comment 6 by malaykeshav@chromium.org, Oct 26 

We would need to add the view as a https://cs.chromium.org/chromium/src/ui/compositor/compositor_observer.h as soon as it is added to the widget. Did you have a solution in mind?
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 26 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/19a2837f0fb81d73e70e596b3978d415c219dbf7

commit 19a2837f0fb81d73e70e596b3978d415c219dbf7
Author: Robert Liao <robliao@chromium.org>
Date: Fri Oct 26 23:40:13 2018

Lazily Obtain the Compositor

If we set |compositor_| during AddedToWidget() and not call Play(), the
compositor could shutdown and we would never clear |compositor_| since
we're not registered as an AnimationObserver and will miss
OnCompositingShuttingDown().

BUG= 899112 

Change-Id: If1dbe4ae9db80d55d3d5db6d939da6008bf97160
Reviewed-on: https://chromium-review.googlesource.com/c/1300694
Commit-Queue: Robert Liao <robliao@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Reviewed-by: Malay Keshav <malaykeshav@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603253}
[modify] https://crrev.com/19a2837f0fb81d73e70e596b3978d415c219dbf7/ui/views/controls/animated_image_view.cc
[modify] https://crrev.com/19a2837f0fb81d73e70e596b3978d415c219dbf7/ui/views/controls/animated_image_view.h

Comment 8 by robliao@chromium.org, Oct 27

Status: Fixed (was: Assigned)
►

Sign in to add a comment