Add locking to ClientDiscardableTextureManager and ClientTransferCache

ClientDiscardableTextureManager and ClientTransferCache are called from special Threadsafe* functions on ContextSupport without the context lock held. We need locking in these cases to prevent data races which are currently possible.

Bug: 794293
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: I70c5a23dc2bd55cb77fb29c3c9057e8c37211e8c
Reviewed-on: https://chromium-review.googlesource.com/823232
Commit-Queue: Eric Karl <ericrk@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523664}

gpu/command_buffer/client/client_discardable_texture_manager.cc

@@ -12,6 +12,7 @@ ClientDiscardableTextureManager::~ClientDiscardableTextureManager() = default;
 ClientDiscardableHandle ClientDiscardableTextureManager::InitializeTexture(
     CommandBuffer* command_buffer,
     uint32_t texture_id) {
+  base::AutoLock hold(lock_);
   DCHECK(texture_id_to_handle_id_.find(texture_id) ==
          texture_id_to_handle_id_.end());
   ClientDiscardableHandle::Id handle_id =
@@ -24,12 +25,14 @@ ClientDiscardableHandle ClientDiscardableTextureManager::InitializeTexture(
 }
 
 bool ClientDiscardableTextureManager::LockTexture(uint32_t texture_id) {
+  base::AutoLock hold(lock_);
   auto found = texture_id_to_handle_id_.find(texture_id);
   DCHECK(found != texture_id_to_handle_id_.end());
   return discardable_manager_.LockHandle(found->second);
 }
 
 void ClientDiscardableTextureManager::FreeTexture(uint32_t texture_id) {
+  base::AutoLock hold(lock_);
   auto found = texture_id_to_handle_id_.find(texture_id);
   if (found == texture_id_to_handle_id_.end())
     return;
@@ -40,12 +43,14 @@ void ClientDiscardableTextureManager::FreeTexture(uint32_t texture_id) {
 
 bool ClientDiscardableTextureManager::TextureIsValid(
     uint32_t texture_id) const {
+  base::AutoLock hold(lock_);
   return texture_id_to_handle_id_.find(texture_id) !=
          texture_id_to_handle_id_.end();
 }
 
 ClientDiscardableHandle ClientDiscardableTextureManager::GetHandleForTesting(
     uint32_t texture_id) {
+  base::AutoLock hold(lock_);
   auto found = texture_id_to_handle_id_.find(texture_id);
   DCHECK(found != texture_id_to_handle_id_.end());
   return discardable_manager_.GetHandle(found->second);

gpu/command_buffer/client/client_discardable_texture_manager.h

@@ -7,6 +7,7 @@
 
 #include <map>
 
+#include "base/synchronization/lock.h"
 #include "gpu/command_buffer/client/client_discardable_manager.h"
 #include "gpu/gpu_export.h"
 
@@ -14,6 +15,11 @@ namespace gpu {
 
 // A helper class used to manage discardable textures. Makes use of
 // ClientDiscardableManager. Used by the GLES2 Implementation.
+//
+// NOTE: The presence of locking on this class does not make it threadsafe.
+// The underlying locking *only* allows calling TextureIsValid and
+// LockTexture without holding the GL context lock. All other calls still
+// require that the context lock be held.
 class GPU_EXPORT ClientDiscardableTextureManager {
  public:
   ClientDiscardableTextureManager();
@@ -33,6 +39,8 @@ class GPU_EXPORT ClientDiscardableTextureManager {
   ClientDiscardableHandle GetHandleForTesting(uint32_t texture_id);
 
  private:
+  // Access to other members must always be done with |lock_| held.
+  mutable base::Lock lock_;
   std::map<uint32_t, ClientDiscardableHandle::Id> texture_id_to_handle_id_;
   ClientDiscardableManager discardable_manager_;
 

gpu/command_buffer/client/client_transfer_cache.cc

@@ -15,6 +15,7 @@ void ClientTransferCache::CreateCacheEntry(
     gles2::GLES2CmdHelper* helper,
     MappedMemoryManager* mapped_memory,
     const cc::ClientTransferCacheEntry& entry) {
+  base::AutoLock hold(lock_);
   ScopedMappedMemoryPtr mapped_alloc(entry.SerializedSize(), helper,
                                      mapped_memory);
   DCHECK(mapped_alloc.valid());
@@ -39,6 +40,7 @@ void ClientTransferCache::CreateCacheEntry(
 bool ClientTransferCache::LockTransferCacheEntry(
     cc::TransferCacheEntryType type,
     uint32_t id) {
+  base::AutoLock hold(lock_);
   auto discardable_handle_id = FindDiscardableHandleId(type, id);
   if (discardable_handle_id.is_null())
     return false;
@@ -55,6 +57,7 @@ void ClientTransferCache::UnlockTransferCacheEntry(
     gles2::GLES2CmdHelper* helper,
     cc::TransferCacheEntryType type,
     uint32_t id) {
+  base::AutoLock hold(lock_);
   DCHECK(!FindDiscardableHandleId(type, id).is_null());
   helper->UnlockTransferCacheEntryINTERNAL(static_cast<uint32_t>(type), id);
 }
@@ -63,6 +66,7 @@ void ClientTransferCache::DeleteTransferCacheEntry(
     gles2::GLES2CmdHelper* helper,
     cc::TransferCacheEntryType type,
     uint32_t id) {
+  base::AutoLock hold(lock_);
   auto discardable_handle_id = FindDiscardableHandleId(type, id);
   if (discardable_handle_id.is_null())
     return;
@@ -75,6 +79,7 @@ void ClientTransferCache::DeleteTransferCacheEntry(
 ClientDiscardableHandle::Id ClientTransferCache::FindDiscardableHandleId(
     cc::TransferCacheEntryType type,
     uint32_t id) {
+  lock_.AssertAcquired();
   const auto& id_map = DiscardableHandleIdMap(type);
   auto id_map_it = id_map.find(id);
   if (id_map_it == id_map.end())
@@ -85,6 +90,7 @@ ClientDiscardableHandle::Id ClientTransferCache::FindDiscardableHandleId(
 std::map<uint32_t, ClientDiscardableHandle::Id>&
 ClientTransferCache::DiscardableHandleIdMap(
     cc::TransferCacheEntryType entry_type) {
+  lock_.AssertAcquired();
   DCHECK_LE(static_cast<uint32_t>(entry_type),
             static_cast<uint32_t>(cc::TransferCacheEntryType::kLast));
   return discardable_handle_id_map_[static_cast<uint32_t>(entry_type)];

gpu/command_buffer/client/client_transfer_cache.h

@@ -7,6 +7,7 @@
 
 #include <map>
 
+#include "base/synchronization/lock.h"
 #include "cc/paint/transfer_cache_entry.h"
 #include "gpu/command_buffer/client/client_discardable_manager.h"
 #include "gpu/command_buffer/client/gles2_impl_export.h"
@@ -34,6 +35,10 @@ class MappedMemoryManager;
 // If an entry is no longer needed:
 //   5) DeleteTransferCacheEntry
 //
+// NOTE: The presence of locking on this class does not make it threadsafe.
+// The underlying locking *only* allows calling LockTransferCacheEntry
+// without holding the GL context lock. All other calls still require that
+// the context lock be held.
 class GLES2_IMPL_EXPORT ClientTransferCache {
  public:
   ClientTransferCache();
@@ -58,6 +63,8 @@ class GLES2_IMPL_EXPORT ClientTransferCache {
   std::map<uint32_t, ClientDiscardableHandle::Id>& DiscardableHandleIdMap(
       cc::TransferCacheEntryType entry_type);
 
+  // Access to other members must always be done with |lock_| held.
+  base::Lock lock_;
   ClientDiscardableManager discardable_manager_;
   std::map<uint32_t, ClientDiscardableHandle::Id> discardable_handle_id_map_
       [static_cast<uint32_t>(cc::TransferCacheEntryType::kLast) + 1];