initial add of SQL based storage

components/offline_pages/offline_page_metadata_store_sql.h

Index: components/offline_pages/offline_page_metadata_store_sql.h
diff --git a/components/offline_pages/offline_page_metadata_store_sql.h b/components/offline_pages/offline_page_metadata_store_sql.h
new file mode 100644
index 0000000000000000000000000000000000000000..ff0a27e613b6004d08bb04d9aebd99ac830f0e9e
--- /dev/null
+++ b/components/offline_pages/offline_page_metadata_store_sql.h
@@ -0,0 +1,92 @@
+// Copyright 2016 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 COMPONENTS_OFFLINE_PAGES_OFFLINE_PAGE_METADATA_STORE_SQL_H_
+#define COMPONENTS_OFFLINE_PAGES_OFFLINE_PAGE_METADATA_STORE_SQL_H_
+
+#include <stdint.h>
+
+#include <memory>
+#include <vector>
+
+#include "base/files/file_path.h"
+#include "base/memory/weak_ptr.h"
+#include "components/offline_pages/offline_page_metadata_store.h"
+
+namespace sql {
+class Connection;
+class MetaTable;
+class Statement;
+class StatementID;
+}
+
+namespace base {
+class SequencedTaskRunner;
+}
+
+namespace offline_pages {
+
+// OfflinePageMetadataStoreSQL is an instance of OfflinePageMetadataStore
+// which is implemented using a SQLite database.
+class OfflinePageMetadataStoreSQL : public OfflinePageMetadataStore {
+ public:
+  OfflinePageMetadataStoreSQL(
+      scoped_refptr<base::SequencedTaskRunner> background_task_runner,
+      const base::FilePath& database_dir);
+  ~OfflinePageMetadataStoreSQL() override;
+
+  // Implementation methods.
+  void Load(const LoadCallback& callback) override;
+  void AddOrUpdateOfflinePage(const OfflinePageItem& offline_page,
+                              const UpdateCallback& callback) override;
+  void RemoveOfflinePages(const std::vector<int64_t>& offline_ids,
+                          const UpdateCallback& callback) override;
+  void Reset(const ResetCallback& callback) override;
+
+  // If true, use an in-memory database to make testing easier.
+  // Only has an effect if you call it before a call to Load(...).
+  // Should only be used for testing.
+  void SetInMemoryDatabaseForTesting(bool in_memory);
+
+ private:
+  // Synchronous implementations, these are run on the background thread
+  // and actually do the work to access SQL.  The implementations above
+  // simply dispatch to the corresponding *Sync method on the background thread.
+  // 'runner' is where to run the callback.
+  void LoadSync(scoped_refptr<base::SingleThreadTaskRunner> runner,
+                const LoadCallback& callback);
+  void AddOrUpdateOfflinePageSync(
+      const OfflinePageItem& offline_page,
+      scoped_refptr<base::SingleThreadTaskRunner> runner,
+      const UpdateCallback& callback);
+  void RemoveOfflinePagesSync(
+      const std::vector<int64_t>& offline_ids,
+      scoped_refptr<base::SingleThreadTaskRunner> runner,
+      const UpdateCallback& callback);
+  void ResetSync(scoped_refptr<base::SingleThreadTaskRunner> runner,
+                 const ResetCallback& callback);
+
+  void NotifyLoadResult(scoped_refptr<base::SingleThreadTaskRunner> runner,
+                        const LoadCallback& callback,
+                        LoadStatus status,
+                        const std::vector<OfflinePageItem>& result);
+
+  // Background thread where all SQL access should be run.
+  scoped_refptr<base::SequencedTaskRunner> background_task_runner_;
+  // Path to the database on disk.
+  base::FilePath db_file_path_;
+
+  std::unique_ptr<sql::Connection> db_;
+
+  // only useful for testing.
+  bool use_in_memory_;
+
+  base::WeakPtrFactory<OfflinePageMetadataStoreSQL> weak_ptr_factory_;

[Scott Hess - ex-Googler, 2016/04/25 19:57:55]

I am not entirely able to follow whether this adheres to the thread-related
comment in weak_ptr.h .  I see that weak_ptr.cc has thread-checker tests.  Do
the tests you added actually run with multiple threads?  I think they may not,
it's hard to tell by inspection.

[Basically, I'm going to delegate this to everyone else, but if you read the
above and think "Wait, what thread-related comment in weak_ptr.h?", then you
should go read that comment.]

[bburns, 2016/04/26 23:27:02]

I believe that this is covered by the fact that in the destructor, we actually
push the destroy onto the background_task_runner_ thread, which means that the
delete
happens after any other access is done.

[Scott Hess - ex-Googler, 2016/04/27 15:21:00]

My understanding is that you can't run is_valid() on a different thread than
you're running GetWeakPtr() on.  You _can_ pass the result of GetWeakPtr() to a
different thread, and have that thread pass it back to the current thread, and
then run is_valid().

The reason is because there is no locking, so the Invalidate() call is
inherently racey WRT dereferencing.  On one thread you could Invalidate()
between the point where get() calls is_valid() and when it returns the pointer. 
The thread calling Invalidate() could immediately assume that it can safely
delete the pointed-to object.

I am not worrying about the db_ instance, here, AFAICT that's fine.  Your weak
pointers are to an instance of OfflienPageMetadataStoreSQL.  AFAICT there is no
code preventing ~OfflinePageMetadataStoreSQL() from running while LoadSync() (or
others) are running in a different thread.

[bburns, 2016/04/27 19:59:21]

So I think that this is covered because all of the callbacks are handled via
base::Bind,
and base::Bind knows how to handle weak pointers.

(additionally, in this case, this class is created once on startup and never
destroyed,
but of course it should still work properly anyway)

[Scott Hess - ex-Googler, 2016/04/27 21:16:45]

base::Bind knows how to use a weak pointer as a target, but it isn't doing
anything special to allow you to safely access the same underlying instance in
multiple threads.

Here is what I believe to be a valid use of weak pointer across threads:

  callback = base::Bind(&MyClass::MyFunction, weak_ptr_factory_.GetWeakPtr());
  background_task_runner_->PostTask(FROM_HERE,
base::Bind(&OtherClass::OtherFunction, other_object,
base::ThreadTaskRunnerHandle::Get(), callback));

void OtherClass::OtherFunction(scoped_refptr<base::SingleThreadTaskRunner>
runner, base::Closure callback) {
  // ...
  runner->PostTask(FROM_HERE, callback);
}

In this case, the weak pointer is passed along to the background thread as
opaque data, which is then passed back to the foreground thread unchanged, and
the weak pointer is not dereferenced until the foreground task runner calls the
callback.

In your case, you are calling a method against the weak pointer in a background
thread.  The weak pointer implementation provides no locking to allow it to
safely check the validity of the weak pointer when on a different thread than
the factory the weak pointer was created on (WeakPtr::Get is racy against
WeakPtrFactory::Invalidate()).  base::Bind() relies on WeakPtr::get(), so the
race remains.  The task runner relies on the callback, so in the end it will
also rely on WeakPtr::get(), so that race remains.  In addition, there is
nothing that prevents the underlying object from being destructed while the
background task runner is executing a method on that underlying object.

I've double-checked this with a few people who sit nearby, and I'm not finding
the hole in my reading of the implementation details.  I'll loop in rsleevi@ in
case he'd like to apply additional advice.

+
+  DISALLOW_COPY_AND_ASSIGN(OfflinePageMetadataStoreSQL);
+};
+
+}  // namespace offline_pages
+
+#endif  // COMPONENTS_OFFLINE_PAGES_OFFLINE_PAGE_METADATA_STORE_SQL_H_