2009-11-12 Jeremy Orlow <jorlow@chromium.org>
authorjorlow@chromium.org <jorlow@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 14 Nov 2009 00:27:25 +0000 (00:27 +0000)
committerjorlow@chromium.org <jorlow@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 14 Nov 2009 00:27:25 +0000 (00:27 +0000)
        Reviewed by Dmitry Titov.

        LocalStorage quota should include key sizes in its count
        https://bugs.webkit.org/show_bug.cgi?id=31451

        * storage/StorageMap.cpp:
        (WebCore::StorageMap::setItem):
            Count keys in the quota when adding a new item.
        (WebCore::StorageMap::removeItem):
            Remove the key's length from the quota if we're removing the item.
        (WebCore::StorageMap::importItem):
            Assume that we're adding things for the first time.
            Count keys in the quota.
2009-11-12  Jeremy Orlow  <jorlow@chromium.org>

        Reviewed by Dmitry Titov.

        Now that we're tracking key size in the quota, we can't fit as much in.
        https://bugs.webkit.org/show_bug.cgi?id=31451

        * storage/domstorage/quota-expected.txt:
        * storage/domstorage/script-tests/quota.js:
        (testQuota):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@50979 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/storage/domstorage/quota-expected.txt
LayoutTests/storage/domstorage/script-tests/quota.js
WebCore/ChangeLog
WebCore/storage/StorageMap.cpp

index 5f1a28ed033d34bf2380b1971676a7756401782e..a79a96c5b3b46a6141245b8f980a5bb7db852149 100644 (file)
@@ -1,3 +1,14 @@
+2009-11-12  Jeremy Orlow  <jorlow@chromium.org>
+
+        Reviewed by Dmitry Titov.
+
+        Now that we're tracking key size in the quota, we can't fit as much in.
+        https://bugs.webkit.org/show_bug.cgi?id=31451
+
+        * storage/domstorage/quota-expected.txt:
+        * storage/domstorage/script-tests/quota.js:
+        (testQuota):
+
 2009-11-13  Shinichiro Hamaji  <hamaji@chromium.org>
 
         Reviewed by Darin Adler.
index 3f1b5541597245181b38c2bb93f40bbc156f2dea..248ff794f4afddaa05bd0448228ea62567821e0d 100644 (file)
@@ -18,12 +18,12 @@ storage.clear()
 PASS storage.length is 0
 Creating 'data' which contains 64K of data
 PASS data.length is 65536
-Putting 'data' into 40 localStorage buckets.
+Putting 'data' into 39 localStorage buckets.
 Putting 'data' into another bucket.h
 PASS Hit exception as expected
 Verify that data was never inserted.
-PASS storage.getItem(40) is null
-Removing bucket 39.
+PASS storage.getItem(39) is null
+Removing bucket 38.
 Adding 'Hello!' into a new bucket.
 PASS Insertion worked.
 PASS successfullyParsed is true
index b6fbb23bfe3eeaf918bb09017a9f904d17b85fcd..ad9afe92ea9aa2182995422d0476f74458c1d605 100644 (file)
@@ -19,23 +19,23 @@ function testQuota(storageString)
         data += data;
     shouldBe("data.length", "65536");
 
-    debug("Putting 'data' into 40 " + storageString + " buckets.");
-    for (var i=0; i<40; i++)
+    debug("Putting 'data' into 39 " + storageString + " buckets.");
+    for (var i=0; i<39; i++)
         storage[i] = data;
 
     debug("Putting 'data' into another bucket.h");
     try {
-        storage[40] = data;
+        storage[39] = data;
         testFailed("Did not hit quota error.");
     } catch (e) {
         testPassed("Hit exception as expected");
     }
 
     debug("Verify that data was never inserted.");
-    shouldBeNull("storage.getItem(40)");
+    shouldBeNull("storage.getItem(39)");
 
