Diff
Modified: trunk/Source/WebCore/ChangeLog (276652 => 276653)
--- trunk/Source/WebCore/ChangeLog 2021-04-27 18:52:48 UTC (rev 276652)
+++ trunk/Source/WebCore/ChangeLog 2021-04-27 19:02:53 UTC (rev 276653)
@@ -1,3 +1,22 @@
+2021-04-27 Chris Dumez <[email protected]>
+
+ Don't keep local storage data in memory in the NetworkProcess
+ https://bugs.webkit.org/show_bug.cgi?id=225065
+
+ Reviewed by Alex Christensen.
+
+ * platform/sql/SQLiteDatabase.h:
+ * platform/sql/SQLiteFileSystem.h:
+ Export a couple more symbols.
+
+ * storage/StorageMap.cpp:
+ (WebCore::StorageMap::clear):
+ * storage/StorageMap.h:
+ Add a clear() function to StorageMap so that StorageArea doesn't reconstruct a
+ new StorageMap object unnecessarily when:
+ - The StorageMap is not shared
+ - The StorageMap is empty (and clear() is a no-op)
+
2021-04-27 Youenn Fablet <[email protected]>
SWContextManager::postMessageToServiceWorker should check for valid service worker
Modified: trunk/Source/WebCore/platform/sql/SQLiteDatabase.h (276652 => 276653)
--- trunk/Source/WebCore/platform/sql/SQLiteDatabase.h 2021-04-27 18:52:48 UTC (rev 276652)
+++ trunk/Source/WebCore/platform/sql/SQLiteDatabase.h 2021-04-27 19:02:53 UTC (rev 276653)
@@ -58,7 +58,7 @@
bool isOpen() const { return m_db; }
WEBCORE_EXPORT void close();
- void updateLastChangesCount();
+ WEBCORE_EXPORT void updateLastChangesCount();
WEBCORE_EXPORT bool executeCommand(const String&);
bool returnsAtLeastOneResult(const String&);
@@ -74,7 +74,7 @@
WEBCORE_EXPORT void interrupt();
int64_t lastInsertRowID();
- int lastChanges();
+ WEBCORE_EXPORT int lastChanges();
void setBusyTimeout(int ms);
void setBusyHandler(int(*)(void*, int));
Modified: trunk/Source/WebCore/platform/sql/SQLiteFileSystem.h (276652 => 276653)
--- trunk/Source/WebCore/platform/sql/SQLiteFileSystem.h 2021-04-27 18:52:48 UTC (rev 276652)
+++ trunk/Source/WebCore/platform/sql/SQLiteFileSystem.h 2021-04-27 19:02:53 UTC (rev 276653)
@@ -89,7 +89,7 @@
static bool truncateDatabaseFile(sqlite3* database);
#endif
- static long long getDatabaseFileSize(const String& fileName);
+ WEBCORE_EXPORT static long long getDatabaseFileSize(const String& fileName);
WEBCORE_EXPORT static Optional<WallTime> databaseCreationTime(const String& fileName);
WEBCORE_EXPORT static Optional<WallTime> databaseModificationTime(const String& fileName);
Modified: trunk/Source/WebCore/storage/StorageMap.cpp (276652 => 276653)
--- trunk/Source/WebCore/storage/StorageMap.cpp 2021-04-27 18:52:48 UTC (rev 276652)
+++ trunk/Source/WebCore/storage/StorageMap.cpp 2021-04-27 19:02:53 UTC (rev 276653)
@@ -175,6 +175,18 @@
return nullptr;
}
+RefPtr<StorageMap> StorageMap::clear()
+{
+ // Implement copy-on-write semantics here. We're guaranteed that the only refs of StorageMaps belong to Storage objects
+ // so if more than one Storage object refs this map, copy it before mutating it.
+ if (refCount() > 1 && !m_map.isEmpty())
+ return StorageMap::create(m_quotaSize);
+
+ m_map.clear();
+ m_currentLength = 0;
+ return nullptr;
+}
+
bool StorageMap::contains(const String& key) const
{
return m_map.contains(key);
Modified: trunk/Source/WebCore/storage/StorageMap.h (276652 => 276653)
--- trunk/Source/WebCore/storage/StorageMap.h 2021-04-27 18:52:48 UTC (rev 276652)
+++ trunk/Source/WebCore/storage/StorageMap.h 2021-04-27 19:02:53 UTC (rev 276653)
@@ -43,6 +43,7 @@
WEBCORE_EXPORT RefPtr<StorageMap> setItem(const String& key, const String& value, String& oldValue, bool& quotaException);
WEBCORE_EXPORT RefPtr<StorageMap> setItemIgnoringQuota(const String& key, const String& value);
WEBCORE_EXPORT RefPtr<StorageMap> removeItem(const String&, String& oldValue);
+ WEBCORE_EXPORT RefPtr<StorageMap> clear();
WEBCORE_EXPORT bool contains(const String& key) const;
Modified: trunk/Source/WebKit/ChangeLog (276652 => 276653)
--- trunk/Source/WebKit/ChangeLog 2021-04-27 18:52:48 UTC (rev 276652)
+++ trunk/Source/WebKit/ChangeLog 2021-04-27 19:02:53 UTC (rev 276653)
@@ -1,5 +1,78 @@
2021-04-27 Chris Dumez <[email protected]>
+ Don't keep local storage data in memory in the NetworkProcess
+ https://bugs.webkit.org/show_bug.cgi?id=225065
+
+ Reviewed by Alex Christensen.
+
+ When a WebPage would start to use the local storage API, the WebProcess
+ would send a sync IPC to the NetworkProcess to retrieve all the local
+ storage entries for the origin. The NetworkProcess would read those
+ entries from a SQLite database and send them back to the WebProcess.
+ Both the NetworkProcess would keep the entries in memory, in a
+ StorageMap object (which is basically a HashMap). On some sites, the
+ strings in the local storage may be very large. It is useful for the
+ WebProcess to keep them in memory for performance reasons, especially
+ considering that the Web API is synchronous. However, there is no real
+ need to keep them in memory in the Network Process side, especially
+ given that the WebProcess already has its own copy. On the network
+ process side, we can get rid of the StorageMap entirely in the local
+ storage case and 100% rely on the SQLite database. This is what this
+ patch implements.
+
+ A/B testing shows this is a 2-3% progression on Membuster. There does
+ not appear to be a meaningful progression on PLUM sadly. This is also
+ neutral on PLT.
+
+ * NetworkProcess/WebStorage/LocalStorageDatabase.cpp:
+ (WebKit::estimateEntrySize):
+ (WebKit::LocalStorageDatabase::create):
+ (WebKit::LocalStorageDatabase::LocalStorageDatabase):
+ (WebKit::LocalStorageDatabase::openDatabase):
+ (WebKit::LocalStorageDatabase::tryToOpenDatabase):
+ (WebKit::LocalStorageDatabase::items const):
+ (WebKit::LocalStorageDatabase::removeItem):
+ (WebKit::LocalStorageDatabase::item const):
+ (WebKit::LocalStorageDatabase::setItem):
+ (WebKit::LocalStorageDatabase::clear):
+ (WebKit::LocalStorageDatabase::close):
+ (WebKit::LocalStorageDatabase::databaseIsEmpty const):
+ (WebKit::LocalStorageDatabase::openIfExisting):
+ (WebKit::LocalStorageDatabase::scopedStatement const):
+ * NetworkProcess/WebStorage/LocalStorageDatabase.h:
+ - Update LocalStorageDatabase API to match more closely the API of
+ StorageMap. This way StorageArea can rely on the LocalStorageDatabase
+ entirely instead of the StorageMap with minimal changes.
+ - Add quota support similarly to what is done in StorageArea so that
+ LocalStorageDatabase::setItem() fails when the quota is reached.
+ - Cache the SQLiteStatements for performance reasons
+
+ * NetworkProcess/WebStorage/LocalStorageNamespace.cpp:
+ (WebKit::LocalStorageNamespace::flushAndClose):
+ We no longer need to call syncToDatabase() before closing since
+ LocalStorageDatabase no longer has a queue on pending operations.
+
+ * NetworkProcess/WebStorage/StorageArea.cpp:
+ (WebKit::StorageArea::StorageArea):
+ (WebKit::StorageArea::addListener):
+ (WebKit::StorageArea::removeListener):
+ (WebKit::StorageArea::setItem):
+ (WebKit::StorageArea::removeItem):
+ (WebKit::StorageArea::clear):
+ (WebKit::StorageArea::items const):
+ (WebKit::StorageArea::ensureDatabase const):
+ * NetworkProcess/WebStorage/StorageArea.h:
+ If the StorageArea is used for local storage, we now don't even initialize
+ a StorageMap and rely entirely on LocalStorageDatabase. If the StorageArea
+ is used for session storage, we keep using a StorageMap since there is no
+ backing database. This is fairly straightforward because I updated the
+ LocalStorageDatabase API to match fairly closely the StorageMap one.
+
+ * NetworkProcess/WebStorage/StorageManagerSet.cpp:
+ (WebKit::StorageManagerSet::waitUntilSyncingLocalStorageFinished):
+
+2021-04-27 Chris Dumez <[email protected]>
+
[IPC Hardening] Make sure IPC::Decoder constructors consistently call markInvalid() when decoding fails
https://bugs.webkit.org/show_bug.cgi?id=225110
<rdar://76547775>
Modified: trunk/Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.cpp (276652 => 276653)
--- trunk/Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.cpp 2021-04-27 18:52:48 UTC (rev 276652)
+++ trunk/Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.cpp 2021-04-27 19:02:53 UTC (rev 276653)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2008, 2009, 2010, 2013, 2019 Apple Inc. All rights reserved.
+ * Copyright (C) 2008-2021 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -27,12 +27,15 @@
#include "LocalStorageDatabase.h"
#include "LocalStorageDatabaseTracker.h"
+#include <WebCore/SQLiteFileSystem.h>
#include <WebCore/SQLiteStatement.h>
+#include <WebCore/SQLiteStatementAutoResetScope.h>
#include <WebCore/SQLiteTransaction.h>
-#include <WebCore/SecurityOrigin.h>
+#include <WebCore/SecurityOriginData.h>
#include <WebCore/StorageMap.h>
#include <WebCore/SuddenTermination.h>
#include <wtf/FileSystem.h>
+#include <wtf/HashMap.h>
#include <wtf/RefPtr.h>
#include <wtf/RunLoop.h>
#include <wtf/WorkQueue.h>
@@ -39,23 +42,30 @@
#include <wtf/text/StringHash.h>
#include <wtf/text/WTFString.h>
-static const auto databaseUpdateInterval = 1_s;
-
-static const int maximumItemsToUpdate = 100;
-
namespace WebKit {
using namespace WebCore;
-Ref<LocalStorageDatabase> LocalStorageDatabase::create(Ref<WorkQueue>&& queue, Ref<LocalStorageDatabaseTracker>&& tracker, const SecurityOriginData& securityOrigin)
+static const char getItemsQueryString[] = "SELECT key, value FROM ItemTable";
+
+static CheckedUint64 estimateEntrySize(const String& key, const String& value)
{
- return adoptRef(*new LocalStorageDatabase(WTFMove(queue), WTFMove(tracker), securityOrigin));
+ CheckedUint64 entrySize;
+ entrySize += key.length() * sizeof(UChar);
+ entrySize += value.length() * sizeof(UChar);
+ return entrySize;
}
-LocalStorageDatabase::LocalStorageDatabase(Ref<WorkQueue>&& queue, Ref<LocalStorageDatabaseTracker>&& tracker, const SecurityOriginData& securityOrigin)
+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));
+}
+
+LocalStorageDatabase::LocalStorageDatabase(Ref<WorkQueue>&& queue, Ref<LocalStorageDatabaseTracker>&& tracker, const SecurityOriginData& securityOrigin, unsigned quotaInBytes)
: m_queue(WTFMove(queue))
, m_tracker(WTFMove(tracker))
, m_securityOrigin(securityOrigin)
, m_databasePath(m_tracker->databasePath(m_securityOrigin))
+ , m_quotaInBytes(quotaInBytes)
{
ASSERT(!RunLoop::isMain());
}
@@ -66,12 +76,13 @@
ASSERT(m_isClosed);
}
-void LocalStorageDatabase::openDatabase(DatabaseOpeningStrategy openingStrategy)
+void LocalStorageDatabase::openDatabase(ShouldCreateDatabase shouldCreateDatabase)
{
+ ASSERT(!RunLoop::isMain());
ASSERT(!m_database.isOpen());
ASSERT(!m_failedToOpenDatabase);
- if (!tryToOpenDatabase(openingStrategy)) {
+ if (!tryToOpenDatabase(shouldCreateDatabase)) {
m_failedToOpenDatabase = true;
return;
}
@@ -80,10 +91,10 @@
m_tracker->didOpenDatabaseWithOrigin(m_securityOrigin);
}
-bool LocalStorageDatabase::tryToOpenDatabase(DatabaseOpeningStrategy openingStrategy)
+bool LocalStorageDatabase::tryToOpenDatabase(ShouldCreateDatabase shouldCreateDatabase)
{
ASSERT(!RunLoop::isMain());
- if (!FileSystem::fileExists(m_databasePath) && openingStrategy == SkipIfNonExistent)
+ if (!FileSystem::fileExists(m_databasePath) && shouldCreateDatabase == ShouldCreateDatabase::No)
return true;
if (m_databasePath.isEmpty()) {
@@ -117,6 +128,7 @@
bool LocalStorageDatabase::migrateItemTableIfNeeded()
{
+ ASSERT(!RunLoop::isMain());
if (!m_database.tableExists("ItemTable"))
return true;
@@ -153,197 +165,175 @@
return true;
}
-void LocalStorageDatabase::importItems(StorageMap& storageMap)
+HashMap<String, String> LocalStorageDatabase::items() const
{
- if (m_didImportItems)
- return;
-
- // FIXME: If it can't import, then the default WebKit behavior should be that of private browsing,
- // not silently ignoring it. https://bugs.webkit.org/show_bug.cgi?id=25894
-
- // We set this to true even if we don't end up importing any items due to failure because
- // there's really no good way to recover other than not importing anything.
- m_didImportItems = true;
-
- openDatabase(SkipIfNonExistent);
+ ASSERT(!RunLoop::isMain());
if (!m_database.isOpen())
- return;
+ return { };
- SQLiteStatement query(m_database, "SELECT key, value FROM ItemTable"_str);
- if (query.prepare() != SQLITE_OK) {
+ auto query = scopedStatement(m_getItemsStatement, getItemsQueryString);
+ if (!query) {
LOG_ERROR("Unable to select items from ItemTable for local storage");
- return;
+ return { };
}
HashMap<String, String> items;
-
- int result = query.step();
+ int result = query->step();
while (result == SQLITE_ROW) {
- String key = query.getColumnText(0);
- String value = query.getColumnBlobAsString(1);
+ String key = query->getColumnText(0);
+ String value = query->getColumnBlobAsString(1);
if (!key.isNull() && !value.isNull())
items.add(WTFMove(key), WTFMove(value));
- result = query.step();
+ result = query->step();
}
- if (result != SQLITE_DONE) {
+ if (result != SQLITE_DONE)
LOG_ERROR("Error reading items from ItemTable for local storage");
- return;
- }
- storageMap.importItems(WTFMove(items));
+ return items;
}
-void LocalStorageDatabase::setItem(const String& key, const String& value)
+void LocalStorageDatabase::removeItem(const String& key, String& oldValue)
{
- itemDidChange(key, value);
-}
+ ASSERT(!RunLoop::isMain());
+ if (!m_database.isOpen())
+ return;
-void LocalStorageDatabase::removeItem(const String& key)
-{
- itemDidChange(key, String());
-}
+ oldValue = item(key);
+ if (oldValue.isNull())
+ return;
-void LocalStorageDatabase::clear()
-{
- m_changedItems.clear();
- m_shouldClearItems = true;
-
- scheduleDatabaseUpdate();
-}
-
-void LocalStorageDatabase::close()
-{
- if (m_isClosed)
+ auto deleteStatement = scopedStatement(m_deleteItemStatement, "DELETE FROM ItemTable WHERE key=?"_s);
+ if (!deleteStatement) {
+ LOG_ERROR("Failed to prepare delete statement - cannot write to local storage database");
return;
- m_isClosed = true;
-
- if (m_didScheduleDatabaseUpdate) {
- updateDatabaseWithChangedItems(m_changedItems);
- m_changedItems.clear();
}
+ deleteStatement->bindText(1, key);
- bool isEmpty = databaseIsEmpty();
-
- if (m_database.isOpen())
- m_database.close();
-
- if (isEmpty)
- m_tracker->deleteDatabaseWithOrigin(m_securityOrigin);
-}
-
-void LocalStorageDatabase::itemDidChange(const String& key, const String& value)
-{
- m_changedItems.set(key, value);
- scheduleDatabaseUpdate();
-}
-
-void LocalStorageDatabase::scheduleDatabaseUpdate()
-{
- if (m_didScheduleDatabaseUpdate)
+ int result = deleteStatement->step();
+ if (result != SQLITE_DONE) {
+ LOG_ERROR("Failed to delete item in the local storage database - %i", result);
return;
+ }
- if (!m_disableSuddenTerminationWhileWritingToLocalStorage)
- m_disableSuddenTerminationWhileWritingToLocalStorage = makeUnique<SuddenTerminationDisabler>();
-
- m_didScheduleDatabaseUpdate = true;
-
- m_queue->dispatch([protectedThis = makeRef(*this)] {
- protectedThis->updateDatabase();
- });
+ if (m_databaseSize) {
+ CheckedUint64 entrySize = estimateEntrySize(key, oldValue);
+ if (entrySize.hasOverflowed() || entrySize.unsafeGet() >= *m_databaseSize)
+ *m_databaseSize = 0;
+ else
+ *m_databaseSize -= entrySize.unsafeGet();
+ }
}
-void LocalStorageDatabase::updateDatabase()
+String LocalStorageDatabase::item(const String& key) const
{
- if (m_isClosed)
- return;
+ ASSERT(!RunLoop::isMain());
+ if (!m_database.isOpen())
+ return { };
- m_didScheduleDatabaseUpdate = false;
+ auto query = scopedStatement(m_getItemStatement, "SELECT value FROM ItemTable WHERE key=?"_s);
+ if (!query) {
+ LOG_ERROR("Unable to get item from ItemTable for local storage");
+ return { };
+ }
+ query->bindText(1, key);
- HashMap<String, String> changedItems;
- if (m_changedItems.size() <= maximumItemsToUpdate) {
- // There are few enough changed items that we can just always write all of them.
- m_changedItems.swap(changedItems);
- updateDatabaseWithChangedItems(changedItems);
- m_disableSuddenTerminationWhileWritingToLocalStorage = nullptr;
- } else {
- for (int i = 0; i < maximumItemsToUpdate; ++i) {
- auto it = m_changedItems.begin();
- changedItems.add(it->key, it->value);
-
- m_changedItems.remove(it);
- }
-
- ASSERT(changedItems.size() <= maximumItemsToUpdate);
-
- // Reschedule the update for the remaining items.
- scheduleDatabaseUpdate();
- updateDatabaseWithChangedItems(changedItems);
- }
+ int result = query->step();
+ if (result == SQLITE_ROW)
+ return query->getColumnBlobAsString(0);
+ if (result != SQLITE_DONE)
+ LOG_ERROR("Error get item from ItemTable for local storage");
+ return { };
}
-void LocalStorageDatabase::updateDatabaseWithChangedItems(const HashMap<String, String>& changedItems)
+void LocalStorageDatabase::setItem(const String& key, const String& value, String& oldValue, bool& quotaException)
{
+ ASSERT(!RunLoop::isMain());
if (!m_database.isOpen())
- openDatabase(CreateIfNonExistent);
+ openDatabase(ShouldCreateDatabase::Yes);
if (!m_database.isOpen())
return;
- if (m_shouldClearItems) {
- m_shouldClearItems = false;
-
- SQLiteStatement clearStatement(m_database, "DELETE FROM ItemTable");
- if (clearStatement.prepare() != SQLITE_OK) {
- LOG_ERROR("Failed to prepare clear statement - cannot write to local storage database");
+ if (m_quotaInBytes != WebCore::StorageMap::noQuota) {
+ if (!m_databaseSize)
+ m_databaseSize = SQLiteFileSystem::getDatabaseFileSize(m_databasePath);
+ if (*m_databaseSize >= m_quotaInBytes) {
+ quotaException = true;
return;
}
-
- int result = clearStatement.step();
- if (result != SQLITE_DONE) {
- LOG_ERROR("Failed to clear all items in the local storage database - %i", result);
+ CheckedUint64 newDatabaseSize = *m_databaseSize;
+ newDatabaseSize += estimateEntrySize(key, value);
+ if (newDatabaseSize.hasOverflowed() || newDatabaseSize.unsafeGet() > m_quotaInBytes) {
+ quotaException = true;
return;
}
+ m_databaseSize = newDatabaseSize.unsafeGet();
}
- SQLiteStatement insertStatement(m_database, "INSERT INTO ItemTable VALUES (?, ?)");
- if (insertStatement.prepare() != SQLITE_OK) {
+ 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");
return;
}
- SQLiteStatement deleteStatement(m_database, "DELETE FROM ItemTable WHERE key=?");
- if (deleteStatement.prepare() != SQLITE_OK) {
- LOG_ERROR("Failed to prepare delete statement - cannot write to local storage database");
- return;
+ insertStatement->bindText(1, key);
+ insertStatement->bindBlob(2, value);
+
+ int result = insertStatement->step();
+ if (result != SQLITE_DONE)
+ LOG_ERROR("Failed to update item in the local storage database - %i", result);
+}
+
+bool LocalStorageDatabase::clear()
+{
+ ASSERT(!RunLoop::isMain());
+ if (!m_database.isOpen())
+ return false;
+
+ auto clearStatement = scopedStatement(m_clearStatement, "DELETE FROM ItemTable"_s);
+ if (!clearStatement) {
+ LOG_ERROR("Failed to prepare clear statement - cannot write to local storage database");
+ return false;
}
- SQLiteTransaction transaction(m_database);
- transaction.begin();
+ int result = clearStatement->step();
+ if (result != SQLITE_DONE) {
+ LOG_ERROR("Failed to clear all items in the local storage database - %i", result);
+ return false;
+ }
- for (auto it = changedItems.begin(), end = changedItems.end(); it != end; ++it) {
- // A null value means that the key/value pair should be deleted.
- SQLiteStatement& statement = it->value.isNull() ? deleteStatement : insertStatement;
+ m_databaseSize = 0;
- statement.bindText(1, it->key);
+ return m_database.lastChanges() > 0;
+}
- // If we're inserting a key/value pair, bind the value as well.
- if (!it->value.isNull())
- statement.bindBlob(2, it->value);
+void LocalStorageDatabase::close()
+{
+ ASSERT(!RunLoop::isMain());
+ if (m_isClosed)
+ return;
+ m_isClosed = true;
- int result = statement.step();
- if (result != SQLITE_DONE) {
- LOG_ERROR("Failed to update item in the local storage database - %i", result);
- break;
- }
+ bool isEmpty = databaseIsEmpty();
- statement.reset();
- }
+ m_clearStatement = nullptr;
+ m_insertStatement = nullptr;
+ m_getItemStatement = nullptr;
+ m_getItemsStatement = nullptr;
+ m_deleteItemStatement = nullptr;
- transaction.commit();
+ if (m_database.isOpen())
+ m_database.close();
+
+ if (isEmpty)
+ m_tracker->deleteDatabaseWithOrigin(m_securityOrigin);
}
-bool LocalStorageDatabase::databaseIsEmpty()
+bool LocalStorageDatabase::databaseIsEmpty() const
{
+ ASSERT(!RunLoop::isMain());
if (!m_database.isOpen())
return false;
@@ -362,6 +352,31 @@
return !query.getColumnInt(0);
}
+void LocalStorageDatabase::openIfExisting()
+{
+ ASSERT(!RunLoop::isMain());
+ if (m_database.isOpen())
+ return;
+
+ openDatabase(ShouldCreateDatabase::No);
+
+ // Prewarm the getItems statement for performance since the pages block synchronously on retrieving the items.
+ if (m_database.isOpen())
+ scopedStatement(m_getItemsStatement, getItemsQueryString);
+}
+
+SQLiteStatementAutoResetScope LocalStorageDatabase::scopedStatement(std::unique_ptr<SQLiteStatement>& statement, const String& query) const
+{
+ ASSERT(!RunLoop::isMain());
+ if (!statement) {
+ statement = makeUnique<SQLiteStatement>(m_database, query);
+ ASSERT(m_database.isOpen());
+ if (statement->prepare() != SQLITE_OK)
+ return SQLiteStatementAutoResetScope { };
+ }
+ return SQLiteStatementAutoResetScope { statement.get() };
+}
+
void LocalStorageDatabase::handleLowMemoryWarning()
{
if (m_database.isOpen())
Modified: trunk/Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.h (276652 => 276653)
--- trunk/Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.h 2021-04-27 18:52:48 UTC (rev 276652)
+++ trunk/Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.h 2021-04-27 19:02:53 UTC (rev 276653)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2008, 2009, 2010, 2013, 2019 Apple Inc. All rights reserved.
+ * Copyright (C) 2008-2021 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -26,16 +26,14 @@
#pragma once
#include <WebCore/SQLiteDatabase.h>
-#include <WebCore/SecurityOriginData.h>
#include <wtf/Forward.h>
-#include <wtf/HashMap.h>
#include <wtf/RefCounted.h>
-#include <wtf/WorkQueue.h>
namespace WebCore {
-class SecurityOrigin;
-class StorageMap;
+class SQLiteStatementAutoResetScope;
class SuddenTerminationDisabler;
+
+struct SecurityOriginData;
}
namespace WebKit {
@@ -44,18 +42,16 @@
class LocalStorageDatabase : public RefCounted<LocalStorageDatabase> {
public:
- static Ref<LocalStorageDatabase> create(Ref<WorkQueue>&&, Ref<LocalStorageDatabaseTracker>&&, const WebCore::SecurityOriginData&);
+ static Ref<LocalStorageDatabase> create(Ref<WorkQueue>&&, Ref<LocalStorageDatabaseTracker>&&, const WebCore::SecurityOriginData&, unsigned quotaInBytes);
~LocalStorageDatabase();
- // Will block until the import is complete.
- void importItems(WebCore::StorageMap&);
+ HashMap<String, String> items() const;
+ void openIfExisting();
+ void removeItem(const String& key, String& oldValue);
+ bool clear();
+ String item(const String& key) const;
+ void setItem(const String& key, const String& value, String& oldValue, bool& quotaException);
- void setItem(const String& key, const String& value);
- void removeItem(const String& key);
- void clear();
-
- void updateDatabase();
-
// Will block until all pending changes have been written to disk.
void close();
@@ -62,39 +58,34 @@
void handleLowMemoryWarning();
private:
- LocalStorageDatabase(Ref<WorkQueue>&&, Ref<LocalStorageDatabaseTracker>&&, const WebCore::SecurityOriginData&);
+ LocalStorageDatabase(Ref<WorkQueue>&&, Ref<LocalStorageDatabaseTracker>&&, const WebCore::SecurityOriginData&, unsigned quotaInBytes);
- enum DatabaseOpeningStrategy {
- CreateIfNonExistent,
- SkipIfNonExistent
- };
- bool tryToOpenDatabase(DatabaseOpeningStrategy);
- void openDatabase(DatabaseOpeningStrategy);
+ enum class ShouldCreateDatabase : bool { No, Yes };
+ bool tryToOpenDatabase(ShouldCreateDatabase);
+ void openDatabase(ShouldCreateDatabase);
bool migrateItemTableIfNeeded();
+ bool databaseIsEmpty() const;
- void itemDidChange(const String& key, const String& value);
+ WebCore::SQLiteStatementAutoResetScope scopedStatement(std::unique_ptr<WebCore::SQLiteStatement>&, const String& query) const;
- void scheduleDatabaseUpdate();
- void updateDatabaseWithChangedItems(const HashMap<String, String>&);
-
- bool databaseIsEmpty();
-
Ref<WorkQueue> m_queue;
Ref<LocalStorageDatabaseTracker> m_tracker;
WebCore::SecurityOriginData m_securityOrigin;
String m_databasePath;
- WebCore::SQLiteDatabase m_database;
+ mutable WebCore::SQLiteDatabase m_database;
+ unsigned m_quotaInBytes { 0 };
bool m_failedToOpenDatabase { false };
- bool m_didImportItems { false };
bool m_isClosed { false };
+ Optional<uint64_t> m_databaseSize;
- bool m_didScheduleDatabaseUpdate { false };
- bool m_shouldClearItems { false };
- HashMap<String, String> m_changedItems;
-
std::unique_ptr<WebCore::SuddenTerminationDisabler> m_disableSuddenTerminationWhileWritingToLocalStorage;
+ mutable std::unique_ptr<WebCore::SQLiteStatement> m_clearStatement;
+ mutable std::unique_ptr<WebCore::SQLiteStatement> m_insertStatement;
+ mutable std::unique_ptr<WebCore::SQLiteStatement> m_getItemStatement;
+ mutable std::unique_ptr<WebCore::SQLiteStatement> m_getItemsStatement;
+ mutable std::unique_ptr<WebCore::SQLiteStatement> m_deleteItemStatement;
};
Modified: trunk/Source/WebKit/NetworkProcess/WebStorage/LocalStorageNamespace.cpp (276652 => 276653)
--- trunk/Source/WebKit/NetworkProcess/WebStorage/LocalStorageNamespace.cpp 2021-04-27 18:52:48 UTC (rev 276652)
+++ trunk/Source/WebKit/NetworkProcess/WebStorage/LocalStorageNamespace.cpp 2021-04-27 19:02:53 UTC (rev 276653)
@@ -73,10 +73,8 @@
void LocalStorageNamespace::flushAndClose(const SecurityOriginData& origin)
{
ASSERT(!RunLoop::isMain());
- if (auto* storageArea = m_storageAreaMap.get(origin)) {
- storageArea->syncToDatabase();
+ if (auto* storageArea = m_storageAreaMap.get(origin))
storageArea->close();
- }
}
Modified: trunk/Source/WebKit/NetworkProcess/WebStorage/StorageArea.cpp (276652 => 276653)
--- trunk/Source/WebKit/NetworkProcess/WebStorage/StorageArea.cpp 2021-04-27 18:52:48 UTC (rev 276652)
+++ trunk/Source/WebKit/NetworkProcess/WebStorage/StorageArea.cpp 2021-04-27 19:02:53 UTC (rev 276653)
@@ -31,6 +31,7 @@
#include "StorageAreaMapMessages.h"
#include "StorageManager.h"
#include <WebCore/StorageMap.h>
+#include <wtf/Scope.h>
namespace WebKit {
@@ -40,11 +41,12 @@
: m_localStorageNamespace(makeWeakPtr(localStorageNamespace))
, m_securityOrigin(securityOrigin)
, m_quotaInBytes(quotaInBytes)
- , m_storageMap(StorageMap::create(m_quotaInBytes))
, m_identifier(Identifier::generate())
, m_queue(WTFMove(queue))
{
ASSERT(!RunLoop::isMain());
+ if (isEphemeral())
+ m_storageMap = StorageMap::create(m_quotaInBytes);
}
StorageArea::~StorageArea()
@@ -61,7 +63,7 @@
ASSERT(!m_eventListeners.contains(connectionID));
if (m_eventListeners.isEmpty() && !isEphemeral())
- openDatabaseAndImportItemsIfNeeded();
+ ensureDatabase();
m_eventListeners.add(connectionID);
}
@@ -74,11 +76,8 @@
if (!m_eventListeners.isEmpty())
return;
- if (!m_localStorageNamespace)
- return;
-
- syncToDatabase();
- m_localStorageNamespace->removeStorageArea(m_securityOrigin);
+ if (m_localStorageNamespace)
+ m_localStorageNamespace->removeStorageArea(m_securityOrigin);
}
bool StorageArea::hasListener(IPC::Connection::UniqueID connectionID) const
@@ -101,20 +100,18 @@
void StorageArea::setItem(IPC::Connection::UniqueID sourceConnection, StorageAreaImplIdentifier storageAreaImplID, const String& key, const String& value, const String& urlString, bool& quotaException)
{
ASSERT(!RunLoop::isMain());
- openDatabaseAndImportItemsIfNeeded();
String oldValue;
+ if (isEphemeral()) {
+ auto newStorageMap = m_storageMap->setItem(key, value, oldValue, quotaException);
+ if (newStorageMap)
+ m_storageMap = WTFMove(newStorageMap);
+ } else
+ ensureDatabase().setItem(key, value, oldValue, quotaException);
- auto newStorageMap = m_storageMap->setItem(key, value, oldValue, quotaException);
- if (newStorageMap)
- m_storageMap = WTFMove(newStorageMap);
-
if (quotaException)
return;
- if (m_localStorageDatabase)
- m_localStorageDatabase->setItem(key, value);
-
dispatchEvents(sourceConnection, storageAreaImplID, key, oldValue, value, urlString);
}
@@ -121,19 +118,17 @@
void StorageArea::removeItem(IPC::Connection::UniqueID sourceConnection, StorageAreaImplIdentifier storageAreaImplID, const String& key, const String& urlString)
{
ASSERT(!RunLoop::isMain());
- openDatabaseAndImportItemsIfNeeded();
String oldValue;
- auto newStorageMap = m_storageMap->removeItem(key, oldValue);
- if (newStorageMap)
- m_storageMap = WTFMove(newStorageMap);
-
+ if (isEphemeral()) {
+ auto newStorageMap = m_storageMap->removeItem(key, oldValue);
+ if (newStorageMap)
+ m_storageMap = WTFMove(newStorageMap);
+ } else
+ ensureDatabase().removeItem(key, oldValue);
if (oldValue.isNull())
return;
- if (m_localStorageDatabase)
- m_localStorageDatabase->removeItem(key);
-
dispatchEvents(sourceConnection, storageAreaImplID, key, oldValue, String(), urlString);
}
@@ -140,35 +135,40 @@
void StorageArea::clear(IPC::Connection::UniqueID sourceConnection, StorageAreaImplIdentifier storageAreaImplID, const String& urlString)
{
ASSERT(!RunLoop::isMain());
- openDatabaseAndImportItemsIfNeeded();
- if (!m_storageMap->length())
- return;
+ if (isEphemeral()) {
+ if (!m_storageMap->length())
+ return;
+ if (auto newStorageMap = m_storageMap->clear())
+ m_storageMap = WTFMove(newStorageMap);
+ } else {
+ if (!ensureDatabase().clear())
+ return;
+ }
- m_storageMap = StorageMap::create(m_quotaInBytes);
-
- if (m_localStorageDatabase)
- m_localStorageDatabase->clear();
-
dispatchEvents(sourceConnection, storageAreaImplID, String(), String(), String(), urlString);
}
-const HashMap<String, String>& StorageArea::items() const
+HashMap<String, String> StorageArea::items() const
{
ASSERT(!RunLoop::isMain());
- openDatabaseAndImportItemsIfNeeded();
+ if (isEphemeral())
+ return m_storageMap->items();
- return m_storageMap->items();
+ return ensureDatabase().items();
}
void StorageArea::clear()
{
ASSERT(!RunLoop::isMain());
- m_storageMap = StorageMap::create(m_quotaInBytes);
-
- if (m_localStorageDatabase) {
- m_localStorageDatabase->close();
- m_localStorageDatabase = nullptr;
+ if (isEphemeral()) {
+ if (auto newStorageMap = m_storageMap->clear())
+ m_storageMap = WTFMove(newStorageMap);
+ } else {
+ if (m_localStorageDatabase) {
+ m_localStorageDatabase->close();
+ m_localStorageDatabase = nullptr;
+ }
}
for (auto it = m_eventListeners.begin(), end = m_eventListeners.end(); it != end; ++it) {
@@ -179,22 +179,17 @@
}
}
-void StorageArea::openDatabaseAndImportItemsIfNeeded() const
+LocalStorageDatabase& StorageArea::ensureDatabase() const
{
- ASSERT(!RunLoop::isMain());
- if (!m_localStorageNamespace)
- return;
+ ASSERT(!isEphemeral());
ASSERT(m_localStorageNamespace->storageManager()->localStorageDatabaseTracker());
// We open the database here even if we've already imported our items to ensure that the database is open if we need to write to it.
- if (!m_localStorageDatabase)
- m_localStorageDatabase = LocalStorageDatabase::create(m_queue.copyRef(), *m_localStorageNamespace->storageManager()->localStorageDatabaseTracker(), m_securityOrigin);
-
- if (m_didImportItemsFromDatabase)
- return;
-
- m_localStorageDatabase->importItems(*m_storageMap);
- m_didImportItemsFromDatabase = true;
+ if (!m_localStorageDatabase) {
+ m_localStorageDatabase = LocalStorageDatabase::create(m_queue.copyRef(), *m_localStorageNamespace->storageManager()->localStorageDatabaseTracker(), m_securityOrigin, m_quotaInBytes);
+ m_localStorageDatabase->openIfExisting();
+ }
+ return *m_localStorageDatabase;
}
void StorageArea::dispatchEvents(IPC::Connection::UniqueID sourceConnection, StorageAreaImplIdentifier storageAreaImplID, const String& key, const String& oldValue, const String& newValue, const String& urlString) const
@@ -211,14 +206,6 @@
}
}
-void StorageArea::syncToDatabase()
-{
- if (!m_localStorageDatabase)
- return;
-
- m_localStorageDatabase->updateDatabase();
-}
-
void StorageArea::close()
{
if (!m_localStorageDatabase)
Modified: trunk/Source/WebKit/NetworkProcess/WebStorage/StorageArea.h (276652 => 276653)
--- trunk/Source/WebKit/NetworkProcess/WebStorage/StorageArea.h 2021-04-27 18:52:48 UTC (rev 276652)
+++ trunk/Source/WebKit/NetworkProcess/WebStorage/StorageArea.h 2021-04-27 19:02:53 UTC (rev 276653)
@@ -63,14 +63,11 @@
void removeItem(IPC::Connection::UniqueID sourceConnection, StorageAreaImplIdentifier, const String& key, const String& urlString);
void clear(IPC::Connection::UniqueID sourceConnection, StorageAreaImplIdentifier, const String& urlString);
- const HashMap<String, String>& items() const;
+ HashMap<String, String> items() const;
void clear();
bool isEphemeral() const { return !m_localStorageNamespace; }
- void openDatabaseAndImportItemsIfNeeded() const;
-
- void syncToDatabase();
void close();
void handleLowMemoryWarning();
@@ -78,10 +75,11 @@
private:
void dispatchEvents(IPC::Connection::UniqueID sourceConnection, StorageAreaImplIdentifier, const String& key, const String& oldValue, const String& newValue, const String& urlString) const;
+ LocalStorageDatabase& ensureDatabase() const;
+
// Will be null if the storage area belongs to a session storage namespace or the storage area is in an ephemeral session.
WeakPtr<LocalStorageNamespace> m_localStorageNamespace;
mutable RefPtr<LocalStorageDatabase> m_localStorageDatabase;
- mutable bool m_didImportItemsFromDatabase { false };
WebCore::SecurityOriginData m_securityOrigin;
unsigned m_quotaInBytes { 0 };
Modified: trunk/Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.cpp (276652 => 276653)
--- trunk/Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.cpp 2021-04-27 18:52:48 UTC (rev 276652)
+++ trunk/Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.cpp 2021-04-27 19:02:53 UTC (rev 276653)
@@ -155,16 +155,7 @@
{
ASSERT(RunLoop::isMain());
- BinarySemaphore semaphore;
- m_queue->dispatch([this, &semaphore] {
- for (const auto& storageArea : m_storageAreas.values()) {
- ASSERT(storageArea);
- if (storageArea)
- storageArea->syncToDatabase();
- }
- semaphore.signal();
- });
- semaphore.wait();
+ m_queue->dispatchSync([] { });
}
void StorageManagerSet::suspend(CompletionHandler<void()>&& completionHandler)
Modified: trunk/Source/WebKitLegacy/ChangeLog (276652 => 276653)
--- trunk/Source/WebKitLegacy/ChangeLog 2021-04-27 18:52:48 UTC (rev 276652)
+++ trunk/Source/WebKitLegacy/ChangeLog 2021-04-27 19:02:53 UTC (rev 276653)
@@ -1,3 +1,18 @@
+2021-04-27 Chris Dumez <[email protected]>
+
+ Don't keep local storage data in memory in the NetworkProcess
+ https://bugs.webkit.org/show_bug.cgi?id=225065
+
+ Reviewed by Alex Christensen.
+
+ * Storage/StorageAreaImpl.cpp:
+ (WebKit::StorageAreaImpl::clear):
+ (WebKit::StorageAreaImpl::clearForOriginDeletion):
+ Use the new StorageMap::clear() to avoid constructing a new StorageMap unnecessarily
+ when:
+ - The StorageMap is not shared
+ - The StorageMap is empty
+
2021-04-26 Alex Christensen <[email protected]>
Update Mac-specific CMake files
Modified: trunk/Source/WebKitLegacy/Storage/StorageAreaImpl.cpp (276652 => 276653)
--- trunk/Source/WebKitLegacy/Storage/StorageAreaImpl.cpp 2021-04-27 18:52:48 UTC (rev 276652)
+++ trunk/Source/WebKitLegacy/Storage/StorageAreaImpl.cpp 2021-04-27 19:02:53 UTC (rev 276653)
@@ -174,8 +174,8 @@
if (!m_storageMap->length())
return;
- unsigned quota = m_storageMap->quota();
- m_storageMap = StorageMap::create(quota);
+ if (auto newStorageMap = m_storageMap->clear())
+ m_storageMap = WTFMove(newStorageMap);
if (m_storageAreaSync)
m_storageAreaSync->scheduleClear();
@@ -214,10 +214,8 @@
ASSERT(!m_isShutdown);
blockUntilImportComplete();
- if (m_storageMap->length()) {
- unsigned quota = m_storageMap->quota();
- m_storageMap = StorageMap::create(quota);
- }
+ if (auto newStorageMap = m_storageMap->clear())
+ m_storageMap = WTFMove(newStorageMap);
if (m_storageAreaSync) {
m_storageAreaSync->scheduleClear();
