cc: check valid thread and resource origin for the resource rasterized.

cc/resources/resource_provider.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "cc/resources/resource_provider.h"
 
 #include <algorithm>
 #include <limits>
 
 #include "base/containers/hash_tables.h"
@@ skipping 710 matching lines @@
     DCHECK_EQ(Bitmap, resource->type);
     delete resource->shared_bitmap;
     resource->pixels = NULL;
   }
   if (resource->pixels) {
     DCHECK(resource->origin == Resource::Internal);
     delete[] resource->pixels;
     resource->pixels = NULL;
   }
   if (resource->gpu_memory_buffer) {
-    DCHECK(resource->origin != Resource::External);

[dshwang, 2014/10/29 15:34:20]

It cannot be Resource::Delegated, which means the resource is received from
child compositor.

+    DCHECK(resource->origin == Resource::Internal);
     delete resource->gpu_memory_buffer;
     resource->gpu_memory_buffer = NULL;
   }
   resources_.erase(it);
 }
 
 ResourceProvider::ResourceType ResourceProvider::GetResourceType(
     ResourceId id) {
   return GetResource(id)->type;
 }
@@ skipping 290 matching lines @@
 ResourceProvider::ScopedWriteLockSoftware::~ScopedWriteLockSoftware() {
   resource_provider_->UnlockForWrite(resource_);
 }
 
 ResourceProvider::ScopedWriteLockGpuMemoryBuffer::
     ScopedWriteLockGpuMemoryBuffer(ResourceProvider* resource_provider,
                                    ResourceProvider::ResourceId resource_id)
     : resource_provider_(resource_provider),
       resource_(resource_provider->LockForWrite(resource_id)),
       gpu_memory_buffer_manager_(resource_provider->gpu_memory_buffer_manager_),
-      gpu_memory_buffer_(nullptr),
       size_(resource_->size),
       format_(resource_->format) {
   DCHECK_EQ(GLTexture, resource_->type);
-  std::swap(gpu_memory_buffer_, resource_->gpu_memory_buffer);
 }
 
 ResourceProvider::ScopedWriteLockGpuMemoryBuffer::
     ~ScopedWriteLockGpuMemoryBuffer() {
   resource_provider_->UnlockForWrite(resource_);
-  if (!gpu_memory_buffer_)
+  if (!resource_->gpu_memory_buffer)
     return;
 
   if (!resource_->image_id) {
     GLES2Interface* gl = resource_provider_->ContextGL();
     DCHECK(gl);
 
     resource_->image_id =
-        gl->CreateImageCHROMIUM(gpu_memory_buffer_->AsClientBuffer(),
+        gl->CreateImageCHROMIUM(resource_->gpu_memory_buffer->AsClientBuffer(),
                                 size_.width(),
                                 size_.height(),
                                 GL_RGBA);
   }
 
-  std::swap(resource_->gpu_memory_buffer, gpu_memory_buffer_);
   resource_->allocated = true;
   resource_->dirty_image = true;
 
   // GpuMemoryBuffer provides direct access to the memory used by the GPU.
   // Read lock fences are required to ensure that we're not trying to map a
   // buffer that is currently in-use by the GPU.
   resource_->read_lock_fences_enabled = true;
 }
 
 gfx::GpuMemoryBuffer*
 ResourceProvider::ScopedWriteLockGpuMemoryBuffer::GetGpuMemoryBuffer() {
-  if (!gpu_memory_buffer_) {
+  if (!resource_->gpu_memory_buffer) {

[reveman, 2014/10/29 16:02:24]

This function might be used from a worker thread and ResourceProvider is not
thread-safe so reaching into a Resource struct that could be used on the
compositor thread from here is scary.

[dshwang, 2014/10/29 16:48:25]

If this code breaks thread-safe, previous code is also not thread-safe, because
ResourceProvider::ScopedWriteLockGpuMemoryBuffer::gpu_memory_buffer_ is used
without mutex.
The raster thread calls this method only if resource_->locked_for_write is true.
The raster thread can use resource_->gpu_memory_buffer safely like The raster
thread can use resource_->locked_for_write in other ScopedWriteLockXXX classes.
It's because current code don't allow compositor thread and raster thread uses
the same resource coincidentally.
I add the comment to explain it.

[reveman, 2014/10/29 17:07:45]

I don't think that's the case. Where is this "resource_->locked_for_write" check
that you're referring to?
The std::swap logic you're removing in this patch exists to make this thread
safe. The ScopedWriteLockGpuMemoryBuffer instance takes ownership if the
existing GpuMemoryBuffer so that there's no chance that we try to access this on
the compositor thread while it might be used on a worker thread.

[dshwang, 2014/10/29 17:32:52]

For example, ResourceProvider::ScopedWriteLockGr::GetSkSurface(). In the new
patch set, I add DCHECK(resource_->locked_for_write) to show it more explicitly.
It's a bug if compositor thread accesses the resource when ScopedWriteLockXXX
object locks the resource. ResourceProvider checks
DCHECK(!resource_->locked_for_write) here and there when the compositor thread
uses the resource.

In addition, std::swap can not guarantee thread safe if this code isn't already
thread-safe. It's because std::swap can happen at the same time for raster
thread uses |gpu_memory_buffer_|. Currently the code is thread-safe because
compositor thread and raster thread works in turn. So std::swap is just
redundant and we can check thread-safe by resource_->locked_for_write.

[reveman, 2014/10/29 18:15:25]

::GetSkSurface() unlike ::GetGpuMemoryBuffer() cannot be used on a worker
thread. If you want to make this more explicit you can add some ThreadCheckers.
It's easier to verify correct behavior if Resource::gpu_memory_buffer is null
while the resource is locked and the GpuMemoryBuffer might be used on another
thread. ie some code can't get to the buffer even if it doesn't respect
locked_for_write. I think it also describes the ownership of the buffer better.
The lock owns the buffer during its lifetime.

     scoped_ptr<gfx::GpuMemoryBuffer> gpu_memory_buffer =
         gpu_memory_buffer_manager_->AllocateGpuMemoryBuffer(
             size_, ToGpuMemoryBufferFormat(format_), gfx::GpuMemoryBuffer::MAP);
-    gpu_memory_buffer_ = gpu_memory_buffer.release();
+    resource_->gpu_memory_buffer = gpu_memory_buffer.release();
   }
 
-  return gpu_memory_buffer_;
+  return resource_->gpu_memory_buffer;
 }
 
 ResourceProvider::ScopedWriteLockGr::ScopedWriteLockGr(
     ResourceProvider* resource_provider,
     ResourceProvider::ResourceId resource_id)
     : resource_provider_(resource_provider),
       resource_(resource_provider->LockForWrite(resource_id)) {
 }
 
 ResourceProvider::ScopedWriteLockGr::~ScopedWriteLockGr() {
@@ skipping 981 matching lines @@
   ContextProvider* context_provider = output_surface_->context_provider();
   return context_provider ? context_provider->ContextGL() : NULL;
 }
 
 class GrContext* ResourceProvider::GrContext() const {
   ContextProvider* context_provider = output_surface_->context_provider();
   return context_provider ? context_provider->GrContext() : NULL;
 }
 
 }  // namespace cc