Fixes use after free caused by delete in RootWindowController

Fixes use after free caused by delete in RootWindowController

RootWindowController::CloseChildWindows() was explicitly deleting
windows. It should only do that for windows that are owned by the
parent, otherwise the window should be removed.

BUG=297028
TEST=covered by test now.
R=oshima@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226347

[oshima, 2013-09-30 22:48:57]

thanks, I didn't know about owned_by_parent flag!

https://codereview.chromium.org/25374002/diff/3001/ash/root_window_controller.cc
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/25374002/diff/3001/ash/root_window_controller...
ash/root_window_controller.cc:448: aura::Window* child =
non_toplevel_window->children()[i];
can't we skip here like

if (child->owned_by_parent())
  return;

Window::~Window seems to take care of removing child when deleted.

[sky, 2013-09-30 23:14:17]

New patch uploaded.

https://codereview.chromium.org/25374002/diff/3001/ash/root_window_controller.cc
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/25374002/diff/3001/ash/root_window_controller...
ash/root_window_controller.cc:448: aura::Window* child =
non_toplevel_window->children()[i];
On 2013/09/30 22:48:57, oshima wrote:
> can't we skip here like
> 
> if (child->owned_by_parent())
>   return;
> 
> Window::~Window seems to take care of removing child when deleted.

Done.

[oshima, 2013-09-30 23:56:32]

lgtm

[commit-bot: I haz the power, 2013-10-01 14:15:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/25374002/7001

[commit-bot: I haz the power, 2013-10-01 15:24:05]

Retried try job too often on linux_chromeos for step(s) ash_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-10-01 17:23:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/25374002/7001

[commit-bot: I haz the power, 2013-10-01 17:43:49]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-10-01 18:03:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/25374002/7001

[commit-bot: I haz the power, 2013-10-01 19:01:42]

Retried try job too often on linux_chromeos for step(s) ash_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[sky, 2013-10-01 20:15:16]

NoCrashShutdownWithAlwaysOnTopWindow was crashing with my patch. I had to make a
slight change to FramePainter to get it working again. After that, I realized I
can simplify RootWindowController even further, so I did that too.
Take another look?

[oshima, 2013-10-01 20:34:27]

On 2013/10/01 20:15:16, sky wrote:
> NoCrashShutdownWithAlwaysOnTopWindow was crashing with my patch. I had to make
a
> slight change to FramePainter to get it working again. After that, I realized
I
> can simplify RootWindowController even further, so I did that too.
> Take another look?

We used to delete child this way, but was causing crash (crbug.com/273310)
because
deleting window/widget may need to access other containers.

[sky, 2013-10-01 20:47:57]

Ok, back to a variant of previous patch.

[oshima, 2013-10-01 20:55:10]

lgtm

[commit-bot: I haz the power, 2013-10-01 21:02:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/25374002/54001

[commit-bot: I haz the power, 2013-10-01 23:50:16]

Change committed as 226347

[dcheng, 2013-10-02 01:19:53]

On 2013/10/01 23:50:16, I haz the power (commit-bot) wrote:
> Change committed as 226347

Sorry, it looks like there's a test failure on the win8 aura bot:
[ RUN      ] RootWindowControllerTest.DontDeleteWindowsNotOwnedByParent
Backtrace:
	std::_Iterator_base12::_Adopt [0x01A51825+197]
	std::_Vector_const_iterator<std::_Vector_val<aura::WindowObserver
*,std::allocator<aura::WindowObserver *> >
>::_Vector_const_iterator<std::_Vector_val<aura::WindowObserver
*,std::allocator<aura::WindowObserver *> > > [0x01A90B11+81]
	std::_Vector_iterator<std::_Vector_val<aura::WindowObserver
*,std::allocator<aura::WindowObserver *> >
>::_Vector_iterator<std::_Vector_val<aura::WindowObserver
*,std::allocator<aura::WindowObserver *> > > [0x01A8ED9E+30]
	std::vector<aura::WindowObserver *,std::allocator<aura::WindowObserver *>
>::end [0x01A8A411+49]
	ObserverListBase<aura::WindowObserver>::RemoveObserver [0x01A87222+82]
	aura::Window::RemoveObserver [0x01A7FA90+32]
	ash::test::DestroyedWindowObserver::Shutdown [0x0057C45C+44]
	ash::test::DestroyedWindowObserver::~DestroyedWindowObserver [0x0057C316+70]
	ash::test::RootWindowControllerTest_DontDeleteWindowsNotOwnedByParent_Test::TestBody
[0x0057BFCE+1662]
	testing::internal::HandleExceptionsInMethodIfSupported<testing::Test,void>
[0x007DC92F+319]
	testing::Test::Run [0x007C738E+174]
	testing::TestInfo::Run [0x007C7D5D+221]
	testing::TestCase::Run [0x007C850F+239]
	testing::internal::UnitTestImpl::RunAllTests [0x007CEE5D+701]
	testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,bool>
[0x007DD557+327]
	testing::UnitTest::Run [0x007CD6C0+192]
	base::TestSuite::Run [0x007A2520+240]
	main [0x006120A7+103]
	__tmainCRTStartup [0x009486AF+447]
(f:\dd\vctools\crt_bld\self_x86\crt\src\crtexe.c:555)
	mainCRTStartup [0x009484DF+15]
(f:\dd\vctools\crt_bld\self_x86\crt\src\crtexe.c:371)
	BaseThreadInitThunk [0x75238543+14]
	RtlInitializeExceptionChain [0x77BFAC69+133]
	RtlInitializeExceptionChain [0x77BFAC3C+88]

I'm going to have to revert