Fix HWND registration order with --enable-viz |
||||||||||
Issue descriptionRenderingWindowManager sets up an HWND hierarchy. The browser process registers the parent HWND first, then the GPU process sends an IPC back to the browser to register the child HWND under the parent. If the GPU sends an IPC with a parent that isn't registered then the GPU process is killed for misbehaving. The parent is unregistered when the viz::Display is destroyed later. With --enable-viz it's possible that for a short lived top level window the register child and unregister parent calls could be reversed. The GPU process would be killed unnecessarily since the parent is unregistered first. The browser process shouldn't unregister the parent until it's safe. This means waiting until the ChildWindowWin is destroyed in the GPU process and the IO thread has processed all previous IPCs. I think waiting on an async IPC from the GPU process (over mojom::FrameSinkManagerClient) that the OutputSurface for SurfaceHandle (which is the parent HWND) was destroyed should be sufficient.
,
Feb 9 2018
,
Aug 15
The original concern that GPU process could be killed unnecessarily no longer exists. GpuProcessHost doesn't kill the GPU process if RenderingWindowManager::RegisterChild() fails. That part of this is fixed.
I think there is still a potential race. We will only call SetParent() once for a parent HWND. When ContextFactory::CreateLayerTreeFrameSink() gets called we reset state in RenderingWindowManager so that we can call SetParent() again. This should work fine except if the following order of events:
ContextFactory::CreateLayerTreeFrameSink()
RWM::UnregisterParent()
* PostTask waiting on GpuChannel
OnEstablishedGpuChannel()
RWM::RegisterParent()
* Send IPC to the GPU to setup RootCompositorFrameSinkImpl and create child HWND.
* Something happens like switching to software compositing that invalidates LayerFreeFrameSinks
ContextFactory::CreateLayerTreeFrameSink()
RWM::UnregisterParent()
OnEstablishedGpuChannel()
* RegisterParent() happens in same stack because we no longer have to wait for GpuChannelHost with software compositing.
RWM::RegisterParent()
* Send IPC to the GPU to setup RootCompositorFrameSinkImpl and create child HWND.
** IOThread: Get first IPC back to call RWM::RegisterChild() for bad child HWND and succeeds.
** IOThread: Get second IPC back to call RWM::RegisterChild() for correct child HWND and fails.
The second IPC fails because SetParent() was called for the old child HWND.
,
Aug 16
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6fc8951dca75d6180ccac70190e60ef493dde963 commit 6fc8951dca75d6180ccac70190e60ef493dde963 Author: kylechar <kylechar@chromium.org> Date: Thu Aug 16 22:10:32 2018 Cleanup ContextProvider error handling in VizProcessTransportFactory. 1. Call reset() instead of assigning nullptr for scoped_refptrs. 2. Don't reset |worker_context_provider_| if |main_context_provider_| fails to get created. Bug: 791660 Change-Id: Ib9610981ed165b220ae2129a2ebd11e52464f693 Reviewed-on: https://chromium-review.googlesource.com/1176343 Commit-Queue: kylechar <kylechar@chromium.org> Reviewed-by: Mohsen Izadi <mohsen@chromium.org> Cr-Commit-Position: refs/heads/master@{#583844} [modify] https://crrev.com/6fc8951dca75d6180ccac70190e60ef493dde963/content/browser/compositor/viz_process_transport_factory.cc
,
Aug 22
,
Aug 22
,
Aug 22
+sunnyps for Windows GPU team input. The easiest and maybe best solution is to remove the check in RenderingWindowManager if there is already a child HWND for some parent HWND. Older child HWNDs will be destroyed by the GPU process anyways, so only the last child HWND will matter, which we'll keep track of. The GPU process can already draw whatever it likes into the browser window. If a misbehaving GPU process registers two child HWNDs for one parent HWND they'll just draw on top of one another. Are there any other security concerns here?
,
Aug 23
Just to be clear, the issue is that the old RegisterChild IPC arrives in the browser process between the new RegisterParent and RegisterChild calls, right? Is this between two GPU channels being created, between GPU process termination and restart, between GPU process termination and software compositor creation, or all three? I suspect it's the latter two because GPU process restart should involve recreating the GpuHost mojo binding so we shouldn't be handling messages from the old GPU process? I don't think it's safe to call RegisterChild with the destroyed HWND. We call SetParent with the HWND, and since HWNDs are not handles, it could belong to another window. I'm not certain, but it seems like this would be a security concern. Can we pass a base::UnguessableToken when creating the GpuChannel, and check against that when we receive the SetChildSurface IPC ignoring the message when it doesn't match?
,
Aug 24
>Just to be clear, the issue is that the old RegisterChild IPC arrives in the browser process between the new RegisterParent and RegisterChild calls, right? Yep! >Is this between two GPU channels being created, between GPU process termination and restart, between GPU process termination and software compositor creation, or all three? It's possible anytime there are two requests to create a RootCompositorFrameSink(), which create a Display/OutputSurface and either GLSurface or SoftwareOutputDevice, for the same browser HWND before the IPC that triggers RWM::RegisterChild() arrives for the first request. It could happen because of a switch to software compositing. A more common case is after the GPU process crashes the mojom::CompositorFrameSink message pipe is notified about a connection error before the GPU channel message pipe. The LayerTreeFrameSink sees the connection error and a new LayerTreeFrameSink gets created which calls CreateRootCompositorFrameSink(). Both GpuChannelHost and ContextProvider don't know anything has happened yet and aren't lost when creating a new LayerTreeFrameSink. By the time the request for the RootCompositorFrameSink is received in the GPU process, the browser has seen the context loss, destroyed the new LayerTreeFrameSink and created another new one with a new ContextProvider. There is probably some improvements possible here but things are complicated by the fact there are many message pipes between the browser and GPU. The order those message pipes find out about connection loss can vary. >I don't think it's safe to call RegisterChild with the destroyed HWND. We call SetParent with the HWND, and since HWNDs are not handles, it could belong to another window. I'm not certain, but it seems like this would be a security concern. We actually have no guarantee today the child HWND is a valid child window. If something has gone wrong the child window might have already been destroyed in the GPU process by the time the IPC arrives in the browser. Additionally if the GPU process is malicious it can just lie about the child HWND. I'm not sure if this presents any kind of security problem today? >Can we pass a base::UnguessableToken when creating the GpuChannel, and check against that when we receive the SetChildSurface IPC ignoring the message when it doesn't match? The IPCs to call RegisterChild() aren't just for GPU composited windows anymore. We also create child windows for software compositing so there isn't necessarily a GpuChannel. We could tie the UnguessableToken to the call to the CreateRootCompositorFrameSink(), but that will only work with OOP-D and would require some changes how gpu::ChildWindowWin triggers the IPC to call RegisterChild() in the browser.
,
Aug 24
Oh I missed that check the child HWND in GpuProcess Host. So we do know that child HWND exists and is a valid window created in the GPU process because of the call to GetWindowThreadProcessId() at [1]. That wouldn't change. [1] https://cs.chromium.org/chromium/src/content/browser/gpu/gpu_process_host.cc?l=1288&rcl=6a1ea3ac8e0d2a4619207345ea8aba022a5b0ffd
,
Aug 28
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/310d28a889dd3d64950319fe9fec641e09f8ea35 commit 310d28a889dd3d64950319fe9fec641e09f8ea35 Author: kylechar <kylechar@chromium.org> Date: Tue Aug 28 19:15:21 2018 Refactor RenderingWindowManager. Remove RenderingWindowManager::DoSetParentOnChild(). This was added so that the call to SetParent() could happen on the UI thread instead of the IO thread where RegisterChild() is called. That sequence doesn't work with OOP-D as RegisterChild() isn't guaranteed to be called before DoSetParentOnChild(). RenderingWindowManager was already modified to call SetParent() on a worker thread with OOP-D. Also call SetParent() on a worker thread without OOP-D. Add a check on the worker thread that the parent/child hasn't changed since the task was posted for extra safety. Also switch out base::Singleton for base::NoDestructor. Bug: 791660 Change-Id: Id78684f63906f866577419c0975969a2e605760a Reviewed-on: https://chromium-review.googlesource.com/1176358 Reviewed-by: ccameron <ccameron@chromium.org> Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org> Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org> Commit-Queue: kylechar <kylechar@chromium.org> Cr-Commit-Position: refs/heads/master@{#586807} [modify] https://crrev.com/310d28a889dd3d64950319fe9fec641e09f8ea35/content/browser/compositor/gpu_process_transport_factory.cc [modify] https://crrev.com/310d28a889dd3d64950319fe9fec641e09f8ea35/ui/compositor/host/host_context_factory_private.cc [modify] https://crrev.com/310d28a889dd3d64950319fe9fec641e09f8ea35/ui/gfx/win/rendering_window_manager.cc [modify] https://crrev.com/310d28a889dd3d64950319fe9fec641e09f8ea35/ui/gfx/win/rendering_window_manager.h
,
Sep 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/89341da811392b6e29e1f9df03156a55d093e688 commit 89341da811392b6e29e1f9df03156a55d093e688 Author: kylechar <kylechar@chromium.org> Date: Mon Sep 10 15:26:27 2018 Make RenderingWindowManager access happen only on UI thread. Remove the ability to access RenderingWindowManager on any thread and remove locks. Make RegisterChild() PostTask back to the UI thread if necessary so it can still be called anywhere. There is already a PostTask before ::SetParent() gets called, so this shouldn't add much extra latency before things show up on screen. Also move the checks that parent HWND was registered and child HWND was created by expected process id to happen immediately before calling ::SetParent(). This ensures that child HWND belongs to the right process. We don't need to check the parent HWND process because we check it registered by the browser process. This also fixes a race with OOP-D where RegisterChild() can get called more than once due to races between different message pipes. Bug: 791660 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel Change-Id: If8e98bd974dbc77e1be21252d3aa1682f3e6e016 Reviewed-on: https://chromium-review.googlesource.com/1194580 Commit-Queue: kylechar <kylechar@chromium.org> Reviewed-by: Antoine Labour <piman@chromium.org> Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org> Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org> Cr-Commit-Position: refs/heads/master@{#589919} [modify] https://crrev.com/89341da811392b6e29e1f9df03156a55d093e688/components/viz/host/gpu_host_impl.cc [modify] https://crrev.com/89341da811392b6e29e1f9df03156a55d093e688/components/viz/service/display_embedder/gpu_display_provider.cc [modify] https://crrev.com/89341da811392b6e29e1f9df03156a55d093e688/services/ws/gpu_host/gpu_host.cc [modify] https://crrev.com/89341da811392b6e29e1f9df03156a55d093e688/ui/gfx/win/rendering_window_manager.cc [modify] https://crrev.com/89341da811392b6e29e1f9df03156a55d093e688/ui/gfx/win/rendering_window_manager.h
,
Sep 14
Merge request for #13. This fixes a potential bug where a window could go black after a GPU crash with OOP-D enabled. It's been in canary for a few days without any issues reported.
,
Sep 14
This bug requires manual review: M70 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Sep 14
,
Sep 17
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/187e8bde1ad72ca01b98e1bddd00e8ca94da7993 commit 187e8bde1ad72ca01b98e1bddd00e8ca94da7993 Author: kylechar <kylechar@chromium.org> Date: Mon Sep 17 13:00:59 2018 Make RenderingWindowManager access happen only on UI thread. Remove the ability to access RenderingWindowManager on any thread and remove locks. Make RegisterChild() PostTask back to the UI thread if necessary so it can still be called anywhere. There is already a PostTask before ::SetParent() gets called, so this shouldn't add much extra latency before things show up on screen. Also move the checks that parent HWND was registered and child HWND was created by expected process id to happen immediately before calling ::SetParent(). This ensures that child HWND belongs to the right process. We don't need to check the parent HWND process because we check it registered by the browser process. This also fixes a race with OOP-D where RegisterChild() can get called more than once due to races between different message pipes. TBR=kylechar@chromium.org (cherry picked from commit 89341da811392b6e29e1f9df03156a55d093e688) Bug: 791660 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel Change-Id: If8e98bd974dbc77e1be21252d3aa1682f3e6e016 Reviewed-on: https://chromium-review.googlesource.com/1194580 Commit-Queue: kylechar <kylechar@chromium.org> Reviewed-by: Antoine Labour <piman@chromium.org> Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org> Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#589919} Reviewed-on: https://chromium-review.googlesource.com/1227839 Reviewed-by: kylechar <kylechar@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#444} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811} [modify] https://crrev.com/187e8bde1ad72ca01b98e1bddd00e8ca94da7993/components/viz/host/gpu_host_impl.cc [modify] https://crrev.com/187e8bde1ad72ca01b98e1bddd00e8ca94da7993/components/viz/service/display_embedder/gpu_display_provider.cc [modify] https://crrev.com/187e8bde1ad72ca01b98e1bddd00e8ca94da7993/services/ws/gpu_host/gpu_host.cc [modify] https://crrev.com/187e8bde1ad72ca01b98e1bddd00e8ca94da7993/ui/gfx/win/rendering_window_manager.cc [modify] https://crrev.com/187e8bde1ad72ca01b98e1bddd00e8ca94da7993/ui/gfx/win/rendering_window_manager.h
,
Sep 17
|
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by kylec...@chromium.org
, Dec 11 2017