-    debug("Removing bucket 39.");
-    storage.removeItem('39');
+    debug("Removing bucket 38.");
+    storage.removeItem('38');
 
     debug("Adding 'Hello!' into a new bucket.");
     try {
index cccf38d717d4d1903dfb95ed8c352b02ca4a94e3..ab20a866fc14fa23173950a80dcf932442751db6 100644 (file)
@@ -1,3 +1,19 @@
+2009-11-12  Jeremy Orlow  <jorlow@chromium.org>
+
+        Reviewed by Dmitry Titov.
+
+        LocalStorage quota should include key sizes in its count
+        https://bugs.webkit.org/show_bug.cgi?id=31451
+
+        * storage/StorageMap.cpp:
+        (WebCore::StorageMap::setItem):
+            Count keys in the quota when adding a new item.
+        (WebCore::StorageMap::removeItem):
+            Remove the key's length from the quota if we're removing the item.
+        (WebCore::StorageMap::importItem):
+            Assume that we're adding things for the first time.
+            Count keys in the quota.
+
 2009-11-13  Dominik Röttsches  <dominik.roettsches@access-company.com>
 
         Reviewed by Eric Seidel.
index 5498d9ee4509b8e05d408133ae2dab320b6d8d47..bb08671a40cb122b14bd744da97e3b5f319a6081 100644 (file)
@@ -111,12 +111,21 @@ PassRefPtr<StorageMap> StorageMap::setItem(const String& key, const String& valu
         return newStorageMap.release();
     }
 
-    // Quota tracking.  If the quota is enabled and this would go over it, bail.
+    // Quota tracking.  This is done in a couple of steps to keep the overflow tracking simple.
+    unsigned newLength = m_currentLength;
+    bool overflow = newLength + value.length() < newLength;
+    newLength += value.length();
+
     oldValue = m_map.get(key);
-    unsigned newLength = m_currentLength + value.length() - oldValue.length();
+    overflow |= newLength - oldValue.length() > newLength;
+    newLength -= oldValue.length();
+
+    unsigned adjustedKeyLength = oldValue.isNull() ? key.length() : 0;
+    overflow |= newLength + adjustedKeyLength < newLength;
+    newLength += adjustedKeyLength;
+
+    ASSERT(!overflow);  // Overflow is bad...even if quotas are off.
     bool overQuota = newLength > m_quotaSize / sizeof(UChar);
-    bool overflow = (newLength > m_currentLength) != (value.length() > oldValue.length());
-    ASSERT(!overflow);  // If we're debugging, make a fuss.  But it's still worth checking this in the following if statement.
     if (m_quotaSize != noQuota && (overflow || overQuota)) {
         quotaException = true;
         return 0;
@@ -143,10 +152,11 @@ PassRefPtr<StorageMap> StorageMap::removeItem(const String& key, String& oldValu
     }
 
     oldValue = m_map.take(key);
-    if (!oldValue.isNull())
+    if (!oldValue.isNull()) {
         invalidateIterator();
-
-    // Update quota.
+        ASSERT(m_currentLength - key.length() <= m_currentLength);
+        m_currentLength -= key.length();
+    }
     ASSERT(m_currentLength - oldValue.length() <= m_currentLength);
     m_currentLength -= oldValue.length();
 
@@ -162,12 +172,11 @@ void StorageMap::importItem(const String& key, const String& value)
 {
     // Be sure to copy the keys/values as items imported on a background thread are destined
     // to cross a thread boundary
-    pair<HashMap<String, String>::iterator, bool> result = m_map.add(key.threadsafeCopy(), String());
-
-    if (result.second)
-        result.first->second = value.threadsafeCopy();
+    pair<HashMap<String, String>::iterator, bool> result = m_map.add(key.threadsafeCopy(), value.threadsafeCopy());
+    ASSERT(result.second);  // True if the key didn't exist previously.
 
-    // Update quota.
+    ASSERT(m_currentLength + key.length() >= m_currentLength);
+    m_currentLength += key.length();
     ASSERT(m_currentLength + value.length() >= m_currentLength);
     m_currentLength += value.length();
 }