Changeset 276689 in webkit

Timestamp:

Apr 27, 2021 9:43:38 PM (15 months ago)

Author:

Chris Dumez

Message:

Improve local storage size estimation for quota limitation
​https://bugs.webkit.org/show_bug.cgi?id=225123

Reviewed by Alex Christensen.

Source/WebCore:

Improve local storage size estimation for quota limitation:

• Rely on String::sizeInBytes() to compute the String size, instead of using String::length() * sizeof(UChar)
• Make estimation consistent between StorageMap & LocalStorageDatabase

• storage/StorageMap.cpp:

(WebCore::StorageMap::setItem):
(WebCore::StorageMap::setItemIgnoringQuota):
(WebCore::StorageMap::removeItem):
(WebCore::StorageMap::clear):
(WebCore::StorageMap::importItems):
(WebCore::StorageMap::Impl::copy const):

• storage/StorageMap.h:

Source/WebKit:

Improve local storage size estimation for quota limitation:

• Rely on String::sizeInBytes() to compute the String size, instead of using String::length() * sizeof(UChar)
• Make estimation consistent between StorageMap & LocalStorageDatabase

• NetworkProcess/WebStorage/LocalStorageDatabase.cpp:

(WebKit::LocalStorageDatabase::removeItem):
(WebKit::LocalStorageDatabase::setItem):
(WebKit::estimateEntrySize): Deleted.

• NetworkProcess/WebStorage/LocalStorageDatabase.h:

LayoutTests:

Update test to use unicode in the Strings so that the file reaches the quota without
changing the test too much. The test was using ASCII and was thus able to store all
the strings without reaching the quota due to our updated String size calculation.

• storage/domstorage/quota.html:

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/storage/domstorage/quota.html (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/storage/StorageMap.cpp (modified) (8 diffs)
• Source/WebCore/storage/StorageMap.h (modified) (2 diffs)
• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.cpp (modified) (4 diffs)
• Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.h (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2021-04-27  Chris Dumez  <cdumez@apple.com>
+
+        Improve local storage size estimation for quota limitation
+        https://bugs.webkit.org/show_bug.cgi?id=225123
+
+        Reviewed by Alex Christensen.
+
+        Update test to use unicode in the Strings so that the file reaches the quota without
+        changing the test too much. The test was using ASCII and was thus able to store all
+        the strings without reaching the quota due to our updated String size calculation.
+
+        * storage/domstorage/quota.html:
+
 2021-04-27  Wenson Hsieh  <wenson_hsieh@apple.com>
 

trunk/LayoutTests/storage/domstorage/quota.html

@@ -23 +23 @@
 
     debug("Creating 'data' which contains 64K of data");
-    data = "X";
+    data = "\u03BB";
     for (var i=0; i<16; i++)
         data += data;

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-04-27  Chris Dumez  <cdumez@apple.com>
+
+        Improve local storage size estimation for quota limitation
+        https://bugs.webkit.org/show_bug.cgi?id=225123
+
+        Reviewed by Alex Christensen.
+
+        Improve local storage size estimation for quota limitation:
+        - Rely on String::sizeInBytes() to compute the String size, instead of using
+          String::length() * sizeof(UChar)
+        - Make estimation consistent between StorageMap & LocalStorageDatabase
+
+        * storage/StorageMap.cpp:
+        (WebCore::StorageMap::setItem):
+        (WebCore::StorageMap::setItemIgnoringQuota):
+        (WebCore::StorageMap::removeItem):
+        (WebCore::StorageMap::clear):
+        (WebCore::StorageMap::importItems):
+        (WebCore::StorageMap::Impl::copy const):
+        * storage/StorageMap.h:
+
 2021-04-27  Wenson Hsieh  <wenson_hsieh@apple.com>
 

trunk/Source/WebCore/storage/StorageMap.cpp

@@ -27 +27 @@
 #include "StorageMap.h"
 
+#include <wtf/CheckedArithmetic.h>
 #include <wtf/SetForScope.h>
 
@@ -94 +95 @@
         m_impl = m_impl->copy();
 
-    // Quota tracking.  This is done in a couple of steps to keep the overflow tracking simple.
-    unsigned newLength = m_impl->currentLength;
-    bool overflow = newLength + value.length() < newLength;
-    newLength += value.length();
+    oldValue = m_impl->map.get(key);
 
-    oldValue = m_impl->map.get(key);
-    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);
-    if (m_quotaSize != noQuota && (overflow || overQuota)) {
+    // Quota tracking. This is done in a couple of steps to keep the overflow tracking simple.
+    CheckedUint32 newSize = m_impl->currentSize;
+    newSize -= oldValue.sizeInBytes();
+    newSize += value.sizeInBytes();
+    if (oldValue.isNull())
+        newSize += key.sizeInBytes();
+    if (m_quotaSize != noQuota && (newSize.hasOverflowed() || newSize.unsafeGet() > m_quotaSize)) {
         quotaException = true;
         return;
     }
-    m_impl->currentLength = newLength;
+    m_impl->currentSize = newSize.unsafeGet();
 
     HashMap<String, String>::AddResult addResult = m_impl->map.add(key, value);
@@ -124 +118 @@
 void StorageMap::setItemIgnoringQuota(const String& key, const String& value)
 {
-    SetForScope<unsigned> quotaSizeChange(m_quotaSize, static_cast<unsigned>(noQuota));
+    SetForScope<unsigned> quotaSizeChange(m_quotaSize, noQuota);
 
     String oldValue;
@@ -139 +133 @@
 
     oldValue = m_impl->map.take(key);
-    if (!oldValue.isNull()) {
-        invalidateIterator();
-        ASSERT(m_impl->currentLength - key.length() <= m_impl->currentLength);
-        m_impl->currentLength -= key.length();
-    }
-    ASSERT(m_impl->currentLength - oldValue.length() <= m_impl->currentLength);
-    m_impl->currentLength -= oldValue.length();
+    if (oldValue.isNull())
+        return;
+
+    invalidateIterator();
+    ASSERT(m_impl->currentSize - key.sizeInBytes() <= m_impl->currentSize);
+    m_impl->currentSize -= key.sizeInBytes();
+    ASSERT(m_impl->currentSize - oldValue.sizeInBytes() <= m_impl->currentSize);
+    m_impl->currentSize -= oldValue.sizeInBytes();
 }
 
@@ -155 +150 @@
     }
     m_impl->map.clear();
-    m_impl->currentLength = 0;
+    m_impl->currentSize = 0;
 }
 
