New blobstoragecontext for use in the main browser process, not plugged in yet.

webkit/blob/blob_storage_context.cc

Index: webkit/blob/blob_storage_context.cc
===================================================================
--- webkit/blob/blob_storage_context.cc	(revision 189105)
+++ webkit/blob/blob_storage_context.cc	(working copy)
@@ -1,10 +1,14 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// 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 "webkit/blob/blob_storage_controller.h"
+#include "webkit/blob/blob_storage_context.h"
 
+#include "base/bind.h"
+#include "base/location.h"
 #include "base/logging.h"
+#include "base/message_loop_proxy.h"
+#include "base/sys_info.h"
 #include "googleurl/src/gurl.h"
 #include "webkit/blob/blob_data.h"
 
@@ -28,33 +32,190 @@
   return GURL(url.spec().substr(0, hash_pos));
 }
 
-static const int64 kMaxMemoryUsage = 1024 * 1024 * 1024;  // 1G
+// base::SysInfo::AmountOfPhysicalMemoryMB()

[ericu, 2013/04/18 02:01:58]

What does this comment mean?  Is this a TODO?

[michaeln, 2013/04/19 21:55:51]

it is a todo, related to that bug you're cc'd on, just a note to self about the
name of that function

+static const int64 kMaxMemoryUsage = 500 * 1024 * 1024;  // Half a gig.

[ericu, 2013/04/18 02:01:58]

How'd you pick this number?  I see it used to be twice that much; what's going
to break?

 
 }  // namespace
 
-BlobStorageController::BlobStorageController()
+//-----------------------------------------------------------------------
+// BlobDataHandle
+//-----------------------------------------------------------------------
+
+BlobDataHandle::BlobDataHandle(BlobData* blob_data, BlobStorageContext* context,
+                               base::SequencedTaskRunner* task_runner)
+    : blob_data_(blob_data),
+      context_(new base::WeakPtr<BlobStorageContext>(context->AsWeakPtr())),
+      io_task_runner_(task_runner) {
+  // Ensures the uuid remains registered and the underlying data is not deleted.
+  context_->get()->IncrementBlobRefCount(blob_data->uuid());
+}
+
+BlobDataHandle::~BlobDataHandle() {

[ericu, 2013/04/18 02:01:58]

There's no need to call the PostTask either if !context_->get().  Perhaps pull
that check up front?

[michaeln, 2013/04/19 21:55:51]

i gotta look more closely, are weakptrs threadsafe these days?

[ericu, 2013/04/22 17:25:14]

Ah, I was assuming that they were; I could be completely wrong about that.

[michaeln, 2013/04/23 21:11:11]

weakptrs may be deleted on the 'wrong thread', but they cannot be deref'd (or
consequently tested) on the wrong thread... so we can't test context_->get()
early.

+  if (io_task_runner_->RunsTasksOnCurrentThread()) {
+    if (context_->get())
+      context_->get()->DecrementBlobRefCount(blob_data_->uuid());
+    return;
+  }
+  io_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&DeleteHelper, base::Passed(&context_), blob_data_));

[michaeln, 2013/04/18 20:45:15]

blob_data_ is not being handled correctly here since its not a threadsafe
refcounted obj... i'll fix that in the next snapshot too

