Persist sessionStorage on disk.

webkit/dom_storage/session_storage_database.cc

+// Copyright (c) 2012 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/dom_storage/session_storage_database.h"
+
+#include "base/file_util.h"
+#include "base/stringprintf.h"
+#include "base/string_number_conversions.h"
+#include "base/utf_string_conversions.h"
+#include "third_party/leveldatabase/src/include/leveldb/db.h"
+#include "third_party/leveldatabase/src/include/leveldb/iterator.h"
+#include "third_party/leveldatabase/src/include/leveldb/status.h"
+#include "third_party/leveldatabase/src/include/leveldb/options.h"
+#include "third_party/leveldatabase/src/include/leveldb/write_batch.h"
+
+// Layout of the database:
+// | key                       | value               |
+// ---------------------------------------------------
+// | area-1 (1 = namespace id) | dummy               | start of area-1-* keys
+// | area-1-origin1            | 1 (mapid)           |
+// | area-1-origin2            | 2                   |
+// | area-2                    | dummy               |
+// | area-2-origin1            | 1                   | shallow copy
+// | area-3                    |                     |
+// | area-3-origin1            | 3                   | deep copy
+// | map-1                     | 2 (refcount)        | start of map-1-* keys
+// | map-1-a                   | b                   | a = b in map 1
+// | ...                       |                     |
+// | next-map-id               | 4                   |

[michaeln, 2012/04/12 01:48:46]

Do we need to ensure that session namespace_ids in this database are unique
across browser runs? As coded now, they start at 1 and go up from there on every
restart (see DomStorageContext), and those values are passed into this class.

[marja, 2012/04/19 10:20:50]

Done. In the latest patch, I made it so that SessionStorageNamespace keeps an
offset which is stored in the database, and the offset is added to the
namespace_ids which get passed via the API. This way the namespaces created on
different sessions don't overlap. And, when namespaces from the last session are
referred to (not in this CL), the namespace ids of them will be negative
(avoiding kLocalStorageNamespaceId which is 0). I'd just need to carefully fetch
the real namespace ids from the data base when associating the namespace ids
with tabs in session restore... Does this sound ok?

[michaeln, 2012/04/22 21:43:34]

Sounds like a plausible approach.