@@ -169 +164 @@
         m_impl->map = WTFMove(items);
         for (auto& pair : m_impl->map) {
-            ASSERT(m_impl->currentLength + pair.key.length() + pair.value.length() >= m_impl->currentLength);
-            m_impl->currentLength += (pair.key.length() + pair.value.length());
+            ASSERT(m_impl->currentSize + pair.key.sizeInBytes() + pair.value.sizeInBytes() >= m_impl->currentSize);
+            m_impl->currentSize += (pair.key.sizeInBytes() + pair.value.sizeInBytes());
         }
         return;
@@ -179 +174 @@
         auto& value = item.value;
 
-        ASSERT(m_impl->currentLength + key.length() + value.length() >= m_impl->currentLength);
-        m_impl->currentLength += (key.length() + value.length());
-        
+        ASSERT(m_impl->currentSize + key.sizeInBytes() + value.sizeInBytes() >= m_impl->currentSize);
+        m_impl->currentSize += (key.sizeInBytes() + value.sizeInBytes());
+
         auto result = m_impl->map.add(WTFMove(key), WTFMove(value));
         ASSERT_UNUSED(result, result.isNewEntry); // True if the key didn't exist previously.
@@ -191 +186 @@
     auto copy = Impl::create();
     copy->map = map;
-    copy->currentLength = currentLength;
+    copy->currentSize = currentSize;
     return copy;
 }

trunk/Source/WebCore/storage/StorageMap.h

@@ -57 +57 @@
     bool isShared() const { return !m_impl->hasOneRef(); }
 
-    static constexpr unsigned noQuota = UINT_MAX;
+    static constexpr unsigned noQuota = std::numeric_limits<unsigned>::max();
 
 private:
