Fix gpu command buffer use after free by GrContext

Fix gpu command buffer use after free by GrContext

ContextProviderCommandBuffer owns a WebGraphicsContext3DCommandBufferImpl and a
GrContextForWebGraphicsContext3D via scoped_ptr. The problem was
that the GrContext object held by GrContextForWebGraphicsContext3D
depended on interface pointers that reference an interface that is owned
by WebGraphicsContext3DCommandBufferImpl, so whenever the
GrContext outlived the ContextProviderCommandBuffer, we ended up in a
state where the interface function pointers are deallocated, but still
referenced. Then, attempts to use the GrContext would result in using
deallocated function pointers. Because the GrContext is a ref counted
object, it can easily outlive the ContextProviderCommandBuffer. This led to
a dangerous situation where we had to be careful about object destruction
order.

This CL fixes the problem for good by wrapping the ownership of the
WebGraphicsContext3DCommandBufferImpl into a subclass of
GrGLInterface, which is a ref counted object that can be owned jointly by
the GrContext and the ContextProviderCommandBuffer, thus guaranteeing
that the command buffer interface will remain valid for the lifetimes of the
GrContext and of the ContextProviderCommandBuffer.

BUG=551143
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/d87aa1f1aee6ab0181eadaae827a5768981c1ccc
Cr-Commit-Position: refs/heads/master@{#359493}

[Justin Novosad, 2015-11-07 06:15:40]

Description was changed from

==========
Fix use after free error in AcceleratedImageBufferSurface destruction

This use after free error was cause by bad destruction order.
No clear repro other than clusterfuzz MSAN case, which does
not repro locally.

BUG=551143
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Fix use after free error in AcceleratedImageBufferSurface destruction

ContextProviderCommandBuffer owns a WebGraphicsContext3DCommandBufferImpl and a
GrContextForWebGraphicsContext3D via scoped_ptr. The problem was
that the GrContext object held by GrContextForWebGraphicsContext3D depended on
interface pointers that reference an interface that is owned
by WebGraphicsContext3DCommandBufferImpl, so whenever the
GrContext outlived the ContextProviderCommandBuffer, we ended up in a
state where the interface function pointers are deallocated, but still
referenced. Then, attempts to use the GrContext would result in using
deallocated function pointers. Because the GrContext is a ref counted
object, it can easily outlive the ContextProviderCommandBuffer. This led to
a dangerous situation where we had to be careful about object destruction
order.

This CL fixes the problem for good by wrapping the ownership of the
WebGraphicsContext3DCommandBufferImpl into a subclass of GrGLInterface, which is
a ref counted object that can be owned jointly by the GrContext and the
ContextProviderCommandBuffer, thus guaranteeing that the command buffer
interface will remain valid for the lifetimes of the GrContext and of the
ContextProviderCommandBuffer.

BUG=551143
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Justin Novosad, 2015-11-09 14:47:31]

Description was changed from

==========
Fix use after free error in AcceleratedImageBufferSurface destruction

ContextProviderCommandBuffer owns a WebGraphicsContext3DCommandBufferImpl and a
GrContextForWebGraphicsContext3D via scoped_ptr. The problem was
that the GrContext object held by GrContextForWebGraphicsContext3D depended on
interface pointers that reference an interface that is owned
by WebGraphicsContext3DCommandBufferImpl, so whenever the
GrContext outlived the ContextProviderCommandBuffer, we ended up in a
state where the interface function pointers are deallocated, but still
referenced. Then, attempts to use the GrContext would result in using
deallocated function pointers. Because the GrContext is a ref counted
object, it can easily outlive the ContextProviderCommandBuffer. This led to
a dangerous situation where we had to be careful about object destruction
order.

This CL fixes the problem for good by wrapping the ownership of the
WebGraphicsContext3DCommandBufferImpl into a subclass of GrGLInterface, which is
a ref counted object that can be owned jointly by the GrContext and the
ContextProviderCommandBuffer, thus guaranteeing that the command buffer
interface will remain valid for the lifetimes of the GrContext and of the
ContextProviderCommandBuffer.

BUG=551143
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Fix use after free error in AcceleratedImageBufferSurface destruction

