New issue
Advanced search Search tips

Issue 788185 link

Starred by 1 user
Status: WontFix
Owner:
riajiang@chromium.org
Closed: Aug 15
Cc:
rjkroege@chromium.org
sadrul@chromium.org
mustash-bugs@google.com
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 752380



Sign in to add a comment

crash in GetFallbackTargetForEventBlockedByModal with a deleted window

Project Member Reported by riajiang@chromium.org, Nov 23 2017 
Run chrome with --mus and --use-viz-hit-test -> open network settings page. This is causing a crash:

    #0 0x56469ee57d51 in __interceptor_backtrace /b/build/slave/linux_upload_clang/build/src/third_party/llvm/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:3754:13
    #1 0x7f120587cb4c in base::debug::StackTrace::StackTrace(unsigned long) ../../../base/debug/stack_trace_posix.cc:801:41
    #2 0x7f12059004b9 in logging::LogMessage::~LogMessage() ../../../base/logging.cc:581:29
    #3 0x7f11ff3aa7c4 in ui::ws::WindowManagerState::GetFallbackTargetForEventBlockedByModal(ui::ws::ServerWindow*) ../../../services/ui/ws/window_manager_state.cc:914:3
    #4 0x7f11ff337f4d in AdjustTargetForModal ../../../services/ui/ws/event_dispatcher.cc:362:34
    #5 0x7f11ff337f4d in ui::ws::EventDispatcher::UpdateCursorProviderByLastKnownLocationOnFoundWindow(ui::ws::EventLocation const&, ui::ws::DeepestWindow const&) ../../../services/ui/ws/event_dispatcher.cc:613:0
    #6 0x7f11ff3443fd in Run ../../../base/callback.h:65:12
    #7 0x7f11ff3443fd in ui::ws::EventTargeter::FindTargetForLocationNow(viz::EventSource, ui::ws::EventLocation const&, base::OnceCallback<void (ui::ws::EventLocation const&, ui::ws::DeepestWindow const&)>) ../../../services/ui/ws/event_targeter.cc:83:0
    #8 0x7f11ff343b09 in ui::ws::EventTargeter::FindTargetForLocation(viz::EventSource, ui::ws::EventLocation const&, base::OnceCallback<void (ui::ws::EventLocation const&, ui::ws::DeepestWindow const&)>) ../../../services/ui/ws/event_targeter.cc:41:5
    #9 0x7f11ff33736f in ui::ws::EventDispatcher::UpdateCursorProviderByLastKnownLocation() ../../../services/ui/ws/event_dispatcher.cc:227:22
    #10 0x7f11ff3c1797 in UpdateNativeCursorFromMouseLocation ../../../services/ui/ws/window_server.cc:777:21
    #11 0x7f11ff3c1797 in ui::ws::WindowServer::OnWindowHierarchyChanged(ui::ws::ServerWindow*, ui::ws::ServerWindow*, ui::ws::ServerWindow*) ../../../services/ui/ws/window_server.cc:923:0
    #12 0x7f11ff36cab8 in ui::ws::ServerWindow::Remove(ui::ws::ServerWindow*) ../../../services/ui/ws/server_window.cc:178:14
    #13 0x7f11ff3e2488 in ui::ws::WindowTree::RemoveWindowFromParent(viz::FrameSinkId const&) ../../../services/ui/ws/window_tree.cc:1261:21
    #14 0x7f11ff3e50d1 in ui::ws::WindowTree::RemoveWindowFromParent(unsigned int, unsigned int) ../../../services/ui/ws/window_tree.cc:1630:18
    #15 0x7f11fac4deb0 in ui::mojom::WindowTreeStubDispatch::Accept(ui::mojom::WindowTree*, mojo::Message*) gen/services/ui/public/interfaces/window_tree.mojom.cc:2653:13

When we are opening the network settings page, we delete the status tray bubble window. Since this changes window hierarchy, we are updating cursor provider for the old parent [1], which finds the target based on the last mouse location. Viz hasn't been notified of this change yet and the status tray bubble window is still in the window mappings in WS (until destroyed), so we still finds this status tray bubble window as target. Then when we adjust this target for modal window, we can't find its root window [2].

[1] https://cs.chromium.org/chromium/src/services/ui/ws/window_server.cc?gsn=GetRootForDrawn&l=920

[2]
https://cs.chromium.org/chromium/src/services/ui/ws/event_dispatcher.cc?type=cs&l=362
Project Member

Comment 1 by bugdroid1@chromium.org, Nov 24 2017 

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

commit 63b59877ed9ca325a200d0dbb91c9fe437d85d2d
Author: Ria Jiang <riajiang@chromium.org>
Date: Fri Nov 24 21:34:11 2017

Don't call to update native cursor from WS when removing a window.

