Revert of Refactor FakeAttachmentStore (patchset #3 id:40001 of https://codereview.chromium.org/601553004/)

Reason for revert:
Speculative revert:
looks like it causes failures on the Linux asan: http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Tests%20%283%29/builds/8113

Original issue's description:
> Refactor FakeAttachmentStore
>
> Refactor FakeAttachmentStore into AttachmentStoreProxy and
> InMemoryAttachmentStore backend. Break AttachmentStore interface into
> AttachmentStoreBase (common interface for backends) and AttachmentStore
> (referenced by datatype and AttachmentService).
>
> BUG=
> R=maniscalco@chromium.org
>
> Committed: https://crrev.com/6ae2b341c73909cf902078446346d6c900273409
> Cr-Commit-Position: refs/heads/master@{#297339}

TBR=maniscalco@chromium.org,pavely@chromium.org
NOTREECHECKS=true
NOTRY=true
BUG=

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

Cr-Commit-Position: refs/heads/master@{#297379}

5 files changed, 449 insertions, 33 deletions

diff --git a/sync/api/attachments/attachment_store.cc b/sync/api/attachments/attachment_store.cc
index ad4489e..e22b040 100644
--- a/sync/api/attachments/attachment_store.cc
+++ b/sync/api/attachments/attachment_store.cc
@@ -4,24 +4,9 @@
 
 #include "sync/api/attachments/attachment_store.h"
 
-#include "base/thread_task_runner_handle.h"
-#include "sync/internal_api/public/attachments/attachment_store_handle.h"
-#include "sync/internal_api/public/attachments/in_memory_attachment_store.h"
-
 namespace syncer {
 
-AttachmentStoreBase::AttachmentStoreBase() {}
-AttachmentStoreBase::~AttachmentStoreBase() {}
-
 AttachmentStore::AttachmentStore() {}
 AttachmentStore::~AttachmentStore() {}
 
-scoped_refptr<AttachmentStore> AttachmentStore::CreateInMemoryStore() {
-  // Both frontend and backend of attachment store will live on current thread.
-  scoped_ptr<AttachmentStoreBase> backend(
-      new InMemoryAttachmentStore(base::ThreadTaskRunnerHandle::Get()));
-  return scoped_refptr<AttachmentStore>(new AttachmentStoreHandle(
-      backend.Pass(), base::ThreadTaskRunnerHandle::Get()));
-}
-
 }  // namespace syncer
diff --git a/sync/api/attachments/attachment_store.h b/sync/api/attachments/attachment_store.h
index 45ccaf9..d487f8c 100644
--- a/sync/api/attachments/attachment_store.h
+++ b/sync/api/attachments/attachment_store.h
@@ -25,8 +25,10 @@ class AttachmentId;
 //
 // Destroying this object does not necessarily cancel outstanding async
 // operations. If you need cancel like semantics, use WeakPtr in the callbacks.
-class SYNC_EXPORT AttachmentStoreBase {
+class SYNC_EXPORT AttachmentStore : public base::RefCounted<AttachmentStore> {
  public:
+  AttachmentStore();
+
   // TODO(maniscalco): Consider udpating Read and Write methods to support
   // resumable transfers (bug 353292).
 
@@ -41,9 +43,6 @@ class SYNC_EXPORT AttachmentStoreBase {
   typedef base::Callback<void(const Result&)> WriteCallback;
   typedef base::Callback<void(const Result&)> DropCallback;
 
-  AttachmentStoreBase();
-  virtual ~AttachmentStoreBase();
-
   // Asynchronously reads the attachments identified by |ids|.
   //
   // |callback| will be invoked when finished. AttachmentStore will attempt to
@@ -82,22 +81,9 @@ class SYNC_EXPORT AttachmentStoreBase {
   // successfully.
   virtual void Drop(const AttachmentIdList& ids,
                     const DropCallback& callback) = 0;
-};
-
-// AttachmentStore is an interface exposed to data type and AttachmentService
-// code. Also contains factory methods for default implementations.
-class SYNC_EXPORT AttachmentStore
-    : public AttachmentStoreBase,
-      public base::RefCountedThreadSafe<AttachmentStore> {
- public:
-  AttachmentStore();
-
-  // Creates an AttachmentStoreHandle backed by in-memory implementation of
-  // attachment store. For now frontend lives on the same thread as backend.
-  static scoped_refptr<AttachmentStore> CreateInMemoryStore();
 
  protected:
-  friend class base::RefCountedThreadSafe<AttachmentStore>;
+  friend class base::RefCounted<AttachmentStore>;
   virtual ~AttachmentStore();
 };
 
diff --git a/sync/api/attachments/fake_attachment_store.cc b/sync/api/attachments/fake_attachment_store.cc
new file mode 100644
index 0000000..d20a9b5
--- /dev/null
+++ b/sync/api/attachments/fake_attachment_store.cc
@@ -0,0 +1,128 @@
+// Copyright 2014 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 "sync/api/attachments/fake_attachment_store.h"
+
+#include "base/bind.h"
+#include "base/location.h"
+#include "base/memory/ref_counted_memory.h"
+#include "base/sequenced_task_runner.h"
+#include "base/single_thread_task_runner.h"
+#include "base/thread_task_runner_handle.h"
+#include "sync/api/attachments/attachment.h"
+
+namespace syncer {
+
+// Backend is where all the work happens.
+class FakeAttachmentStore::Backend
+    : public base::RefCountedThreadSafe<FakeAttachmentStore::Backend> {
+ public:
+  // Construct a Backend that posts its results to |frontend_task_runner|.
+  Backend(
+      const scoped_refptr<base::SingleThreadTaskRunner>& frontend_task_runner);
+
+  void Read(const AttachmentIdList& ids, const ReadCallback& callback);
+  void Write(const AttachmentList& attachments, const WriteCallback& callback);
+  void Drop(const AttachmentIdList& ids, const DropCallback& callback);
+
+ private:
+  friend class base::RefCountedThreadSafe<Backend>;
+
+  ~Backend();
+
+  scoped_refptr<base::SingleThreadTaskRunner> frontend_task_runner_;
+  AttachmentMap attachments_;
+};
+
+FakeAttachmentStore::Backend::Backend(
+    const scoped_refptr<base::SingleThreadTaskRunner>& frontend_task_runner)
+    : frontend_task_runner_(frontend_task_runner) {}
+
+FakeAttachmentStore::Backend::~Backend() {}
+
+void FakeAttachmentStore::Backend::Read(const AttachmentIdList& ids,
+                                        const ReadCallback& callback) {
+  Result result_code = SUCCESS;
+  AttachmentIdList::const_iterator id_iter = ids.begin();
+  AttachmentIdList::const_iterator id_end = ids.end();
+  scoped_ptr<AttachmentMap> result_map(new AttachmentMap);
+  scoped_ptr<AttachmentIdList> unavailable_attachments(new AttachmentIdList);
+  for (; id_iter != id_end; ++id_iter) {
+    const AttachmentId& id = *id_iter;
+    syncer::AttachmentMap::iterator attachment_iter =
+        attachments_.find(*id_iter);
+    if (attachment_iter != attachments_.end()) {
+      const Attachment& attachment = attachment_iter->second;
+      result_map->insert(std::make_pair(id, attachment));
+    } else {
+      unavailable_attachments->push_back(id);
+    }
+  }
+  if (!unavailable_attachments->empty()) {
+    result_code = UNSPECIFIED_ERROR;
+  }
+  frontend_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(callback,
+                 result_code,
+                 base::Passed(&result_map),
+                 base::Passed(&unavailable_attachments)));
+}
+
+void FakeAttachmentStore::Backend::Write(const AttachmentList& attachments,
+                                         const WriteCallback& callback) {
+  AttachmentList::const_iterator iter = attachments.begin();
+  AttachmentList::const_iterator end = attachments.end();
+  for (; iter != end; ++iter) {
+    attachments_.insert(std::make_pair(iter->GetId(), *iter));
+  }
+  frontend_task_runner_->PostTask(FROM_HERE, base::Bind(callback, SUCCESS));
+}
+
+void FakeAttachmentStore::Backend::Drop(const AttachmentIdList& ids,
+                                        const DropCallback& callback) {
+  Result result = SUCCESS;
+  AttachmentIdList::const_iterator ids_iter = ids.begin();
+  AttachmentIdList::const_iterator ids_end = ids.end();
+  for (; ids_iter != ids_end; ++ids_iter) {
+    AttachmentMap::iterator attachments_iter = attachments_.find(*ids_iter);
+    if (attachments_iter != attachments_.end()) {
+      attachments_.erase(attachments_iter);
+    }
+  }
+  frontend_task_runner_->PostTask(FROM_HERE, base::Bind(callback, result));
+}
+
+FakeAttachmentStore::FakeAttachmentStore(
+    const scoped_refptr<base::SequencedTaskRunner>& backend_task_runner)
+    : backend_(new Backend(base::ThreadTaskRunnerHandle::Get())),
+      backend_task_runner_(backend_task_runner) {}
+
+FakeAttachmentStore::~FakeAttachmentStore() {}
+
+void FakeAttachmentStore::Read(const AttachmentIdList& ids,
+                               const ReadCallback& callback) {
+  backend_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&FakeAttachmentStore::Backend::Read, backend_, ids, callback));
+}
+
+void FakeAttachmentStore::Write(const AttachmentList& attachments,
+                                const WriteCallback& callback) {
+  backend_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&FakeAttachmentStore::Backend::Write,
+                 backend_,
+                 attachments,
+                 callback));
+}
+
+void FakeAttachmentStore::Drop(const AttachmentIdList& ids,
+                               const DropCallback& callback) {
+  backend_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&FakeAttachmentStore::Backend::Drop, backend_, ids, callback));
+}
+
+}  // namespace syncer
diff --git a/sync/api/attachments/fake_attachment_store.h b/sync/api/attachments/fake_attachment_store.h
new file mode 100644
index 0000000..9ac7006
--- /dev/null
+++ b/sync/api/attachments/fake_attachment_store.h
@@ -0,0 +1,60 @@
+// Copyright 2014 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.
+
+#ifndef SYNC_API_ATTACHMENTS_FAKE_ATTACHMENT_STORE_H_
+#define SYNC_API_ATTACHMENTS_FAKE_ATTACHMENT_STORE_H_
+
+#include <map>
+
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/stl_util.h"
+#include "sync/api/attachments/attachment_store.h"
+#include "sync/base/sync_export.h"
+
+namespace base {
+class SequencedTaskRunner;
+class SingleThreadTaskRunner;
+}  // namespace base
+
+namespace sync_pb {
+class AttachmentId;
+}  // namespace sync_pb
+
+namespace syncer {
+
+class Attachment;
+
+// A in-memory only implementation of AttachmentStore used for testing.
+//
+// Requires that the current thread has a MessageLoop.
+class SYNC_EXPORT FakeAttachmentStore : public AttachmentStore {
+ public:
+  // Construct a FakeAttachmentStore whose "IO" will be performed in
+  // |backend_task_runner|.
+  explicit FakeAttachmentStore(
+      const scoped_refptr<base::SequencedTaskRunner>& backend_task_runner);
+
+  // AttachmentStore implementation.
+  virtual void Read(const AttachmentIdList& id,
+                    const ReadCallback& callback) OVERRIDE;
+  virtual void Write(const AttachmentList& attachments,
+                     const WriteCallback& callback) OVERRIDE;
+  virtual void Drop(const AttachmentIdList& id,
+                    const DropCallback& callback) OVERRIDE;
+
+ private:
+  class Backend;
+
+  virtual ~FakeAttachmentStore();
+
+  scoped_refptr<Backend> backend_;
+  scoped_refptr<base::SequencedTaskRunner> backend_task_runner_;
+
+  DISALLOW_COPY_AND_ASSIGN(FakeAttachmentStore);
+};
+
+}  // namespace syncer
+
+#endif  // SYNC_API_ATTACHMENTS_FAKE_ATTACHMENT_STORE_H_
diff --git a/sync/api/attachments/fake_attachment_store_unittest.cc b/sync/api/attachments/fake_attachment_store_unittest.cc
new file mode 100644
index 0000000..71b9e50
--- /dev/null
+++ b/sync/api/attachments/fake_attachment_store_unittest.cc
@@ -0,0 +1,257 @@
+// Copyright 2014 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 "sync/api/attachments/fake_attachment_store.h"
+
+#include "base/bind.h"
+#include "base/memory/ref_counted_memory.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/message_loop/message_loop.h"
+#include "base/thread_task_runner_handle.h"
+#include "sync/api/attachments/attachment.h"
+#include "sync/protocol/sync.pb.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace syncer {
+
+const char kTestData1[] = "test data 1";
+const char kTestData2[] = "test data 2";
+
+class FakeAttachmentStoreTest : public testing::Test {
+ protected:
+  base::MessageLoop message_loop;
+  scoped_refptr<FakeAttachmentStore> store;
+  AttachmentStore::Result result;
+  scoped_ptr<AttachmentMap> attachments;
+  scoped_ptr<AttachmentIdList> failed_attachment_ids;
+
+  AttachmentStore::ReadCallback read_callback;
+  AttachmentStore::WriteCallback write_callback;
+  AttachmentStore::DropCallback drop_callback;
+
+  scoped_refptr<base::RefCountedString> some_data1;
+  scoped_refptr<base::RefCountedString> some_data2;
+
+  FakeAttachmentStoreTest()
+      : store(new FakeAttachmentStore(base::ThreadTaskRunnerHandle::Get())) {}
+
+  virtual void SetUp() {
+    Clear();
+    read_callback = base::Bind(&FakeAttachmentStoreTest::CopyResultAttachments,
+                               base::Unretained(this),
+                               &result,
+                               &attachments,
+                               &failed_attachment_ids);
+    write_callback = base::Bind(
+        &FakeAttachmentStoreTest::CopyResult, base::Unretained(this), &result);
+    drop_callback = write_callback;
+
+    some_data1 = new base::RefCountedString;
+    some_data1->data() = kTestData1;
+
+    some_data2 = new base::RefCountedString;
+    some_data2->data() = kTestData2;
+  }
+
+  virtual void ClearAndPumpLoop() {
+    Clear();
+    message_loop.RunUntilIdle();
+  }
+
+ private:
+  void Clear() {
+    result = AttachmentStore::UNSPECIFIED_ERROR;
+    attachments.reset();
+    failed_attachment_ids.reset();
+  }
+
+  void CopyResult(AttachmentStore::Result* destination_result,
+                  const AttachmentStore::Result& source_result) {
+    *destination_result = source_result;
+  }
+
+  void CopyResultAttachments(
+      AttachmentStore::Result* destination_result,
+      scoped_ptr<AttachmentMap>* destination_attachments,
+      scoped_ptr<AttachmentIdList>* destination_failed_attachment_ids,
+      const AttachmentStore::Result& source_result,
+      scoped_ptr<AttachmentMap> source_attachments,
+      scoped_ptr<AttachmentIdList> source_failed_attachment_ids) {
+    CopyResult(destination_result, source_result);
+    *destination_attachments = source_attachments.Pass();
+    *destination_failed_attachment_ids = source_failed_attachment_ids.Pass();
+  }
+};
+
+// Verify that we do not overwrite existing attachments and that we do not treat
+// it as an error.
+TEST_F(FakeAttachmentStoreTest, Write_NoOverwriteNoError) {
+  // Create two attachments with the same id but different data.
+  Attachment attachment1 = Attachment::Create(some_data1);
+  Attachment attachment2 =
+      Attachment::CreateWithId(attachment1.GetId(), some_data2);
+
+  // Write the first one.
+  AttachmentList some_attachments;
+  some_attachments.push_back(attachment1);
+  store->Write(some_attachments, write_callback);
+  ClearAndPumpLoop();
+  EXPECT_EQ(result, AttachmentStore::SUCCESS);
+
+  // Write the second one.
+  some_attachments.clear();
+  some_attachments.push_back(attachment2);
+  store->Write(some_attachments, write_callback);
+  ClearAndPumpLoop();
+  EXPECT_EQ(result, AttachmentStore::SUCCESS);
+
+  // Read it back and see that it was not overwritten.
+  AttachmentIdList some_attachment_ids;
+  some_attachment_ids.push_back(attachment1.GetId());
+  store->Read(some_attachment_ids, read_callback);
+  ClearAndPumpLoop();
+  EXPECT_EQ(result, AttachmentStore::SUCCESS);
+  EXPECT_EQ(attachments->size(), 1U);
+  EXPECT_EQ(failed_attachment_ids->size(), 0U);
+  AttachmentMap::const_iterator a1 = attachments->find(attachment1.GetId());
+  EXPECT_TRUE(a1 != attachments->end());
+  EXPECT_TRUE(attachment1.GetData()->Equals(a1->second.GetData()));
+}
+
+// Verify that we can write some attachments and read them back.
+TEST_F(FakeAttachmentStoreTest, Write_RoundTrip) {
+  Attachment attachment1 = Attachment::Create(some_data1);
+  Attachment attachment2 = Attachment::Create(some_data2);
+  AttachmentList some_attachments;
+  some_attachments.push_back(attachment1);
+  some_attachments.push_back(attachment2);
+
+  store->Write(some_attachments, write_callback);
+  ClearAndPumpLoop();
+  EXPECT_EQ(result, AttachmentStore::SUCCESS);
+
+  AttachmentIdList some_attachment_ids;
+  some_attachment_ids.push_back(attachment1.GetId());
+  some_attachment_ids.push_back(attachment2.GetId());
+  store->Read(some_attachment_ids, read_callback);
+  ClearAndPumpLoop();
+  EXPECT_EQ(result, AttachmentStore::SUCCESS);
+  EXPECT_EQ(attachments->size(), 2U);
+  EXPECT_EQ(failed_attachment_ids->size(), 0U);
+
+  AttachmentMap::const_iterator a1 = attachments->find(attachment1.GetId());
+  EXPECT_TRUE(a1 != attachments->end());
+  EXPECT_TRUE(attachment1.GetData()->Equals(a1->second.GetData()));
+
+  AttachmentMap::const_iterator a2 = attachments->find(attachment2.GetId());
+  EXPECT_TRUE(a2 != attachments->end());
+  EXPECT_TRUE(attachment2.GetData()->Equals(a2->second.GetData()));
+}
+
+// Try to read two attachments when only one exists.
+TEST_F(FakeAttachmentStoreTest, Read_OneNotFound) {
+  Attachment attachment1 = Attachment::Create(some_data1);
+  Attachment attachment2 = Attachment::Create(some_data2);
+
+  AttachmentList some_attachments;
+  // Write attachment1 only.
+  some_attachments.push_back(attachment1);
+  store->Write(some_attachments, write_callback);
+  ClearAndPumpLoop();
+  EXPECT_EQ(result, AttachmentStore::SUCCESS);
+
+  // Try to read both attachment1 and attachment2.
+  AttachmentIdList ids;
+  ids.push_back(attachment1.GetId());
+  ids.push_back(attachment2.GetId());
+  store->Read(ids, read_callback);
+  ClearAndPumpLoop();
+
+  // See that only attachment1 was read.
+  EXPECT_EQ(result, AttachmentStore::UNSPECIFIED_ERROR);
+  EXPECT_EQ(attachments->size(), 1U);
+  EXPECT_EQ(failed_attachment_ids->size(), 1U);
+}
+
+// Try to drop two attachments when only one exists. Verify that no error occurs
+// and that the existing attachment was dropped.
+TEST_F(FakeAttachmentStoreTest, Drop_DropTwoButOnlyOneExists) {
+  // First, create two attachments.
+  Attachment attachment1 = Attachment::Create(some_data1);
+  Attachment attachment2 = Attachment::Create(some_data2);
+  AttachmentList some_attachments;
+  some_attachments.push_back(attachment1);
+  some_attachments.push_back(attachment2);
+  store->Write(some_attachments, write_callback);
+  ClearAndPumpLoop();
+  EXPECT_EQ(result, AttachmentStore::SUCCESS);
+
+  // Drop attachment1 only.
+  AttachmentIdList ids;
+  ids.push_back(attachment1.GetId());
+  store->Drop(ids, drop_callback);
+  ClearAndPumpLoop();
+  EXPECT_EQ(result, AttachmentStore::SUCCESS);
+
+  // See that attachment1 is gone.
+  store->Read(ids, read_callback);
+  ClearAndPumpLoop();
+  EXPECT_EQ(result, AttachmentStore::UNSPECIFIED_ERROR);
+  EXPECT_EQ(attachments->size(), 0U);
+  EXPECT_EQ(failed_attachment_ids->size(), 1U);
+  EXPECT_EQ((*failed_attachment_ids)[0], attachment1.GetId());
+
+  // Drop both attachment1 and attachment2.
+  ids.clear();
+  ids.push_back(attachment1.GetId());
+  ids.push_back(attachment2.GetId());
+  store->Drop(ids, drop_callback);
+  ClearAndPumpLoop();
+  EXPECT_EQ(result, AttachmentStore::SUCCESS);
+
+  // See that attachment2 is now gone.
+  ids.clear();
+  ids.push_back(attachment2.GetId());
+  store->Read(ids, read_callback);
+  ClearAndPumpLoop();
+  EXPECT_EQ(result, AttachmentStore::UNSPECIFIED_ERROR);
+  EXPECT_EQ(attachments->size(), 0U);
+  EXPECT_EQ(failed_attachment_ids->size(), 1U);
+  EXPECT_EQ((*failed_attachment_ids)[0], attachment2.GetId());
+}
+
+// Verify that attempting to drop an attachment that does not exist is not an
+// error.
+TEST_F(FakeAttachmentStoreTest, Drop_DoesNotExist) {
+  Attachment attachment1 = Attachment::Create(some_data1);
+  AttachmentList some_attachments;
+  some_attachments.push_back(attachment1);
+  store->Write(some_attachments, write_callback);
+  ClearAndPumpLoop();
+  EXPECT_EQ(result, AttachmentStore::SUCCESS);
+
+  // Drop the attachment.
+  AttachmentIdList ids;
+  ids.push_back(attachment1.GetId());
+  store->Drop(ids, drop_callback);
+  ClearAndPumpLoop();
+  EXPECT_EQ(result, AttachmentStore::SUCCESS);
+
+  // See that it's gone.
+  store->Read(ids, read_callback);
+  ClearAndPumpLoop();
+  EXPECT_EQ(result, AttachmentStore::UNSPECIFIED_ERROR);
+  EXPECT_EQ(attachments->size(), 0U);
+  EXPECT_EQ(failed_attachment_ids->size(), 1U);
+  EXPECT_EQ((*failed_attachment_ids)[0], attachment1.GetId());
+
+  // Drop again, see that no error occurs.
+  ids.clear();
+  ids.push_back(attachment1.GetId());
+  store->Drop(ids, drop_callback);
+  ClearAndPumpLoop();
+  EXPECT_EQ(result, AttachmentStore::SUCCESS);
+}
+
+}  // namespace syncer