+
+namespace {
+
+// Helper functions for creating the keys needed for the schema.
+std::string AreaStartKey(const std::string& namespace_id) {

[michaeln, 2012/04/12 01:48:46]

Maybe NamespaceStartKey() for clarity?

[marja, 2012/04/19 10:20:50]

Done.

+  return base::StringPrintf("area-%s", namespace_id.c_str());
+}
+
+std::string AreaStartKey(int64 namespace_id) {

[michaeln, 2012/04/12 01:48:46]

ditto

[marja, 2012/04/19 10:20:50]

Done.

+  return AreaStartKey(base::Int64ToString(namespace_id));
+}
+
+std::string AreaKey(const std::string& namespace_id,
+                    const std::string& origin) {
+  return base::StringPrintf("area-%s-%s", namespace_id.c_str(), origin.c_str());
+}
+
+std::string AreaKey(int64 namespace_id, const GURL& origin) {
+  return AreaKey(base::Int64ToString(namespace_id), origin.spec());
+}
+
+std::string MapStartKey(const std::string& map_id) {

[michaeln, 2012/04/12 01:48:46]

Maybe MapRefcountKey for clarity?

[marja, 2012/04/19 10:20:50]

Done.

+  return base::StringPrintf("map-%s", map_id.c_str());
+}
+
+std::string MapKey(const std::string& map_id, const std::string& key) {
+  return base::StringPrintf("map-%s-%s", map_id.c_str(), key.c_str());
+}
+
+std::string NextMapIdKey() {
+  return "next-map-id";
+}
+
+}  // namespace
+
+namespace dom_storage {
+
+SessionStorageDatabase::SessionStorageDatabase(const FilePath& file_path)
+    : DomStorageDatabase(file_path) { }
+
+SessionStorageDatabase::~SessionStorageDatabase() { }
+
+void SessionStorageDatabase::ReadAllValues(int64 namespace_id,
+                                           const GURL& origin,
+                                           ValuesMap* result) {
+  // We don't create a database if it doesn't exist. In that case, there is
+  // nothing to be added to the result.
+  if (!LazyOpen(false))
+    return;
+  // Check if there is map for namespace_id-origin.
+  std::string area_key = AreaKey(namespace_id, origin);
+  std::string map_id;
+  leveldb::Status s = db_->Get(leveldb::ReadOptions(), area_key, &map_id);

[michaeln, 2012/04/12 01:48:46]

How should we handle genuine leveldb errors in this class, in the methods that
write data out in particular? You have DCHECKs that assert no genuine errors can
happen right now... but they can.

[marja, 2012/04/19 10:20:50]

Yes, I'll add proper error handling when I'm more confident that there are no
bugs. Atm, it seems that sometimes on of these DCHECKs fails, when doing a deep
copy, and I'm not sure why.

In the proper error handling, what should we do if we detect that the database
is in a corrupted state? We could just delete and recreate it, but to be able to
do deep and shallow copies, the things-to-copy would need to exist and I'm not
sure how it would work. Or clear the database and go to a "no more writes during
this session" mode? (We could also do that if there is a genuine db error.)

+  if (!s.ok() || map_id.empty())
+    return;
+  ReadValuesInMap(map_id, result);
+}
+
+bool SessionStorageDatabase::CommitChanges(int64 namespace_id,
+                                           const GURL& origin,
+                                           bool clear_all_first,
+                                           const ValuesMap& changes) {
+  if (!LazyOpen(!changes.empty())) {
+    // If we're being asked to commit changes that will result in an
+    // empty database, we return true if the database file doesn't exist.
+    return clear_all_first && changes.empty() &&
+           !file_util::PathExists(file_path_);
+  }
+  leveldb::WriteBatch batch;
+  // Ensure that the key area-N (see the schema above) exists.
+  batch.Put(AreaStartKey(namespace_id), "");
+
+  std::string area_key = AreaKey(namespace_id, origin);
+  std::string map_id;
+  leveldb::Status s;
+  s = db_->Get(leveldb::ReadOptions(), area_key, &map_id);
+  DCHECK(s.ok() || s.IsNotFound());
+  if (s.IsNotFound()) {

[michaeln, 2012/04/12 01:48:46]

style nit: dont need {}'s for one-liners

[marja, 2012/04/19 10:20:50]

Done.

+    CreateNewMap(area_key, &batch, &map_id);
+  } else if (clear_all_first) {
+    DeleteValuesInMap(map_id, &batch);
+  }
+
+  WriteValuesToMap(map_id, changes, &batch);
+
+  s = db_->Write(leveldb::WriteOptions(), &batch);
+  DCHECK(s.ok());
+  return true;
+}
+
+bool SessionStorageDatabase::ShallowCopy(int64 namespace_id,
+                                         const GURL& origin,
+                                         int64 new_namespace_id) {
+  // When doing a shallow copy, an existing map is associated with |origin| in
+  // |namespace_id| and its ref count is increased.

[michaeln, 2012/04/12 01:48:46]

When we a Clone() a session namespace, we're going to do create a shallow copy
for each origin in the src namespace. Should we arrange to have all of those
mutations be performed in a single WriteBatch?

Maybe the method we really want on this class is CloneNamespace(srcid, dstid)?

[marja, 2012/04/19 10:20:50]

Done. For this, I needed an uglyish hack for committing the data in the database
before the shallow copy is made. But it's unavoidable, I guess.

+
+  // Example, data before shallow copy:
+  // | area-1 (1 = namespace id) | dummy               |
+  // | area-1-origin1            | 1 (mapid)           |
+  // | map-1                     | 1 (refcount)        |
+  // | map-1-a                   | b                   | a = b in map 1
+
+  // Example, data after shallow copy:
+  // | area-1 (1 = namespace id) | dummy               |
+  // | area-1-origin1            | 1 (mapid)           |
+  // | area-2                    | dummy               |
+  // | area-2-origin1            | 1 (mapid)           | references the same map
+  // | map-1                     | 2 (inc. refcount)   |
+  // | map-1-a                   | b                   | a = b in map 1
+
+  if (!LazyOpen(true)) {
+    return false;
+  }
+  leveldb::Status s;
+  std::string old_area_key = AreaKey(namespace_id, origin);
+  std::string new_area_key = AreaKey(new_namespace_id, origin);
+  leveldb::WriteBatch batch;
+  batch.Put(AreaStartKey(new_namespace_id), "");
+  std::string map_id;
+  s = db_->Get(leveldb::ReadOptions(), old_area_key, &map_id);
+  DCHECK(s.ok());
+  batch.Put(new_area_key, map_id);
+  // Increase the ref count for the map.
+  std::string map_key = MapStartKey(map_id);
+  std::string old_ref_count_string;
+  s = db_->Get(leveldb::ReadOptions(), map_key, &old_ref_count_string);
+  DCHECK(s.ok());
+  int64 old_ref_count;
+  bool conversion_ok =
+      base::StringToInt64(old_ref_count_string, &old_ref_count);
+  DCHECK(conversion_ok);
+  batch.Put(map_key, base::Int64ToString(++old_ref_count));
+  s = db_->Write(leveldb::WriteOptions(), &batch);
+  DCHECK(s.ok());
+  return true;
+}
+
+bool SessionStorageDatabase::DeepCopy(int64 namespace_id,
+                                      const GURL& origin) {
+  // When doing a shallow copy, an existing map is associated with |origin| in
+  // |namespace_id| and its ref count is increased.
+
+  // Example, data before shallow copy:
+  // | area-1 (1 = namespace id) | dummy               |
+  // | area-1-origin1            | 1 (mapid)           |
+  // | area-2                    | dummy               |
+  // | area-2-origin1            | 1 (mapid)           | references the same map
+  // | map-1                     | 2 (refcount)        |
+  // | map-1-a                   | b                   | a = b in map 1
+
+  // Example, data before shallow copy:
+  // | area-1 (1 = namespace id) | dummy               |
+  // | area-1-origin1            | 1 (mapid)           |
+  // | area-2                    | dummy               |
+  // | area-2-origin1            | 2 (mapid)           | references the new map
+  // | map-1                     | 1 (dec. refcount)   |
+  // | map-1-a                   | b                   | a = b in map 1
+  // | map-2                     | 1 (refcount)        |
+  // | map-2-a                   | b                   | a = b in map 2
+
+  if (!LazyOpen(true)) {
+    return false;
+  }
+
+  leveldb::Status s;
+  std::string area_key = AreaKey(namespace_id, origin);
+  leveldb::WriteBatch batch;
+  std::string old_map_id;
+  s = db_->Get(leveldb::ReadOptions(), area_key, &old_map_id);
+  DCHECK(s.ok());
+  std::string new_map_id;
+  CreateNewMap(area_key, &batch, &new_map_id);
+
+  DecreaseRefCount(old_map_id, &batch);
+
+  // Copy the values in the map.
+  ValuesMap values;
+  ReadValuesInMap(old_map_id, &values);
+  WriteValuesToMap(new_map_id, values, &batch);
+
+  s = db_->Write(leveldb::WriteOptions(), &batch);
+  DCHECK(s.ok());
+  return true;
+}
+
+void SessionStorageDatabase::DeleteOrigin(int64 namespace_id,
+                                          const GURL& origin) {
+  if (!LazyOpen(false)) {
+    // No need to create the database if it doesn't exist.
+    return;
+  }
+  leveldb::WriteBatch batch;
+  DeleteOrigin(base::Int64ToString(namespace_id), origin.spec(), &batch);
+  leveldb::Status s = db_->Write(leveldb::WriteOptions(), &batch);
+  DCHECK(s.ok());
+}
+
+void SessionStorageDatabase::DeleteNamespace(int64 namespace_id) {
+  if (!LazyOpen(false)) {
+    // No need to create the database if it doesn't exist.
+    return;
+  }
+  leveldb::WriteBatch batch;
+  DeleteNamespace(base::Int64ToString(namespace_id), &batch);
+  leveldb::Status s = db_->Write(leveldb::WriteOptions(), &batch);
+  DCHECK(s.ok());
+}
+
+void SessionStorageDatabase::DeleteLeftoverData() {

[michaeln, 2012/04/12 01:48:46]

As coded, this method appears to delete everything. I see this method being
called in the ctor of the context class. So everything that was saved will
promptly be deleted upon constructing the new context on restart. Can you
describe what you have in mind here?

[marja, 2012/04/19 10:20:50]

I was planning to do the "protect some origins from deletion to be able to do a
session restore" in a separate CL. So, the first CL would be the
SessionStorageDatabase and tests, the second CL would integrate it to
DomStorageArea, but never keep any origins, and the third CL would associate the
origins in the database with the session restore system, and protect them from
deletion. Does that sound ok?

+  if (!LazyOpen(false)) {
+    // No need to create the database if it doesn't exist.
+    return;
+  }
+  leveldb::WriteBatch batch;
+  scoped_ptr<leveldb::Iterator> it(db_->NewIterator(leveldb::ReadOptions()));
+  for (it->SeekToFirst(); it->Valid(); it->Next()) {
+    // If is of the form "area-<namespaceid>", delete the corresponding
+    // namespace.
+    std::string key = it->key().ToString();
+    if (key.find("area-") != 0) {
+      // Itereated past the namespaces.
+      break;
+    }
+    size_t second_dash = key.find('-', 5);
+    if (second_dash == std::string::npos) {
+      std::string namespace_id = key.substr(5);
+      DeleteNamespace(namespace_id, &batch);
+    }
+  }
+  leveldb::Status s = db_->Write(leveldb::WriteOptions(), &batch);
+  DCHECK(s.ok());
+}
+
+bool SessionStorageDatabase::LazyOpen(bool create_if_needed) {
+  if (failed_to_open_) {
+    // Don't try to open a database that we know has failed
+    // already.
+    return false;
+  }
+
+  if (IsOpen())
+    return true;
+
+  bool directory_exists = file_util::PathExists(file_path_);
+
+  if (!directory_exists && !create_if_needed) {
+    // If the directory doesn't exist already and we haven't been asked to
+    // create a file on disk, then we don't bother opening the database. This
+    // means we wait until we absolutely need to put something onto disk before
+    // we do so.
+    return false;
+  }
+
+  leveldb::Options options;
+  // The directory exists but a valid leveldb database might not exist inside it
+  // (e.g., a subset of the needed fiels might be missing). Handle this
+  // situation gracefully by creating the database now.

[michaeln, 2012/04/12 01:48:46]

tzik recently added logic to handle this sort of situation with
filesystemdirectory leveldb's in the webkit/fileapi world, please take a look at
how this has been handled there and consider if the handling here should be
modified in anyway.

[marja, 2012/04/19 10:20:50]

Do you mean the recovery options in FileSystemDirectoryDatabase::Init? They look
independent of this "we cannot from the outside detect if the db exists or not"
problem.

I'm not sure if this "Avoid creating the database if it doesn't exist" logic is
worth it in the session storage. If we only read from the db, we would avoid
creating and opening it, but for the (deep & shallow) copying to work, we'd need
to write empty placeholders more often... And how do we detect that "the
database exists", is it enough that there is at least one file in the
directory...

But, I added "if we fail to open the database, clear the directory and try
again" code here, it seems like a reasonable thing to do.

+  options.create_if_missing = true;
+  leveldb::Status s;
+  leveldb::DB* db;
+#if defined(OS_WIN)
+  s = leveldb::DB::Open(options, WideToUTF8(file_path_.value()), &db);
+#elif defined(OS_POSIX)
+  s = leveldb::DB::Open(options, file_path_.value(), &db);
+#endif
+  if (!s.ok()) {
+    LOG(WARNING) << "Failed to open leveldb in " << file_path_.value()
+                 << ", error: " << s.ToString();
+    DCHECK(db == NULL);
+    failed_to_open_ = true;
+    return false;
+  }
+
+  db_.reset(db);
+  return true;
+}
+
+bool SessionStorageDatabase::IsOpen() const {
+  return db_.get();
+}
+
+void SessionStorageDatabase::ReadValuesInMap(const std::string& map_id,
+                                             ValuesMap* result) {
+  scoped_ptr<leveldb::Iterator> it(db_->NewIterator(leveldb::ReadOptions()));
+  std::string map_start_key = MapStartKey(map_id);
+  // Skip the dummy entry, then start iterating the keys in the map.
+  for (it->Seek(map_start_key), it->Next(); it->Valid(); it->Next()) {

[michaeln, 2012/04/12 01:48:46]

is it valid to call Next() on an invalid iterator, idk one way or the other,
just asking since it looks like Seek() could return an invalid 'it' initially

[marja, 2012/04/19 10:20:50]

Done. (It's not ok to call Next() on an invalid iterator; restructured this.)

+    // Key is of the form "map-<mapid>-<key>".
+    std::string key = it->key().ToString();
+    size_t second_dash = key.find('-', 4);
+    if (second_dash == std::string::npos ||
+        key.substr(4, second_dash - 4) != map_id) {
+      // Iterated beyond the keys in this map.
+      break;
+    }
+    string16 key16 = UTF8ToUTF16(key.substr(second_dash + 1));
+    // Convert the raw data stored in std::string (it->value()) to raw data
+    // stored in string16.
+    // FIXME(marja): Add tests.
+    string16 value;
+    size_t len = it->value().size() / sizeof(char16);
+    value.resize(len);
+    value.assign(reinterpret_cast<const char16*>(it->value().data()), len);
+    (*result)[key16] = NullableString16(value, false);
+  }
+}
+
+bool SessionStorageDatabase::CreateNewMap(const std::string& area_key,
+                                          leveldb::WriteBatch* batch,
+                                          std::string* map_id) {
+  // Create a new map.
+  std::string next_map_id_key = NextMapIdKey();
+  leveldb::Status s = db_->Get(leveldb::ReadOptions(), next_map_id_key, map_id);
+  DCHECK(s.ok() || s.IsNotFound());
+  int64 next_map_id = 0;
+  if (s.IsNotFound()) {
+    *map_id = "0";
+  } else {
+    bool conversion_ok = base::StringToInt64(*map_id, &next_map_id);
+    DCHECK(conversion_ok);
+    // FIXME(marja): What to do if the database is corrupt?
+  }
+  batch->Put(next_map_id_key, base::Int64ToString(++next_map_id));
+  batch->Put(area_key, *map_id);
+  batch->Put(MapStartKey(*map_id), base::Int64ToString(1));
+  return true;
+}
+
+void SessionStorageDatabase::WriteValuesToMap(const std::string& map_id,
+                                              const ValuesMap& values,
+                                              leveldb::WriteBatch* batch) {
+  for(ValuesMap::const_iterator it = values.begin(); it != values.end(); ++it) {
+    NullableString16 value = it->second;
+    std::string key = MapKey(map_id, UTF16ToUTF8(it->first));
+    if (value.is_null()) {
+      batch->Delete(key);
+    } else {
+      // Convert the raw data stored in string16 to raw data stored in
+      // std::string.
+      // FIXME(marja): Add tests.
+      const char* data = reinterpret_cast<const char*>(value.string().data());
+      size_t size = value.string().size() * 2;
+      batch->Put(key, std::string(data, size));
+    }
+  }
+}
+
+void SessionStorageDatabase::DecreaseRefCount(const std::string& map_id,
+                                              leveldb::WriteBatch* batch) {
+  // Decrease the ref count for the map.
+  std::string map_key = MapStartKey(map_id);
+  std::string ref_count_string;
+  leveldb::Status s =
+      db_->Get(leveldb::ReadOptions(), map_key, &ref_count_string);
+  DCHECK(s.ok());
+  int64 ref_count;
+  bool conversion_ok = base::StringToInt64(ref_count_string, &ref_count);
+  DCHECK(conversion_ok);
+  if (--ref_count > 0) {
+    batch->Put(map_key, base::Int64ToString(ref_count));
+  } else {
+    // Clear all keys in the map.
+    DeleteValuesInMap(map_id, batch);

[michaeln, 2012/04/12 01:48:46]

This method name is a little decieving since it also may actually delete all the
values in the map. A better name might help?

[marja, 2012/04/19 10:20:50]

Done. DeleteValuesInMap -> ClearMap.

+    batch->Delete(map_key);
+  }
+}
+
+void SessionStorageDatabase::DeleteValuesInMap(const std::string& map_id,
+                                               leveldb::WriteBatch* batch) {
+  ValuesMap values;
+  ReadValuesInMap(map_id, &values);

[michaeln, 2012/04/12 01:48:46]

Might be nice to read the keys only since that's all that's needed to delete.

But would be really nice to have a batch->RangeDelete("map-1", "map-1~")
function in leveldb :)

[marja, 2012/04/19 10:20:50]

Done. (Only reading the keys, not the RangeDelete.:)

+  for (ValuesMap::const_iterator it = values.begin(); it != values.end(); ++it)
+    batch->Delete(MapKey(map_id, UTF16ToUTF8(it->first)));
+}
+
+void SessionStorageDatabase::DeleteOrigin(const std::string& namespace_id,
+                                          const std::string& origin,
+                                          leveldb::WriteBatch* batch) {
+  std::string area_key = AreaKey(namespace_id, origin);
+  std::string map_id;
+  leveldb::Status s = db_->Get(leveldb::ReadOptions(), area_key, &map_id);
+  DCHECK(s.ok() || s.IsNotFound());
+  if (s.IsNotFound())
+    return;  // Nothing to delete.
+  DecreaseRefCount(map_id, batch);
+  batch->Delete(area_key);
+}
+
+void SessionStorageDatabase::DeleteNamespace(const std::string& namespace_id,
+                                             leveldb::WriteBatch* batch) {
+  std::string area_start_key = AreaStartKey(namespace_id);
+  // Skip the dummy entry, then start iterating the origins in the area.

[michaeln, 2012/04/12 01:48:46]

The "origins in the area" is a confusing comment since areas are a per-origin
thing. This is iterating the areas in a namespace.

Maybe the method should be NamespaceStartKey() instead?

[marja, 2012/04/19 10:20:50]

Done. (Fixed the comment, AreaStartKey is now NamespaceStartKey.)

+  scoped_ptr<leveldb::Iterator> it(db_->NewIterator(leveldb::ReadOptions()));
+  for (it->Seek(area_start_key), it->Next(); it->Valid(); it->Next()) {
+    // Key is of the form "area-<namespaceid>-<origin>".
+    std::string key = it->key().ToString();
+    size_t second_dash = key.find('-', 5);
+    if (second_dash == std::string::npos ||
+        key.substr(5, second_dash - 5) != namespace_id) {
+      // Iterated beyond the keys in this map.
+      break;
+    }
+    std::string origin = key.substr(second_dash + 1);
+    DeleteOrigin(namespace_id, origin, batch);
+  }
+  batch->Delete(area_start_key);
+}
+
+// FIXME(marja): Remove this (or dump more intelligently).
+void SessionStorageDatabase::DumpData() const {
+  scoped_ptr<leveldb::Iterator> it(db_->NewIterator(leveldb::ReadOptions()));
+  LOG(WARNING) << "Dumping session storage";
+  for (it->SeekToFirst(); it->Valid(); it->Next())
+    LOG(WARNING) << it->key().ToString() << ": " << it->value().ToString();
+  LOG(WARNING) << "Dumping session storage complete";
+}
+
+}  // namespace dom_storage