Prevent crash when desktop controller is NULL

Prevent crash when desktop controller is NULL

BUG=222218
TEST=manual


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

[oshima, 2013-04-03 15:14:12]

https://codereview.chromium.org/13434005/diff/1/ash/root_window_controller.cc
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/13434005/diff/1/ash/root_window_controller.cc...
ash/root_window_controller.cc:469: views::Widget* widget = background ?
background->widget() : NULL;
when it can become null?

https://codereview.chromium.org/13434005/diff/1/ash/root_window_controller.cc...
ash/root_window_controller.cc:479: }
if (!widget)
  return;
if (menu_runner.RunMenuAt(....)) {
  return;
}

is easier to follow

[Dmitry Polukhin, 2013-04-03 18:49:59]

https://codereview.chromium.org/13434005/diff/1/ash/root_window_controller.cc
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/13434005/diff/1/ash/root_window_controller.cc...
ash/root_window_controller.cc:469: views::Widget* widget = background ?
background->widget() : NULL;
On 2013/04/03 15:14:12, oshima wrote:
> when it can become null?

I wasn't able to reproduce this crash but you can see stack trace in the bug
https://code.google.com/p/chromium/issues/detail?id=222218

The crash happens because background controller is NULL. I see several places
where it can be set to NULL (on place in this file in
RootWindowController::CloseChildWindows). It looks like it is set to NULL when
windows are destroying. But I suspect the windows still could receive message. I
think we can remove check for widget but I added it to be on safe side.

https://codereview.chromium.org/13434005/diff/1/ash/root_window_controller.cc...
ash/root_window_controller.cc:479: }
On 2013/04/03 15:14:12, oshima wrote:
> if (!widget)
>   return;
> if (menu_runner.RunMenuAt(....)) {
>   return;
> }
> 
> is easier to follow

Done.

[oshima, 2013-04-03 18:56:50]

+varunjain

I think this is caused by the "we open context menu on both press and release"
issue, and Varun was looking into this.

Varun, what's the status of this issue?

[Dmitry Polukhin, 2013-04-03 19:05:13]

On 2013/04/03 18:56:50, oshima wrote:
> +varunjain
> 
> I think this is caused by the "we open context menu on both press and release"
> issue, and Varun was looking into this.
> 
> Varun, what's the status of this issue?