ContextProviderCommandBuffer owns a WebGraphicsContext3DCommandBufferImpl and a
GrContextForWebGraphicsContext3D via scoped_ptr. The problem was
that the GrContext object held by GrContextForWebGraphicsContext3D
depended on interface pointers that reference an interface that is owned
by WebGraphicsContext3DCommandBufferImpl, so whenever the
GrContext outlived the ContextProviderCommandBuffer, we ended up in a
state where the interface function pointers are deallocated, but still
referenced. Then, attempts to use the GrContext would result in using
deallocated function pointers. Because the GrContext is a ref counted
object, it can easily outlive the ContextProviderCommandBuffer. This led to
a dangerous situation where we had to be careful about object destruction
order.

This CL fixes the problem for good by wrapping the ownership of the
WebGraphicsContext3DCommandBufferImpl into a subclass of
GrGLInterface, which is a ref counted object that can be owned jointly by
the GrContext and the ContextProviderCommandBuffer, thus guaranteeing
that the command buffer interface will remain valid for the lifetimes of the
GrContext and of the ContextProviderCommandBuffer.

BUG=551143
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Justin Novosad, 2015-11-09 15:21:03]

Description was changed from

==========
Fix use after free error in AcceleratedImageBufferSurface destruction

ContextProviderCommandBuffer owns a WebGraphicsContext3DCommandBufferImpl and a
GrContextForWebGraphicsContext3D via scoped_ptr. The problem was
that the GrContext object held by GrContextForWebGraphicsContext3D
depended on interface pointers that reference an interface that is owned
by WebGraphicsContext3DCommandBufferImpl, so whenever the
GrContext outlived the ContextProviderCommandBuffer, we ended up in a
state where the interface function pointers are deallocated, but still
referenced. Then, attempts to use the GrContext would result in using
deallocated function pointers. Because the GrContext is a ref counted
object, it can easily outlive the ContextProviderCommandBuffer. This led to
a dangerous situation where we had to be careful about object destruction
order.

This CL fixes the problem for good by wrapping the ownership of the
WebGraphicsContext3DCommandBufferImpl into a subclass of
GrGLInterface, which is a ref counted object that can be owned jointly by
the GrContext and the ContextProviderCommandBuffer, thus guaranteeing
that the command buffer interface will remain valid for the lifetimes of the
GrContext and of the ContextProviderCommandBuffer.

BUG=551143
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Fix gpu command buffer use after free by GrContext

ContextProviderCommandBuffer owns a WebGraphicsContext3DCommandBufferImpl and a
GrContextForWebGraphicsContext3D via scoped_ptr. The problem was
that the GrContext object held by GrContextForWebGraphicsContext3D
depended on interface pointers that reference an interface that is owned
by WebGraphicsContext3DCommandBufferImpl, so whenever the
GrContext outlived the ContextProviderCommandBuffer, we ended up in a
state where the interface function pointers are deallocated, but still
referenced. Then, attempts to use the GrContext would result in using
deallocated function pointers. Because the GrContext is a ref counted
object, it can easily outlive the ContextProviderCommandBuffer. This led to
a dangerous situation where we had to be careful about object destruction
order.

This CL fixes the problem for good by wrapping the ownership of the
WebGraphicsContext3DCommandBufferImpl into a subclass of
GrGLInterface, which is a ref counted object that can be owned jointly by
the GrContext and the ContextProviderCommandBuffer, thus guaranteeing
that the command buffer interface will remain valid for the lifetimes of the
GrContext and of the ContextProviderCommandBuffer.

BUG=551143
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Justin Novosad, 2015-11-09 15:39:05]

junov@chromium.org changed reviewers:
+ danakj@chromium.org, kbr@chromium.org, piman@chromium.org, rbyers@chromium.org

[Justin Novosad, 2015-11-09 15:39:05]

Reviewers:
rbyers for third_party/WebKit/public/OWNERS
kbr for content/common/gpu/OWNERS and content/browser/gpu/OWNERS
piman for gpu/OWNERS
danakj for cc/OWNERS and ui/compositor/OWNERS

