New issue
Advanced search Search tips

Issue 595851 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: May 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 557406



Sign in to add a comment

Mash Shell Shelf Overflow Bubble

Project Member Reported by msw@chromium.org, Mar 17 2016

Issue description

Mash Shell Shelf Overflow Bubble

The mash shelf shows an overflow button, but clicking it causes a crash.

See parent  Issue 557406 
 
Owner: jamescook@chromium.org
Status: Assigned (was: Untriaged)
#0 0x7fe96f02f9ce base::debug::StackTrace::StackTrace()
#1 0x7fe96f02f50f base::debug::(anonymous namespace)::StackDumpSignalHandler()
#2 0x7fe96dacd340 <unknown>
#3 0x7fe96bcdb005 std::unique_ptr<>::get()
#4 0x7fe96bdccdcd std::unique_ptr<>::operator->()
#5 0x7fe96bdc9ce9 ash::RootWindowController::GetSystemTray()
#6 0x7fe96bdd5060 ash::OverflowBubble::Show()
#7 0x7fe96be0706e ash::ShelfView::ToggleOverflowBubble()
#8 0x7fe96be07ef3 ash::ShelfView::ButtonPressed()
#9 0x7fe968ffea17 views::Button::NotifyClick()
#10 0x7fe969001682 views::CustomButton::NotifyClick()
#11 0x7fe969000625 views::CustomButton::OnMouseReleased()
#12 0x7fe9690c2ca2 views::View::ProcessMouseReleased()
#13 0x7fe9690c24b6 views::View::OnMouseEvent()
#14 0x7fe969988547 ui::EventHandler::OnEvent()
#15 0x7fe969983fc0 ui::EventDispatcher::DispatchEvent()
#16 0x7fe96998391b ui::EventDispatcher::ProcessEvent()
#17 0x7fe9699836b2 ui::EventDispatcherDelegate::DispatchEventToTarget()
#18 0x7fe969983592 ui::EventDispatcherDelegate::DispatchEvent()
#19 0x7fe9690d8e3b views::internal::RootView::OnMouseReleased()
#20 0x7fe9690e0816 views::Widget::OnMouseEvent()
#21 0x7fe9689501fe views::NativeWidgetMus::OnMouseEvent()
#22 0x7fe969988547 ui::EventHandler::OnEvent()
#23 0x7fe969983fc0 ui::EventDispatcher::DispatchEvent()
#24 0x7fe96998391b ui::EventDispatcher::ProcessEvent()
#25 0x7fe9699836b2 ui::EventDispatcherDelegate::DispatchEventToTarget()
#26 0x7fe969983592 ui::EventDispatcherDelegate::DispatchEvent()
#27 0x7fe9699891fb ui::EventProcessor::OnEventFromSource()
#28 0x7fe96998a265 ui::EventSource::DeliverEventToProcessor()
#29 0x7fe969989eec ui::EventSource::SendEventToProcessor()
#30 0x7fe968d00d4b aura::WindowTreeHostPlatform::DispatchEvent()
#31 0x7fe9689728b6 views::WindowTreeHostMus::DispatchEvent()
#32 0x7fe968957d35 views::PlatformWindowMus::OnWindowInputEvent()
#33 0x7fe96899d392 mus::WindowTreeClientImpl::OnWindowInputEvent()
#34 0x7fe968a732f4 mus::mojom::WindowTreeClientStub::Accept()
#35 0x7fe9689c9f9d mojo::internal::InterfaceEndpointClient::HandleValidatedMessage()
#36 0x7fe9689c9a51 mojo::internal::InterfaceEndpointClient::HandleIncomingMessageThunk::Accept()
#37 0x7fe968a74dc2 mus::mojom::WindowTreeClientRequestValidator::Accept()
#38 0x7fe9689cb632 mojo::internal::InterfaceEndpointClient::HandleIncomingMessage()
#39 0x7fe9689d6549 mojo::internal::MultiplexRouter::ProcessIncomingMessage()
#40 0x7fe9689d5dc9 mojo::internal::MultiplexRouter::Accept()
#41 0x7fe9689d346d mojo::internal::MessageHeaderValidator::Accept()
#42 0x7fe9689c4398 mojo::internal::Connector::ReadSingleMessage()

Status: Started (was: Assigned)
I'll take a look at the crash now
Cc: jonr...@chromium.org
+jonross

I have a fix for the crash out for review at https://codereview.chromium.org/1929203002/

