SessionStorageDatabase fix.

BUG=104292
TEST=SessionStorageDatabaseTest.DeleteNamespaceConfusion


Review URL: https://chromiumcodereview.appspot.com/10451036

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@139540 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 59 insertions, 50 deletions

diff --git a/webkit/dom_storage/session_storage_database.cc b/webkit/dom_storage/session_storage_database.cc
index 66e502f..f4bc6c5 100644
--- a/webkit/dom_storage/session_storage_database.cc
+++ b/webkit/dom_storage/session_storage_database.cc
@@ -19,17 +19,17 @@
 // Layout of the database:
 // | key                            | value                              |
 // -----------------------------------------------------------------------
-// | map-1                          | 2 (refcount, start of map-1-* keys)|
+// | map-1-                         | 2 (refcount, start of map-1-* keys)|
 // | map-1-a                        | b (a = b in map 1)                 |
 // | ...                            |                                    |
 // | namespace-                     | dummy (start of namespace-* keys)  |
-// | namespace-1 (1 = namespace id) | dummy (start of namespace-1-* keys)|
+// | namespace-1- (1 = namespace id)| dummy (start of namespace-1-* keys)|
 // | namespace-1-origin1            | 1 (mapid)                          |
 // | namespace-1-origin2            | 2                                  |
-// | namespace-2                    | dummy                              |
+// | namespace-2-                   | dummy                              |
 // | namespace-2-origin1            | 1 (shallow copy)                   |
 // | namespace-2-origin2            | 2 (shallow copy)                   |
-// | namespace-3                    | dummy                              |
+// | namespace-3-                   | dummy                              |
 // | namespace-3-origin1            | 3 (deep copy)                      |
 // | namespace-3-origin2            | 2 (shallow copy)                   |
 // | next-namespace-id              | 4                                  |
