[BlobAsync] Patch 1: BlobStorageRegistry

storage/browser/blob/blob_storage_registry.cc

+// Copyright 2015 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 "base/bind.h"
+#include "base/callback.h"
+#include "base/location.h"
+#include "base/logging.h"
+#include "base/message_loop/message_loop.h"
+#include "base/stl_util.h"
+#include "storage/browser/blob/blob_storage_registry.h"

[michaeln, 2015/08/18 02:25:46]

you can put the main include up top followed by a blank line

[dmurph, 2015/09/15 00:54:34]

Whoops, thanks.

+#include "url/gurl.h"
+
+namespace storage {
+using BlobState = BlobStorageRegistry::BlobState;
+
+namespace {
+// We can't use GURL directly for these hash fragment manipulations
+// since it doesn't have specific knowlege of the BlobURL format. GURL
+// treats BlobURLs as if they were PathURLs which don't support hash
+// fragments.
+
+bool BlobUrlHasRef(const GURL& url) {
+  return url.spec().find('#') != std::string::npos;
+}
+
+GURL ClearBlobUrlRef(const GURL& url) {
+  size_t hash_pos = url.spec().find('#');
+  if (hash_pos == std::string::npos)
+    return url;
+  return GURL(url.spec().substr(0, hash_pos));
+}
+
+}  // namespace
+
+BlobStorageRegistry::BlobRegistryEntry::BlobRegistryEntry(int refcount,
+                                                          BlobState state)
+    : refcount(refcount), state(state), flags(0) {}
+
+BlobStorageRegistry::BlobRegistryEntry::~BlobRegistryEntry() {}
+
+bool BlobStorageRegistry::BlobRegistryEntry::IsFlagSet(int flag) {
+  return ((flags & flag) != 0);

[michaeln, 2015/08/18 02:25:46]

probably can inline the flag getters/settings and use the is_flag_set() naming
style

[dmurph, 2015/09/15 00:54:34]

done.

+}
+
+void BlobStorageRegistry::BlobRegistryEntry::SetFlag(int flag) {
+  flags |= flag;
+}
+
+void BlobStorageRegistry::BlobRegistryEntry::UnsetFlag(int flag) {
+  flags &= !flag;

[michaeln, 2015/08/18 02:25:46]

do you want the bitwise operator here?

[dmurph, 2015/09/15 00:54:34]

Yes. This is the current behavior in the system. I'm not changing this right
now.

[michaeln, 2015/10/09 02:16:14]

Why, your changing most all other aspects of the current system? This line of
code is mixing bitwise and boolean logic in a confusing way, unless there's a
reason to not clean it up, why not clean it up?

[dmurph, 2015/10/10 00:11:33]

Done.

+}
+
+BlobStorageRegistry::BlobStorageRegistry() {}
+
+BlobStorageRegistry::~BlobStorageRegistry() {
+  auto begin = blob_map_.begin();
+  auto end = blob_map_.end();
+  while (begin != end) {

[michaeln, 2015/08/18 02:25:46]

this is oddly formed for each loop, would a for (const auto& pair : blob_map_)
work here?

[dmurph, 2015/09/15 00:54:34]

Done.

+    auto temp = begin;
+    ++begin;
+    BlobRegistryEntry* entry = temp->second;

[kinuko, 2015/08/14 10:12:18]

scoped_ptr<BlobRegistryEntry> entry(temp->second);

and remove line 71

[dmurph, 2015/09/15 00:54:34]

Done.

+    // We need to make sure we cleanup all of the callbacks so we don't leave
+    // any dangling URL requests around.
+    if (entry && !entry->construction_complete_callbacks.empty()) {

[michaeln, 2015/08/18 02:25:46]

Can |entry| ever be null? i don't see how it can?

[dmurph, 2015/09/15 00:54:34]

Nope.  Removed.

+      for (const auto& callback : entry->construction_complete_callbacks) {
+        base::MessageLoop::current()->PostTask(FROM_HERE,
+                                               base::Bind(callback, false));
+      }
+    }
+    delete temp->second;
+  }
+}
+
+BlobStorageRegistry::BlobRegistryEntry* BlobStorageRegistry::RegisterBlobUUID(
+    const std::string& uuid) {
+  if (blob_map_.find(uuid) != blob_map_.end())

[kinuko, 2015/08/14 10:12:18]

nit: prefer ContainsKey() in stl_util.h in general

[michaeln, 2015/08/18 02:25:46]

Is attempting to reuse a uuid ever expected? I can see given bogus ipc message
inputs from a renderer that this might happen, but otherwise i don't think it
would ever be expected. We do need to check test for valid ipc message inputs,
but those tests should be higher up in the stack closer to where the msg is
received.

At this lower level, i'd rather see us asserting that we're getting good inputs
from our caller. Otherwise there's a little more code and these runtime checks
will actually be done twice in the product, once at the higher level closer to
weeding out bad ipc inputs and again here. Not the end of the world to be doing
so, but to not be doing so would be a slight improvement.

I have similar questions/comments about all of the existance tests and early
returns. Which are actually expected at runtime and which would be indicative of
a bug higher up?

[dmurph, 2015/09/15 00:54:34]

Tests accidentally do this a lot, as I've done myself.

This actually ends up being the one check that gets made, other than in the
blob_storage_host.  The context just calls the registry in most cases and
handles the error case when the registry says something doesn't exist, etc.

Plus, it's much easier to test this code and it's edge cases.

[michaeln, 2015/10/09 02:16:14]

Fix the test, it's buggy.
Ok, but what happens is callers have to handle the null case, and their callers
have to handle the return path from there, etc... net more code thats never
expected or supposed to run that nonetheless is there and needs to be tested
(which is yet more code :) Thats one reason why I prefer making strong
assertions rather than branches in low level code, it reduces the number of ways
control can flow.

The other is clarity. Theres code to handle the not found case. Does that mean
it's expected to occur in the product, you'd think so since theres code for it.
So when somebody does see it happening in the product... instead of recognizing
the problem is upstream, they "fix" it by adding yet more code to the downstream
control flow branch that shouldn't happen (or even be there imho).

[dmurph, 2015/10/10 00:11:33]

Ok, I made it a dcheck

+    return nullptr;
+  auto* entry = new BlobRegistryEntry(1, BlobState::RESERVED);
+  blob_map_[uuid] = entry;
+  return entry;
+}
+
+bool BlobStorageRegistry::RemoveBlobEntry(const std::string& uuid) {
+  BlobMap::iterator found = blob_map_.find(uuid);
+  if (found == blob_map_.end()) {
+    return false;
+  }
+  delete found->second;
+  blob_map_.erase(found);
+  return true;
+}
+
+bool BlobStorageRegistry::IsUUIDRegistered(const std::string& uuid) const {
+  return blob_map_.find(uuid) != blob_map_.end();
+}
+
+BlobStorageRegistry::BlobRegistryEntry* BlobStorageRegistry::GetBlobEntry(
+    const std::string& uuid) {
+  BlobMap::iterator found = blob_map_.find(uuid);
+  if (found == blob_map_.end()) {
+    return nullptr;
+  }
+  return found->second;
+}
+
+BlobState BlobStorageRegistry::GetBlobState(const std::string& uuid) const {
+  BlobMap::const_iterator found = blob_map_.find(uuid);
+  return found == blob_map_.end() ? BlobState::UNKNOWN : found->second->state;
+}
+
+bool BlobStorageRegistry::TestAndSetState(const std::string& uuid,
+                                          BlobState expected,
+                                          BlobState set) {
+  BlobRegistryEntry* entry = GetBlobEntry(uuid);
+  if (!entry || entry->state != expected)
+    return false;
+  entry->state = set;
+  return true;
+}
+
+bool BlobStorageRegistry::RegisterURLMapping(const GURL& blob_url,
+                                             const std::string& uuid) {
+  if (!IsUUIDRegistered(uuid) || IsURLMapped(blob_url))
+    return false;
+  url_to_uuid_[blob_url] = uuid;
+  return true;
+}
+
+bool BlobStorageRegistry::RemoveURLMapping(const GURL& blob_url,
+                                           std::string* uuid) {
+  DCHECK(!BlobUrlHasRef(blob_url));
+  URLMap::iterator found = url_to_uuid_.find(blob_url);
+  if (found == url_to_uuid_.end()) {
+    return false;
+  }
+  if (uuid) {
+    uuid->assign(found->second);
+  }
+  url_to_uuid_.erase(found);
+  return true;
+}
+
+bool BlobStorageRegistry::IsURLMapped(const GURL& blob_url) const {
+  return url_to_uuid_.find(blob_url) != url_to_uuid_.end();

[kinuko, 2015/08/14 10:12:18]

nit: ContainsKey

[dmurph, 2015/09/15 00:54:34]

Done.

+}
+
+BlobStorageRegistry::BlobRegistryEntry*
+BlobStorageRegistry::GetBlobEntryFromURL(const GURL& url, std::string* uuid) {
+  URLMap::iterator found =
+      url_to_uuid_.find(BlobUrlHasRef(url) ? ClearBlobUrlRef(url) : url);
+  if (found == url_to_uuid_.end()) {
+    return nullptr;
+  }
+  BlobRegistryEntry* entry = GetBlobEntry(found->second);
+  if (entry && uuid) {
+    uuid->assign(found->second);
+  }
+  return entry;
+}
+
+}  // namespace storage