[Rick Byers, 2015-11-09 16:01:39]

WebKit/public RS LGTM

[piman, 2015-11-09 19:29:25]

My main concern is that now the context may be kept alive longer, and possibly
being destroyed on the wrong thread (it *must* be destroyed on the thread that
called InitializeOnCurrentThread). What guarantees do we have (can you please cc
me on the bug?)

https://codereview.chromium.org/1414683003/diff/80001/content/common/gpu/clie...
File content/common/gpu/client/context_provider_command_buffer.cc (right):

https://codereview.chromium.org/1414683003/diff/80001/content/common/gpu/clie...
content/common/gpu/client/context_provider_command_buffer.cc:119:
DCHECK(context_thread_checker_.CalledOnValidThread());
nit: can remove those, since they're covered by WebContext3D()

[Justin Novosad, 2015-11-09 19:40:42]

On 2015/11/09 19:29:25, piman (slow to review) wrote:
> My main concern is that now the context may be kept alive longer, and possibly
> being destroyed on the wrong thread (it *must* be destroyed on the thread that
> called InitializeOnCurrentThread). What guarantees do we have (can you please
cc
> me on the bug?)

GrContext is also not allowed to be used cross-thread. It can only be used and
destructed on the same thread
as the associated WebGraphicsContext3D. That is true before and after this CL.
Still, I can add some checks.

> 
>
https://codereview.chromium.org/1414683003/diff/80001/content/common/gpu/clie...
> File content/common/gpu/client/context_provider_command_buffer.cc (right):
> 
>
https://codereview.chromium.org/1414683003/diff/80001/content/common/gpu/clie...
> content/common/gpu/client/context_provider_command_buffer.cc:119:
> DCHECK(context_thread_checker_.CalledOnValidThread());
> nit: can remove those, since they're covered by WebContext3D()

[Justin Novosad, 2015-11-09 21:01:25]

junov@chromium.org changed reviewers:
+ boliu@chromium.org

[Justin Novosad, 2015-11-09 21:01:25]

+ boliu for content/browser/android/in_process/OWNERS

[piman, 2015-11-09 23:24:02]

lgtm

[boliu, 2015-11-10 00:51:30]

in_process lgtm

https://codereview.chromium.org/1414683003/diff/120001/content/browser/androi...
File content/browser/android/in_process/context_provider_in_process.cc (right):

https://codereview.chromium.org/1414683003/diff/120001/content/browser/androi...
content/browser/android/in_process/context_provider_in_process.cc:81:
gr_interface_->WebContext3D());
nit: This can just call WebContext3D (or maybe the other way around makes
sense?), then the DCHECKs don't need to be repeated.

[Ken Russell (switch to Gerrit), 2015-11-11 19:14:49]

lgtm if this has been tested, but I didn't check all of the code that uses this
in detail. Could you please also CC: me on the bug report?

https://codereview.chromium.org/1414683003/diff/170001/content/common/gpu/cli...
File content/common/gpu/client/context_provider_command_buffer.cc (right):

