Relax strict quota limit checks when reading pre-existing DomStorage database files. The other checks will disallow increases that exceed the limit, but when it comes to existing files its not clear that the checks on open will pass. So we'll grandfather in the existing files.

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

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

3 files changed, 29 insertions, 23 deletions

diff --git a/webkit/dom_storage/dom_storage_map.cc b/webkit/dom_storage/dom_storage_map.cc
index aa34752..6c086b6 100644
--- a/webkit/dom_storage/dom_storage_map.cc
+++ b/webkit/dom_storage/dom_storage_map.cc
@@ -72,14 +72,17 @@ bool DomStorageMap::SetItem(
 
   size_t old_item_size = old_value->is_null() ?
       0 : size_of_item(key, old_value->string());
-  size_t new_size = bytes_used_ - old_item_size + size_of_item(key, value);
-  if (new_size > quota_)
+  size_t new_item_size = size_of_item(key, value);
+  size_t new_bytes_used = bytes_used_ - old_item_size + new_item_size;
+
+  // Only check quota if the size is increasing, this allows
+  // shrinking changes to pre-existing files that are over budget.
+  if (new_item_size > old_item_size && new_bytes_used > quota_)
     return false;
 
   values_[key] = NullableString16(value, false);
   ResetKeyIterator();
-  bytes_used_ -= old_item_size;
-  bytes_used_ += size_of_item(key, value);
+  bytes_used_ = new_bytes_used;
   return true;
 }
 
@@ -96,18 +99,14 @@ bool DomStorageMap::RemoveItem(
   return true;
 }
 
-bool DomStorageMap::SwapValues(ValuesMap* values) {
-  size_t new_size = CountBytes(*values);
-  if (new_size > quota_)
-    return false;
+void DomStorageMap::SwapValues(ValuesMap* values) {
+  // Note: A pre-existing file may be over the quota budget.
   values_.swap(*values);
-  bytes_used_ = new_size;
+  bytes_used_ = CountBytes(values_);
   ResetKeyIterator();
-  return true;
 }
 
 DomStorageMap* DomStorageMap::DeepCopy() const {
-  DCHECK(CountBytes(values_) <= quota_);
   DomStorageMap* copy = new DomStorageMap(quota_);
   copy->values_ = values_;
   copy->bytes_used_ = bytes_used_;
diff --git a/webkit/dom_storage/dom_storage_map.h b/webkit/dom_storage/dom_storage_map.h
index d8151e6..ed1ae89 100644
--- a/webkit/dom_storage/dom_storage_map.h
+++ b/webkit/dom_storage/dom_storage_map.h
@@ -30,10 +30,13 @@ class DomStorageMap
                NullableString16* old_value);
   bool RemoveItem(const string16& key, string16* old_value);
 
-  // Replaces values_ with |map|. Returns true on success, false
-  // if the swap was prevented because |map| would exceed this
-  // DomStorageMap's quota_.
-  bool SwapValues(ValuesMap* map);
+  // Swaps this instances values_ with |map|.
+  // Note: to grandfather in pre-existing files that are overbudget,
+  // this method does not do quota checking.
+  void SwapValues(ValuesMap* map);
+
+  // Creates a new instance of DomStorageMap containing
+  // a deep copy of values_.
   DomStorageMap* DeepCopy() const;
 
   size_t bytes_used() const { return bytes_used_; }
diff --git a/webkit/dom_storage/dom_storage_map_unittest.cc b/webkit/dom_storage/dom_storage_map_unittest.cc
index 67c5bb5..89edbc3 100644
--- a/webkit/dom_storage/dom_storage_map_unittest.cc
+++ b/webkit/dom_storage/dom_storage_map_unittest.cc
@@ -37,7 +37,7 @@ TEST(DomStorageMapTest, DomStorageMapBasics) {
   copy = map->DeepCopy();
   EXPECT_EQ(0u, copy->Length());
   EXPECT_EQ(0u, copy->bytes_used());
-  EXPECT_TRUE(map->SwapValues(&swap));
+  map->SwapValues(&swap);
   EXPECT_TRUE(swap.empty());
 
   // Check the behavior of a map containing some values.
@@ -76,7 +76,7 @@ TEST(DomStorageMapTest, DomStorageMapBasics) {
   EXPECT_TRUE(copy->Key(2).is_null());
   EXPECT_EQ(kItemBytes + kItem2Bytes, copy->bytes_used());
 
-  EXPECT_TRUE(map->SwapValues(&swap));
+  map->SwapValues(&swap);
   EXPECT_EQ(2ul, swap.size());
   EXPECT_EQ(0u, map->Length());
   EXPECT_EQ(0u, map->bytes_used());
@@ -105,16 +105,20 @@ TEST(DomStorageMapTest, EnforcesQuota) {
   EXPECT_TRUE(map->SetItem(kKey2, kValue, &old_nullable_value));
   EXPECT_EQ(1u, map->Length());
 
+  // Verify that the SwapValues method does not do quota checking.
   ValuesMap swap;
-  EXPECT_TRUE(map->SwapValues(&swap));
-  EXPECT_EQ(0u, map->Length());
-
   swap[kKey] = NullableString16(kValue, false);
   swap[kKey2] = NullableString16(kValue, false);
+  map->SwapValues(&swap);
+  EXPECT_GT(map->bytes_used(), kQuota);
 
-  // swap is now too big to fit in the map, the swap should fail.
-  EXPECT_FALSE(map->SwapValues(&swap));
-  EXPECT_EQ(0u, map->Length());
+  // When overbudget, a new value of greater size than the existing value can
+  // not be set, but a new value of lesser or equal size can be set.
+  EXPECT_TRUE(map->SetItem(kKey, kValue, &old_nullable_value));
+  EXPECT_FALSE(map->SetItem(kKey, string16(kValue + kValue),
+                            &old_nullable_value));
+  EXPECT_TRUE(map->SetItem(kKey, string16(), &old_nullable_value));
+  EXPECT_EQ(kValue, old_nullable_value.string());
 }
 
 }  // namespace dom_storage