Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
132473007:
aura: Destroy the compositor before destroying the backing window. (Closed)
|
Created:
6 years, 11 months ago by danakj Modified:
6 years, 11 months ago CC:
chromium-reviews, tfarina, ben+views_chromium.org, piman Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionaura: Destroy the compositor before destroying the backing window.
This was causing linux_aura single-process browser tests to crash since
they don't set up a default X error handler. When the compositor is
shutting down it does a finish and this might try to swap to the X
window.
Also change the windows shutdown ordering to match.
R=ben, enne
BUG=270918
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245028
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245586
Patch Set 1 #
Total comments: 4
Patch Set 2 : destroy-window: comment #Patch Set 3 : destroy-window: windows #
Total comments: 5
Patch Set 4 : destroy-window: windows2 #Patch Set 5 : destroy-window: windows3 #Patch Set 6 : destroy-window: compile #
Total comments: 2
Patch Set 7 : destroy-window: end #Patch Set 8 : destroy-window: #
Messages
Total messages: 28 (0 generated)
lgtm. Sorry for not being careful enough with where I left that DestroyCompositor.
ben@chromium.org: OWNERS review
ben: ping for review
https://codereview.chromium.org/132473007/diff/1/ui/views/widget/desktop_aura... File ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc (right): https://codereview.chromium.org/132473007/diff/1/ui/views/widget/desktop_aura... ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc:323: DestroyCompositor(); So, I wonder if we should do this on windows too. I find it confusing when one platform has an ordering dependency/difference like this and another does not. If so, the appropriate place to call this in DesktopRootWindowHostWin is in DRWHW::HandleDestroying(). Then, WindowTreeHost::DestroyCompositor can DCHECK that GetAcceleratedWidget() returns non-NULL (though I think we may also want to NULL out hwnd_ in HandleDestroyed()).
https://codereview.chromium.org/132473007/diff/1/ui/views/widget/desktop_aura... File ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc (right): https://codereview.chromium.org/132473007/diff/1/ui/views/widget/desktop_aura... ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc:322: // Destroy the compositor before destroying the |xwindow_|. nit: documenting what the code does isn't helpful for future generations. Instead document why the code is the way it is.
https://codereview.chromium.org/132473007/diff/1/ui/views/widget/desktop_aura... File ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc (right): https://codereview.chromium.org/132473007/diff/1/ui/views/widget/desktop_aura... ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc:322: // Destroy the compositor before destroying the |xwindow_|. On 2014/01/13 23:51:29, sky wrote: > nit: documenting what the code does isn't helpful for future generations. > Instead document why the code is the way it is. Good point, done. https://codereview.chromium.org/132473007/diff/1/ui/views/widget/desktop_aura... ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc:323: DestroyCompositor(); On 2014/01/13 23:11:47, Ben Goodger (Google) wrote: > So, I wonder if we should do this on windows too. I find it confusing when one > platform has an ordering dependency/difference like this and another does not. It seems that when the compositor shuts down and tries to swap on windows, and the window is gone, that it does not cause the test to crash. > If so, the appropriate place to call this in DesktopRootWindowHostWin is in > DRWHW::HandleDestroying(). Then, WindowTreeHost::DestroyCompositor can DCHECK > that GetAcceleratedWidget() returns non-NULL (though I think we may also want to > NULL out hwnd_ in HandleDestroyed()). It's not at all obvious to me how to go about destroying the hwnd (I don't see any destruction of the hwnd_ in the window_impl.cc class?
On 2014/01/13 23:54:38, danakj wrote: > https://codereview.chromium.org/132473007/diff/1/ui/views/widget/desktop_aura... > File ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc (right): > > https://codereview.chromium.org/132473007/diff/1/ui/views/widget/desktop_aura... > ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc:322: // Destroy the > compositor before destroying the |xwindow_|. > On 2014/01/13 23:51:29, sky wrote: > > nit: documenting what the code does isn't helpful for future generations. > > Instead document why the code is the way it is. > > Good point, done. > > https://codereview.chromium.org/132473007/diff/1/ui/views/widget/desktop_aura... > ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc:323: > DestroyCompositor(); > On 2014/01/13 23:11:47, Ben Goodger (Google) wrote: > > So, I wonder if we should do this on windows too. I find it confusing when one > > platform has an ordering dependency/difference like this and another does not. > > It seems that when the compositor shuts down and tries to swap on windows, and > the window is gone, that it does not cause the test to crash. > > > If so, the appropriate place to call this in DesktopRootWindowHostWin is in > > DRWHW::HandleDestroying(). Then, WindowTreeHost::DestroyCompositor can DCHECK > > that GetAcceleratedWidget() returns non-NULL (though I think we may also want > to > > NULL out hwnd_ in HandleDestroyed()). > > It's not at all obvious to me how to go about destroying the hwnd (I don't see > any destruction of the hwnd_ in the window_impl.cc class? Ah.. I think I see what you meant now, and I found the DestroyWindow. How does this look?
Lots of green bots. ping for review please.
https://codereview.chromium.org/132473007/diff/240001/ui/gfx/win/window_impl.cc File ui/gfx/win/window_impl.cc (right): https://codereview.chromium.org/132473007/diff/240001/ui/gfx/win/window_impl.... ui/gfx/win/window_impl.cc:222: void WindowImpl::Destroy() { you actually can't rely on people calling this function. for example, Windows might destroy your window, and just send you a WM_DESTROY to let you know it did. WM_DESTROY - "Windows is destroying your window, and the HWND is still valid. Do some cleanup that needs the HWND to be alive. There is no way out." WM_NCDESTROY - "Windows destroyed your window. The HWND is now bogus. Goodbye." https://codereview.chromium.org/132473007/diff/240001/ui/gfx/win/window_impl.... ui/gfx/win/window_impl.cc:224: hwnd_ = NULL; ... and so this should be set in a handler for WM_NCDESTROY in WndProc below. https://codereview.chromium.org/132473007/diff/240001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/132473007/diff/240001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:455: Destroy(); this line should remain unchanged with the changes I recommended to WindowImpl.
https://codereview.chromium.org/132473007/diff/240001/ui/gfx/win/window_impl.cc File ui/gfx/win/window_impl.cc (right): https://codereview.chromium.org/132473007/diff/240001/ui/gfx/win/window_impl.... ui/gfx/win/window_impl.cc:224: hwnd_ = NULL; On 2014/01/14 21:48:03, Ben Goodger (Google) wrote: > ... and so this should be set in a handler for WM_NCDESTROY in WndProc below. Oh I see. The handler for WM_NCDESTROY appears to be in hwnd_message_handler.cc, so from there I called SetDestroyed(), a method on this class that sets hwnd_ = NULL. Then I DCHECK that the hwnd is NULL out in the DesktopWindowTreeHostWin::HandleDestroyed. https://codereview.chromium.org/132473007/diff/240001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/132473007/diff/240001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:455: Destroy(); On 2014/01/14 21:48:03, Ben Goodger (Google) wrote: > this line should remain unchanged with the changes I recommended to WindowImpl. Done.
On 2014/01/14 21:58:41, danakj wrote: > https://codereview.chromium.org/132473007/diff/240001/ui/gfx/win/window_impl.cc > File ui/gfx/win/window_impl.cc (right): > > https://codereview.chromium.org/132473007/diff/240001/ui/gfx/win/window_impl.... > ui/gfx/win/window_impl.cc:224: hwnd_ = NULL; > On 2014/01/14 21:48:03, Ben Goodger (Google) wrote: > > ... and so this should be set in a handler for WM_NCDESTROY in WndProc below. > > Oh I see. The handler for WM_NCDESTROY appears to be in hwnd_message_handler.cc, > so from there I called SetDestroyed(), a method on this class that sets hwnd_ = > NULL. Then I DCHECK that the hwnd is NULL out in the > DesktopWindowTreeHostWin::HandleDestroyed. > > https://codereview.chromium.org/132473007/diff/240001/ui/views/win/hwnd_messa... > File ui/views/win/hwnd_message_handler.cc (right): > > https://codereview.chromium.org/132473007/diff/240001/ui/views/win/hwnd_messa... > ui/views/win/hwnd_message_handler.cc:455: Destroy(); > On 2014/01/14 21:48:03, Ben Goodger (Google) wrote: > > this line should remain unchanged with the changes I recommended to > WindowImpl. > > Done. your upload seems to have gone wrong.
On 2014/01/15 05:17:10, Ben Goodger (Google) wrote: > On 2014/01/14 21:58:41, danakj wrote: > > > https://codereview.chromium.org/132473007/diff/240001/ui/gfx/win/window_impl.cc > > File ui/gfx/win/window_impl.cc (right): > > > > > https://codereview.chromium.org/132473007/diff/240001/ui/gfx/win/window_impl.... > > ui/gfx/win/window_impl.cc:224: hwnd_ = NULL; > > On 2014/01/14 21:48:03, Ben Goodger (Google) wrote: > > > ... and so this should be set in a handler for WM_NCDESTROY in WndProc > below. > > > > Oh I see. The handler for WM_NCDESTROY appears to be in > hwnd_message_handler.cc, > > so from there I called SetDestroyed(), a method on this class that sets hwnd_ > = > > NULL. Then I DCHECK that the hwnd is NULL out in the > > DesktopWindowTreeHostWin::HandleDestroyed. > > > > > https://codereview.chromium.org/132473007/diff/240001/ui/views/win/hwnd_messa... > > File ui/views/win/hwnd_message_handler.cc (right): > > > > > https://codereview.chromium.org/132473007/diff/240001/ui/views/win/hwnd_messa... > > ui/views/win/hwnd_message_handler.cc:455: Destroy(); > > On 2014/01/14 21:48:03, Ben Goodger (Google) wrote: > > > this line should remain unchanged with the changes I recommended to > > WindowImpl. > > > > Done. > > your upload seems to have gone wrong. DCHECK_EQ(NULL, hwnd()) was comparing an int and an HWND which did not compile. I've added a static_cast to it.
https://codereview.chromium.org/132473007/diff/540001/ui/gfx/win/window_impl.cc File ui/gfx/win/window_impl.cc (right): https://codereview.chromium.org/132473007/diff/540001/ui/gfx/win/window_impl.... ui/gfx/win/window_impl.cc:222: void WindowImpl::SetDestroyed() { hwnd_ = NULL; } sorry to continue to nitpick this, but could you do this by handling WM_NCDESTROY at the end of WndProc() below (after it calls OnWndProc)... this makes sure that hwnd_ is reset properly for all users of this class, rather than just those that call SetDestroyed(). BTW I made one assertion that was incorrect which I will correct now for the record: When WM_NCDESTROY is sent your hwnd_ is not invalid, but it probably is shortly after you return. MSDN defines WM_DESTROY as "before child windows are destroyed" and WM_NCDESTROY as "after child windows are destroyed."
https://codereview.chromium.org/132473007/diff/540001/ui/gfx/win/window_impl.cc File ui/gfx/win/window_impl.cc (right): https://codereview.chromium.org/132473007/diff/540001/ui/gfx/win/window_impl.... ui/gfx/win/window_impl.cc:222: void WindowImpl::SetDestroyed() { hwnd_ = NULL; } On 2014/01/15 19:16:20, Ben Goodger (Google) wrote: > sorry to continue to nitpick this, but could you do this by handling > WM_NCDESTROY at the end of WndProc() below (after it calls OnWndProc)... this > makes sure that hwnd_ is reset properly for all users of this class, rather than > just those that call SetDestroyed(). > > BTW I made one assertion that was incorrect which I will correct now for the > record: When WM_NCDESTROY is sent your hwnd_ is not invalid, but it probably is > shortly after you return. MSDN defines WM_DESTROY as "before child windows are > destroyed" and WM_NCDESTROY as "after child windows are destroyed." I see, OK done. I set hwnd_ to NULL after calling window->OnWndProc, since the OnWndProc uses the hwnd_. This means I can't DCHECK in DesktopWindowTreeHostWin::HandleDestroyed anymore, so I've removed that.
lgtm, thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/danakj@chromium.org/132473007/600002
Message was sent while issue was closed.
Change committed as 245028
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/137893013/ by danakj@chromium.org. The reason for reverting is: http://code.google.com/p/chromium/issues/detail?id=335108.
ben: PTAL The WindowImpl can be destroyed it seems by the ProcessWindowMessage or the DefWindowProc calls. So clear the hwnd_ before that instead of after to avoid dereffing a freed pointer.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/danakj@chromium.org/132473007/830001
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/danakj@chromium.org/132473007/830001
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/danakj@chromium.org/132473007/830001
Message was sent while issue was closed.
Change committed as 245586 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