I agree that it could be more issues (I haven't seen them) but there is no sense
to keep crashing. This CL adds safe checks that should prevent crash.

[oshima, 2013-04-03 20:33:20]

On 2013/04/03 19:05:13, Dmitry Polukhin wrote:
> On 2013/04/03 18:56:50, oshima wrote:
> > +varunjain
> > 
> > I think this is caused by the "we open context menu on both press and
release"
> > issue, and Varun was looking into this.
> > 
> > Varun, what's the status of this issue?
> 
> I agree that it could be more issues (I haven't seen them) but there is no
sense
> to keep crashing. This CL adds safe checks that should prevent crash.

If this shouldn't happen, then we should fix it in a right way (assuming that
there is one), rather than simply ignoring it, which can lead to different type
of problem.
 What if we accidentally removing somewhere for example? That'd lead to no
context menu when a user clicked background.

If you want to land some short term fix and do investigate later, that's fine
with me, but in that case, please add comment and reference to the bug.

[varunjain, 2013-04-04 04:29:59]

On 2013/04/03 20:33:20, oshima wrote:
> On 2013/04/03 19:05:13, Dmitry Polukhin wrote:
> > On 2013/04/03 18:56:50, oshima wrote:
> > > +varunjain
> > > 
> > > I think this is caused by the "we open context menu on both press and
> release"
> > > issue, and Varun was looking into this.
> > > 
> > > Varun, what's the status of this issue?
> > 
> > I agree that it could be more issues (I haven't seen them) but there is no
> sense
> > to keep crashing. This CL adds safe checks that should prevent crash.
> 
> If this shouldn't happen, then we should fix it in a right way (assuming that
> there is one), rather than simply ignoring it, which can lead to different
type
> of problem.
>  What if we accidentally removing somewhere for example? That'd lead to no
> context menu when a user clicked background.
> 
> If you want to land some short term fix and do investigate later, that's fine
> with me, but in that case, please add comment and reference to the bug.

My change was reverted in r188517. Please check if the bug still repros

[oshima, 2013-04-04 08:00:14]

On 2013/04/04 04:29:59, varunjain wrote:
> On 2013/04/03 20:33:20, oshima wrote:
> > On 2013/04/03 19:05:13, Dmitry Polukhin wrote:
> > > On 2013/04/03 18:56:50, oshima wrote:
> > > > +varunjain
> > > > 
> > > > I think this is caused by the "we open context menu on both press and
> > release"
> > > > issue, and Varun was looking into this.
> > > > 
> > > > Varun, what's the status of this issue?
> > > 
> > > I agree that it could be more issues (I haven't seen them) but there is no
> > sense
> > > to keep crashing. This CL adds safe checks that should prevent crash.
> > 
> > If this shouldn't happen, then we should fix it in a right way (assuming
that
> > there is one), rather than simply ignoring it, which can lead to different
> type
> > of problem.
> >  What if we accidentally removing somewhere for example? That'd lead to no
> > context menu when a user clicked background.
> > 
> > If you want to land some short term fix and do investigate later, that's
fine
> > with me, but in that case, please add comment and reference to the bug.
> 
> My change was reverted in r188517. Please check if the bug still repros

So we're still opening a context menu on both events and it will not change?

[Dmitry Polukhin, 2013-04-04 10:26:40]

I reproduce crash from crbug.com/222218. I thought that it could happen on sign
out when windows destroying but I didn't manage to reproduce it. But I reproduce
crash when click happens during windows creation on sign in (or in case of Guest
mode on startup, we restart browser process for Guest session). I set breakpoint
in ShowWallpaperAnimationObserver::OnImplicitAnimationsScheduled, when debugger
stops on this breakpoint UI is already visible and you can make right click on
status area. It triggers call for RootWindowController::ShowContextMenu before
kDesktopController property set (the property set in
ShowWallpaperAnimationObserver::OnImplicitAnimationsCompleted).

Stack trace from debugger:
Program received signal SIGSEGV, Segmentation fault.
0x00007ffff406093a in ash::internal::DesktopBackgroundWidgetController::widget
(this=0x0) at
../../ash/desktop_background/desktop_background_widget_controller.h:47
(gdb) bt
#0  0x00007ffff406093a in
ash::internal::DesktopBackgroundWidgetController::widget (this=0x0) at
../../ash/desktop_background/desktop_background_widget_controller.h:47
#1  0x00007ffff40b399c in ash::internal::RootWindowController::ShowContextMenu
(this=0x6e21f5af180, location_in_screen=...) at
../../ash/root_window_controller.cc:469
#2  0x00007ffff40c57a6 in ash::Shell::ShowContextMenu (this=0x6e21fa7eb20,
location_in_screen=...) at ../../ash/shell.cc:621
#3  0x00007ffff40a681f in ash::internal::LauncherView::ShowContextMenuForView
(this=0x6e22012e620, source=0x6e22012e620, point=...) at
../../ash/launcher/launcher_view.cc:1403
#4  0x00007fffee476f9a in views::View::ShowContextMenu (this=0x6e22012e620,
p=..., is_mouse_gesture=true) at ../../ui/views/view.cc:1130
#5  0x00007fffee47b3c3 in views::View::ProcessMouseReleased (this=0x6e22012e620,
event=...) at ../../ui/views/view.cc:2108
#6  0x00007fffee476608 in views::View::OnMouseEvent (this=0x6e22012e620,
event=0x7fffffffbbb0) at ../../ui/views/view.cc:953
#7  0x00007ffff7c66d13 in ui::EventHandler::OnEvent (this=0x6e22012e648,
event=0x7fffffffbbb0) at ../../ui/base/events/event_handler.cc:27
#8  0x00007ffff7c68249 in ui::EventTarget::OnEvent (this=0x6e22012e648,
event=0x7fffffffbbb0) at ../../ui/base/events/event_target.cc:52
#9  0x00007ffff7c65430 in ui::EventDispatcher::DispatchEventToSingleHandler
(this=0x7fffffffbb00, handler=0x6e22012e648, event=0x7fffffffbbb0) at
../../ui/base/events/event_dispatcher.cc:151
#10 0x00007ffff7c653d1 in ui::EventDispatcher::DispatchEvent
(this=0x7fffffffbb00, handler=0x6e22012e648, event=0x7fffffffbbb0) at
../../ui/base/events/event_dispatcher.cc:144
#11 0x00007ffff7c64fe8 in ui::EventDispatcher::ProcessEvent
(this=0x7fffffffbb00, target=0x6e22012e648, event=0x7fffffffbbb0) at
../../ui/base/events/event_dispatcher.cc:93
#12 0x00007ffff7c64d19 in ui::EventDispatcherDelegate::DispatchEvent
(this=0x6e21f6bb1c0, target=0x6e22012e648, event=0x7fffffffbbb0) at
../../ui/base/events/event_dispatcher.cc:48
#13 0x00007fffee484c00 in views::internal::RootView::DispatchEventToTarget
(this=0x6e21f6bb020, target=0x6e22012e620, event=0x7fffffffbbb0) at
../../ui/views/widget/root_view.cc:675
#14 0x00007fffee483f30 in views::internal::RootView::OnMouseReleased
(this=0x6e21f6bb020, event=...) at ../../ui/views/widget/root_view.cc:494
#15 0x00007fffee48fba0 in views::Widget::OnMouseEvent (this=0x6e21fde5fe0,
event=0x7fffffffc490) at ../../ui/views/widget/widget.cc:1170
#16 0x00007fffee488cfc in views::NativeWidgetAura::OnMouseEvent
(this=0x6e21fcfda20, event=0x7fffffffc490) at
../../ui/views/widget/native_widget_aura.cc:770
#17 0x00007ffff7c66d13 in ui::EventHandler::OnEvent (this=0x6e21fcfda30,
event=0x7fffffffc490) at ../../ui/base/events/event_handler.cc:27
#18 0x00007ffff7c6822e in ui::EventTarget::OnEvent (this=0x6e21fdea4c0,
event=0x7fffffffc490) at ../../ui/base/events/event_target.cc:50
#19 0x00007ffff7c65430 in ui::EventDispatcher::DispatchEventToSingleHandler
(this=0x7fffffffc110, handler=0x6e21fdea4c0, event=0x7fffffffc490) at
../../ui/base/events/event_dispatcher.cc:151
#20 0x00007ffff7c653d1 in ui::EventDispatcher::DispatchEvent
(this=0x7fffffffc110, handler=0x6e21fdea4c0, event=0x7fffffffc490) at
../../ui/base/events/event_dispatcher.cc:144
#21 0x00007ffff7c64fe8 in ui::EventDispatcher::ProcessEvent
(this=0x7fffffffc110, target=0x6e21fdea4c0, event=0x7fffffffc490) at
../../ui/base/events/event_dispatcher.cc:93
#22 0x00007ffff7c64d19 in ui::EventDispatcherDelegate::DispatchEvent
(this=0x6e21f6afe08, target=0x6e21fdea4c0, event=0x7fffffffc490) at
../../ui/base/events/event_dispatcher.cc:48
#23 0x00007fffee1b99f0 in aura::RootWindow::ProcessEvent (this=0x6e21f6afc20,
target=0x6e21fdea4a0, event=0x7fffffffc490) at ../../ui/aura/root_window.cc:747
#24 0x00007fffee1bb140 in aura::RootWindow::DispatchMouseEventToTarget
(this=0x6e21f6afc20, event=0x7fffffffc490, target=0x6e21fdea4a0) at
../../ui/aura/root_window.cc:1116
#25 0x00007fffee1bae1e in aura::RootWindow::DispatchMouseEventImpl
(this=0x6e21f6afc20, event=0x7fffffffc490) at ../../ui/aura/root_window.cc:1060
#26 0x00007fffee1ba3cb in aura::RootWindow::OnHostMouseEvent
(this=0x6e21f6afc20, event=0x7fffffffc490) at ../../ui/aura/root_window.cc:902
#27 0x00007fffee1b45c4 in
aura::RootWindowHostX11::TranslateAndDispatchMouseEvent (this=0x6e21f36e020,
event=0x7fffffffc490) at ../../ui/aura/root_window_host_x11.cc:1105
#28 0x00007fffee1b4234 in aura::RootWindowHostX11::DispatchXI2Event
(this=0x6e21f36e020, event=@0x7fffffffce68: 0x7fffffffd050) at
../../ui/aura/root_window_host_x11.cc:1049
#29 0x00007fffee1b1e23 in aura::RootWindowHostX11::Dispatch (this=0x6e21f36e020,
event=@0x7fffffffce68: 0x7fffffffd050) at
../../ui/aura/root_window_host_x11.cc:508
#30 0x00007ffff54b16bf in base::MessagePumpAuraX11::Dispatch
(this=0x6e21f36ec80, xev=@0x7fffffffce68: 0x7fffffffd050) at
../../base/message_pump_aurax11.cc:311
#31 0x00007ffff54b12f7 in base::MessagePumpAuraX11::ProcessXEvent
(this=0x6e21f36ec80, dispatcher=0x6e21f36ece8, xev=0x7fffffffd050) at
../../base/message_pump_aurax11.cc:256
#32 0x00007ffff54b0cbf in base::MessagePumpAuraX11::DispatchXEvents
(this=0x6e21f36ec80) at ../../base/message_pump_aurax11.cc:198
#33 0x00007ffff54b04ba in (anonymous namespace)::XSourceDispatch
(source=0x6e21f3af2a0, unused_func=0x0, data=0x6e21f36ec80) at
../../base/message_pump_aurax11.cc:33
#34 0x00007fffe8523d53 in g_main_dispatch (context=0x6e21f3b2200) at
/build/buildd/glib2.0-2.32.3/./glib/gmain.c:2539
#35 g_main_context_dispatch (context=0x6e21f3b2200) at
/build/buildd/glib2.0-2.32.3/./glib/gmain.c:3075
#36 0x00007fffe85240a0 in g_main_context_iterate (dispatch=1, block=<optimized
out>, context=0x6e21f3b2200, self=<optimized out>) at
/build/buildd/glib2.0-2.32.3/./glib/gmain.c:3146
#37 g_main_context_iterate (context=0x6e21f3b2200, block=<optimized out>,
dispatch=1, self=<optimized out>) at
/build/buildd/glib2.0-2.32.3/./glib/gmain.c:3083
#38 0x00007fffe8524164 in g_main_context_iteration (context=0x6e21f3b2200,
may_block=0) at /build/buildd/glib2.0-2.32.3/./glib/gmain.c:3207
#39 0x00007ffff54ae790 in base::MessagePumpGlib::RunWithDispatcher
(this=0x6e21f36ec80, delegate=0x6e21f3b5460, dispatcher=0x0) at
../../base/message_pump_glib.cc:199
#40 0x00007ffff54aec60 in base::MessagePumpGlib::Run (this=0x6e21f36ec80,
delegate=0x6e21f3b5460) at ../../base/message_pump_glib.cc:296
#41 0x00007ffff551385b in base::MessageLoop::RunInternal (this=0x6e21f3b5460) at
../../base/message_loop.cc:431
#42 0x00007ffff5513716 in base::MessageLoop::RunHandler (this=0x6e21f3b5460) at
../../base/message_loop.cc:404
#43 0x00007ffff5549430 in base::RunLoop::Run (this=0x7fffffffd810) at
../../base/run_loop.cc:45
#44 0x00005555578c9293 in ChromeBrowserMainParts::MainMessageLoopRun
(this=0x7fffe38b7b60, result_code=0x6e21f371d38) at
../../chrome/browser/chrome_browser_main.cc:1647
#45 0x00007fffeb2e3fcd in content::BrowserMainLoop::RunMainMessageLoopParts
(this=0x6e21f371d20) at ../../content/browser/browser_main_loop.cc:517
#46 0x00007fffeb2e7efa in content::BrowserMainRunnerImpl::Run
(this=0x6e21f37d2c0) at ../../content/browser/browser_main_runner.cc:124
#47 0x00007fffeb2e219c in content::BrowserMain (parameters=...) at
../../content/browser/browser_main.cc:22
#48 0x00007fffeb2b36ba in content::RunNamedProcessTypeMain
(process_type=<std::string::_Rep::_S_empty_rep_storage+24> "",
main_function_params=..., delegate=0x7fffffffe1d0) at
../../content/app/content_main_runner.cc:459
#49 0x00007fffeb2b4614 in content::ContentMainRunnerImpl::Run
(this=0x7fffe39297a0) at ../../content/app/content_main_runner.cc:764
#50 0x00007fffeb2b2c67 in content::ContentMain (argc=2, argv=0x7fffffffe328,
delegate=0x7fffffffe1d0) at ../../content/app/content_main.cc:35
#51 0x0000555555be71e1 in ChromeMain (argc=2, argv=0x7fffffffe328) at
../../chrome/app/chrome_main.cc:32
#52 0x0000555555be71ac in main (argc=2, argv=0x7fffffffe328) at
../../chrome/app/chrome_exe_main_aura.cc:17