However, even with the crash fixed the overflow bubble doesn't open. Rather, it opens then immediately closes. This is due to a focus/activation issue -- the bubble sees an activation change and closes itself. The stack is below; I'm not sure why the InFlightFocusChange is reverted. Maybe there are two competing focus requests?

#1 0x7f3a759adf48 views::NativeWidgetMus::Close()
#2 0x7f3a76164428 views::Widget::Close()
#3 0x7f3a7607b287 views::BubbleDialogDelegateView::OnWidgetActivationChanged()
#4 0x7f3a76165c2a views::Widget::OnNativeWidgetActivationChanged()
#5 0x7f3a759ac49b views::NativeWidgetMus::OnActivationChanged()
#6 0x7f3a759d1c80 views::WindowTreeHostMus::OnActivationChanged()
#7 0x7f3a759b6bda views::PlatformWindowMus::OnWindowFocusChanged()
#8 0x7f3a759f8cbe mus::WindowTreeClientImpl::LocalSetFocus()
#9 0x7f3a759d3d98 mus::InFlightFocusChange::Revert()
#10 0x7f3a759fca41 mus::WindowTreeClientImpl::OnChangeCompleted()
#11 0x7f3a75ad2e2a mus::mojom::WindowTreeClientStub::Accept()

Something is causing your focus request to be rejected by the window server. The reversion of the InFlightFocusChange is then triggered to update the client app.

I'd take a look at WindowTree::SetFocus to see where it's rejecting.
Project Member

Comment 5 by bugdroid1@chromium.org, Apr 29 2016

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

commit 2a5ffa2195e262691da5d7bab4ac1364622cfcde
Author: jamescook <jamescook@chromium.org>
Date: Fri Apr 29 17:44:02 2016

mash: Fix crash when clicking on the shelf overflow icon

Under mus/mash, each widget has its own RootWindow. Only the RootWindow for
the display has a RootWindowController. OverflowBubble was attempting to look
up the display's RootWindowController from the shelf's widget, which doesn't
have one.

There is a separate problem that prevents the overflow bubble from showing
(it immediately closes due to a focus/activation issue) but with this CL at
least we don't crash on click.

We'll need to audit other callers of RootWindowController::ForWindow() to
ensure they are safe; I'll file a bug for that.

BUG= 595851 
TEST=Run mash, launch some apps, resize window to get overflow icon to show,
click icon, no crash.

