Mus-WS: Improve FrameSinkId allocation |
|||||||||||||
Issue descriptionCurrently we stuff the WindowID in the cient_id portion of the FrameSinkId in Mus, and we don't use the sink_id portion at all. This makes debugging more difficult at the very least, and it also exposes server side WindowIds to clients. It's unclear whether this poses any security concerns but it's probably better to allocate FrameSinkIds independently from WindowIds using FrameSinkIdAllocator anyway to match Chrome behavior today. There should probably be a FrameSinkIdAllocator per window tree.
,
Apr 19 2017
,
Apr 26 2017
I don't oppose mapping WindowId::client_id_ to FrameSinkId::client_id if sky@ does not oppose. The original intent of the sink ID is for it to be allocated by the client so maybe perhaps the parent should allocate the sink ID for the child window it creates?
,
May 3 2017
,
May 4 2017
,
May 10 2017
,
May 12 2017
I found there are several kinds of ids in mus. 1. client id - every window tree has a unique client id, both client and WS know it. 2. ClientWindowId - generated by client. High 16 bits are client id of window tree, the low 16 bits are number generated by client. 3. WindowId - Generated by WS. High 16 bits are client id of window tree, the low 16 bits are number generated by WS. 4. FrameSinkId - WS generates it. It contains two int32_t (client_id_, sink_id_). The client_id_ is WindowId. As my test, for a window, ClientWindowId == WindowId == FrameSinkId::client_id_. And some code also assume they are the same (see[1]). I think maybe we could simply them to one Id. It could avoid unnecessary IPC between client and WS. [1] https://cs.chromium.org/chromium/src/services/ui/ws/window_tree.cc?rcl=a66d812aa78debb856c1938f439e98feabeaeada&l=1118
,
May 12 2017
[1] is a CL for supporting Exo in Mushrome and Mustash. It need retrieve the FrameSinkId for a window from WS. It needs a two way IPC for it, however the FrameSinkId is the ClientWindowId actually. [1] https://codereview.chromium.org/2875753002/diff/60001/services/ui/public/interfaces/window_tree.mojom
,
May 23 2017
,
Jun 13 2017
,
Jun 13 2017
,
Jun 20 2017
This one is not blocking 720600 anymore. And I am not working on this issue anymore.
,
Jul 27 2017
,
Jul 27 2017
for reference, take a look at this too: https://codereview.chromium.org/2886873002
,
Jul 30 2017
Note that FrameSinkIdAllocator shouldn't be used by the viz service anyway. An unprivileged parent can always create children, and so I think FrameSinkIdAllocator belongs in components/viz/client.
,
Aug 2 2017
Discussion conclusion: Replace client-window-id with FrameSinkId. Update clients to use FrameSinkId as the client-window-id, client-window-id <-> window-id mapping in mus-ws would become frame-sink-id <-> window-id mapping. https://docs.google.com/a/chromium.org/document/d/1HsmPblOrB87Fj8B31fyLp4Z7vqUIvV6GNvDEOcQJNRg/edit?usp=sharing
,
Aug 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f6be97ed655f03e03f8e91dc2342a001450a6cf7 commit f6be97ed655f03e03f8e91dc2342a001450a6cf7 Author: Ria Jiang <riajiang@chromium.org> Date: Mon Aug 28 22:38:09 2017 mus: Ignore the client id sent by clients. Instead of using the client id sent by clients as the ClientWindowId ::client_id part in WS, WindowTree would ignore invalid client ids sent by clients and use its WindowTree::id_ as the client_id. Clients can still use their server_id to reference their windows as we are using WindowTree::id_ as the high 16 bits before looking up windows as well. After this change, ClientWindowIds in WS should be globally unique. From https://chromium-review.googlesource.com/c/608270 to avoid gerrit issues. Bug: 712302 Test: covered by tests Change-Id: I8cf2466ac532566fe6733e044e131dc8ffd401c2 Reviewed-on: https://chromium-review.googlesource.com/617378 Commit-Queue: Ria Jiang <riajiang@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org> Cr-Commit-Position: refs/heads/master@{#497908} [modify] https://crrev.com/f6be97ed655f03e03f8e91dc2342a001450a6cf7/content/renderer/mus/renderer_window_tree_client.cc [modify] https://crrev.com/f6be97ed655f03e03f8e91dc2342a001450a6cf7/content/renderer/mus/renderer_window_tree_client.h [modify] https://crrev.com/f6be97ed655f03e03f8e91dc2342a001450a6cf7/services/ui/public/interfaces/window_manager.mojom [modify] https://crrev.com/f6be97ed655f03e03f8e91dc2342a001450a6cf7/services/ui/public/interfaces/window_tree.mojom [modify] https://crrev.com/f6be97ed655f03e03f8e91dc2342a001450a6cf7/services/ui/ws/ids.h [modify] https://crrev.com/f6be97ed655f03e03f8e91dc2342a001450a6cf7/services/ui/ws/server_window.cc [modify] https://crrev.com/f6be97ed655f03e03f8e91dc2342a001450a6cf7/services/ui/ws/test_change_tracker.cc [modify] https://crrev.com/f6be97ed655f03e03f8e91dc2342a001450a6cf7/services/ui/ws/test_change_tracker.h [modify] https://crrev.com/f6be97ed655f03e03f8e91dc2342a001450a6cf7/services/ui/ws/test_utils.cc [modify] https://crrev.com/f6be97ed655f03e03f8e91dc2342a001450a6cf7/services/ui/ws/test_utils.h [modify] https://crrev.com/f6be97ed655f03e03f8e91dc2342a001450a6cf7/services/ui/ws/transient_windows_unittest.cc [modify] https://crrev.com/f6be97ed655f03e03f8e91dc2342a001450a6cf7/services/ui/ws/window_manager_client_unittest.cc [modify] https://crrev.com/f6be97ed655f03e03f8e91dc2342a001450a6cf7/services/ui/ws/window_manager_state_unittest.cc [modify] https://crrev.com/f6be97ed655f03e03f8e91dc2342a001450a6cf7/services/ui/ws/window_tree.cc [modify] https://crrev.com/f6be97ed655f03e03f8e91dc2342a001450a6cf7/services/ui/ws/window_tree.h [modify] https://crrev.com/f6be97ed655f03e03f8e91dc2342a001450a6cf7/services/ui/ws/window_tree_client_unittest.cc [modify] https://crrev.com/f6be97ed655f03e03f8e91dc2342a001450a6cf7/services/ui/ws/window_tree_unittest.cc [modify] https://crrev.com/f6be97ed655f03e03f8e91dc2342a001450a6cf7/ui/aura/mus/window_tree_client.cc [modify] https://crrev.com/f6be97ed655f03e03f8e91dc2342a001450a6cf7/ui/aura/mus/window_tree_client.h [modify] https://crrev.com/f6be97ed655f03e03f8e91dc2342a001450a6cf7/ui/aura/test/mus/test_window_tree_client_setup.cc [modify] https://crrev.com/f6be97ed655f03e03f8e91dc2342a001450a6cf7/ui/aura/test/mus/window_tree_client_private.cc [modify] https://crrev.com/f6be97ed655f03e03f8e91dc2342a001450a6cf7/ui/aura/test/mus/window_tree_client_private.h
,
Sep 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/45fdfdd6904ca9af645b0d40b16d5f2186c42368 commit 45fdfdd6904ca9af645b0d40b16d5f2186c42368 Author: Ria Jiang <riajiang@chromium.org> Date: Wed Sep 13 16:13:25 2017 mus: Merge ClientWindowId and FrameSinkId. After https://chromium-review.googlesource.com/c/chromium/src/+/617378, ClientWindowId is globally unique, so we can change FrameSinkId to have the value of the corresponding ClientWindowId. This also means that clients now know about their FrameSinkId. Changed ClientWindowId to be of type FrameSinkId to essentially merge these two ids in mus-ws. Updated uses of FrameSinkId in WindowManagerState and unittests. Fixed some more places in WindowTree that needed ClientWindowId conversions. Bug: 712302 Test: covered by tests Change-Id: I3a77487b7e10af5ba1850f2585b81f43768f2184 Reviewed-on: https://chromium-review.googlesource.com/639954 Commit-Queue: Ria Jiang <riajiang@chromium.org> Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org> Cr-Commit-Position: refs/heads/master@{#501650} [modify] https://crrev.com/45fdfdd6904ca9af645b0d40b16d5f2186c42368/services/ui/ws/DEPS [modify] https://crrev.com/45fdfdd6904ca9af645b0d40b16d5f2186c42368/services/ui/ws/default_access_policy.cc [modify] https://crrev.com/45fdfdd6904ca9af645b0d40b16d5f2186c42368/services/ui/ws/display.cc [modify] https://crrev.com/45fdfdd6904ca9af645b0d40b16d5f2186c42368/services/ui/ws/focus_controller_unittest.cc [modify] https://crrev.com/45fdfdd6904ca9af645b0d40b16d5f2186c42368/services/ui/ws/ids.h [modify] https://crrev.com/45fdfdd6904ca9af645b0d40b16d5f2186c42368/services/ui/ws/server_window.cc [modify] https://crrev.com/45fdfdd6904ca9af645b0d40b16d5f2186c42368/services/ui/ws/server_window.h [modify] https://crrev.com/45fdfdd6904ca9af645b0d40b16d5f2186c42368/services/ui/ws/server_window_drawn_tracker_unittest.cc [modify] https://crrev.com/45fdfdd6904ca9af645b0d40b16d5f2186c42368/services/ui/ws/transient_windows_unittest.cc [modify] https://crrev.com/45fdfdd6904ca9af645b0d40b16d5f2186c42368/services/ui/ws/window_manager_access_policy.cc [modify] https://crrev.com/45fdfdd6904ca9af645b0d40b16d5f2186c42368/services/ui/ws/window_manager_display_root.cc [modify] https://crrev.com/45fdfdd6904ca9af645b0d40b16d5f2186c42368/services/ui/ws/window_manager_state.cc [modify] https://crrev.com/45fdfdd6904ca9af645b0d40b16d5f2186c42368/services/ui/ws/window_manager_state_unittest.cc [modify] https://crrev.com/45fdfdd6904ca9af645b0d40b16d5f2186c42368/services/ui/ws/window_server.cc [modify] https://crrev.com/45fdfdd6904ca9af645b0d40b16d5f2186c42368/services/ui/ws/window_server.h [modify] https://crrev.com/45fdfdd6904ca9af645b0d40b16d5f2186c42368/services/ui/ws/window_tree.cc [modify] https://crrev.com/45fdfdd6904ca9af645b0d40b16d5f2186c42368/services/ui/ws/window_tree.h [modify] https://crrev.com/45fdfdd6904ca9af645b0d40b16d5f2186c42368/services/ui/ws/window_tree_client_unittest.cc [modify] https://crrev.com/45fdfdd6904ca9af645b0d40b16d5f2186c42368/services/ui/ws/window_tree_unittest.cc
,
Sep 14 2017
doc: https://docs.google.com/a/google.com/document/d/1L7gcX3gReE0M6wjDeMM3uoeT19Uf-Jdy0C4v7OooyTg/edit?usp=sharing
,
Nov 14 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a5f21099f19b67c37df517bcd12db360c6a6fa63 commit a5f21099f19b67c37df517bcd12db360c6a6fa63 Author: Ria Jiang <riajiang@chromium.org> Date: Tue Nov 14 01:48:46 2017 Change FrameSinkId for embed windows to be in embedded tree's id space. When a client submits hit-test data for its own children, the client_id part of the frame_sink_id of its children is 0 and HitTestManager is supposed to fill in the correct client_id. However, if the root window of this client (e.g. browser, quick-launch, renderer; WindowManager is an exception) is being embedded, then the root window is really created by its embedder and has the client_id of its embedder. In these cases, we are filling in the wrong client_id if we just get the client_id part in CompositorFrameSinkSupport::frame_sink_id_. Essentially, in mus/mash, we are submitting compositor frames and hit-test data using parent’s client_id. This CL changes FrameSinkId for embed windows to be in embedded tree's id space. Currently, there are two ways for embedding: 1) An embedded client creates a top level window by asking its WindowTree in NewTopLevelWindow(). The window_id provided in this call by the client is the same window_id used in its WindowTree. However, the associated ServerWindow is created by the WindowManager(embedder), and so its FrameSinkId used to include the WindowManager's client id. This CL changes that, so that the FrameSinkId of the ServerWindow is generated using the client_id (and window_id) of the embedded client. 2) The embedder creates the aura::Window (the embed window) and asks its WindowTreeClient -> WindoeTree -> WindowServer to create a new WT for the embed client and embed it at the embed window. That embedded client will get notified in OnEmbed with that new tree and the embed window (and other information). For 1), this CL plumbs the client_window_id for the embed window from the embedded tree to the embedder tree and use that as the new FrameSinkId to make sure we don't use that FrameSinkId in the embedded tree again. For 2), right now it's only used by renderers (RendererWindowTreeClient) and we don't provide a window id to use for the embed window like case 1. This CL changes the new FrameSinkId for the embed window to be (embedded tree's id, 0) so that next_window_id_ on the embedded client side can remain unchanged. Bug: 712302 Test: covered by tests Change-Id: Iaa6c5e477784f9887db696f74fa2f7bd938ab67c Reviewed-on: https://chromium-review.googlesource.com/748442 Commit-Queue: Ria Jiang <riajiang@chromium.org> Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org> Reviewed-by: Tom Sepez <tsepez@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Cr-Commit-Position: refs/heads/master@{#516142} [modify] https://crrev.com/a5f21099f19b67c37df517bcd12db360c6a6fa63/services/ui/public/interfaces/window_manager.mojom [modify] https://crrev.com/a5f21099f19b67c37df517bcd12db360c6a6fa63/services/ui/ws/server_window.cc [modify] https://crrev.com/a5f21099f19b67c37df517bcd12db360c6a6fa63/services/ui/ws/server_window.h [modify] https://crrev.com/a5f21099f19b67c37df517bcd12db360c6a6fa63/services/ui/ws/test_utils.cc [modify] https://crrev.com/a5f21099f19b67c37df517bcd12db360c6a6fa63/services/ui/ws/test_utils.h [modify] https://crrev.com/a5f21099f19b67c37df517bcd12db360c6a6fa63/services/ui/ws/window_manager_state_unittest.cc [modify] https://crrev.com/a5f21099f19b67c37df517bcd12db360c6a6fa63/services/ui/ws/window_server.cc [modify] https://crrev.com/a5f21099f19b67c37df517bcd12db360c6a6fa63/services/ui/ws/window_server.h [modify] https://crrev.com/a5f21099f19b67c37df517bcd12db360c6a6fa63/services/ui/ws/window_tree.cc [modify] https://crrev.com/a5f21099f19b67c37df517bcd12db360c6a6fa63/services/ui/ws/window_tree.h [modify] https://crrev.com/a5f21099f19b67c37df517bcd12db360c6a6fa63/services/ui/ws/window_tree_client_unittest.cc [modify] https://crrev.com/a5f21099f19b67c37df517bcd12db360c6a6fa63/services/ui/ws/window_tree_unittest.cc [modify] https://crrev.com/a5f21099f19b67c37df517bcd12db360c6a6fa63/ui/aura/mus/window_tree_client.cc [modify] https://crrev.com/a5f21099f19b67c37df517bcd12db360c6a6fa63/ui/aura/mus/window_tree_client.h |
|||||||||||||
►
Sign in to add a comment |
|||||||||||||
Comment 1 by kylec...@chromium.org
, Apr 18 2017