@@ -115,17 +115,17 @@ bool SessionStorageDatabase::CloneNamespace(int64 namespace_id,
   // for them in |new_namespace_id|, and associate them with the existing maps.
 
   // Example, data before shallow copy:
-  // | map-1                          | 1 (refcount)        |
+  // | map-1-                         | 1 (refcount)        |
   // | map-1-a                        | b                   |
-  // | namespace-1 (1 = namespace id) | dummy               |
+  // | namespace-1- (1 = namespace id)| dummy               |
   // | namespace-1-origin1            | 1 (mapid)           |
 
   // Example, data after shallow copy:
-  // | map-1                          | 2 (inc. refcount)   |
+  // | map-1-                         | 2 (inc. refcount)   |
   // | map-1-a                        | b                   |
-  // | namespace-1 (1 = namespace id) | dummy               |
+  // | namespace-1-(1 = namespace id) | dummy               |
   // | namespace-1-origin1            | 1 (mapid)           |
-  // | namespace-2                    | dummy               |
+  // | namespace-2-                   | dummy               |
   // | namespace-2-origin1            | 1 (mapid) << references the same map
 
   if (!LazyOpen(true))
@@ -344,17 +344,14 @@ bool SessionStorageDatabase::GetAreasInNamespace(
   if (!DatabaseErrorCheck(it->status().ok()))
     return false;
 
-  // Skip the dummy entry "namespace-<namespaceid>" and iterate the origins.
+  // Skip the dummy entry "namespace-<namespaceid>-" and iterate the origins.
   for (it->Next(); it->Valid(); it->Next()) {
     std::string key = it->key().ToString();
     if (key.find(namespace_start_key) != 0) {
       // Iterated past the origins for this namespace.
       break;
     }
-    size_t second_dash = key.find('-', namespace_start_key.length());
-    if (!ConsistencyCheck(second_dash != std::string::npos))
-      return false;
-    std::string origin = key.substr(second_dash + 1);
+    std::string origin = key.substr(namespace_start_key.length());
     std::string map_id = it->value().ToString();
     (*areas)[origin] = map_id;
   }
@@ -450,18 +447,15 @@ bool SessionStorageDatabase::ReadMap(const std::string& map_id,
     return false;
   if (!DatabaseErrorCheck(it->status().ok()))
     return false;
-  const int kPrefixLength = std::string(MapPrefix()).length();
-  // Skip the dummy entry "map-<mapid>".
+  // Skip the dummy entry "map-<mapid>-".
   for (it->Next(); it->Valid(); it->Next()) {
-    // Key is of the form "map-<mapid>-<key>".
     std::string key = it->key().ToString();
-    size_t second_dash = key.find('-', kPrefixLength);
-    if (second_dash == std::string::npos ||
-        key.substr(kPrefixLength, second_dash - kPrefixLength) != map_id) {
-      // Iterated beyond the keys in this map.
+    if (key.find(map_start_key) != 0) {
+      // Iterated past the keys in this map.
       break;
     }
-    string16 key16 = UTF8ToUTF16(key.substr(second_dash + 1));
+    // Key is of the form "map-<mapid>-<key>".
+    string16 key16 = UTF8ToUTF16(key.substr(map_start_key.length()));
     if (only_keys) {
       (*result)[key16] = NullableString16(true);
     } else {
@@ -551,21 +545,21 @@ bool SessionStorageDatabase::DeepCopyArea(
     int64 namespace_id, const GURL& origin, bool copy_data,
     std::string* map_id, leveldb::WriteBatch* batch) {
   // Example, data before deep copy:
-  // | namespace-1 (1 = namespace id) | dummy               |
+  // | namespace-1- (1 = namespace id)| dummy               |
   // | namespace-1-origin1            | 1 (mapid)           |
-  // | namespace-2                    | dummy               |
+  // | namespace-2-                   | dummy               |
   // | namespace-2-origin1            | 1 (mapid) << references the same map
-  // | map-1                          | 2 (refcount)        |
+  // | map-1-                         | 2 (refcount)        |
   // | map-1-a                        | b                   |
 
   // Example, data after deep copy copy:
-  // | namespace-1 (1 = namespace id) | dummy               |
+  // | namespace-1-(1 = namespace id) | dummy               |
   // | namespace-1-origin1            | 1 (mapid)           |
-  // | namespace-2                    | dummy               |
+  // | namespace-2-                   | dummy               |
   // | namespace-2-origin1            | 2 (mapid) << references the new map
-  // | map-1                          | 1 (dec. refcount)   |
+  // | map-1-                         | 1 (dec. refcount)   |
   // | map-1-a                        | b                   |
-  // | map-2                          | 1 (refcount)        |
+  // | map-2-                         | 1 (refcount)        |
   // | map-2-a                        | b                   |
 
   // Read the values from the old map here. If we don't need to copy the data,
@@ -586,7 +580,7 @@ bool SessionStorageDatabase::DeepCopyArea(
 
 std::string SessionStorageDatabase::NamespaceStartKey(
     const std::string& namespace_id_str) {
-  return base::StringPrintf("namespace-%s", namespace_id_str.c_str());
+  return base::StringPrintf("namespace-%s-", namespace_id_str.c_str());
 }
 
 std::string SessionStorageDatabase::NamespaceStartKey(int64 namespace_id,
@@ -616,7 +610,7 @@ const char* SessionStorageDatabase::NamespacePrefix() {
 }
 
 std::string SessionStorageDatabase::MapRefCountKey(const std::string& map_id) {
-  return base::StringPrintf("map-%s", map_id.c_str());
+  return base::StringPrintf("map-%s-", map_id.c_str());
 }
 
 std::string SessionStorageDatabase::MapKey(const std::string& map_id,
@@ -624,10 +618,6 @@ std::string SessionStorageDatabase::MapKey(const std::string& map_id,
   return base::StringPrintf("map-%s-%s", map_id.c_str(), key.c_str());
 }
 
-const char* SessionStorageDatabase::MapPrefix() {
-  return "map-";
-}
-
 const char* SessionStorageDatabase::NextNamespaceIdKey() {
   return "next-namespace-id";
 }
diff --git a/webkit/dom_storage/session_storage_database_unittest.cc b/webkit/dom_storage/session_storage_database_unittest.cc
index a9d159c..57352df 100644
--- a/webkit/dom_storage/session_storage_database_unittest.cc
+++ b/webkit/dom_storage/session_storage_database_unittest.cc
@@ -107,11 +107,13 @@ bool SessionStorageDatabaseTest::IsNamespaceKey(const std::string& key,
     return false;
 
   size_t second_dash = key.find('-', namespace_prefix.length());
-  if (second_dash != std::string::npos)
+  if (second_dash != key.length() - 1)
     return false;
 
-  // Key is of the form "namespace-<namespaceid>".
-  std::string namespace_id_str = key.substr(namespace_prefix.length());
+  // Key is of the form "namespace-<namespaceid>-".
+  std::string namespace_id_str = key.substr(
+      namespace_prefix.length(),
+      second_dash - namespace_prefix.length());
   bool conversion_ok = base::StringToInt64(namespace_id_str, namespace_id);
   EXPECT_TRUE(conversion_ok);
   return true;
@@ -124,13 +126,14 @@ bool SessionStorageDatabaseTest::IsNamespaceOriginKey(const std::string& key,
   if (key.find(namespace_prefix) != 0)
     return false;
   size_t second_dash = key.find('-', namespace_prefix.length());
-  if (second_dash == std::string::npos)
+  if (second_dash == std::string::npos || second_dash == key.length() - 1)
     return false;
+
   // Key is of the form "namespace-<namespaceid>-<origin>", and the value
   // is the map id.
-  std::string namespace_id_str =
-      key.substr(namespace_prefix.length(),
-                 second_dash - namespace_prefix.length());
+  std::string namespace_id_str = key.substr(
+      namespace_prefix.length(),
+      second_dash - namespace_prefix.length());
   bool conversion_ok = base::StringToInt64(namespace_id_str, namespace_id);
   EXPECT_TRUE(conversion_ok);
   return true;
@@ -139,14 +142,15 @@ bool SessionStorageDatabaseTest::IsNamespaceOriginKey(const std::string& key,
 // static
 bool SessionStorageDatabaseTest::IsMapRefCountKey(const std::string& key,
                                                   int64* map_id) {
-  std::string map_prefix = SessionStorageDatabase::MapPrefix();
+  std::string map_prefix = "map-";
   if (key.find(map_prefix) != 0)
     return false;
   size_t second_dash = key.find('-', map_prefix.length());
-  if (second_dash != std::string::npos)
+  if (second_dash != key.length() - 1)
     return false;
-  // Key is of the form "map-<mapid>" and the value is the ref count.
-  std::string map_id_str = key.substr(map_prefix.length(), second_dash);
+  // Key is of the form "map-<mapid>-" and the value is the ref count.
+  std::string map_id_str = key.substr(map_prefix.length(),
+                                      second_dash - map_prefix.length());
   bool conversion_ok = base::StringToInt64(map_id_str, map_id);
   EXPECT_TRUE(conversion_ok);
   return true;
@@ -155,15 +159,15 @@ bool SessionStorageDatabaseTest::IsMapRefCountKey(const std::string& key,
 // static
 bool SessionStorageDatabaseTest::IsMapValueKey(const std::string& key,
                                                int64* map_id) {
-  std::string map_prefix = SessionStorageDatabase::MapPrefix();
+  std::string map_prefix = "map-";
   if (key.find(map_prefix) != 0)
     return false;
   size_t second_dash = key.find('-', map_prefix.length());
-  if (second_dash == std::string::npos)
+  if (second_dash == std::string::npos || second_dash == key.length() - 1)
     return false;
   // Key is of the form "map-<mapid>-key".
-  std::string map_id_str =
-      key.substr(map_prefix.length(), second_dash - map_prefix.length());
+  std::string map_id_str = key.substr(map_prefix.length(),
+                                      second_dash - map_prefix.length());
   bool conversion_ok = base::StringToInt64(map_id_str, map_id);
   EXPECT_TRUE(conversion_ok);
   return true;
@@ -851,4 +855,19 @@ TEST_F(SessionStorageDatabaseTest, NamespaceOffsetDeleteNamespace) {
   CheckAreaData(-1, kOrigin1, data1);
 }
 
+TEST_F(SessionStorageDatabaseTest, DeleteNamespaceConfusion) {
+  // Regression test for a bug where a namespace with id 10 prevented deleting
+  // the namespace with id 1.
+
+  // Create namespace with IDs 0 to 10. The real IDs in the DB will correspond
+  // to these IDs.
+  ValuesMap data1;
+  data1[kKey1] = kValue1;
+  for (int i = 0; i <= 10; ++i)
+    ASSERT_TRUE(db_->CommitAreaChanges(i, kOrigin1, false, data1));
+
+  // Delete the namespace with ID 1.
+  EXPECT_TRUE(db_->DeleteNamespace(1));
+}
+
 }  // namespace dom_storage