Review-Url: https://codereview.chromium.org/1929203002
Cr-Commit-Position: refs/heads/master@{#390690}

[modify] https://crrev.com/2a5ffa2195e262691da5d7bab4ac1364622cfcde/ash/root_window_controller.h
[modify] https://crrev.com/2a5ffa2195e262691da5d7bab4ac1364622cfcde/ash/shelf/overflow_bubble.cc
[modify] https://crrev.com/2a5ffa2195e262691da5d7bab4ac1364622cfcde/ash/system/tray/tray_background_view.cc
[modify] https://crrev.com/2a5ffa2195e262691da5d7bab4ac1364622cfcde/ash/system/tray/tray_background_view.h
[modify] https://crrev.com/2a5ffa2195e262691da5d7bab4ac1364622cfcde/ash/system/tray/tray_bubble_wrapper.cc

The problem is that the overflow bubble widget is being created as non-activatable (WidgetDelegate::CanActivate() is false) but shown with ui::SHOW_STATE_DEFAULT which results in an attempt to activate it in NativeWidgetMus.

https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/mus/native_widget_mus.cc&q=showwithwindowstate&sq=package:chromium&type=cs&l=712

It is shown with SHOW_STATE_DEFAULT due to some unusual logic in views::Widget wrt to saved_show_state_.

https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/widget.cc&q=Widget::Show&sq=package:chromium&type=cs&l=612

In this case it skips the check for WidgetDelegate::CanActivate().

NativeWidgetAura::ShowWithWindowState() has an explicit check for CanActivate():

https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/native_widget_aura.cc&q=ShowWIthWindowState&sq=package:chromium&type=cs&l=496

While views::Widget is technically wrong here (it should probably also check CanActivate) there might be cases in Chrome's views code that rely on the existing Widget::Show() behavior, and it's hard to be sure.

I think we should fix this by making NativeWidgetMus match the behavior of NativeWidgetAura. I'll put up a CL shortly.

Project Member

Comment 7 by bugdroid1@chromium.org, May 3 2016

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

commit b69da6a7e57248527c6ec7fea8a5044f6ebc804d
Author: jamescook <jamescook@chromium.org>
Date: Tue May 03 18:23:06 2016

mus: Don't activate non-activatable widgets on Show()

This was preventing the shelf overflow bubble from opening. See bug for
details.

Make NativeWidgetMus::Show() behave similarly to NativeWidgetAura and check
WidgetDelegate::CanActivate() before activating the widget.

BUG= 595851 
TEST=added to views_mus_unittest

Review-Url: https://codereview.chromium.org/1936823002
Cr-Commit-Position: refs/heads/master@{#391303}

[modify] https://crrev.com/b69da6a7e57248527c6ec7fea8a5044f6ebc804d/ui/views/mus/native_widget_mus.cc
[modify] https://crrev.com/b69da6a7e57248527c6ec7fea8a5044f6ebc804d/ui/views/mus/native_widget_mus_unittest.cc

Project Member

Comment 8 by bugdroid1@chromium.org, May 3 2016

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

commit e27b07278681ba98ec911b145968315b5eb63440
Author: jamescook <jamescook@chromium.org>
Date: Tue May 03 19:59:25 2016

mash: Fix shelf overflow bubble not closing on click outside its bounds

* Add screen location to views::PointerWatcher notifications.
* Convert OverflowBubble from a global ui::EventHandler to a PointerWatcher,
which allows it to work on mus.

Add/remove the PointerWatcher in the constructor/destructor which ensures
it is properly cleaned up when the widget is destroyed (unlike the previous
pre-target handler).

BUG= 595851 , 608508 
TEST=existing ash_unittests for overflow bubble. For mash, shrink the window
to show overflow arrow, click to spawn it, click outside bubble to close it.

Review-Url: https://codereview.chromium.org/1934123004
Cr-Commit-Position: refs/heads/master@{#391334}

[modify] https://crrev.com/e27b07278681ba98ec911b145968315b5eb63440/ash/pointer_watcher_delegate_aura.cc
[modify] https://crrev.com/e27b07278681ba98ec911b145968315b5eb63440/ash/pointer_watcher_delegate_aura.h
[modify] https://crrev.com/e27b07278681ba98ec911b145968315b5eb63440/ash/shelf/overflow_bubble.cc
[modify] https://crrev.com/e27b07278681ba98ec911b145968315b5eb63440/ash/shelf/overflow_bubble.h
[modify] https://crrev.com/e27b07278681ba98ec911b145968315b5eb63440/ash/system/tray/tray_event_filter.cc
[modify] https://crrev.com/e27b07278681ba98ec911b145968315b5eb63440/ash/system/tray/tray_event_filter.h
[modify] https://crrev.com/e27b07278681ba98ec911b145968315b5eb63440/ui/views/mus/window_manager_connection.cc
[modify] https://crrev.com/e27b07278681ba98ec911b145968315b5eb63440/ui/views/mus/window_manager_connection_unittest.cc
[modify] https://crrev.com/e27b07278681ba98ec911b145968315b5eb63440/ui/views/pointer_watcher.h

Labels: -mustash mash
Status: Fixed (was: Started)
It now opens and closes and doesn't crash. I suspect there may be drag and drop issues, but we can track that in another bug.
Project Member

Comment 11 by bugdroid1@chromium.org, Aug 23

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

commit 1723281f4514b58fe1a1ef17c66a92e79517827c
Author: James Cook <jamescook@chromium.org>
Date: Thu Aug 23 19:48:28 2018

chromeos: Convert OverflowBubble to ui::EventHandler

Now that ash and the ui service are in the same process we don't need
to use PointerWatcher in ash anymore.

This is a partial manual revert of:
https://codereview.chromium.org/1934123004/

Bug:  595851 ,  872450 
Test: ash_unittests
Change-Id: Ifbbb407210d0b3ec8d20993bff7a0f2cf44ba38e
Reviewed-on: https://chromium-review.googlesource.com/1186032
Commit-Queue: James Cook <jamescook@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585583}
[modify] https://crrev.com/1723281f4514b58fe1a1ef17c66a92e79517827c/ash/shelf/overflow_bubble.cc
[modify] https://crrev.com/1723281f4514b58fe1a1ef17c66a92e79517827c/ash/shelf/overflow_bubble.h

Sign in to add a comment