+}
+
+BlobData* BlobDataHandle::data() const {
+  DCHECK(io_task_runner_->RunsTasksOnCurrentThread());
+  return blob_data_.get();
+}
+
+// static
+void BlobDataHandle::DeleteHelper(
+    scoped_ptr<base::WeakPtr<BlobStorageContext> > context,
+    scoped_refptr<BlobData> blob_data) {
+  if (context->get())
+    context->get()->DecrementBlobRefCount(blob_data->uuid());
+}
+
+//-----------------------------------------------------------------------
+// BlobStorageHost
+//-----------------------------------------------------------------------
+
+// TODO(michaeln): I need to tighten this up, some assertions and better
+// map usage (aka use ->find more). Do something reasonable given bad inputs
+// like addref'ing an id that does not exist.
+
+BlobStorageHost::BlobStorageHost(BlobStorageContext* context)
+    : context_(context->AsWeakPtr()) {
+}
+
+BlobStorageHost::~BlobStorageHost() {
+  if (!context_)
+    return;
+  for (BlobReferenceMap::iterator iter = blobs_inuse_map_.begin();
+       iter != blobs_inuse_map_.end(); ++iter) {
+    for (int i = 0; i < iter->second; ++i)
+      context_->DecrementBlobRefCount(iter->first);
+  }
+
+  for (std::set<GURL>::iterator iter = public_blob_urls_.begin();
+       iter != public_blob_urls_.end(); ++iter) {
+    context_->RevokePublicBlobURL(*iter);
+  }

[ericu, 2013/04/18 02:01:58]

Stylistically, I'd probably revoke the public URL for a blob before removing its
last local reference, but it's just a nit.

[michaeln, 2013/04/19 21:55:51]

Done.

+}
+
+void BlobStorageHost::StartBuildingBlob(const std::string& uuid) {
+  if (!context_)
+    return;
+  DCHECK(blobs_inuse_map_.find(uuid) == blobs_inuse_map_.end());
+  blobs_inuse_map_[uuid] = 1;
+  context_->StartBuildingBlob(uuid);
+}
+
+void BlobStorageHost::AppendBlobDataItem(
+    const std::string& uuid, const BlobData::Item& data_item) {
+  if (!context_)
+    return;
+  DCHECK_EQ(blobs_inuse_map_[uuid], 1);

[ericu, 2013/04/18 02:01:58]

How about a TODO for something harsher here [and perhaps elsewhere in this file]
than a DCHECK?
This could indicate a compromised renderer.

[michaeln, 2013/04/19 21:55:51]

i'll just make these changes now and put the return values in and such

+  context_->AppendBlobDataItem(uuid, data_item);
+}
+
+void BlobStorageHost::CancelBuildingBlob(const std::string& uuid) {
+  if (!context_)
+    return;
+  DCHECK_EQ(blobs_inuse_map_[uuid], 1);
+  blobs_inuse_map_.erase(uuid);
+  context_->CancelBuildingBlob(uuid);
+}
+
+void BlobStorageHost::FinishBuildingBlob(
+    const std::string& uuid, const std::string& content_type) {
+  if (!context_)
+    return;
+  DCHECK_EQ(blobs_inuse_map_[uuid], 1);
+  context_->FinishBuildingBlob(uuid, content_type);
+}
+
+void BlobStorageHost::AddFinishedBlob(const BlobData* blob_data) {

[ericu, 2013/04/18 02:01:58]

This function stands out as different.  Where is it used?  Where did the caller
get a BlobData that's not referenced by this host's process?

[michaeln, 2013/04/19 21:55:51]

this one is different, it's not in direct support of renderer delivered ipcs.
test cases use this

i need to provide a recipe for creating a blob in the browser process to be sent
to a renderer process. i think the similar context method is involved in that,
but you're right... this host method probably does not belong.

+  if (!context_)
+    return;
+  DCHECK(blobs_inuse_map_.find(blob_data->uuid()) == blobs_inuse_map_.end());
+  blobs_inuse_map_[blob_data->uuid()] = 1;
+  context_->AddFinishedBlob(blob_data);
+}
+
+void BlobStorageHost::IncrementBlobRefCount(const std::string& uuid) {
+  if (!context_)
+    return;
+  blobs_inuse_map_[uuid] += 1;
+  context_->IncrementBlobRefCount(uuid);
+}
+
+void BlobStorageHost::DecrementBlobRefCount(const std::string& uuid) {
+  if (!context_)
+    return;
+  DCHECK_GT(blobs_inuse_map_[uuid], 0);
+  blobs_inuse_map_[uuid] -= 1;
+  if (blobs_inuse_map_[uuid] == 0)
+    blobs_inuse_map_.erase(uuid);
+  context_->DecrementBlobRefCount(uuid);
+}
+
+void BlobStorageHost::RegisterPublicBlobURL(
+    const GURL& blob_url, const std::string& uuid) {
+  if (!context_)
+    return;
+  DCHECK_GT(blobs_inuse_map_[uuid], 0);
+  public_blob_urls_.insert(blob_url);
+  context_->RegisterPublicBlobURL(blob_url, uuid);
+}
+
+void BlobStorageHost::RevokePublicBlobURL(const GURL& blob_url) {
+  if (!context_)
+    return;
+  public_blob_urls_.erase(blob_url);
+  context_->RevokePublicBlobURL(blob_url);
+}
+
+//-----------------------------------------------------------------------
+// BlobStorageContext
+//-----------------------------------------------------------------------
+
+BlobStorageContext::BlobStorageContext()
     : memory_usage_(0) {
 }
 
