New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 801720 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Destroying CommandBufferProxy during a context loss causes a crash

Project Member Reported by bajones@chromium.org, Jan 12 2018

Issue description

If the WebGL DrawingBuffer is deleted while handling context loss it appears to cause a crash in the CommandBufferProxy (which the drawing buffer owns).

This behavior was seen in context_lost_tests during the GPUProcessCrashesExactlyOncePerVisitToAboutGpuCrash when a change was made to WebGLRenderingContextBase to remove it's reference to drawing_buffer_ during the context loss handler. It yielded the following call stack:

[34099:775:0112/121950.545982:ERROR:gpu_process_transport_factory.cc(1019)] Lost UI shared context.
[34107:775:0112/121950.537115:FATAL:lock_impl_posix.cc(76)] Check failed: rv == 0 (22 vs. 0). Invalid argument
0   Chromium Framework                  0x000000011094483c base::debug::StackTrace::StackTrace(unsigned long) + 28
1   Chromium Framework                  0x0000000110969a12 logging::LogMessage::~LogMessage() + 210
2   Chromium Framework                  0x00000001109e2807 base::internal::LockImpl::Lock() + 231
3   Chromium Framework                  0x000000010dd65d40 gpu::CommandBufferProxyImpl::OnGpuAsyncMessageError(gpu::error::ContextLostReason, gpu::error::Error) + 192
4   Chromium Framework                  0x000000010dd65e05 gpu::CommandBufferProxyImpl::OnChannelError() + 149
5   Chromium Framework                  0x000000010dd6ed07 base::internal::Invoker<base::internal::BindState<void (IPC::Listener::*)(), base::WeakPtr<IPC::Listener> >, void ()>::Run(base::internal::BindStateBase*) + 183
6   Chromium Framework                  0x00000001109451b5 base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask*) + 261
[Snip...]

The cause seems to be that the loss of all references of the DrawingBuffer causes the CommandBufferProxy to be destroyed while it is in the middle of a statement that has an auto lock. When the statement exist the autolock attempts to unlock and references a now deleted lock.

The proposed solution is to hold on to the DrawingBuffer reference in a task fired from the context lost handling code will will keep the DrawingBuffer alive until after the current CommandBufferProxy code has completed, preserving the autolock integrity.

 

Comment 1 by piman@chromium.org, Jan 12 2018

Cc: danakj@chromium.org sunn...@chromium.org
Looking more closely at the code, I think it should be possible/not too hard to make CommandBufferProxyImpl safe wrt getting deleted from the callback *for single-thread contexts* (those without lock_) - making sure we release the last_state_lock_ before the callback (as we do now) and not reacquiring it afterwards (we don't need it).
It's a lot harder for those with locks - the lost context callback is semantically supposed to be called with the lock held, and the lock itself belongs to the ContextProviderCommandBuffer (which outlives CommandBufferProxyImpl), so there's no good simple semantics for the ContextProviderCommandBuffer (hence the lock) being destroyed from the lost context callback in that case.
Project Member

Comment 2 by bugdroid1@chromium.org, Jan 17 2018

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

commit 85bb86d691d38322326a480473be8494d89361fe
Author: Brandon Jones <bajones@chromium.org>
Date: Wed Jan 17 06:54:53 2018

Remove WebGL's DrawingBuffer immediately upon context loss

Fixes an issue that occured when the DrawingBuffer was deleted during a
call to LoseContextImpl that was initiated by the CommandBufferProxy it
owns. This allows the DrawingBuffer to be removed from the
WebGLRenderingContextBase immediately upon loss in all cases.

Bug:  801720 
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: If256babc75f2622312cac937dc80c6191c52138a
Reviewed-on: https://chromium-review.googlesource.com/868936
Reviewed-by: Kenneth Russell <kbr@chromium.org>
Commit-Queue: Brandon Jones <bajones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#529656}
[modify] https://crrev.com/85bb86d691d38322326a480473be8494d89361fe/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
[modify] https://crrev.com/85bb86d691d38322326a480473be8494d89361fe/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h

Status: Fixed (was: Assigned)

Sign in to add a comment