- Revision
- 276689
- Author
- cdu...@apple.com
- Date
- 2021-04-27 21:43:38 -0700 (Tue, 27 Apr 2021)
Log 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:
Modified Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (276688 => 276689)
--- trunk/LayoutTests/ChangeLog 2021-04-28 02:54:40 UTC (rev 276688)
+++ trunk/LayoutTests/ChangeLog 2021-04-28 04:43:38 UTC (rev 276689)
@@ -1,3 +1,16 @@
+2021-04-27 Chris Dumez <cdu...@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_hs...@apple.com>
[iOS] Web content process occasionally crashes under VisibleSelection::adjustPositionForEnd
Modified: trunk/LayoutTests/storage/domstorage/quota.html (276688 => 276689)
--- trunk/LayoutTests/storage/domstorage/quota.html 2021-04-28 02:54:40 UTC (rev 276688)
+++ trunk/LayoutTests/storage/domstorage/quota.html 2021-04-28 04:43:38 UTC (rev 276689)
@@ -22,7 +22,7 @@
shouldBe("storage.length", "0");
debug("Creating 'data' which contains 64K of data");
- data = ""
+ data = ""
for (var i=0; i<16; i++)
data += data;
shouldBe("data.length", "65536");
Modified: trunk/Source/WebCore/ChangeLog (276688 => 276689)
--- trunk/Source/WebCore/ChangeLog 2021-04-28 02:54:40 UTC (rev 276688)
+++ trunk/Source/WebCore/ChangeLog 2021-04-28 04:43:38 UTC (rev 276689)
@@ -1,3 +1,24 @@
+2021-04-27 Chris Dumez <cdu...@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_hs...@apple.com>
[iOS] Web content process occasionally crashes under VisibleSelection::adjustPositionForEnd
Modified: trunk/Source/WebCore/storage/StorageMap.cpp (276688 => 276689)
--- trunk/Source/WebCore/storage/StorageMap.cpp 2021-04-28 02:54:40 UTC (rev 276688)
+++ trunk/Source/WebCore/storage/StorageMap.cpp 2021-04-28 04:43:38 UTC (rev 276689)
@@ -26,6 +26,7 @@
#include "config.h"
#include "StorageMap.h"
+#include <wtf/CheckedArithmetic.h>
#include <wtf/SetForScope.h>
namespace WebCore {
@@ -93,26 +94,19 @@
if (m_impl->refCount() > 1)
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);
- 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);
if (!addResult.isNewEntry)
@@ -123,7 +117,7 @@
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;
bool quotaException;
@@ -138,13 +132,14 @@
m_impl = m_impl->copy();
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();
}
void StorageMap::clear()
@@ -154,7 +149,7 @@
return;
}
m_impl->map.clear();
- m_impl->currentLength = 0;
+ m_impl->currentSize = 0;
}
bool StorageMap::contains(const String& key) const
@@ -168,8 +163,8 @@
// Fast path.
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;
}
@@ -178,9 +173,9 @@
auto& key = item.key;
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.
}
@@ -190,7 +185,7 @@
{
auto copy = Impl::create();
copy->map = map;
- copy->currentLength = currentLength;
+ copy->currentSize = currentSize;
return copy;
}
Modified: trunk/Source/WebCore/storage/StorageMap.h (276688 => 276689)
--- trunk/Source/WebCore/storage/StorageMap.h 2021-04-28 02:54:40 UTC (rev 276688)
+++ trunk/Source/WebCore/storage/StorageMap.h 2021-04-28 04:43:38 UTC (rev 276689)
@@ -56,7 +56,7 @@
bool isShared() const { return !m_impl->hasOneRef(); }
- static constexpr unsigned noQuota = UINT_MAX;
+ static constexpr unsigned noQuota = std::numeric_limits<unsigned>::max();
private:
void invalidateIterator();
@@ -73,7 +73,7 @@
HashMap<String, String> map;
HashMap<String, String>::iterator iterator { map.end() };
unsigned iteratorIndex { std::numeric_limits<unsigned>::max() };
- unsigned currentLength { 0 }; // Measured in UChars.
+ unsigned currentSize { 0 };
};
Ref<Impl> m_impl;
Modified: trunk/Source/WebKit/ChangeLog (276688 => 276689)
--- trunk/Source/WebKit/ChangeLog 2021-04-28 02:54:40 UTC (rev 276688)
+++ trunk/Source/WebKit/ChangeLog 2021-04-28 04:43:38 UTC (rev 276689)
@@ -1,3 +1,21 @@
+2021-04-27 Chris Dumez <cdu...@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_hs...@apple.com>
[iOS] Web content process occasionally crashes under VisibleSelection::adjustPositionForEnd
Modified: trunk/Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.cpp (276688 => 276689)
--- trunk/Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.cpp 2021-04-28 02:54:40 UTC (rev 276688)
+++ trunk/Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.cpp 2021-04-28 04:43:38 UTC (rev 276689)
@@ -47,14 +47,6 @@
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)
{
return adoptRef(*new LocalStorageDatabase(WTFMove(queue), WTFMove(tracker), securityOrigin, quotaInBytes));
@@ -217,11 +209,11 @@
}
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;
}
}
@@ -254,15 +246,16 @@
if (!m_database.isOpen())
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;
return;
@@ -270,8 +263,6 @@
m_databaseSize = newDatabaseSize.unsafeGet();
}
- oldValue = item(key);
-
auto insertStatement = scopedStatement(m_insertStatement, "INSERT INTO ItemTable VALUES (?, ?)"_s);
if (!insertStatement) {
LOG_ERROR("Failed to prepare insert statement - cannot write to local storage database");
Modified: trunk/Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.h (276688 => 276689)
--- trunk/Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.h 2021-04-28 02:54:40 UTC (rev 276688)
+++ trunk/Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.h 2021-04-28 04:43:38 UTC (rev 276689)
@@ -75,10 +75,10 @@
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;
mutable std::unique_ptr<WebCore::SQLiteStatement> m_clearStatement;