When we remove a window, e.g. opening the network settings page, which
is going to remove the status tray bubble window, we changes the
window hierarchy and updates cursor provider for the old parent [1],
which finds the target based on the last mouse location. Viz hasn't been
notified of this change yet and the status tray bubble window is still
in the window mappings in mus-ws (until destroyed), so we still finds
this status tray bubble window as target with --use-viz-hit-test. Then
when we adjust this target for modal window, we fail to find its root
window [2].

[1] https://cs.chromium.org/chromium/src/services/ui/ws/window_server.cc?gsn=GetRootForDrawn&l=920
[2] https://cs.chromium.org/chromium/src/services/ui/ws/event_dispatcher.cc?type=cs&l=362

This CL checks if we are actually removing a window before we call to
update native cursor in WS; the logic for moving a window from one
display to another remains unchanged. When we remove a window,
EventDispatcher::WindowNoLongerValidTarget is going update cursor if
necessary, so there's no need to call to update cursor in
WindowServer::OnWindowHierarchyChanged and do the work twice.

Bug:  788185 
Change-Id: I48a7bfa4e981ae1b768768b0f011e53d6b968451
Reviewed-on: https://chromium-review.googlesource.com/788230
Commit-Queue: Ria Jiang <riajiang@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519162}
[modify] https://crrev.com/63b59877ed9ca325a200d0dbb91c9fe437d85d2d/services/ui/ws/window_server.cc
[modify] https://crrev.com/63b59877ed9ca325a200d0dbb91c9fe437d85d2d/services/ui/ws/window_tree_unittest.cc
Project Member

Comment 2 by bugdroid1@chromium.org, Nov 28 2017 

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

commit 6f32bb34c044097090f59f9d0493412685c69e2f
Author: Ria Jiang <riajiang@chromium.org>
Date: Tue Nov 28 03:26:22 2017

Revert "Don't call to update native cursor from WS when removing a window."

This reverts commit 63b59877ed9ca325a200d0dbb91c9fe437d85d2d.

Reason for revert: Mistakenly thought that cursor provider would be updated if necessary in https://cs.chromium.org/chromium/src/services/ui/ws/event_dispatcher.cc?type=cs&l=754 when called from WindowNoLongerValidTarget. But we only update mouse_cursor_source_window and not cursor provider because it was not the capture_window.

Original change's description:
> Don't call to update native cursor from WS when removing a window.
> 
> When we remove a window, e.g. opening the network settings page, which
> is going to remove the status tray bubble window, we changes the
> window hierarchy and updates cursor provider for the old parent [1],
> which finds the target based on the last mouse location. Viz hasn't been
> notified of this change yet and the status tray bubble window is still
> in the window mappings in mus-ws (until destroyed), so we still finds
> this status tray bubble window as target with --use-viz-hit-test. Then
> when we adjust this target for modal window, we fail to find its root
> window [2].
> 
> [1] https://cs.chromium.org/chromium/src/services/ui/ws/window_server.cc?gsn=GetRootForDrawn&l=920
> [2] https://cs.chromium.org/chromium/src/services/ui/ws/event_dispatcher.cc?type=cs&l=362
> 
> This CL checks if we are actually removing a window before we call to
> update native cursor in WS; the logic for moving a window from one
> display to another remains unchanged. When we remove a window,
> EventDispatcher::WindowNoLongerValidTarget is going update cursor if
> necessary, so there's no need to call to update cursor in
> WindowServer::OnWindowHierarchyChanged and do the work twice.
> 
> Bug:  788185 
> Change-Id: I48a7bfa4e981ae1b768768b0f011e53d6b968451
> Reviewed-on: https://chromium-review.googlesource.com/788230
> Commit-Queue: Ria Jiang <riajiang@chromium.org>
> Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#519162}

TBR=sadrul@chromium.org,riajiang@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  788185 
Change-Id: Ib1c2b4366eda94ccbf8f4d0ff22cf9012a27803b
Reviewed-on: https://chromium-review.googlesource.com/792150
Reviewed-by: Ria Jiang <riajiang@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Ria Jiang <riajiang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519544}
[modify] https://crrev.com/6f32bb34c044097090f59f9d0493412685c69e2f/services/ui/ws/window_server.cc
[modify] https://crrev.com/6f32bb34c044097090f59f9d0493412685c69e2f/services/ui/ws/window_tree_unittest.cc

Comment 3 by lafo...@chromium.org, Feb 26 2018

Components: -Internals>MUS Internals>Services>WindowService

Comment 4 by jamescook@chromium.org, Apr 24 2018

Labels: -Proj-Mustash-Mus-WS
Deprecating label Proj-Mustash-Mus-WS in favor of Components.

Comment 5 by sky@chromium.org, Aug 15

Status: WontFix (was: Started)
This code no longer exists.
►

Sign in to add a comment