-BlobStorageController::~BlobStorageController() {
+BlobStorageContext::~BlobStorageContext() {
 }
 
-void BlobStorageController::StartBuildingBlob(const GURL& url) {
-  DCHECK(url.SchemeIs("blob"));
-  DCHECK(!BlobUrlHasRef(url));
-  BlobData* blob_data = new BlobData;
-  unfinalized_blob_map_[url.spec()] = blob_data;
-  IncrementBlobDataUsage(blob_data);
+scoped_ptr<BlobDataHandle> BlobStorageContext::GetBlobDataFromUUID(
+    const std::string& uuid) {
+  scoped_ptr<BlobDataHandle> result;
+  BlobMap::iterator found = blob_map_.find(uuid);
+  if (found == blob_map_.end())
+    return result.Pass();
+  result.reset(new BlobDataHandle(found->second.second, this,
+                                  base::MessageLoopProxy::current()));
+  return result.Pass();
 }
 
-void BlobStorageController::AppendBlobDataItem(
-    const GURL& url, const BlobData::Item& item) {
-  DCHECK(url.SchemeIs("blob"));
-  DCHECK(!BlobUrlHasRef(url));
-  BlobMap::iterator found = unfinalized_blob_map_.find(url.spec());
+scoped_ptr<BlobDataHandle> BlobStorageContext::GetBlobDataFromPublicURL(
+    const GURL& url) {
+  BlobURLMap::iterator found = public_blob_urls_.find(
+      BlobUrlHasRef(url) ? ClearBlobUrlRef(url) : url);
+  if (found == public_blob_urls_.end())
+    return scoped_ptr<BlobDataHandle>();
+  return GetBlobDataFromUUID(found->second);
+}
+
+void BlobStorageContext::StartBuildingBlob(const std::string& uuid) {
+  DCHECK(unfinalized_blob_map_.find(uuid) == unfinalized_blob_map_.end());

[ericu, 2013/04/18 02:01:58]

This one definitely needs something stronger than a DCHECK; it allows a bad
renderer to run the browser out of memory.

[ericu, 2013/04/24 23:54:43]

Still there.

[michaeln, 2013/04/25 01:21:37]

See the Host class where renderer inputs are examined prior to calling this
private method. The gate keeper is the Host class.

+  unfinalized_blob_map_[uuid] = std::make_pair(1 , new BlobData(uuid));
+}
+
+void BlobStorageContext::AppendBlobDataItem(
+    const std::string& uuid, const BlobData::Item& item) {
+  BlobMap::iterator found = unfinalized_blob_map_.find(uuid);
   if (found == unfinalized_blob_map_.end())
     return;
-  BlobData* target_blob_data = found->second;
+  BlobData* target_blob_data = found->second.second;
   DCHECK(target_blob_data);
 
   memory_usage_ -= target_blob_data->GetMemoryUsage();
@@ -67,7 +228,7 @@
   // 3) The FileSystem File item is denoted by the FileSystem URL, the range
   //    and the expected modification time.
   // All the Blob items in the passing blob data are resolved and expanded into

[ericu, 2013/04/18 02:01:58]

What is "the passing blob data"?

-  // a set of Data and File items.
+  // a set of Data, File, and FileSystemFile items.
 
   DCHECK(item.length() > 0);
   switch (item.type()) {
@@ -84,17 +245,17 @@
       break;
     case BlobData::Item::TYPE_FILE_FILESYSTEM:
       AppendFileSystemFileItem(target_blob_data,
-                               item.url(),
+                               item.filesystem_url(),
                                item.offset(),
                                item.length(),
                                item.expected_modification_time());
       break;
     case BlobData::Item::TYPE_BLOB: {

[ericu, 2013/04/18 02:01:58]

If we always convert BlobData::Items to lowest form, how do we ever get one of
these?  Does the basic Blob constructor not come through this code?

[michaeln, 2013/04/23 21:15:10]

we'll get here by way of script...

var compoundBlob = new Blob(["hello\n", existingBlob])

[ericu, 2013/04/24 23:54:43]

Thx.

-      BlobData* src_blob_data = GetBlobDataFromUrl(item.url());
-      DCHECK(src_blob_data);
-      if (src_blob_data)
+      scoped_ptr<BlobDataHandle> src = GetBlobDataFromUUID(item.blob_uuid());
+      DCHECK(src.get());
+      if (src.get())
         AppendStorageItems(target_blob_data,
-                           src_blob_data,
+                           src->data(),
                            item.offset(),
                            item.length());
       break;
@@ -109,74 +270,82 @@
   // If we're using too much memory, drop this blob.
   // TODO(michaeln): Blob memory storage does not yet spill over to disk,
   // until it does, we'll prevent memory usage over a max amount.

[ericu, 2013/04/18 02:01:58]

We don't really prevent it--we just prevent it from lasting past the end of this
function.  We should really stop it *before* it gets huge, or at least within 1
BlobData::Item of going over the line.  Add a TODO?

[michaeln, 2013/04/19 21:55:51]

good point, i can test above instead of after the fact

[michaeln, 2013/04/25 01:05:01]

Done.

-  if (memory_usage_ > kMaxMemoryUsage)
-    RemoveBlob(url);
+  if (memory_usage_ > kMaxMemoryUsage) {
+    DCHECK_EQ(1, found->second.first);
+    memory_usage_ -= target_blob_data->GetMemoryUsage();
+    unfinalized_blob_map_.erase(found);
+  }
 }
 
-void BlobStorageController::FinishBuildingBlob(
-    const GURL& url, const std::string& content_type) {
-  DCHECK(url.SchemeIs("blob"));
-  DCHECK(!BlobUrlHasRef(url));
-  BlobMap::iterator found = unfinalized_blob_map_.find(url.spec());
-  if (found == unfinalized_blob_map_.end())
+void BlobStorageContext::FinishBuildingBlob(
+    const std::string& uuid, const std::string& content_type) {
+  BlobMap::iterator found = unfinalized_blob_map_.find(uuid);
+  if (found == unfinalized_blob_map_.end()) {
+    DCHECK(false);
     return;
-  found->second->set_content_type(content_type);
-  blob_map_[url.spec()] = found->second;
+  }
+  found->second.second->set_content_type(content_type);
+  blob_map_[uuid] = std::make_pair(1, found->second.second);
   unfinalized_blob_map_.erase(found);
 }
 
-void BlobStorageController::AddFinishedBlob(const GURL& url,
-                                            const BlobData* data) {
-  StartBuildingBlob(url);
+void BlobStorageContext::CancelBuildingBlob(const std::string& uuid) {
+  DCHECK(unfinalized_blob_map_.find(uuid) != unfinalized_blob_map_.end());

[ericu, 2013/04/18 02:01:58]

Also too dangerous for just DCHECK.

+  DecrementBlobRefCountHelper(&unfinalized_blob_map_, uuid);
+  DCHECK(unfinalized_blob_map_.find(uuid) == unfinalized_blob_map_.end());
+}
+
+void BlobStorageContext::AddFinishedBlob(const BlobData* data) {
+  StartBuildingBlob(data->uuid());
   for (std::vector<BlobData::Item>::const_iterator iter =
            data->items().begin();
        iter != data->items().end(); ++iter) {
-    AppendBlobDataItem(url, *iter);
+    AppendBlobDataItem(data->uuid(), *iter);
   }
-  FinishBuildingBlob(url, data->content_type());
+  FinishBuildingBlob(data->uuid(), data->content_type());
 }
 
-void BlobStorageController::CloneBlob(
-    const GURL& url, const GURL& src_url) {
-  DCHECK(url.SchemeIs("blob"));
-  DCHECK(!BlobUrlHasRef(url));
-
-  BlobData* blob_data = GetBlobDataFromUrl(src_url);
-  DCHECK(blob_data);
-  if (!blob_data)
+void BlobStorageContext::IncrementBlobRefCount(const std::string& uuid) {
+  BlobMap::iterator found = blob_map_.find(uuid);
+  if (found == blob_map_.end()) {
+    DCHECK(false);
     return;
-
-  blob_map_[url.spec()] = blob_data;
-  IncrementBlobDataUsage(blob_data);
+  }
+  ++(found->second.first);
 }
 
-void BlobStorageController::RemoveBlob(const GURL& url) {
-  DCHECK(url.SchemeIs("blob"));
-  DCHECK(!BlobUrlHasRef(url));
-
-  if (!RemoveFromMapHelper(&unfinalized_blob_map_, url))
-    RemoveFromMapHelper(&blob_map_, url);
+void BlobStorageContext::DecrementBlobRefCount(const std::string& uuid) {
+  if (!DecrementBlobRefCountHelper(&unfinalized_blob_map_, uuid))
+    DecrementBlobRefCountHelper(&blob_map_, uuid);
 }
 
-bool BlobStorageController::RemoveFromMapHelper(
-    BlobMap* map, const GURL& url) {
-  BlobMap::iterator found = map->find(url.spec());
+bool BlobStorageContext::DecrementBlobRefCountHelper(
+    BlobMap* map, const std::string& uuid) {
+  BlobMap::iterator found = map->find(uuid);
   if (found == map->end())
     return false;
-  if (DecrementBlobDataUsage(found->second))
-    memory_usage_ -= found->second->GetMemoryUsage();
-  map->erase(found);
+  if (--(found->second.first) == 0) {
+    DCHECK_EQ(found->second.second->uuid(), uuid);

[ericu, 2013/04/18 02:01:58]

This DCHECK_EQ could be hoisted to line 327.

[michaeln, 2013/04/19 21:55:51]

Done.

+    memory_usage_ -= found->second.second->GetMemoryUsage();
+    map->erase(found);
+  }
   return true;
 }
 
+void BlobStorageContext::RegisterPublicBlobURL(
+    const GURL& blob_url, const std::string& uuid) {
+  DCHECK(!BlobUrlHasRef(blob_url));
+  IncrementBlobRefCount(uuid);
+  public_blob_urls_[blob_url] = uuid;

[ericu, 2013/04/18 02:01:58]

What if this URL is already in the map?

[michaeln, 2013/04/19 21:55:51]

yup... another of those 'what you talkin about willis' error conditions that a
renderer may present us with

+}
 
-BlobData* BlobStorageController::GetBlobDataFromUrl(const GURL& url) {
-  BlobMap::iterator found = blob_map_.find(
-      BlobUrlHasRef(url) ? ClearBlobUrlRef(url).spec() : url.spec());
-  return (found != blob_map_.end()) ? found->second : NULL;
+void BlobStorageContext::RevokePublicBlobURL(const GURL& blob_url) {
+  DCHECK(!BlobUrlHasRef(blob_url));
+  DecrementBlobRefCount(public_blob_urls_[blob_url]);
+  public_blob_urls_.erase(blob_url);
 }
 
-void BlobStorageController::AppendStorageItems(
+void BlobStorageContext::AppendStorageItems(
     BlobData* target_blob_data, BlobData* src_blob_data,
     uint64 offset, uint64 length) {
   DCHECK(target_blob_data && src_blob_data &&
@@ -209,7 +378,7 @@
     } else {
       DCHECK(iter->type() == BlobData::Item::TYPE_FILE_FILESYSTEM);
       AppendFileSystemFileItem(target_blob_data,
-                               iter->url(),
+                               iter->filesystem_url(),
                                iter->offset() + offset,
                                new_length,
                                iter->expected_modification_time());
@@ -219,7 +388,7 @@
   }
 }
 
-void BlobStorageController::AppendFileItem(
+void BlobStorageContext::AppendFileItem(
     BlobData* target_blob_data,
     const base::FilePath& file_path, uint64 offset, uint64 length,
     const base::Time& expected_modification_time) {
@@ -233,25 +402,12 @@
     target_blob_data->AttachShareableFileReference(shareable_file);
 }
 
-void BlobStorageController::AppendFileSystemFileItem(
+void BlobStorageContext::AppendFileSystemFileItem(
     BlobData* target_blob_data,
-    const GURL& url, uint64 offset, uint64 length,
+    const GURL& filesystem_url, uint64 offset, uint64 length,
     const base::Time& expected_modification_time) {
-  target_blob_data->AppendFileSystemFile(url, offset, length,
+  target_blob_data->AppendFileSystemFile(filesystem_url, offset, length,
                                          expected_modification_time);
 }
 
-void BlobStorageController::IncrementBlobDataUsage(BlobData* blob_data) {
-  blob_data_usage_count_[blob_data] += 1;
-}
-
-bool BlobStorageController::DecrementBlobDataUsage(BlobData* blob_data) {
-  BlobDataUsageMap::iterator found = blob_data_usage_count_.find(blob_data);
-  DCHECK(found != blob_data_usage_count_.end());
-  if (--(found->second))
-    return false;  // Still in use
-  blob_data_usage_count_.erase(found);
-  return true;
-}
-
 }  // namespace webkit_blob