From d87aa1f1aee6ab0181eadaae827a5768981c1ccc Mon Sep 17 00:00:00 2001 From: junov Date: Thu, 12 Nov 2015 20:47:15 -0800 Subject: 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 Review URL: https://codereview.chromium.org/1414683003 Cr-Commit-Position: refs/heads/master@{#359493} --- .../in_process/context_provider_in_process.cc | 48 ++++++++++++++-------- .../in_process/context_provider_in_process.h | 7 +++- 2 files changed, 35 insertions(+), 20 deletions(-) (limited to 'content/browser/android/in_process') diff --git a/content/browser/android/in_process/context_provider_in_process.cc b/content/browser/android/in_process/context_provider_in_process.cc index fda13aa..71f8010 100644 --- a/content/browser/android/in_process/context_provider_in_process.cc +++ b/content/browser/android/in_process/context_provider_in_process.cc @@ -22,11 +22,11 @@ class ContextProviderInProcess::LostContextCallbackProxy public: explicit LostContextCallbackProxy(ContextProviderInProcess* provider) : provider_(provider) { - provider_->context3d_->setContextLostCallback(this); + provider_->WebContext3DImpl()->setContextLostCallback(this); } virtual ~LostContextCallbackProxy() { - provider_->context3d_->setContextLostCallback(NULL); + provider_->WebContext3DImpl()->setContextLostCallback(NULL); } virtual void onContextLost() { @@ -49,10 +49,12 @@ scoped_refptr ContextProviderInProcess::Create( ContextProviderInProcess::ContextProviderInProcess( scoped_ptr context3d, const std::string& debug_name) - : context3d_(context3d.Pass()), - debug_name_(debug_name) { + : debug_name_(debug_name) { DCHECK(main_thread_checker_.CalledOnValidThread()); - DCHECK(context3d_); + DCHECK(context3d); + gr_interface_ = skia::AdoptRef(new GrGLInterfaceForWebGraphicsContext3D( + context3d.Pass())); + DCHECK(gr_interface_->WebContext3D()); context_thread_checker_.DetachFromThread(); } @@ -65,11 +67,20 @@ blink::WebGraphicsContext3D* ContextProviderInProcess::WebContext3D() { DCHECK(lost_context_callback_proxy_); // Is bound to thread. DCHECK(context_thread_checker_.CalledOnValidThread()); - return context3d_.get(); + return WebContext3DImpl(); +} + +gpu_blink::WebGraphicsContext3DInProcessCommandBufferImpl* + ContextProviderInProcess::WebContext3DImpl() { + DCHECK(gr_interface_->WebContext3D()); + + return + static_cast( + gr_interface_->WebContext3D()); } bool ContextProviderInProcess::BindToCurrentThread() { - DCHECK(context3d_); + DCHECK(WebContext3DImpl()); // This is called on the thread the context will be used. DCHECK(context_thread_checker_.CalledOnValidThread()); @@ -77,14 +88,15 @@ bool ContextProviderInProcess::BindToCurrentThread() { if (lost_context_callback_proxy_) return true; - if (!context3d_->InitializeOnCurrentThread()) + if (!WebContext3DImpl()->InitializeOnCurrentThread()) return false; + gr_interface_->BindToCurrentThread(); InitializeCapabilities(); const std::string unique_context_name = - base::StringPrintf("%s-%p", debug_name_.c_str(), context3d_.get()); - context3d_->traceBeginCHROMIUM("gpu_toplevel", + base::StringPrintf("%s-%p", debug_name_.c_str(), WebContext3DImpl()); + WebContext3DImpl()->traceBeginCHROMIUM("gpu_toplevel", unique_context_name.c_str()); lost_context_callback_proxy_.reset(new LostContextCallbackProxy(this)); @@ -96,9 +108,9 @@ void ContextProviderInProcess::DetachFromThread() { } void ContextProviderInProcess::InitializeCapabilities() { - capabilities_.gpu = context3d_->GetImplementation()->capabilities(); + capabilities_.gpu = WebContext3DImpl()->GetImplementation()->capabilities(); - size_t mapped_memory_limit = context3d_->GetMappedMemoryLimit(); + size_t mapped_memory_limit = WebContext3DImpl()->GetMappedMemoryLimit(); capabilities_.max_transfer_buffer_usage_bytes = mapped_memory_limit == WebGraphicsContext3DInProcessCommandBufferImpl::kNoLimit @@ -114,21 +126,21 @@ ContextProviderInProcess::ContextCapabilities() { } ::gpu::gles2::GLES2Interface* ContextProviderInProcess::ContextGL() { - DCHECK(context3d_); + DCHECK(WebContext3DImpl()); DCHECK(lost_context_callback_proxy_); // Is bound to thread. DCHECK(context_thread_checker_.CalledOnValidThread()); - return context3d_->GetGLInterface(); + return WebContext3DImpl()->GetGLInterface(); } ::gpu::ContextSupport* ContextProviderInProcess::ContextSupport() { - DCHECK(context3d_); + DCHECK(WebContext3DImpl()); if (!lost_context_callback_proxy_) return NULL; // Not bound to anything. DCHECK(context_thread_checker_.CalledOnValidThread()); - return context3d_->GetContextSupport(); + return WebContext3DImpl()->GetContextSupport(); } class GrContext* ContextProviderInProcess::GrContext() { @@ -138,7 +150,7 @@ class GrContext* ContextProviderInProcess::GrContext() { if (gr_context_) return gr_context_->get(); - gr_context_.reset(new GrContextForWebGraphicsContext3D(context3d_.get())); + gr_context_.reset(new GrContextForWebGraphicsContext3D(gr_interface_)); return gr_context_->get(); } @@ -151,7 +163,7 @@ void ContextProviderInProcess::InvalidateGrContext(uint32_t state) { } void ContextProviderInProcess::SetupLock() { - context3d_->SetLock(&context_lock_); + WebContext3DImpl()->SetLock(&context_lock_); } base::Lock* ContextProviderInProcess::GetLock() { diff --git a/content/browser/android/in_process/context_provider_in_process.h b/content/browser/android/in_process/context_provider_in_process.h index 761caf0..81726d6 100644 --- a/content/browser/android/in_process/context_provider_in_process.h +++ b/content/browser/android/in_process/context_provider_in_process.h @@ -12,6 +12,7 @@ #include "base/synchronization/lock.h" #include "base/threading/thread_checker.h" #include "cc/blink/context_provider_web_context.h" +#include "skia/ext/refptr.h" namespace blink { class WebGraphicsContext3D; } @@ -22,6 +23,7 @@ class WebGraphicsContext3DInProcessCommandBufferImpl; namespace content { class GrContextForWebGraphicsContext3D; +class GrGLInterfaceForWebGraphicsContext3D; class ContextProviderInProcess : NON_EXPORTED_BASE(public cc_blink::ContextProviderWebContext) { @@ -41,6 +43,8 @@ class ContextProviderInProcess // cc_blink::ContextProviderWebContext: blink::WebGraphicsContext3D* WebContext3D() override; + gpu_blink::WebGraphicsContext3DInProcessCommandBufferImpl* WebContext3DImpl(); + // cc::ContextProvider: bool BindToCurrentThread() override; void DetachFromThread() override; @@ -61,8 +65,7 @@ class ContextProviderInProcess base::ThreadChecker main_thread_checker_; base::ThreadChecker context_thread_checker_; - scoped_ptr - context3d_; + skia::RefPtr gr_interface_; scoped_ptr gr_context_; LostContextCallback lost_context_callback_; -- cgit v1.1