https://codereview.chromium.org/1414683003/diff/170001/content/common/gpu/cli...
content/common/gpu/client/context_provider_command_buffer.cc:85:
ContextProviderCommandBuffer::WebContext3DNoChecks() {
Why is this internal entry point needed? At least gr_interface_ needs to be
non-null (verified with the DCHECK above) to avoid a crash here. Can this be
removed in favor of WebContext3D()?

https://codereview.chromium.org/1414683003/diff/170001/content/common/gpu/cli...
content/common/gpu/client/context_provider_command_buffer.cc:102:
gr_interface_->BindToCurrentThread();
This call to GrGLInterfaceForWebGraphicsContext3D::BindToCurrentThread looks
like the biggest semantic change to me. Is that correct? Why is this needed if
the GrContext can't be used cross-thread?

[Ken Russell (switch to Gerrit), 2015-11-11 19:16:52]

https://codereview.chromium.org/1414683003/diff/170001/content/browser/gpu/gp...
File content/browser/gpu/gpu_ipc_browsertests.cc (right):

https://codereview.chromium.org/1414683003/diff/170001/content/browser/gpu/gp...
content/browser/gpu/gpu_ipc_browsertests.cc:209:
MAYBE_GrContextKeepsGpuChannelAlive) {
It looks like this new test's failing on some of the trybots.

[Justin Novosad, 2015-11-12 19:18:46]

https://codereview.chromium.org/1414683003/diff/170001/content/common/gpu/cli...
File content/common/gpu/client/context_provider_command_buffer.cc (right):

https://codereview.chromium.org/1414683003/diff/170001/content/common/gpu/cli...
content/common/gpu/client/context_provider_command_buffer.cc:85:
ContextProviderCommandBuffer::WebContext3DNoChecks() {
On 2015/11/11 19:14:49, Ken Russell wrote:
> Why is this internal entry point needed? At least gr_interface_ needs to be
> non-null (verified with the DCHECK above) to avoid a crash here. Can this be
> removed in favor of WebContext3D()?

It is because some times we need to access the context at times when the
callback is not yet configured, which was triggering DCHECKS. But you're right,
I should still DCHECK(gr_interface_) here.

https://codereview.chromium.org/1414683003/diff/170001/content/common/gpu/cli...
content/common/gpu/client/context_provider_command_buffer.cc:102:
gr_interface_->BindToCurrentThread();
On 2015/11/11 19:14:48, Ken Russell wrote:
> This call to GrGLInterfaceForWebGraphicsContext3D::BindToCurrentThread looks
> like the biggest semantic change to me. Is that correct? Why is this needed if
> the GrContext can't be used cross-thread?

This is just for DCHECKs. It sets up a ThreadChecker object to make sure the
GrGLInterfaceForWebGraphicsContext3D is destroyed on the correct thread.

[Justin Novosad, 2015-11-12 21:36:37]

junov@chromium.org changed reviewers:
+ dtrainor@chromium.org

[Justin Novosad, 2015-11-12 21:36:38]

+dtrainor for blimp/OWNERS

[David Trainor- moved to gerrit, 2015-11-12 22:06:03]

blimp lgtm

[Justin Novosad, 2015-11-13 03:41:55]

The CQ bit was checked by junov@chromium.org

[Justin Novosad, 2015-11-13 03:41:55]

The patchset sent to the CQ was uploaded after l-g-t-m from rbyers@chromium.org,
piman@chromium.org, boliu@chromium.org, kbr@chromium.org, dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/1414683003/#ps250001
(title: "test fixup")

[commit-bot: I haz the power, 2015-11-13 03:42:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414683003/250001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414683003/250001

[commit-bot: I haz the power, 2015-11-13 04:47:23]

Committed patchset #14 (id:250001)

[commit-bot: I haz the power, 2015-11-13 04:48:49]

Patchset 14 (id:??) landed as
https://crrev.com/d87aa1f1aee6ab0181eadaae827a5768981c1ccc
Cr-Commit-Position: refs/heads/master@{#359493}

[cbiesinger, 2015-11-13 18:57:40]

A revert of this CL (patchset #14 id:250001) has been created in
https://codereview.chromium.org/1444683002/ by cbiesinger@chromium.org.

The reason for reverting is: Reverting because this breaks several layout tests:
STDERR:
[3888:1580:1113/081604:621866:FATAL:context_provider_command_buffer.cc(79)]
Check failed: context_thread_checker_.CalledOnValidThread(). 
STDERR: Backtrace:
STDERR: 	base::debug::StackTrace::StackTrace [0x00D8DC31+33]
STDERR: 	logging::LogMessage::~LogMessage [0x00DE69DB+75]
STDERR: 	content::ContextProviderCommandBuffer::WebContext3D [0x11BB5DA7+887]
STDERR: 	content::ContextProviderCommandBuffer::ContextGL [0x11BB481B+267]
STDERR: 	content::RenderThreadImpl::SharedWorkerContextProvider [0x1202B2A0+400]
STDERR: 	content::RenderWidget::CreateOutputSurface [0x12089254+452]
STDERR: 	content::RenderWidgetCompositor::RequestNewOutputSurface
[0x11E7767F+127]


https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Win7__dbg...

https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7%20%28dbg%....