Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2764433002:
Fix destruction order in mus_demo (Closed)
DescriptionFix destruction order in mus_demo
Problem:
Unique pointers are deleted from the bottom to the upwards as they are
defined in a class definition. Before this CL, WindowTreeClient was
destroyed after WNState had its destructor called, which led to segfault.
Solution:
WindowTreeClient must be deleted before Env and WNState, otherwise signal 11 occurs.
It happens because Env and WNState are singletons, which are owned by
MusDemo. Once WindowTreeClient goes into its destructor, it uses them to
remove itself as an observer.
This fix is mainly aimed for
https://build.chromium.org/p/chromium.fyi/builders/Ozone%20Linux
Call stack-
Received signal 11 SEGV_MAPERR 000000000000
#0 0x7f501a7b83fb base::debug::StackTrace::StackTrace()
#1 0x7f501a7b6a8c base::debug::StackTrace::StackTrace()
#2 0x7f501a7b7f0f base::debug::(anonymous namespace)::StackDumpSignalHandler()
#3 0x7f501ac3e390 <unknown>
#4 0x7f50179b46fb aura::WindowTreeClient::~WindowTreeClient()
#5 0x7f50179b4d69 aura::WindowTreeClient::~WindowTreeClient()
#6 0x000000431f9f std::default_delete<>::operator()()
#7 0x00000043198c std::unique_ptr<>::reset()
#8 0x000000430b59 std::unique_ptr<>::~unique_ptr()
#9 0x00000042f85c ui::demo::MusDemo::~MusDemo()
#10 0x00000043852f ui::demo::MusDemoExternal::~MusDemoExternal()
#11 0x000000438579 ui::demo::MusDemoExternal::~MusDemoExternal()
#12 0x000000431f9f std::default_delete<>::operator()()
#13 0x00000063080c std::unique_ptr<>::reset()
#14 0x00000062ece9 std::unique_ptr<>::~unique_ptr()
#15 0x00000062db7b service_manager::ServiceContext::~ServiceContext()
#16 0x00000062dbc9 service_manager::ServiceContext::~ServiceContext()
BUG=666958
Review-Url: https://codereview.chromium.org/2764433002
Cr-Commit-Position: refs/heads/master@{#458388}
Committed: https://chromium.googlesource.com/chromium/src/+/9ca8fc91927b335661ab095f925b1337486e64d0
Patch Set 1 #
Total comments: 3
Patch Set 2 : add comment #
Total comments: 3
Patch Set 3 : add more extended comment. destroy env and wm_state explicitly #Messages
Total messages: 47 (30 generated)
The CQ bit was checked by msisov@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fix destruction order in mus_demo BUG= ========== to ========== Fix destruction order in mus_demo WindowTreeClient must be deleted before Env and WNState, otherwise signal 11 occurs. This fix is aimed for https://build.chromium.org/p/chromium.fyi/builders/Ozone%20Linux BUG= ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Fix destruction order in mus_demo WindowTreeClient must be deleted before Env and WNState, otherwise signal 11 occurs. This fix is aimed for https://build.chromium.org/p/chromium.fyi/builders/Ozone%20Linux BUG= ========== to ========== Fix destruction order in mus_demo WindowTreeClient must be deleted before Env and WNState, otherwise signal 11 occurs. This fix is mainly aimed for https://build.chromium.org/p/chromium.fyi/builders/Ozone%20Linux BUG= ==========
msisov@igalia.com changed reviewers: + fwang@igalia.com, kylechar@chromium.org, tonikitoo@igalia.com
ptal
informal l g t m https://codereview.chromium.org/2764433002/diff/1/services/ui/demo/mus_demo.cc File services/ui/demo/mus_demo.cc (right): https://codereview.chromium.org/2764433002/diff/1/services/ui/demo/mus_demo.c... services/ui/demo/mus_demo.cc:28: window_tree_client_.reset(); Maybe add a comment that the destruction order is important?
The CQ bit was checked by msisov@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by msisov@igalia.com
The CQ bit was checked by msisov@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2764433002/diff/1/services/ui/demo/mus_demo.cc File services/ui/demo/mus_demo.cc (right): https://codereview.chromium.org/2764433002/diff/1/services/ui/demo/mus_demo.c... services/ui/demo/mus_demo.cc:28: window_tree_client_.reset(); On 2017/03/20 11:45:39, fwang wrote: > Maybe add a comment that the destruction order is important? Yes, please add a comment explaining the order.
Can you please update the desc to link to bug 666958 and maybe explaining/pasting the segfault error? Also, IIUC it also happens on the ChromiumOS Ozone bot?
There is still another error in linux. XOpenDisplay returns nullptr and DCHECK happens. [19450:19450:0320/051016.340929:1138878197:FATAL:x11_event_source.cc(106)] Check failed: display_. #0 0x000000557a67 base::debug::StackTrace::StackTrace() #1 0x00000055ed3b logging::LogMessage::~LogMessage() #2 0x000000b07725 ui::X11EventSource::X11EventSource() #3 0x000000b0681d ui::X11EventSourceLibevent::X11EventSourceLibevent() #4 0x000000509142 ui::(anonymous namespace)::OzonePlatformX11::InitializeCommon() #5 0x000000508fc1 ui::(anonymous namespace)::OzonePlatformX11::InitializeUI() #6 0x0000004ba7fc ui::OzonePlatform::InitializeForUI() #7 0x000000410829 ui::Service::OnStart() Can it be due to the fact that bots are headless? https://codereview.chromium.org/2764433002/diff/1/services/ui/demo/mus_demo.cc File services/ui/demo/mus_demo.cc (right): https://codereview.chromium.org/2764433002/diff/1/services/ui/demo/mus_demo.c... services/ui/demo/mus_demo.cc:28: window_tree_client_.reset(); On 2017/03/20 13:27:42, kylechar wrote: > On 2017/03/20 11:45:39, fwang wrote: > > Maybe add a comment that the destruction order is important? > > Yes, please add a comment explaining the order. Done.
Description was changed from ========== Fix destruction order in mus_demo WindowTreeClient must be deleted before Env and WNState, otherwise signal 11 occurs. This fix is mainly aimed for https://build.chromium.org/p/chromium.fyi/builders/Ozone%20Linux BUG= ========== to ========== Fix destruction order in mus_demo WindowTreeClient must be deleted before Env and WNState, otherwise signal 11 occurs. This fix is mainly aimed for https://build.chromium.org/p/chromium.fyi/builders/Ozone%20Linux Call stack- Received signal 11 SEGV_MAPERR 000000000000 #0 0x7f501a7b83fb base::debug::StackTrace::StackTrace() #1 0x7f501a7b6a8c base::debug::StackTrace::StackTrace() #2 0x7f501a7b7f0f base::debug::(anonymous namespace)::StackDumpSignalHandler() #3 0x7f501ac3e390 <unknown> #4 0x7f50179b46fb aura::WindowTreeClient::~WindowTreeClient() #5 0x7f50179b4d69 aura::WindowTreeClient::~WindowTreeClient() #6 0x000000431f9f std::default_delete<>::operator()() #7 0x00000043198c std::unique_ptr<>::reset() #8 0x000000430b59 std::unique_ptr<>::~unique_ptr() #9 0x00000042f85c ui::demo::MusDemo::~MusDemo() #10 0x00000043852f ui::demo::MusDemoExternal::~MusDemoExternal() #11 0x000000438579 ui::demo::MusDemoExternal::~MusDemoExternal() #12 0x000000431f9f std::default_delete<>::operator()() #13 0x00000063080c std::unique_ptr<>::reset() #14 0x00000062ece9 std::unique_ptr<>::~unique_ptr() #15 0x00000062db7b service_manager::ServiceContext::~ServiceContext() #16 0x00000062dbc9 service_manager::ServiceContext::~ServiceContext() BUG= ==========
Description was changed from ========== Fix destruction order in mus_demo WindowTreeClient must be deleted before Env and WNState, otherwise signal 11 occurs. This fix is mainly aimed for https://build.chromium.org/p/chromium.fyi/builders/Ozone%20Linux Call stack- Received signal 11 SEGV_MAPERR 000000000000 #0 0x7f501a7b83fb base::debug::StackTrace::StackTrace() #1 0x7f501a7b6a8c base::debug::StackTrace::StackTrace() #2 0x7f501a7b7f0f base::debug::(anonymous namespace)::StackDumpSignalHandler() #3 0x7f501ac3e390 <unknown> #4 0x7f50179b46fb aura::WindowTreeClient::~WindowTreeClient() #5 0x7f50179b4d69 aura::WindowTreeClient::~WindowTreeClient() #6 0x000000431f9f std::default_delete<>::operator()() #7 0x00000043198c std::unique_ptr<>::reset() #8 0x000000430b59 std::unique_ptr<>::~unique_ptr() #9 0x00000042f85c ui::demo::MusDemo::~MusDemo() #10 0x00000043852f ui::demo::MusDemoExternal::~MusDemoExternal() #11 0x000000438579 ui::demo::MusDemoExternal::~MusDemoExternal() #12 0x000000431f9f std::default_delete<>::operator()() #13 0x00000063080c std::unique_ptr<>::reset() #14 0x00000062ece9 std::unique_ptr<>::~unique_ptr() #15 0x00000062db7b service_manager::ServiceContext::~ServiceContext() #16 0x00000062dbc9 service_manager::ServiceContext::~ServiceContext() BUG= ========== to ========== Fix destruction order in mus_demo Problem: Unique ptrs are deleted from the bottom to the upwards as they are defined in a class definition. Before this CL, WindowTreeClient was destroyed after WNState had its dtor called, which led to segfault. Solution: WindowTreeClient must be deleted before Env and WNState, otherwise signal 11 occurs. It happens because Env and WNState are singletons, which are owned by MusDemo. Once WindowTreeClient goes into it's destructor, it uses them to remove itself as an observer. This fix is mainly aimed for https://build.chromium.org/p/chromium.fyi/builders/Ozone%20Linux Call stack- Received signal 11 SEGV_MAPERR 000000000000 #0 0x7f501a7b83fb base::debug::StackTrace::StackTrace() #1 0x7f501a7b6a8c base::debug::StackTrace::StackTrace() #2 0x7f501a7b7f0f base::debug::(anonymous namespace)::StackDumpSignalHandler() #3 0x7f501ac3e390 <unknown> #4 0x7f50179b46fb aura::WindowTreeClient::~WindowTreeClient() #5 0x7f50179b4d69 aura::WindowTreeClient::~WindowTreeClient() #6 0x000000431f9f std::default_delete<>::operator()() #7 0x00000043198c std::unique_ptr<>::reset() #8 0x000000430b59 std::unique_ptr<>::~unique_ptr() #9 0x00000042f85c ui::demo::MusDemo::~MusDemo() #10 0x00000043852f ui::demo::MusDemoExternal::~MusDemoExternal() #11 0x000000438579 ui::demo::MusDemoExternal::~MusDemoExternal() #12 0x000000431f9f std::default_delete<>::operator()() #13 0x00000063080c std::unique_ptr<>::reset() #14 0x00000062ece9 std::unique_ptr<>::~unique_ptr() #15 0x00000062db7b service_manager::ServiceContext::~ServiceContext() #16 0x00000062dbc9 service_manager::ServiceContext::~ServiceContext() BUG=666958 ==========
Description was changed from ========== Fix destruction order in mus_demo Problem: Unique ptrs are deleted from the bottom to the upwards as they are defined in a class definition. Before this CL, WindowTreeClient was destroyed after WNState had its dtor called, which led to segfault. Solution: WindowTreeClient must be deleted before Env and WNState, otherwise signal 11 occurs. It happens because Env and WNState are singletons, which are owned by MusDemo. Once WindowTreeClient goes into it's destructor, it uses them to remove itself as an observer. This fix is mainly aimed for https://build.chromium.org/p/chromium.fyi/builders/Ozone%20Linux Call stack- Received signal 11 SEGV_MAPERR 000000000000 #0 0x7f501a7b83fb base::debug::StackTrace::StackTrace() #1 0x7f501a7b6a8c base::debug::StackTrace::StackTrace() #2 0x7f501a7b7f0f base::debug::(anonymous namespace)::StackDumpSignalHandler() #3 0x7f501ac3e390 <unknown> #4 0x7f50179b46fb aura::WindowTreeClient::~WindowTreeClient() #5 0x7f50179b4d69 aura::WindowTreeClient::~WindowTreeClient() #6 0x000000431f9f std::default_delete<>::operator()() #7 0x00000043198c std::unique_ptr<>::reset() #8 0x000000430b59 std::unique_ptr<>::~unique_ptr() #9 0x00000042f85c ui::demo::MusDemo::~MusDemo() #10 0x00000043852f ui::demo::MusDemoExternal::~MusDemoExternal() #11 0x000000438579 ui::demo::MusDemoExternal::~MusDemoExternal() #12 0x000000431f9f std::default_delete<>::operator()() #13 0x00000063080c std::unique_ptr<>::reset() #14 0x00000062ece9 std::unique_ptr<>::~unique_ptr() #15 0x00000062db7b service_manager::ServiceContext::~ServiceContext() #16 0x00000062dbc9 service_manager::ServiceContext::~ServiceContext() BUG=666958 ========== to ========== Fix destruction order in mus_demo Problem: Unique ptrs are deleted from the bottom to the upwards as they are defined in a class definition. Before this CL, WindowTreeClient was destroyed after WNState had its dtor called, which led to segfault. Solution: WindowTreeClient must be deleted before Env and WNState, otherwise signal 11 occurs. It happens because Env and WNState are singletons, which are owned by MusDemo. Once WindowTreeClient goes into it's destructor, it uses them to remove itself as an observer. This fix is mainly aimed for https://build.chromium.org/p/chromium.fyi/builders/Ozone%20Linux Call stack- Received signal 11 SEGV_MAPERR 000000000000 #0 0x7f501a7b83fb base::debug::StackTrace::StackTrace() #1 0x7f501a7b6a8c base::debug::StackTrace::StackTrace() #2 0x7f501a7b7f0f base::debug::(anonymous namespace)::StackDumpSignalHandler() #3 0x7f501ac3e390 <unknown> #4 0x7f50179b46fb aura::WindowTreeClient::~WindowTreeClient() #5 0x7f50179b4d69 aura::WindowTreeClient::~WindowTreeClient() #6 0x000000431f9f std::default_delete<>::operator()() #7 0x00000043198c std::unique_ptr<>::reset() #8 0x000000430b59 std::unique_ptr<>::~unique_ptr() #9 0x00000042f85c ui::demo::MusDemo::~MusDemo() #10 0x00000043852f ui::demo::MusDemoExternal::~MusDemoExternal() #11 0x000000438579 ui::demo::MusDemoExternal::~MusDemoExternal() #12 0x000000431f9f std::default_delete<>::operator()() #13 0x00000063080c std::unique_ptr<>::reset() #14 0x00000062ece9 std::unique_ptr<>::~unique_ptr() #15 0x00000062db7b service_manager::ServiceContext::~ServiceContext() #16 0x00000062dbc9 service_manager::ServiceContext::~ServiceContext() BUG=666958 ==========
Description was changed from ========== Fix destruction order in mus_demo Problem: Unique ptrs are deleted from the bottom to the upwards as they are defined in a class definition. Before this CL, WindowTreeClient was destroyed after WNState had its dtor called, which led to segfault. Solution: WindowTreeClient must be deleted before Env and WNState, otherwise signal 11 occurs. It happens because Env and WNState are singletons, which are owned by MusDemo. Once WindowTreeClient goes into it's destructor, it uses them to remove itself as an observer. This fix is mainly aimed for https://build.chromium.org/p/chromium.fyi/builders/Ozone%20Linux Call stack- Received signal 11 SEGV_MAPERR 000000000000 #0 0x7f501a7b83fb base::debug::StackTrace::StackTrace() #1 0x7f501a7b6a8c base::debug::StackTrace::StackTrace() #2 0x7f501a7b7f0f base::debug::(anonymous namespace)::StackDumpSignalHandler() #3 0x7f501ac3e390 <unknown> #4 0x7f50179b46fb aura::WindowTreeClient::~WindowTreeClient() #5 0x7f50179b4d69 aura::WindowTreeClient::~WindowTreeClient() #6 0x000000431f9f std::default_delete<>::operator()() #7 0x00000043198c std::unique_ptr<>::reset() #8 0x000000430b59 std::unique_ptr<>::~unique_ptr() #9 0x00000042f85c ui::demo::MusDemo::~MusDemo() #10 0x00000043852f ui::demo::MusDemoExternal::~MusDemoExternal() #11 0x000000438579 ui::demo::MusDemoExternal::~MusDemoExternal() #12 0x000000431f9f std::default_delete<>::operator()() #13 0x00000063080c std::unique_ptr<>::reset() #14 0x00000062ece9 std::unique_ptr<>::~unique_ptr() #15 0x00000062db7b service_manager::ServiceContext::~ServiceContext() #16 0x00000062dbc9 service_manager::ServiceContext::~ServiceContext() BUG=666958 ========== to ========== Fix destruction order in mus_demo Problem: Unique ptrs are deleted from the bottom to the upwards as they are defined in a class definition. Before this CL, WindowTreeClient was destroyed after WNState had its dtor called, which led to segfault. Solution: WindowTreeClient must be deleted before Env and WNState, otherwise signal 11 occurs. It happens because Env and WNState are singletons, which are owned by MusDemo. Once WindowTreeClient goes into it's destructor, it uses them to remove itself as an observer. This fix is mainly aimed for https://build.chromium.org/p/chromium.fyi/builders/Ozone%20Linux Call stack- Received signal 11 SEGV_MAPERR 000000000000 #0 0x7f501a7b83fb base::debug::StackTrace::StackTrace() #1 0x7f501a7b6a8c base::debug::StackTrace::StackTrace() #2 0x7f501a7b7f0f base::debug::(anonymous namespace)::StackDumpSignalHandler() #3 0x7f501ac3e390 <unknown> #4 0x7f50179b46fb aura::WindowTreeClient::~WindowTreeClient() #5 0x7f50179b4d69 aura::WindowTreeClient::~WindowTreeClient() #6 0x000000431f9f std::default_delete<>::operator()() #7 0x00000043198c std::unique_ptr<>::reset() #8 0x000000430b59 std::unique_ptr<>::~unique_ptr() #9 0x00000042f85c ui::demo::MusDemo::~MusDemo() #10 0x00000043852f ui::demo::MusDemoExternal::~MusDemoExternal() #11 0x000000438579 ui::demo::MusDemoExternal::~MusDemoExternal() #12 0x000000431f9f std::default_delete<>::operator()() #13 0x00000063080c std::unique_ptr<>::reset() #14 0x00000062ece9 std::unique_ptr<>::~unique_ptr() #15 0x00000062db7b service_manager::ServiceContext::~ServiceContext() #16 0x00000062dbc9 service_manager::ServiceContext::~ServiceContext() BUG=666958 ==========
The CQ bit was checked by msisov@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> Can it be due to the fact that bots are headless? Bots are compiled for the X11 platform and executed under Xvbf. At least that's what kylechar told me for the ChromiumOS bot. @dpranke: Should we do some additional set up for the Linux OS bot too? It has been broken since the tests are enabled: https://build.chromium.org/p/chromium.fyi/builders/Ozone%20Linux/
On 2017/03/20 14:13:51, fwang wrote: > > Can it be due to the fact that bots are headless? > > Bots are compiled for the X11 platform and executed under Xvbf. At least that's > what kylechar told me for the ChromiumOS bot. > > @dpranke: Should we do some additional set up for the Linux OS bot too? It has > been broken since the tests are enabled: > https://build.chromium.org/p/chromium.fyi/builders/Ozone%20Linux/ It looks like 'Ozone Linux' isn't running with Xvfb. I'll switch the default Ozone platform on it back to headless for now.
On 2017/03/20 14:14:52, kylechar wrote: > On 2017/03/20 14:13:51, fwang wrote: > > > Can it be due to the fact that bots are headless? > > > > Bots are compiled for the X11 platform and executed under Xvbf. At least > that's > > what kylechar told me for the ChromiumOS bot. > > > > @dpranke: Should we do some additional set up for the Linux OS bot too? It has > > been broken since the tests are enabled: > > https://build.chromium.org/p/chromium.fyi/builders/Ozone%20Linux/ > > It looks like 'Ozone Linux' isn't running with Xvfb. I'll switch the default > Ozone platform on it back to headless for now. CL to switch it back: https://codereview.chromium.org/2760903002
On 2017/03/20 14:16:45, kylechar wrote: > On 2017/03/20 14:14:52, kylechar wrote: > > On 2017/03/20 14:13:51, fwang wrote: > > > > Can it be due to the fact that bots are headless? > > > > > > Bots are compiled for the X11 platform and executed under Xvbf. At least > > that's > > > what kylechar told me for the ChromiumOS bot. > > > > > > @dpranke: Should we do some additional set up for the Linux OS bot too? It > has > > > been broken since the tests are enabled: > > > https://build.chromium.org/p/chromium.fyi/builders/Ozone%20Linux/ > > > > It looks like 'Ozone Linux' isn't running with Xvfb. I'll switch the default > > Ozone platform on it back to headless for now. > > CL to switch it back: https://codereview.chromium.org/2760903002 Great, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Fix destruction order in mus_demo Problem: Unique ptrs are deleted from the bottom to the upwards as they are defined in a class definition. Before this CL, WindowTreeClient was destroyed after WNState had its dtor called, which led to segfault. Solution: WindowTreeClient must be deleted before Env and WNState, otherwise signal 11 occurs. It happens because Env and WNState are singletons, which are owned by MusDemo. Once WindowTreeClient goes into it's destructor, it uses them to remove itself as an observer. This fix is mainly aimed for https://build.chromium.org/p/chromium.fyi/builders/Ozone%20Linux Call stack- Received signal 11 SEGV_MAPERR 000000000000 #0 0x7f501a7b83fb base::debug::StackTrace::StackTrace() #1 0x7f501a7b6a8c base::debug::StackTrace::StackTrace() #2 0x7f501a7b7f0f base::debug::(anonymous namespace)::StackDumpSignalHandler() #3 0x7f501ac3e390 <unknown> #4 0x7f50179b46fb aura::WindowTreeClient::~WindowTreeClient() #5 0x7f50179b4d69 aura::WindowTreeClient::~WindowTreeClient() #6 0x000000431f9f std::default_delete<>::operator()() #7 0x00000043198c std::unique_ptr<>::reset() #8 0x000000430b59 std::unique_ptr<>::~unique_ptr() #9 0x00000042f85c ui::demo::MusDemo::~MusDemo() #10 0x00000043852f ui::demo::MusDemoExternal::~MusDemoExternal() #11 0x000000438579 ui::demo::MusDemoExternal::~MusDemoExternal() #12 0x000000431f9f std::default_delete<>::operator()() #13 0x00000063080c std::unique_ptr<>::reset() #14 0x00000062ece9 std::unique_ptr<>::~unique_ptr() #15 0x00000062db7b service_manager::ServiceContext::~ServiceContext() #16 0x00000062dbc9 service_manager::ServiceContext::~ServiceContext() BUG=666958 ========== to ========== Fix destruction order in mus_demo Problem: Unique pointers are deleted from the bottom to the upwards as they are defined in a class definition. Before this CL, WindowTreeClient was destroyed after WNState had its destructor called, which led to segfault. Solution: WindowTreeClient must be deleted before Env and WNState, otherwise signal 11 occurs. It happens because Env and WNState are singletons, which are owned by MusDemo. Once WindowTreeClient goes into its destructor, it uses them to remove itself as an observer. This fix is mainly aimed for https://build.chromium.org/p/chromium.fyi/builders/Ozone%20Linux Call stack- Received signal 11 SEGV_MAPERR 000000000000 #0 0x7f501a7b83fb base::debug::StackTrace::StackTrace() #1 0x7f501a7b6a8c base::debug::StackTrace::StackTrace() #2 0x7f501a7b7f0f base::debug::(anonymous namespace)::StackDumpSignalHandler() #3 0x7f501ac3e390 <unknown> #4 0x7f50179b46fb aura::WindowTreeClient::~WindowTreeClient() #5 0x7f50179b4d69 aura::WindowTreeClient::~WindowTreeClient() #6 0x000000431f9f std::default_delete<>::operator()() #7 0x00000043198c std::unique_ptr<>::reset() #8 0x000000430b59 std::unique_ptr<>::~unique_ptr() #9 0x00000042f85c ui::demo::MusDemo::~MusDemo() #10 0x00000043852f ui::demo::MusDemoExternal::~MusDemoExternal() #11 0x000000438579 ui::demo::MusDemoExternal::~MusDemoExternal() #12 0x000000431f9f std::default_delete<>::operator()() #13 0x00000063080c std::unique_ptr<>::reset() #14 0x00000062ece9 std::unique_ptr<>::~unique_ptr() #15 0x00000062db7b service_manager::ServiceContext::~ServiceContext() #16 0x00000062dbc9 service_manager::ServiceContext::~ServiceContext() BUG=666958 ==========
https://codereview.chromium.org/2764433002/diff/20001/services/ui/demo/mus_de... File services/ui/demo/mus_demo.cc (right): https://codereview.chromium.org/2764433002/diff/20001/services/ui/demo/mus_de... services/ui/demo/mus_demo.cc:32: window_tree_client_.reset(); Two points: 1) I think you should explain that windows (from window_tree_data_list_) must be destroyed before the window tree client. 2) Maybe you can explicitly destroy Env and WMState to make things clearer.
https://codereview.chromium.org/2764433002/diff/20001/services/ui/demo/mus_de... File services/ui/demo/mus_demo.cc (right): https://codereview.chromium.org/2764433002/diff/20001/services/ui/demo/mus_de... services/ui/demo/mus_demo.cc:32: window_tree_client_.reset(); On 2017/03/21 07:12:02, fwang wrote: > Two points: > 1) I think you should explain that windows (from window_tree_data_list_) must be > destroyed before the window tree client. > 2) Maybe you can explicitly destroy Env and WMState to make things clearer. Done.
The CQ bit was checked by msisov@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
kelychar@, any more comments?
lgtm https://codereview.chromium.org/2764433002/diff/20001/services/ui/demo/mus_de... File services/ui/demo/mus_demo.cc (right): https://codereview.chromium.org/2764433002/diff/20001/services/ui/demo/mus_de... services/ui/demo/mus_demo.cc:30: // dtor. s/dtor/destructor
Ignore that comment, you had already fixed it :)
Ignore that comment, you had already fixed it :)
The CQ bit was checked by msisov@igalia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490102474650910,
"parent_rev": "9bab651a8a170a6016af11dc0ee4f6407456fcb9", "commit_rev":
"9ca8fc91927b335661ab095f925b1337486e64d0"}
Message was sent while issue was closed.
Description was changed from ========== Fix destruction order in mus_demo Problem: Unique pointers are deleted from the bottom to the upwards as they are defined in a class definition. Before this CL, WindowTreeClient was destroyed after WNState had its destructor called, which led to segfault. Solution: WindowTreeClient must be deleted before Env and WNState, otherwise signal 11 occurs. It happens because Env and WNState are singletons, which are owned by MusDemo. Once WindowTreeClient goes into its destructor, it uses them to remove itself as an observer. This fix is mainly aimed for https://build.chromium.org/p/chromium.fyi/builders/Ozone%20Linux Call stack- Received signal 11 SEGV_MAPERR 000000000000 #0 0x7f501a7b83fb base::debug::StackTrace::StackTrace() #1 0x7f501a7b6a8c base::debug::StackTrace::StackTrace() #2 0x7f501a7b7f0f base::debug::(anonymous namespace)::StackDumpSignalHandler() #3 0x7f501ac3e390 <unknown> #4 0x7f50179b46fb aura::WindowTreeClient::~WindowTreeClient() #5 0x7f50179b4d69 aura::WindowTreeClient::~WindowTreeClient() #6 0x000000431f9f std::default_delete<>::operator()() #7 0x00000043198c std::unique_ptr<>::reset() #8 0x000000430b59 std::unique_ptr<>::~unique_ptr() #9 0x00000042f85c ui::demo::MusDemo::~MusDemo() #10 0x00000043852f ui::demo::MusDemoExternal::~MusDemoExternal() #11 0x000000438579 ui::demo::MusDemoExternal::~MusDemoExternal() #12 0x000000431f9f std::default_delete<>::operator()() #13 0x00000063080c std::unique_ptr<>::reset() #14 0x00000062ece9 std::unique_ptr<>::~unique_ptr() #15 0x00000062db7b service_manager::ServiceContext::~ServiceContext() #16 0x00000062dbc9 service_manager::ServiceContext::~ServiceContext() BUG=666958 ========== to ========== Fix destruction order in mus_demo Problem: Unique pointers are deleted from the bottom to the upwards as they are defined in a class definition. Before this CL, WindowTreeClient was destroyed after WNState had its destructor called, which led to segfault. Solution: WindowTreeClient must be deleted before Env and WNState, otherwise signal 11 occurs. It happens because Env and WNState are singletons, which are owned by MusDemo. Once WindowTreeClient goes into its destructor, it uses them to remove itself as an observer. This fix is mainly aimed for https://build.chromium.org/p/chromium.fyi/builders/Ozone%20Linux Call stack- Received signal 11 SEGV_MAPERR 000000000000 #0 0x7f501a7b83fb base::debug::StackTrace::StackTrace() #1 0x7f501a7b6a8c base::debug::StackTrace::StackTrace() #2 0x7f501a7b7f0f base::debug::(anonymous namespace)::StackDumpSignalHandler() #3 0x7f501ac3e390 <unknown> #4 0x7f50179b46fb aura::WindowTreeClient::~WindowTreeClient() #5 0x7f50179b4d69 aura::WindowTreeClient::~WindowTreeClient() #6 0x000000431f9f std::default_delete<>::operator()() #7 0x00000043198c std::unique_ptr<>::reset() #8 0x000000430b59 std::unique_ptr<>::~unique_ptr() #9 0x00000042f85c ui::demo::MusDemo::~MusDemo() #10 0x00000043852f ui::demo::MusDemoExternal::~MusDemoExternal() #11 0x000000438579 ui::demo::MusDemoExternal::~MusDemoExternal() #12 0x000000431f9f std::default_delete<>::operator()() #13 0x00000063080c std::unique_ptr<>::reset() #14 0x00000062ece9 std::unique_ptr<>::~unique_ptr() #15 0x00000062db7b service_manager::ServiceContext::~ServiceContext() #16 0x00000062dbc9 service_manager::ServiceContext::~ServiceContext() BUG=666958 Review-Url: https://codereview.chromium.org/2764433002 Cr-Commit-Position: refs/heads/master@{#458388} Committed: https://chromium.googlesource.com/chromium/src/+/9ca8fc91927b335661ab095f925b... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/9ca8fc91927b335661ab095f925b... |
