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

cc/resources/resource_provider.cc

Index: cc/resources/resource_provider.cc
diff --git a/cc/resources/resource_provider.cc b/cc/resources/resource_provider.cc
index b638cb749126649edbb6fe13e4626d946efb3bbf..099d4533316d0e2c27df78d4909193ef8bdcb50f 100644
--- a/cc/resources/resource_provider.cc
+++ b/cc/resources/resource_provider.cc
@@ -728,7 +728,7 @@ void ResourceProvider::DeleteResourceInternal(ResourceMap::iterator it,
     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;
   }
@@ -1039,17 +1039,15 @@ ResourceProvider::ScopedWriteLockGpuMemoryBuffer::
     : 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) {
@@ -1057,13 +1055,12 @@ ResourceProvider::ScopedWriteLockGpuMemoryBuffer::
     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;
 
@@ -1075,14 +1072,14 @@ ResourceProvider::ScopedWriteLockGpuMemoryBuffer::
 
 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(