Skip to content

Commit

Permalink
Revert of Allow BlobDataHandles to be copied, and have their UUIDs re…
Browse files Browse the repository at this point in the history 
…ad, on any thread. (https://codereview.chromium.org/259773006/)

Reason for revert:
I'm sorry to revert this change. It looks like it breaks the ASAN bot:

http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Tests%20%282%29/builds/2161/steps/content_unittests/logs/ResolveBlobAndCreateUploadDataStream

Direct leak of 120 byte(s) in 3 object(s) allocated from:
    #0 0x520dbb in operator new(unsigned long) /usr/local/google/work/chromium/src/third_party/llvm/projects/compiler-rt/lib/asan/asan_new_delete.cc:62
    chromium#1 0x3a0f611 in webkit_blob::BlobDataHandle::BlobDataHandle(webkit_blob::BlobData*, webkit_blob::BlobStorageContext*, base::SequencedTaskRunner*) webkit/browser/blob/blob_data_handle.cc:42
    chromium#2 0x3a0fd71 in webkit_blob::BlobStorageContext::GetBlobDataFromUUID(std::string const&) webkit/browser/blob/blob_storage_context.cc:69
    chromium#3 0x268955d in ResolveBlobReference content/browser/loader/upload_data_stream_builder.cc:73
...

Original issue's description:
> Allow BlobDataHandles to be copied, and have their UUIDs read, on any thread.
> 
> BUG=108012
> 
> Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266817
> 
> Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267423

TBR=michaeln@chromium.org,piman@chromium.org,ericu@chromium.org
NOTREECHECKS=true
NOTRY=true
BUG=108012

Review URL: https://codereview.chromium.org/261683005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@267479 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
falken@chromium.org committed May 1, 2014
1 parent 9edf15b commit ac6f8c9
Show file tree
Hide file tree
Showing 5 changed files with 36 additions and 101 deletions.
27 changes: 0 additions & 27 deletions content/browser/fileapi/blob_storage_context_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
#include "base/memory/ref_counted.h"
#include "base/memory/scoped_ptr.h"
#include "base/message_loop/message_loop.h"
#include "base/run_loop.h"
#include "base/time/time.h"
#include "content/browser/fileapi/blob_storage_host.h"
#include "testing/gtest/include/gtest/gtest.h"
Expand Down Expand Up @@ -42,21 +41,13 @@ TEST(BlobStorageContextTest, IncrementDecrementRef) {
blob_data_handle = context.GetBlobDataFromUUID(kId);
EXPECT_TRUE(blob_data_handle);
blob_data_handle.reset();
{ // Clean up for ASAN
base::RunLoop run_loop;
run_loop.RunUntilIdle();
}

// Make sure its still there after inc/dec.
EXPECT_TRUE(host.IncrementBlobRefCount(kId));
EXPECT_TRUE(host.DecrementBlobRefCount(kId));
blob_data_handle = context.GetBlobDataFromUUID(kId);
EXPECT_TRUE(blob_data_handle);
blob_data_handle.reset();
{ // Clean up for ASAN
base::RunLoop run_loop;
run_loop.RunUntilIdle();
}

// Make sure it goes away in the end.
EXPECT_TRUE(host.DecrementBlobRefCount(kId));
Expand Down Expand Up @@ -91,10 +82,6 @@ TEST(BlobStorageContextTest, BlobDataHandle) {
// Should disappear after dropping both handles.
blob_data_handle.reset();
another_handle.reset();
{ // Clean up for ASAN
base::RunLoop run_loop;
run_loop.RunUntilIdle();
}
blob_data_handle = context.GetBlobDataFromUUID(kId);
EXPECT_FALSE(blob_data_handle);
}
Expand Down Expand Up @@ -144,12 +131,6 @@ TEST(BlobStorageContextTest, CompoundBlobs) {
blob_data_handle = context.AddFinishedBlob(blob_data2.get());
ASSERT_TRUE(blob_data_handle.get());
EXPECT_TRUE(*(blob_data_handle->data()) == *canonicalized_blob_data2.get());

blob_data_handle.reset();
{ // Clean up for ASAN
base::RunLoop run_loop;
run_loop.RunUntilIdle();
}
}

TEST(BlobStorageContextTest, PublicBlobUrls) {
Expand All @@ -169,21 +150,13 @@ TEST(BlobStorageContextTest, PublicBlobUrls) {
ASSERT_TRUE(blob_data_handle.get());
EXPECT_EQ(kId, blob_data_handle->data()->uuid());
blob_data_handle.reset();
{ // Clean up for ASAN
base::RunLoop run_loop;
run_loop.RunUntilIdle();
}

// The url registration should keep the blob alive even after
// explicit references are dropped.
EXPECT_TRUE(host.DecrementBlobRefCount(kId));
blob_data_handle = context.GetBlobDataFromPublicURL(kUrl);
EXPECT_TRUE(blob_data_handle);
blob_data_handle.reset();
{ // Clean up for ASAN
base::RunLoop run_loop;
run_loop.RunUntilIdle();
}

// Finally get rid of the url registration and the blob.
EXPECT_TRUE(host.RevokePublicBlobURL(kUrl));
Expand Down
7 changes: 0 additions & 7 deletions content/browser/loader/upload_data_stream_builder_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
#include "base/files/file_path.h"
#include "base/message_loop/message_loop.h"
#include "base/message_loop/message_loop_proxy.h"
#include "base/run_loop.h"
#include "base/time/time.h"
#include "content/common/resource_request_body.h"
#include "net/base/upload_bytes_element_reader.h"
Expand Down Expand Up @@ -258,12 +257,6 @@ TEST(UploadDataStreamBuilderTest, ResolveBlobAndCreateUploadDataStream) {
EXPECT_TRUE(AreElementsEqual(*upload->element_readers()[5], blob_element1));
EXPECT_TRUE(AreElementsEqual(*upload->element_readers()[6], blob_element2));
EXPECT_TRUE(AreElementsEqual(*upload->element_readers()[7], upload_element2));

// Clean up for ASAN.
handle1.reset();
handle2.reset();
base::RunLoop run_loop;
run_loop.RunUntilIdle();
}

} // namespace content
66 changes: 27 additions & 39 deletions webkit/browser/blob/blob_data_handle.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,56 +13,44 @@

namespace webkit_blob {

BlobDataHandle::BlobDataHandleShared::BlobDataHandleShared(
BlobData* blob_data,
BlobStorageContext* context,
base::SequencedTaskRunner* task_runner)
: blob_data_(blob_data),
context_(context->AsWeakPtr()) {
context_->IncrementBlobRefCount(blob_data->uuid());
}

BlobData* BlobDataHandle::BlobDataHandleShared::data() const {
return blob_data_;
}

const std::string& BlobDataHandle::BlobDataHandleShared::uuid() const {
return blob_data_->uuid();
}

BlobDataHandle::BlobDataHandleShared::~BlobDataHandleShared() {
if (context_.get())
context_->DecrementBlobRefCount(blob_data_->uuid());
}

BlobDataHandle::BlobDataHandle(BlobData* blob_data,
BlobStorageContext* context,
BlobDataHandle::BlobDataHandle(BlobData* blob_data, BlobStorageContext* context,
base::SequencedTaskRunner* task_runner)
: io_task_runner_(task_runner),
shared_(new BlobDataHandleShared(blob_data, context, task_runner)) {
DCHECK(io_task_runner_);
: blob_data_(blob_data),
context_(context->AsWeakPtr()),
io_task_runner_(task_runner) {
// Ensures the uuid remains registered and the underlying data is not deleted.
DCHECK(io_task_runner_->RunsTasksOnCurrentThread());
}

BlobDataHandle::BlobDataHandle(const BlobDataHandle& other) {
io_task_runner_ = other.io_task_runner_;
shared_ = other.shared_;
context_->IncrementBlobRefCount(blob_data->uuid());
blob_data_->AddRef();
}

BlobDataHandle::~BlobDataHandle() {
BlobDataHandleShared* raw = shared_.get();
raw->AddRef();
shared_ = 0;
io_task_runner_->ReleaseSoon(FROM_HERE, raw);
if (io_task_runner_->RunsTasksOnCurrentThread()) {
// Note: Do not test context_ or alter the blob_data_ refcount
// on the wrong thread.
if (context_.get())
context_->DecrementBlobRefCount(blob_data_->uuid());
blob_data_->Release();
return;
}

io_task_runner_->PostTask(
FROM_HERE,
base::Bind(&DeleteHelper, context_, base::Unretained(blob_data_)));
}

BlobData* BlobDataHandle::data() const {
DCHECK(io_task_runner_->RunsTasksOnCurrentThread());
return shared_->data();
return blob_data_;
}

std::string BlobDataHandle::uuid() const {
return shared_->uuid();
// static
void BlobDataHandle::DeleteHelper(
base::WeakPtr<BlobStorageContext> context,
BlobData* blob_data) {
if (context.get())
context->DecrementBlobRefCount(blob_data->uuid());
blob_data->Release();
}

} // namespace webkit_blob
35 changes: 8 additions & 27 deletions webkit/browser/blob/blob_data_handle.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,42 +28,23 @@ class BlobStorageContext;
class WEBKIT_STORAGE_BROWSER_EXPORT BlobDataHandle
: public base::SupportsUserData::Data {
public:
BlobDataHandle(const BlobDataHandle& other); // May be copied on any thread.
virtual ~BlobDataHandle(); // Maybe be deleted on any thread.
BlobData* data() const; // May only be accessed on the IO thread.

std::string uuid() const; // May be accessed on any thread.

private:
class BlobDataHandleShared
: public base::RefCountedThreadSafe<BlobDataHandleShared> {
public:
BlobDataHandleShared(BlobData* blob_data,
BlobStorageContext* context,
base::SequencedTaskRunner* task_runner);

BlobData* data() const;
const std::string& uuid() const;

private:
friend class base::DeleteHelper<BlobDataHandleShared>;
friend class base::RefCountedThreadSafe<BlobDataHandleShared>;
friend class BlobDataHandle;

virtual ~BlobDataHandleShared();

scoped_refptr<BlobData> blob_data_;
base::WeakPtr<BlobStorageContext> context_;

DISALLOW_COPY_AND_ASSIGN(BlobDataHandleShared);
};

friend class BlobStorageContext;
BlobDataHandle(BlobData* blob_data, BlobStorageContext* context,
base::SequencedTaskRunner* task_runner);

static void DeleteHelper(
base::WeakPtr<BlobStorageContext> context,
BlobData* blob_data);

BlobData* blob_data_; // Intentionally a raw ptr to a non-thread-safe ref.
base::WeakPtr<BlobStorageContext> context_;
scoped_refptr<base::SequencedTaskRunner> io_task_runner_;
scoped_refptr<BlobDataHandleShared> shared_;

DISALLOW_COPY_AND_ASSIGN(BlobDataHandle);
};

} // namespace webkit_blob
Expand Down
2 changes: 1 addition & 1 deletion webkit/browser/blob/blob_storage_context.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ class WEBKIT_STORAGE_BROWSER_EXPORT BlobStorageContext

private:
friend class content::BlobStorageHost;
friend class BlobDataHandle::BlobDataHandleShared;
friend class BlobDataHandle;
friend class ViewBlobInternalsJob;

enum EntryFlags {
Expand Down

0 comments on commit ac6f8c9

Please sign in to comment.