@@ -74 +74 @@
         HashMap<String, String>::iterator iterator { map.end() };
         unsigned iteratorIndex { std::numeric_limits<unsigned>::max() };
-        unsigned currentLength { 0 }; // Measured in UChars.
+        unsigned currentSize { 0 };
     };
 

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2021-04-27  Chris Dumez  <cdumez@apple.com>
+
+        Improve local storage size estimation for quota limitation
+        https://bugs.webkit.org/show_bug.cgi?id=225123
+
+        Reviewed by Alex Christensen.
+
+        Improve local storage size estimation for quota limitation:
+        - Rely on String::sizeInBytes() to compute the String size, instead of using
+          String::length() * sizeof(UChar)
+        - Make estimation consistent between StorageMap & LocalStorageDatabase
+
+        * NetworkProcess/WebStorage/LocalStorageDatabase.cpp:
+        (WebKit::LocalStorageDatabase::removeItem):
+        (WebKit::LocalStorageDatabase::setItem):
+        (WebKit::estimateEntrySize): Deleted.
+        * NetworkProcess/WebStorage/LocalStorageDatabase.h:
+
 2021-04-27  Wenson Hsieh  <wenson_hsieh@apple.com>
 

trunk/Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.cpp

@@ -48 +48 @@
 static const char getItemsQueryString[] = "SELECT key, value FROM ItemTable";
 
-static CheckedUint64 estimateEntrySize(const String& key, const String& value)
-{
-    CheckedUint64 entrySize;
-    entrySize += key.length() * sizeof(UChar);
-    entrySize += value.length() * sizeof(UChar);
-    return entrySize;
-}
-
 Ref<LocalStorageDatabase> LocalStorageDatabase::create(Ref<WorkQueue>&& queue, Ref<LocalStorageDatabaseTracker>&& tracker, const SecurityOriginData& securityOrigin, unsigned quotaInBytes)
 {
@@ -218 +210 @@
 
     if (m_databaseSize) {
-        CheckedUint64 entrySize = estimateEntrySize(key, oldValue);
-        if (entrySize.hasOverflowed() || entrySize.unsafeGet() >= *m_databaseSize)
+        auto sizeDecrease = key.sizeInBytes() + oldValue.sizeInBytes();
+        if (sizeDecrease >= *m_databaseSize)
             *m_databaseSize = 0;
         else
-            *m_databaseSize -= entrySize.unsafeGet();
+            *m_databaseSize -= sizeDecrease;
     }
 }
@@ -255 +247 @@
         return;
 
+    oldValue = item(key);
+
     if (m_quotaInBytes != WebCore::StorageMap::noQuota) {
         if (!m_databaseSize)
             m_databaseSize = SQLiteFileSystem::getDatabaseFileSize(m_databasePath);
-        if (*m_databaseSize >= m_quotaInBytes) {
-            quotaException = true;
-            return;
-        }
-        CheckedUint64 newDatabaseSize = *m_databaseSize;
-        newDatabaseSize += estimateEntrySize(key, value);
+        CheckedUint32 newDatabaseSize = *m_databaseSize;
+        newDatabaseSize -= oldValue.sizeInBytes();
+        newDatabaseSize += value.sizeInBytes();
+        if (oldValue.isNull())
+            newDatabaseSize += key.sizeInBytes();
         if (newDatabaseSize.hasOverflowed() || newDatabaseSize.unsafeGet() > m_quotaInBytes) {
             quotaException = true;
@@ -271 +264 @@
     }
 
-    oldValue = item(key);
-
     auto insertStatement = scopedStatement(m_insertStatement, "INSERT INTO ItemTable VALUES (?, ?)"_s);
     if (!insertStatement) {

trunk/Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.h

@@ -76 +76 @@
     String m_databasePath;
     mutable WebCore::SQLiteDatabase m_database;
-    unsigned m_quotaInBytes { 0 };
+    const unsigned m_quotaInBytes { 0 };
     bool m_failedToOpenDatabase { false };
     bool m_isClosed { false };
-    Optional<uint64_t> m_databaseSize;
+    Optional<unsigned> m_databaseSize;
 
     std::unique_ptr<WebCore::SuddenTerminationDisabler> m_disableSuddenTerminationWhileWritingToLocalStorage;