Establish GPU connection early in RenderProcessHostImpl |
||||
Issue descriptionThe original discussion is on crbug.com/768253, #28 Right now, RenderProcessHostImpl creates GPU connection on demand, i.e., once a renderer process (RenderThreadImpl) asks for a Gpu channel from the browser, the browser side (RenderProcessHostImpl) establishes a Gpu connection and return it to the renderer. However, we can establish Gpu connection early in RenderProcessHostImpl as soon as we create the renderer process, so when RenderThreadImpl asks for it through a sync call, it can be passed on directly.
,
Nov 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b5dec6f0f0a08f99d75431551b3ded92ae6b4eca commit b5dec6f0f0a08f99d75431551b3ded92ae6b4eca Author: Zhenyao Mo <zmo@chromium.org> Date: Thu Nov 30 06:03:35 2017 PreEstablish a GPU channel in RenderProcessHostImpl before renderer asks for one. BUG= 783512 TEST=bots R=piman@chromium.org Change-Id: I331bc073ebb2fb77aaffd074fd97e979b057ee44 Reviewed-on: https://chromium-review.googlesource.com/773538 Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org> Reviewed-by: Antoine Labour <piman@chromium.org> Commit-Queue: Zhenyao Mo <zmo@chromium.org> Cr-Commit-Position: refs/heads/master@{#520456} [modify] https://crrev.com/b5dec6f0f0a08f99d75431551b3ded92ae6b4eca/content/browser/gpu/gpu_client.cc [modify] https://crrev.com/b5dec6f0f0a08f99d75431551b3ded92ae6b4eca/content/browser/gpu/gpu_client.h [modify] https://crrev.com/b5dec6f0f0a08f99d75431551b3ded92ae6b4eca/content/browser/renderer_host/render_process_host_impl.cc [modify] https://crrev.com/b5dec6f0f0a08f99d75431551b3ded92ae6b4eca/content/browser/renderer_host/render_process_host_impl.h [modify] https://crrev.com/b5dec6f0f0a08f99d75431551b3ded92ae6b4eca/services/ui/public/cpp/gpu/gpu.cc
,
Nov 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f9b2a9030b90d5388529fe1dd1d00a87c773490f commit f9b2a9030b90d5388529fe1dd1d00a87c773490f Author: Henrik Andreasson <henrika@chromium.org> Date: Thu Nov 30 08:43:02 2017 Revert "PreEstablish a GPU channel in RenderProcessHostImpl before renderer asks for one." This reverts commit b5dec6f0f0a08f99d75431551b3ded92ae6b4eca. Reason for revert: Seems to break browser tests on linux-chromeos-dbg Builder. https://uberchromegw.corp.google.com/i/chromium.chromiumos/builders/linux-chromeos-dbg First round of failing tests: https://uberchromegw.corp.google.com/i/chromium.chromiumos/builders/linux-chromeos-dbg/builds/2911 Contains error messages that leads me to suspect this CL (GPU related). [15470:15753:1129/231746.825540:FATAL:interface_endpoint_client.cc(32)] Check failed: !is_valid. The callback passed to Gpu::EstablishGpuChannel() was never run. It should either be run before destruction, or else destroyed after first closing the pipe. Original change's description: > PreEstablish a GPU channel in RenderProcessHostImpl before renderer asks for one. > > BUG= 783512 > TEST=bots > R=piman@chromium.org > > Change-Id: I331bc073ebb2fb77aaffd074fd97e979b057ee44 > Reviewed-on: https://chromium-review.googlesource.com/773538 > Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org> > Reviewed-by: Antoine Labour <piman@chromium.org> > Commit-Queue: Zhenyao Mo <zmo@chromium.org> > Cr-Commit-Position: refs/heads/master@{#520456} TBR=sadrul@chromium.org,zmo@chromium.org,fsamuel@chromium.org,piman@chromium.org Change-Id: I18060e5d7fb9903b21eff7a40de05efa5a5bb295 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 783512 Reviewed-on: https://chromium-review.googlesource.com/799870 Reviewed-by: Henrik Andreasson <henrika@chromium.org> Commit-Queue: Henrik Andreasson <henrika@chromium.org> Cr-Commit-Position: refs/heads/master@{#520477} [modify] https://crrev.com/f9b2a9030b90d5388529fe1dd1d00a87c773490f/content/browser/gpu/gpu_client.cc [modify] https://crrev.com/f9b2a9030b90d5388529fe1dd1d00a87c773490f/content/browser/gpu/gpu_client.h [modify] https://crrev.com/f9b2a9030b90d5388529fe1dd1d00a87c773490f/content/browser/renderer_host/render_process_host_impl.cc [modify] https://crrev.com/f9b2a9030b90d5388529fe1dd1d00a87c773490f/content/browser/renderer_host/render_process_host_impl.h [modify] https://crrev.com/f9b2a9030b90d5388529fe1dd1d00a87c773490f/services/ui/public/cpp/gpu/gpu.cc
,
Nov 30 2017
It sounds like the CL exposed a race in ui::Gpu or one of its clients. I wonder if this might be related to crbug.com/787774
,
Nov 30 2017
The stacktrace: [17154:17181:1129/231806.541038:FATAL:interface_endpoint_client.cc(32)] Check failed: !is_valid. The callback passed to Gpu::EstablishGpuChannel() was never run. It should either be run before destruction, or else destroyed after first closing the pipe. #0 0x7f1d686c25dd base::debug::StackTrace::StackTrace() #1 0x7f1d686c0b5c base::debug::StackTrace::StackTrace() #2 0x7f1d6874591d logging::LogMessage::~LogMessage() #3 0x7f1d642625ec mojo::(anonymous namespace)::DCheckIfInvalid() #4 0x7f1d64262461 mojo::(anonymous namespace)::ResponderThunk::DCheckInvalid() #5 0x7f1d5f6df39c ui::mojom::Gpu_EstablishGpuChannel_ProxyToResponder::~Gpu_EstablishGpuChannel_ProxyToResponder() #6 0x7f1d5f6dea90 base::internal::PassedWrapper<>::~PassedWrapper() #7 0x7f1d5f6df255 std::__1::__tuple_leaf<>::~__tuple_leaf() #8 0x7f1d5f6df235 _ZNSt3__112__tuple_implINS_15__tuple_indicesIJLm0EEEEJN4base8internal13PassedWrapperINS_10unique_ptrIN2ui5mojom40Gpu_EstablishGpuChannel_ProxyToResponderENS_14default_deleteIS9_EEEEEEEED2Ev #9 0x7f1d5f6df215 _ZNSt3__15tupleIJN4base8internal13PassedWrapperINS_10unique_ptrIN2ui5mojom40Gpu_EstablishGpuChannel_ProxyToResponderENS_14default_deleteIS7_EEEEEEEED2Ev #10 0x7f1d5f6df1e3 _ZN4base8internal9BindStateIMN2ui5mojom40Gpu_EstablishGpuChannel_ProxyToResponderEFviN4mojo16ScopedHandleBaseINS5_17MessagePipeHandleEEERKN3gpu7GPUInfoERKNS9_14GpuFeatureInfoEEJNS0_13PassedWrapperINSt3__110unique_ptrIS4_NSJ_14default_deleteIS4_EEEEEEEED2Ev #11 0x7f1d5f6df1a7 _ZN4base8internal9BindStateIMN2ui5mojom40Gpu_EstablishGpuChannel_ProxyToResponderEFviN4mojo16ScopedHandleBaseINS5_17MessagePipeHandleEEERKN3gpu7GPUInfoERKNS9_14GpuFeatureInfoEEJNS0_13PassedWrapperINSt3__110unique_ptrIS4_NSJ_14default_deleteIS4_EEEEEEEE7DestroyEPKNS0_13BindStateBaseE #12 0x7f1d68670705 base::internal::BindStateBaseRefCountTraits::Destruct() #13 0x7f1d68671118 base::RefCountedThreadSafe<>::Release() #14 0x7f1d686710d5 scoped_refptr<>::Release() #15 0x7f1d68670e12 scoped_refptr<>::operator=() #16 0x7f1d68670939 base::internal::CallbackBase::Reset() #17 0x7f1d6057e2d3 content::GpuClient::OnError() ^^^ Instead of Reset()ing on error, maybe you still need to run the callback? (although running the callback would just not do anything since error, I think?) rockot@: if I get a callback from the client that I am going to ack in the future, but the client dies, then do we still need to run the callback? Is there a better way to dispose the callback?
,
Nov 30 2017
It looks to me the following happened: 1) EstablishGpuChannel(callback) is called, but a PreEstablishGpuChannel is ongoing, so we cache the callback. 2) OnError() happened. At this point, this my CL, we simply reset the callback_, which is a mistake and triggered this crash. Instead. we should execute callback_ with empty data and then reset it.
,
Nov 30 2017
You need to either run the callback or close the binding that received the callback. Rather than running the callback after you know the client is dead, I would recommend just closing the binding.
,
Nov 30 2017
@rockot: if you look at GpuClient::OnError(), if bindings_ are not empty, we did nothing. So we should do bindings_.CloseAllBindings() instead?
,
Nov 30 2017
Does BindingSet::CloseAllBindings() actually close the bindings? I'm looking at it, and it just calls std::map::clear().
,
Nov 30 2017
Sorry didn't realize we were talking about a BindingSet. You *probably* don't want to CloseAllBindings just because one client is in a bad state. Re #9: Yes, if you look at what the map actually owns, the values are essentially Binding scopers and so clearing the map closes all bindings.
,
Nov 30 2017
AH, misleading naming ... Looking at ui::mojom::Gpu, I don't see a close function. How do we actually close a binding?
,
Nov 30 2017
RemoveBinding closes a specific binding using the BindingId that was originally returned by AddBinding. BUT if you're receiving an OnError call, the relevant binding was already closed and removed from the set. If you're hitting this DCHECK it's because the relevant binding is still open at the time you're destroying the callback, which means you're dropping a callback for a binding that didn't receive an OnError notification. I don't understand the code well enough to know why you're hitting this, but hopefully that information is helpful.
,
Nov 30 2017
My understanding is that there can be more than 1 binding to the same renderer (I count up to 3), and if the renderer crashes we receive OnError (more than once possibly?), but the first one is not necessarily specific to the one binding that asked for the GPU channel. Maybe we can clear the callback after the (!bindings_.empty()) test?
,
Nov 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e36bfd5b9a11989786d4a40afe4d2f21a941b979 commit e36bfd5b9a11989786d4a40afe4d2f21a941b979 Author: Zhenyao Mo <zmo@chromium.org> Date: Thu Nov 30 22:25:31 2017 [reland] PreEstablish a GPU channel in RenderProcessHostImpl before renderer asks for one. This CL was originally reviewed in https://chromium-review.googlesource.com/c/chromium/src/+/773538. Patchset 1 is exactly the same. Later patches are new fixes. BUG= 783512 TEST=bots R=piman@chromium.org TBR=sadrul@chromium.org Change-Id: If664ad636da5382fad35a49766e3a3162031f1ab Reviewed-on: https://chromium-review.googlesource.com/801333 Commit-Queue: Zhenyao Mo <zmo@chromium.org> Reviewed-by: Antoine Labour <piman@chromium.org> Reviewed-by: Zhenyao Mo <zmo@chromium.org> Cr-Commit-Position: refs/heads/master@{#520719} [modify] https://crrev.com/e36bfd5b9a11989786d4a40afe4d2f21a941b979/content/browser/gpu/gpu_client.cc [modify] https://crrev.com/e36bfd5b9a11989786d4a40afe4d2f21a941b979/content/browser/gpu/gpu_client.h [modify] https://crrev.com/e36bfd5b9a11989786d4a40afe4d2f21a941b979/content/browser/renderer_host/render_process_host_impl.cc [modify] https://crrev.com/e36bfd5b9a11989786d4a40afe4d2f21a941b979/content/browser/renderer_host/render_process_host_impl.h [modify] https://crrev.com/e36bfd5b9a11989786d4a40afe4d2f21a941b979/services/ui/public/cpp/gpu/gpu.cc
,
Dec 5 2017
It seems my CL will stay. |
||||
►
Sign in to add a comment |
||||
Comment 1 by zmo@chromium.org
, Nov 10 2017