gpu shader cache: Clarify lifetime/ownership/threadiness of objects.

content/browser/gpu/shader_disk_cache.cc

 // Copyright (c) 2013 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 "content/browser/gpu/shader_disk_cache.h"
 
 #include "base/macros.h"
+#include "base/memory/ptr_util.h"
 #include "base/threading/thread_checker.h"
-#include "content/public/browser/browser_thread.h"
 #include "gpu/command_buffer/common/constants.h"
 #include "net/base/cache_type.h"
 #include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"
 
 namespace content {
 
 namespace {
 
 static const base::FilePath::CharType kGpuCachePath[] =
     FILE_PATH_LITERAL("GPUCache");
 
-void EntryCloser(disk_cache::Entry* entry) {
-  entry->Close();
-}
-
-void FreeDiskCacheIterator(
-    std::unique_ptr<disk_cache::Backend::Iterator> iterator) {}
+static ShaderCacheFactory* factory_instance = nullptr;
 
 }  // namespace
 
 // ShaderDiskCacheEntry handles the work of caching/updating the cached
 // shaders.
-class ShaderDiskCacheEntry
-    : public base::ThreadChecker,
-      public base::RefCounted<ShaderDiskCacheEntry> {
+class ShaderDiskCacheEntry : public base::ThreadChecker {
  public:
-  ShaderDiskCacheEntry(base::WeakPtr<ShaderDiskCache> cache,
+  ShaderDiskCacheEntry(ShaderDiskCache* cache,
                        const std::string& key,
                        const std::string& shader);
+  ~ShaderDiskCacheEntry();
+
   void Cache();
 
  private:
-  friend class base::RefCounted<ShaderDiskCacheEntry>;
-
   enum OpType {
-    TERMINATE,
     OPEN_ENTRY,
     WRITE_DATA,
     CREATE_ENTRY,
   };
 
-  ~ShaderDiskCacheEntry();
-
   void OnOpComplete(int rv);
 
   int OpenCallback(int rv);
   int WriteCallback(int rv);
   int IOComplete(int rv);
 
-  base::WeakPtr<ShaderDiskCache> cache_;
+  ShaderDiskCache* cache_;
   OpType op_type_;
   std::string key_;
   std::string shader_;
   disk_cache::Entry* entry_;
+  base::WeakPtrFactory<ShaderDiskCacheEntry> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ShaderDiskCacheEntry);
 };
 
 // ShaderDiskReadHelper is used to load all of the cached shaders from the
 // disk cache and send to the memory cache.
-class ShaderDiskReadHelper
-    : public base::ThreadChecker,
-      public base::RefCounted<ShaderDiskReadHelper> {
+class ShaderDiskReadHelper : public base::ThreadChecker {
  public:
   using ShaderLoadedCallback = ShaderDiskCache::ShaderLoadedCallback;
-  ShaderDiskReadHelper(base::WeakPtr<ShaderDiskCache> cache,
+  ShaderDiskReadHelper(ShaderDiskCache* cache,
                        const ShaderLoadedCallback& callback);
+  ~ShaderDiskReadHelper();
+
   void LoadCache();
 
  private:
-  friend class base::RefCounted<ShaderDiskReadHelper>;
-
   enum OpType {
     TERMINATE,
     OPEN_NEXT,
     OPEN_NEXT_COMPLETE,
     READ_COMPLETE,
     ITERATION_FINISHED
   };
 
 
-  ~ShaderDiskReadHelper();
-
   void OnOpComplete(int rv);
 
   int OpenNextEntry();
   int OpenNextEntryComplete(int rv);
   int ReadComplete(int rv);
   int IterationComplete(int rv);
 
-  base::WeakPtr<ShaderDiskCache> cache_;
+  ShaderDiskCache* cache_;
   ShaderLoadedCallback shader_loaded_callback_;
   OpType op_type_;
   std::unique_ptr<disk_cache::Backend::Iterator> iter_;
   scoped_refptr<net::IOBufferWithSize> buf_;
   disk_cache::Entry* entry_;
+  base::WeakPtrFactory<ShaderDiskReadHelper> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ShaderDiskReadHelper);
 };
 
-class ShaderClearHelper
-    : public base::RefCounted<ShaderClearHelper>,
-      public base::SupportsWeakPtr<ShaderClearHelper> {
+class ShaderClearHelper : public base::RefCounted<ShaderClearHelper>,
+                          public base::SupportsWeakPtr<ShaderClearHelper>,
+                          public base::ThreadChecker {
  public:
   ShaderClearHelper(scoped_refptr<ShaderDiskCache> cache,
                     const base::FilePath& path,
                     const base::Time& delete_begin,
                     const base::Time& delete_end,
                     const base::Closure& callback);
   void Clear();
 
  private:
   friend class base::RefCounted<ShaderClearHelper>;
@@ skipping 11 matching lines @@
   scoped_refptr<ShaderDiskCache> cache_;
   OpType op_type_;
   base::FilePath path_;
   base::Time delete_begin_;
   base::Time delete_end_;
   base::Closure callback_;
 
   DISALLOW_COPY_AND_ASSIGN(ShaderClearHelper);
 };
 
-ShaderDiskCacheEntry::ShaderDiskCacheEntry(base::WeakPtr<ShaderDiskCache> cache,
+////////////////////////////////////////////////////////////////////////////////
+// ShaderDiskCacheEntry
+
+ShaderDiskCacheEntry::ShaderDiskCacheEntry(ShaderDiskCache* cache,
                                            const std::string& key,
                                            const std::string& shader)
-  : cache_(cache),
-    op_type_(OPEN_ENTRY),
-    key_(key),
-    shader_(shader),
-    entry_(NULL) {
-}
+    : cache_(cache),
+      op_type_(OPEN_ENTRY),
+      key_(key),
+      shader_(shader),
+      entry_(nullptr),
+      weak_ptr_factory_(this) {}
 
 ShaderDiskCacheEntry::~ShaderDiskCacheEntry() {
+  DCHECK(CalledOnValidThread());
   if (entry_)
-    BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
-                            base::Bind(&EntryCloser, entry_));
+    entry_->Close();
 }
 
 void ShaderDiskCacheEntry::Cache() {
   DCHECK(CalledOnValidThread());
-  if (!cache_.get())
-    return;
-
   int rv = cache_->backend()->OpenEntry(
-      key_,
-      &entry_,
-      base::Bind(&ShaderDiskCacheEntry::OnOpComplete, this));
+      key_, &entry_, base::Bind(&ShaderDiskCacheEntry::OnOpComplete,
+                                weak_ptr_factory_.GetWeakPtr()));
   if (rv != net::ERR_IO_PENDING)
     OnOpComplete(rv);
 }
 
 void ShaderDiskCacheEntry::OnOpComplete(int rv) {
   DCHECK(CalledOnValidThread());
-  if (!cache_.get())
-    return;
-
   do {
     switch (op_type_) {
       case OPEN_ENTRY:
         rv = OpenCallback(rv);
         break;
       case CREATE_ENTRY:
         rv = WriteCallback(rv);
         break;
       case WRITE_DATA:
         rv = IOComplete(rv);
         break;
-      case TERMINATE:
-        rv = net::ERR_IO_PENDING;  // break the loop.
-        break;
-      default:
-        NOTREACHED();  // Invalid op_type_ provided.
-        break;
     }
   } while (rv != net::ERR_IO_PENDING);
 }
 
 int ShaderDiskCacheEntry::OpenCallback(int rv) {
   DCHECK(CalledOnValidThread());
-  // Called through OnOpComplete, so we know |cache_| is valid.
   if (rv == net::OK) {
     cache_->backend()->OnExternalCacheHit(key_);
     cache_->EntryComplete(this);
-    op_type_ = TERMINATE;
-    return rv;
+    return net::ERR_IO_PENDING;

[piman, 2016/11/02 18:11:15]

I'm not a huge fan of this transformation, because it makes the code less
readable IMO. net::ERR_IO_PENDING is an implementation detail of the loop in
OnOpComplete, but looking here, it's not obvious what's pending (actually,
nothing is).

In particular if we want to handle error cases differently than success, we
would need this rv to correctly flow to OnOpComplete, I think.

[sadrul, 2016/11/02 19:24:53]

I have reverted this part of the code. Instead of this, OnOpComplete() holds one
a WeakPtr<> and inspects that to determine if it has been destroyed.

   }
 
   op_type_ = CREATE_ENTRY;
   return cache_->backend()->CreateEntry(
-      key_,
-      &entry_,
-      base::Bind(&ShaderDiskCacheEntry::OnOpComplete, this));
+      key_, &entry_, base::Bind(&ShaderDiskCacheEntry::OnOpComplete,
+                                weak_ptr_factory_.GetWeakPtr()));
 }
 
 int ShaderDiskCacheEntry::WriteCallback(int rv) {
   DCHECK(CalledOnValidThread());
-  // Called through OnOpComplete, so we know |cache_| is valid.
   if (rv != net::OK) {
     LOG(ERROR) << "Failed to create shader cache entry: " << rv;
     cache_->EntryComplete(this);
-    op_type_ = TERMINATE;
-    return rv;
+    return net::ERR_IO_PENDING;
   }
 
   op_type_ = WRITE_DATA;
   scoped_refptr<net::StringIOBuffer> io_buf = new net::StringIOBuffer(shader_);
-  return entry_->WriteData(
-      1,
-      0,
-      io_buf.get(),
-      shader_.length(),
-      base::Bind(&ShaderDiskCacheEntry::OnOpComplete, this),
-      false);
+  return entry_->WriteData(1, 0, io_buf.get(), shader_.length(),
+                           base::Bind(&ShaderDiskCacheEntry::OnOpComplete,
+                                      weak_ptr_factory_.GetWeakPtr()),
+                           false);
 }
 
 int ShaderDiskCacheEntry::IOComplete(int rv) {
   DCHECK(CalledOnValidThread());
-  // Called through OnOpComplete, so we know |cache_| is valid.
   cache_->EntryComplete(this);
-  op_type_ = TERMINATE;
-  return rv;
+  return net::ERR_IO_PENDING;
 }
 
-ShaderDiskReadHelper::ShaderDiskReadHelper(base::WeakPtr<ShaderDiskCache> cache,
+////////////////////////////////////////////////////////////////////////////////
+// ShaderDiskReadHelper
+
+ShaderDiskReadHelper::ShaderDiskReadHelper(ShaderDiskCache* cache,
                                            const ShaderLoadedCallback& callback)
     : cache_(cache),
       shader_loaded_callback_(callback),
       op_type_(OPEN_NEXT),
       buf_(NULL),
-      entry_(NULL) {}
+      entry_(NULL),
+      weak_ptr_factory_(this) {}
+
+ShaderDiskReadHelper::~ShaderDiskReadHelper() {
+  DCHECK(CalledOnValidThread());
+  if (entry_)
+    entry_->Close();
+  iter_ = nullptr;
+}
 
 void ShaderDiskReadHelper::LoadCache() {
   DCHECK(CalledOnValidThread());
-  if (!cache_.get())
-    return;
   OnOpComplete(net::OK);
 }
 
 void ShaderDiskReadHelper::OnOpComplete(int rv) {
   DCHECK(CalledOnValidThread());
-  if (!cache_.get())
-    return;
-
   do {
     switch (op_type_) {
       case OPEN_NEXT:
         rv = OpenNextEntry();
         break;
       case OPEN_NEXT_COMPLETE:
         rv = OpenNextEntryComplete(rv);
         break;
       case READ_COMPLETE:
         rv = ReadComplete(rv);
         break;
       case ITERATION_FINISHED:
         rv = IterationComplete(rv);
         break;
       case TERMINATE:
         cache_->ReadComplete();
         rv = net::ERR_IO_PENDING;  // break the loop
         break;
       default:
         NOTREACHED();  // Invalid state for read helper
         rv = net::ERR_FAILED;
         break;
     }
   } while (rv != net::ERR_IO_PENDING);
 }
 
 int ShaderDiskReadHelper::OpenNextEntry() {
   DCHECK(CalledOnValidThread());
-  // Called through OnOpComplete, so we know |cache_| is valid.
   op_type_ = OPEN_NEXT_COMPLETE;
   if (!iter_)
     iter_ = cache_->backend()->CreateIterator();
-  return iter_->OpenNextEntry(
-      &entry_, base::Bind(&ShaderDiskReadHelper::OnOpComplete, this));
+  return iter_->OpenNextEntry(&entry_,
+                              base::Bind(&ShaderDiskReadHelper::OnOpComplete,
+                                         weak_ptr_factory_.GetWeakPtr()));
 }
 
 int ShaderDiskReadHelper::OpenNextEntryComplete(int rv) {
   DCHECK(CalledOnValidThread());
-  // Called through OnOpComplete, so we know |cache_| is valid.
   if (rv == net::ERR_FAILED) {
     iter_.reset();
     op_type_ = ITERATION_FINISHED;
     return net::OK;
   }
 
   if (rv < 0)
     return rv;
 
   op_type_ = READ_COMPLETE;
   buf_ = new net::IOBufferWithSize(entry_->GetDataSize(1));
-  return entry_->ReadData(
-      1,
-      0,
-      buf_.get(),
-      buf_->size(),
-      base::Bind(&ShaderDiskReadHelper::OnOpComplete, this));
+  return entry_->ReadData(1, 0, buf_.get(), buf_->size(),
+                          base::Bind(&ShaderDiskReadHelper::OnOpComplete,
+                                     weak_ptr_factory_.GetWeakPtr()));
 }
 
 int ShaderDiskReadHelper::ReadComplete(int rv) {
   DCHECK(CalledOnValidThread());
-  // Called through OnOpComplete, so we know |cache_| is valid.
   if (rv && rv == buf_->size() && !shader_loaded_callback_.is_null()) {
     shader_loaded_callback_.Run(entry_->GetKey(),
                                 std::string(buf_->data(), buf_->size()));
   }
 
   buf_ = NULL;
   entry_->Close();
   entry_ = NULL;
 
   op_type_ = OPEN_NEXT;
   return net::OK;
 }
 
 int ShaderDiskReadHelper::IterationComplete(int rv) {
   DCHECK(CalledOnValidThread());
-  // Called through OnOpComplete, so we know |cache_| is valid.
   iter_.reset();
   op_type_ = TERMINATE;
   return net::OK;
 }
 
-ShaderDiskReadHelper::~ShaderDiskReadHelper() {
-  if (entry_) {
-    BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
-                            base::Bind(&EntryCloser, entry_));
-  }
-  if (iter_) {
-    BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
-                            base::Bind(&FreeDiskCacheIterator,
-                                       base::Passed(&iter_)));
-  }
-}
+////////////////////////////////////////////////////////////////////////////////
+// ShaderClearHelper
 
 ShaderClearHelper::ShaderClearHelper(scoped_refptr<ShaderDiskCache> cache,
                     const base::FilePath& path,
                     const base::Time& delete_begin,
                     const base::Time& delete_end,
                     const base::Closure& callback)
     : cache_(cache),
       op_type_(VERIFY_CACHE_SETUP),
       path_(path),
       delete_begin_(delete_begin),
       delete_end_(delete_end),
       callback_(callback) {
 }
 
 ShaderClearHelper::~ShaderClearHelper() {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK(CalledOnValidThread());
 }
 
 void ShaderClearHelper::Clear() {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK(CalledOnValidThread());
   DoClearShaderCache(net::OK);
 }
 
 void ShaderClearHelper::DoClearShaderCache(int rv) {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK(CalledOnValidThread());
 
   // Hold a ref to ourselves so when we do the CacheCleared call we don't get
   // auto-deleted when our ref count drops to zero.
   scoped_refptr<ShaderClearHelper> helper = this;
 
   while (rv != net::ERR_IO_PENDING) {
     switch (op_type_) {
       case VERIFY_CACHE_SETUP:
         rv = cache_->SetAvailableCallback(
             base::Bind(&ShaderClearHelper::DoClearShaderCache, AsWeakPtr()));
@@ skipping 11 matching lines @@
         rv = net::ERR_IO_PENDING;  // Break the loop.
         break;
       default:
         NOTREACHED();  // Invalid state provided.
         op_type_ = TERMINATE;
         break;
     }
   }
 }
 
+////////////////////////////////////////////////////////////////////////////////
+// ShaderCacheFactory
+
+// static
+void ShaderCacheFactory::InitInstance(
+    scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+    scoped_refptr<base::SingleThreadTaskRunner> cache_task_runner) {
+  if (task_runner->BelongsToCurrentThread()) {
+    CreateFactoryInstance(std::move(cache_task_runner));
+  } else {
+    task_runner->PostTask(FROM_HERE,
+                          base::Bind(&ShaderCacheFactory::CreateFactoryInstance,
+                                     std::move(cache_task_runner)));
+  }
+}
+
 // static
 ShaderCacheFactory* ShaderCacheFactory::GetInstance() {
-  return base::Singleton<ShaderCacheFactory,
-                         base::LeakySingletonTraits<ShaderCacheFactory>>::get();
+  factory_instance =
+      base::Singleton<ShaderCacheFactory,
+                      base::LeakySingletonTraits<ShaderCacheFactory>>::get();

[piman, 2016/11/02 18:11:15]

Can we instantiate the singleton in CreateFactoryInstance, and instead here
DCHECK(factory_instance)?
This is to ensure no-one tries to call ShaderCacheFactory::GetInstance before it
gets initialized.

[sadrul, 2016/11/02 19:24:53]

Done.

[sadrul, 2016/11/02 20:36:39]

This change ended up catching a few places that calls GetInstance() directly
without initializing it first. But a number of those tests don't actually do
anything with the cache/factory. So would it make sense to have GetInstance()
return null if it hasn't been initialized, instead of being fatal? Or maybe have
a separate 'bool HasInstance()'?

The atlernative is to create the ShaderCacheFactory in
content::UnitTestTestSuite so that it's always there. The problem with this is
though that there are no task-runner when UnitTestTestSuite runs. So
ShaderCacheFactory will have to deal with null task-runners.

[piman, 2016/11/02 20:56:17]

I'm ok with returning null from GetInstance() if it hasn't been initialized.
Maybe add a comment that it's the case?

[sadrul, 2016/11/03 03:57:51]

Done.

+  DCHECK(factory_instance->CalledOnValidThread());
+  return factory_instance;
 }
 
 ShaderCacheFactory::ShaderCacheFactory() {
 }
 
 ShaderCacheFactory::~ShaderCacheFactory() {
 }
 
+// static
+void ShaderCacheFactory::CreateFactoryInstance(
+    scoped_refptr<base::SingleThreadTaskRunner> cache_task_runner) {
+  DCHECK(!factory_instance);
+  GetInstance()->cache_task_runner_ = std::move(cache_task_runner);
+}
+
 void ShaderCacheFactory::SetCacheInfo(int32_t client_id,
                                       const base::FilePath& path) {
+  DCHECK(CalledOnValidThread());
   client_id_to_path_map_[client_id] = path;
 }
 
 void ShaderCacheFactory::RemoveCacheInfo(int32_t client_id) {
+  DCHECK(CalledOnValidThread());
   client_id_to_path_map_.erase(client_id);
 }
 
 scoped_refptr<ShaderDiskCache> ShaderCacheFactory::Get(int32_t client_id) {
+  DCHECK(CalledOnValidThread());
   ClientIdToPathMap::iterator iter =
       client_id_to_path_map_.find(client_id);
   if (iter == client_id_to_path_map_.end())
     return NULL;
   return ShaderCacheFactory::GetByPath(iter->second);
 }
 
 scoped_refptr<ShaderDiskCache> ShaderCacheFactory::GetByPath(
     const base::FilePath& path) {
+  DCHECK(CalledOnValidThread());
   ShaderCacheMap::iterator iter = shader_cache_map_.find(path);
   if (iter != shader_cache_map_.end())
     return iter->second;
 
   ShaderDiskCache* cache = new ShaderDiskCache(path);
-  cache->Init();
+  cache->Init(cache_task_runner_);
   return cache;
 }
 
 void ShaderCacheFactory::AddToCache(const base::FilePath& key,
                                     ShaderDiskCache* cache) {
+  DCHECK(CalledOnValidThread());
   shader_cache_map_[key] = cache;
 }
 
 void ShaderCacheFactory::RemoveFromCache(const base::FilePath& key) {
+  DCHECK(CalledOnValidThread());
   shader_cache_map_.erase(key);
 }
 
 void ShaderCacheFactory::ClearByPath(const base::FilePath& path,
                                      const base::Time& delete_begin,
                                      const base::Time& delete_end,
                                      const base::Closure& callback) {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK(CalledOnValidThread());
   DCHECK(!callback.is_null());
 
   scoped_refptr<ShaderClearHelper> helper = new ShaderClearHelper(
       GetByPath(path), path, delete_begin, delete_end, callback);
 
   // We could receive requests to clear the same path with different
   // begin/end times. So, we keep a list of requests. If we haven't seen this
   // path before we kick off the clear and add it to the list. If we have see it
   // already, then we already have a clear running. We add this clear to the
   // list and wait for any previous clears to finish.
   ShaderClearMap::iterator iter = shader_clear_map_.find(path);
   if (iter != shader_clear_map_.end()) {
     iter->second.push(helper);
     return;
   }
 
   shader_clear_map_.insert(
       std::pair<base::FilePath, ShaderClearQueue>(path, ShaderClearQueue()));
   shader_clear_map_[path].push(helper);
   helper->Clear();
 }
 
 void ShaderCacheFactory::CacheCleared(const base::FilePath& path) {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK(CalledOnValidThread());
 
   ShaderClearMap::iterator iter = shader_clear_map_.find(path);
   if (iter == shader_clear_map_.end()) {
     LOG(ERROR) << "Completed clear but missing clear helper.";
     return;
   }
 
   iter->second.pop();
 
   // If there are remaining items in the list we trigger the Clear on the
   // next one.
   if (!iter->second.empty()) {
     iter->second.front()->Clear();
     return;
   }
 
   shader_clear_map_.erase(path);
 }
 
+////////////////////////////////////////////////////////////////////////////////
+// ShaderDiskCache
+
 ShaderDiskCache::ShaderDiskCache(const base::FilePath& cache_path)
     : cache_available_(false),
       cache_path_(cache_path),
       is_initialized_(false) {
   ShaderCacheFactory::GetInstance()->AddToCache(cache_path_, this);
 }
 
 ShaderDiskCache::~ShaderDiskCache() {
   ShaderCacheFactory::GetInstance()->RemoveFromCache(cache_path_);
 }
 
-void ShaderDiskCache::Init() {
+void ShaderDiskCache::Init(
+    scoped_refptr<base::SingleThreadTaskRunner> cache_task_runner) {
   if (is_initialized_) {
     NOTREACHED();  // can't initialize disk cache twice.
     return;
   }
   is_initialized_ = true;
 
   int rv = disk_cache::CreateCacheBackend(
       net::SHADER_CACHE, net::CACHE_BACKEND_DEFAULT,
       cache_path_.Append(kGpuCachePath),
-      gpu::kDefaultMaxProgramCacheMemoryBytes, true,
-      BrowserThread::GetTaskRunnerForThread(BrowserThread::CACHE).get(), NULL,
+      gpu::kDefaultMaxProgramCacheMemoryBytes, true, cache_task_runner, NULL,
       &backend_, base::Bind(&ShaderDiskCache::CacheCreatedCallback, this));
 
   if (rv == net::OK)
     cache_available_ = true;
 }
 
 void ShaderDiskCache::Cache(const std::string& key, const std::string& shader) {
   if (!cache_available_)
     return;
 
-  scoped_refptr<ShaderDiskCacheEntry> shim =
-      new ShaderDiskCacheEntry(AsWeakPtr(), key, shader);
+  auto shim = base::MakeUnique<ShaderDiskCacheEntry>(this, key, shader);
   shim->Cache();
-
-  entry_map_[shim.get()] = shim;
+  entries_.push_back(std::move(shim));
 }
 
 int ShaderDiskCache::Clear(
     const base::Time begin_time, const base::Time end_time,
     const net::CompletionCallback& completion_callback) {
   int rv;
   if (begin_time.is_null()) {
     rv = backend_->DoomAllEntries(completion_callback);
   } else {
     rv = backend_->DoomEntriesBetween(begin_time, end_time,
@@ skipping 14 matching lines @@
     return net::OK;
   available_callback_ = callback;
   return net::ERR_IO_PENDING;
 }
 
 void ShaderDiskCache::CacheCreatedCallback(int rv) {
   if (rv != net::OK) {
     LOG(ERROR) << "Shader Cache Creation failed: " << rv;
     return;
   }
-  helper_ = new ShaderDiskReadHelper(AsWeakPtr(), shader_loaded_callback_);
+  helper_ =
+      base::MakeUnique<ShaderDiskReadHelper>(this, shader_loaded_callback_);
   helper_->LoadCache();
 }
 
-void ShaderDiskCache::EntryComplete(void* entry) {
-  entry_map_.erase(entry);
+void ShaderDiskCache::EntryComplete(ShaderDiskCacheEntry* entry) {
+  entries_.erase(
+      std::remove_if(entries_.begin(), entries_.end(),
+                     [entry](const std::unique_ptr<ShaderDiskCacheEntry>& e) {
+                       return e.get() == entry;
+                     }));

[piman, 2016/11/02 18:11:15]

This changes from O(log n) to O(n) - is it really ok? I think we can have a lot
of entries.

Could we use a std::set (std::unordered_set?) instead of std::vector to keep the
O(log n) (or O(1)) behavior?

[sadrul, 2016/11/02 19:24:53]

I couldn't find a clean way of using std::[unordered_]set. So I went back to
using std::unordered_map<>.

 
-  if (entry_map_.empty() && !cache_complete_callback_.is_null())
+  if (entries_.empty() && !cache_complete_callback_.is_null())
     cache_complete_callback_.Run(net::OK);
 }
 
 void ShaderDiskCache::ReadComplete() {
-  helper_ = NULL;
+  helper_ = nullptr;
 
   // The cache is considered available after we have finished reading any
   // of the old cache values off disk. This prevents a potential race where we
   // are reading from disk and execute a cache clear at the same time.
   cache_available_ = true;
   if (!available_callback_.is_null()) {
     available_callback_.Run(net::OK);
     available_callback_.Reset();
   }
 }
 
 int ShaderDiskCache::SetCacheCompleteCallback(
     const net::CompletionCallback& callback) {
-  if (entry_map_.empty()) {
+  if (entries_.empty()) {
     return net::OK;
   }
   cache_complete_callback_ = callback;
   return net::ERR_IO_PENDING;
 }
 
 }  // namespace content