I also was able to reproduce it on device without debugger. I did it on Daisy
device, click on Browse as Guest button and immediately start right clicking
(two fingers click) on the place of button. I was able to crash browser several
times in the row.

I think it is right approach is to ignore right click on status area before
animation completion.

[oshima, 2013-04-04 13:44:50]

Thank you for digging down into the issue.
That makes sense and I'm fine with this, but
have a few more comments.

https://codereview.chromium.org/13434005/diff/6001/ash/root_window_controller.cc
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/13434005/diff/6001/ash/root_window_controller...
ash/root_window_controller.cc:469: if (!background)
can you explain when this happens in the comment with a reference to the bug?

https://codereview.chromium.org/13434005/diff/6001/ash/root_window_controller...
ash/root_window_controller.cc:474: return;
can this happen?

[Dmitry Polukhin, 2013-04-04 14:42:44]

https://codereview.chromium.org/13434005/diff/6001/ash/root_window_controller.cc
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/13434005/diff/6001/ash/root_window_controller...
ash/root_window_controller.cc:469: if (!background)
On 2013/04/04 13:44:51, oshima wrote:
> can you explain when this happens in the comment with a reference to the bug?

Done.

https://codereview.chromium.org/13434005/diff/6001/ash/root_window_controller...
ash/root_window_controller.cc:474: return;
On 2013/04/04 13:44:51, oshima wrote:
> can this happen?

I haven't seen this - removed.

[oshima, 2013-04-04 15:42:07]

lgtm

[commit-bot: I haz the power, 2013-04-04 16:04:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dpolukhin@chromium.org/13434005/15002

[commit-bot: I haz the power, 2013-04-04 16:12:44]

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-04-04 18:02:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dpolukhin@chromium.org/13434005/15002

[commit-bot: I haz the power, 2013-04-04 18:06:56]

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=mac&number...

[commit-bot: I haz the power, 2013-04-04 19:28:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dpolukhin@chromium.org/13434005/15002

[commit-bot: I haz the power, 2013-04-04 19:29:40]

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=mac_rel&nu...

[commit-bot: I haz the power, 2013-04-05 04:28:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dpolukhin@chromium.org/13434005/15002

[commit-bot: I haz the power, 2013-04-05 05:59:00]

Change committed as 192513