ContextProviderCommandBuffer and MemoryDumpProvider have different expectations for calling threads |
|||
Issue descriptionThe ContextProviderCommandBuffer allows for destruction on either the main thread or the thread it is bound to (https://cs.chromium.org/chromium/src/services/ws/public/cpp/gpu/context_provider_command_buffer.cc?rcl=9e78ce89bf6d39341b4cf64c5f80105c7dc598de&l=80), where as the MemoryDumpProvider requires to be strictly called from the thread the ContextProviderCommandBuffer is called from. Since the ContextProviderCommandBuffer calls into the MemoryDumpProvider directly (https://cs.chromium.org/chromium/src/services/ws/public/cpp/gpu/context_provider_command_buffer.cc?rcl=9e78ce89bf6d39341b4cf64c5f80105c7dc598de&l=90), this relationship should be revised. Some suggestions are: - Narrow what is acceptable for ContextProviderCommandBuffer (ie, have it only be able to be destroyed on the bound thread). - Post all calls to MemoryDumpProvider from ContextProviderCommandBuffer.
,
Oct 26
Thanks for the analysis. I think we should remove the DCHECK allowing the ContextProvider to be destroyed on the main thread. Anyone who did would already fail the DCHECK in the MemoryDumpProvider anyway. I double checked and the compositor's context is destroyed on the bound thread even though the LayerTreeFrameSink is not. I think the main thread allowance is historical now.
,
Oct 27
This is related to issue 898600 , which I was looking at. I agree with danakj@ - we should just always DCHECK that ContextProvider is cleaned up on the binding thread, if it was ever bound. There's the weird case where we destroy the context before binding it, in which case we may not have a thread checker for the context thread. In this case it should be safe enough to destroy on any thread though (will check a bit more). Despite MemoryDumpProvider being the only place with a DCHECK, I think other parts of the code were relying on us being destroyed on the context thread, as we pass a task runner for that thread to a number of places, and they all manipulate the ContextProviderCommandBuffer's internals via the task runner. |
|||
►
Sign in to add a comment |
|||
Comment 1 by lethalantidote@chromium.org
, Oct 26