Regression (r235236): NetworkStorageManager sends messages to wrong StorageAreaMap
https://bugs.webkit.org/show_bug.cgi?id=235553
Reviewed by Darin Adler.
Started from r235236, NetworkStorageManager uses StorageAreaMapIdentifier instead of StorageAreaIdentifier as
the message destination (as web process stores StorageAreaMap by StorageAreaMapIdentifier). However, the
destination is not changed accordingly when sending didSetItem, didRemoveItem and didClear messages, so the
messages are dispatched to wrong StorageAreaMaps.
Since these messages are reply for some message sent from StorageAreaMap to NetworkStorageManager (e.g.
didSetItem is reply of setItem), we may just use sendMessageWithAsyncReply when sending original message, and
we don't need to worry about the destination.
* NetworkProcess/WebStorage/StorageManagerSet.cpp:
(WebKit::StorageManagerSet::setItem):
(WebKit::StorageManagerSet::removeItem):
(WebKit::StorageManagerSet::clear):
* NetworkProcess/WebStorage/StorageManagerSet.h:
* NetworkProcess/WebStorage/StorageManagerSet.messages.in:
* NetworkProcess/storage/NetworkStorageManager.cpp:
(WebKit::NetworkStorageManager::setItem):
(WebKit::NetworkStorageManager::removeItem):
(WebKit::NetworkStorageManager::clear):
* NetworkProcess/storage/NetworkStorageManager.h:
* NetworkProcess/storage/NetworkStorageManager.messages.in:
* WebProcess/WebStorage/StorageAreaMap.cpp:
(WebKit::StorageAreaMap::setItem):
(WebKit::StorageAreaMap::removeItem):
(WebKit::StorageAreaMap::clear):
* WebProcess/WebStorage/StorageAreaMap.h:
* WebProcess/WebStorage/StorageAreaMap.messages.in:
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@288551 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
index 8c308bc..6396180 100644
--- a/Source/WebKit/ChangeLog
+++ b/Source/WebKit/ChangeLog
@@ -1,3 +1,38 @@
+2022-01-25 Sihui Liu <sihui_liu@apple.com>
+
+ Regression (r235236): NetworkStorageManager sends messages to wrong StorageAreaMap
+ https://bugs.webkit.org/show_bug.cgi?id=235553
+
+ Reviewed by Darin Adler.
+
+ Started from r235236, NetworkStorageManager uses StorageAreaMapIdentifier instead of StorageAreaIdentifier as
+ the message destination (as web process stores StorageAreaMap by StorageAreaMapIdentifier). However, the
+ destination is not changed accordingly when sending didSetItem, didRemoveItem and didClear messages, so the
+ messages are dispatched to wrong StorageAreaMaps.
+
+ Since these messages are reply for some message sent from StorageAreaMap to NetworkStorageManager (e.g.
+ didSetItem is reply of setItem), we may just use sendMessageWithAsyncReply when sending original message, and
+ we don't need to worry about the destination.
+
+ * NetworkProcess/WebStorage/StorageManagerSet.cpp:
+ (WebKit::StorageManagerSet::setItem):
+ (WebKit::StorageManagerSet::removeItem):
+ (WebKit::StorageManagerSet::clear):
+ * NetworkProcess/WebStorage/StorageManagerSet.h:
+ * NetworkProcess/WebStorage/StorageManagerSet.messages.in:
+ * NetworkProcess/storage/NetworkStorageManager.cpp:
+ (WebKit::NetworkStorageManager::setItem):
+ (WebKit::NetworkStorageManager::removeItem):
+ (WebKit::NetworkStorageManager::clear):
+ * NetworkProcess/storage/NetworkStorageManager.h:
+ * NetworkProcess/storage/NetworkStorageManager.messages.in:
+ * WebProcess/WebStorage/StorageAreaMap.cpp:
+ (WebKit::StorageAreaMap::setItem):
+ (WebKit::StorageAreaMap::removeItem):
+ (WebKit::StorageAreaMap::clear):
+ * WebProcess/WebStorage/StorageAreaMap.h:
+ * WebProcess/WebStorage/StorageAreaMap.messages.in:
+
2022-01-24 Myles C. Maxfield <mmaxfield@apple.com>
REGRESSION(r282320): [Cocoa] User-installed fonts don't work in the GPU Process (in WKWebView)
diff --git a/Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.cpp b/Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.cpp
index 3feedcd..a252804 100644
--- a/Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.cpp
+++ b/Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.cpp
@@ -399,7 +399,7 @@
completionHandler(storageArea ? storageArea->items() : HashMap<String, String>());
}
-void StorageManagerSet::setItem(IPC::Connection& connection, StorageAreaIdentifier storageAreaID, StorageAreaImplIdentifier storageAreaImplID, uint64_t storageMapSeed, const String& key, const String& value, const String& urlString)
+void StorageManagerSet::setItem(IPC::Connection& connection, StorageAreaIdentifier storageAreaID, StorageAreaImplIdentifier storageAreaImplID, const String& key, const String& value, const String& urlString, CompletionHandler<void(bool)>&& completionHandler)
{
ASSERT(!RunLoop::isMain());
@@ -410,10 +410,10 @@
if (storageArea)
storageArea->setItem(connection.uniqueID(), storageAreaImplID, key, value, urlString, quotaError);
- connection.send(Messages::StorageAreaMap::DidSetItem(storageMapSeed, key, quotaError), storageAreaID);
+ completionHandler(quotaError);
}
-void StorageManagerSet::removeItem(IPC::Connection& connection, StorageAreaIdentifier storageAreaID, StorageAreaImplIdentifier storageAreaImplID, uint64_t storageMapSeed, const String& key, const String& urlString)
+void StorageManagerSet::removeItem(IPC::Connection& connection, StorageAreaIdentifier storageAreaID, StorageAreaImplIdentifier storageAreaImplID, const String& key, const String& urlString, CompletionHandler<void()>&& completionHandler)
{
ASSERT(!RunLoop::isMain());
@@ -423,10 +423,10 @@
if (storageArea)
storageArea->removeItem(connection.uniqueID(), storageAreaImplID, key, urlString);
- connection.send(Messages::StorageAreaMap::DidRemoveItem(storageMapSeed, key), storageAreaID);
+ completionHandler();
}
-void StorageManagerSet::clear(IPC::Connection& connection, StorageAreaIdentifier storageAreaID, StorageAreaImplIdentifier storageAreaImplID, uint64_t storageMapSeed, const String& urlString)
+void StorageManagerSet::clear(IPC::Connection& connection, StorageAreaIdentifier storageAreaID, StorageAreaImplIdentifier storageAreaImplID, const String& urlString, CompletionHandler<void()>&& completionHandler)
{
ASSERT(!RunLoop::isMain());
@@ -436,7 +436,7 @@
if (storageArea)
storageArea->clear(connection.uniqueID(), storageAreaImplID, urlString);
- connection.send(Messages::StorageAreaMap::DidClear(storageMapSeed), storageAreaID);
+ completionHandler();
}
void StorageManagerSet::cloneSessionStorageNamespace(IPC::Connection&, PAL::SessionID sessionID, StorageNamespaceIdentifier fromStorageNamespaceID, StorageNamespaceIdentifier toStorageNamespaceID)
diff --git a/Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.h b/Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.h
index d602a5f..ac15fee 100644
--- a/Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.h
+++ b/Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.h
@@ -89,9 +89,9 @@
void connectToSessionStorageArea(IPC::Connection&, PAL::SessionID, StorageNamespaceIdentifier, SecurityOriginData&&, ConnectToStorageAreaCallback&&);
void disconnectFromStorageArea(IPC::Connection&, StorageAreaIdentifier);
void getValues(IPC::Connection&, StorageAreaIdentifier, GetValuesCallback&&);
- void setItem(IPC::Connection&, StorageAreaIdentifier, StorageAreaImplIdentifier, uint64_t storageMapSeed, const String& key, const String& value, const String& urlString);
- void removeItem(IPC::Connection&, StorageAreaIdentifier, StorageAreaImplIdentifier, uint64_t storageMapSeed, const String& key, const String& urlString);
- void clear(IPC::Connection&, StorageAreaIdentifier, StorageAreaImplIdentifier, uint64_t storageMapSeed, const String& urlString);
+ void setItem(IPC::Connection&, StorageAreaIdentifier, StorageAreaImplIdentifier, const String& key, const String& value, const String& urlString, CompletionHandler<void(bool)>&&);
+ void removeItem(IPC::Connection&, StorageAreaIdentifier, StorageAreaImplIdentifier, const String& key, const String& urlString, CompletionHandler<void()>&&);
+ void clear(IPC::Connection&, StorageAreaIdentifier, StorageAreaImplIdentifier, const String& urlString, CompletionHandler<void()>&&);
void cloneSessionStorageNamespace(IPC::Connection&, PAL::SessionID, StorageNamespaceIdentifier fromStorageNamespaceID, StorageNamespaceIdentifier toStorageNamespaceID);
HashMap<PAL::SessionID, std::unique_ptr<StorageManager>> m_storageManagers;
diff --git a/Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.messages.in b/Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.messages.in
index 6d06c1d..02495d2 100644
--- a/Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.messages.in
+++ b/Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.messages.in
@@ -28,7 +28,7 @@
GetValues(WebKit::StorageAreaIdentifier storageAreaID) -> (HashMap<String, String> values) Synchronous WantsConnection
CloneSessionStorageNamespace(PAL::SessionID sessionID, WebKit::StorageNamespaceIdentifier fromStorageNamespaceID, WebKit::StorageNamespaceIdentifier toStorageNamespaceID) WantsConnection
- SetItem(WebKit::StorageAreaIdentifier storageAreaID, WebKit::StorageAreaImplIdentifier storageAreaImplID, uint64_t storageMapSeed, String key, String value, String urlString) WantsConnection
- RemoveItem(WebKit::StorageAreaIdentifier storageAreaID, WebKit::StorageAreaImplIdentifier storageAreaImplID, uint64_t storageMapSeed, String key, String urlString) WantsConnection
- Clear(WebKit::StorageAreaIdentifier storageAreaID, WebKit::StorageAreaImplIdentifier storageAreaImplID, uint64_t storageMapSeed, String urlString) WantsConnection
+ SetItem(WebKit::StorageAreaIdentifier identifier, WebKit::StorageAreaImplIdentifier implIdentifier, String key, String value, String urlString) -> (bool quotaException) Async WantsConnection
+ RemoveItem(WebKit::StorageAreaIdentifier identifier, WebKit::StorageAreaImplIdentifier implIdentifier, String key, String urlString) -> () Async WantsConnection
+ Clear(WebKit::StorageAreaIdentifier identifier, WebKit::StorageAreaImplIdentifier implIdentifier, String urlString) -> () Async WantsConnection
}
diff --git a/Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp b/Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp
index d974e6d..90fa1df 100644
--- a/Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp
+++ b/Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp
@@ -678,7 +678,7 @@
}
}
-void NetworkStorageManager::setItem(IPC::Connection& connection, StorageAreaIdentifier identifier, StorageAreaImplIdentifier implIdentifier, uint64_t storageMapSeed, String&& key, String&& value, String&& urlString)
+void NetworkStorageManager::setItem(IPC::Connection& connection, StorageAreaIdentifier identifier, StorageAreaImplIdentifier implIdentifier, String&& key, String&& value, String&& urlString, CompletionHandler<void(bool)>&& completionHandler)
{
ASSERT(!RunLoop::isMain());
@@ -689,27 +689,27 @@
hasQuotaError = (result.error() == StorageError::QuotaExceeded);
}
- connection.send(Messages::StorageAreaMap::DidSetItem(storageMapSeed, key, hasQuotaError), identifier);
+ completionHandler(hasQuotaError);
}
-void NetworkStorageManager::removeItem(IPC::Connection& connection, StorageAreaIdentifier identifier, StorageAreaImplIdentifier implIdentifier, uint64_t storageMapSeed, String&& key, String&& urlString)
+void NetworkStorageManager::removeItem(IPC::Connection& connection, StorageAreaIdentifier identifier, StorageAreaImplIdentifier implIdentifier, String&& key, String&& urlString, CompletionHandler<void()>&& completionHandler)
{
ASSERT(!RunLoop::isMain());
if (auto storageArea = m_storageAreaRegistry->getStorageArea(identifier))
- storageArea->removeItem(connection.uniqueID(), implIdentifier, key, WTFMove(urlString));
+ storageArea->removeItem(connection.uniqueID(), implIdentifier, WTFMove(key), WTFMove(urlString));
- connection.send(Messages::StorageAreaMap::DidRemoveItem(storageMapSeed, WTFMove(key)), identifier);
+ completionHandler();
}
-void NetworkStorageManager::clear(IPC::Connection& connection, StorageAreaIdentifier identifier, StorageAreaImplIdentifier implIdentifier, uint64_t storageMapSeed, String&& urlString)
+void NetworkStorageManager::clear(IPC::Connection& connection, StorageAreaIdentifier identifier, StorageAreaImplIdentifier implIdentifier, String&& urlString, CompletionHandler<void()>&& completionHandler)
{
ASSERT(!RunLoop::isMain());
if (auto storageArea = m_storageAreaRegistry->getStorageArea(identifier))
storageArea->clear(connection.uniqueID(), implIdentifier, WTFMove(urlString));
- connection.send(Messages::StorageAreaMap::DidClear(storageMapSeed), identifier);
+ completionHandler();
}
} // namespace WebKit
diff --git a/Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h b/Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h
index a1f3076..15c0372 100644
--- a/Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h
+++ b/Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h
@@ -114,9 +114,9 @@
void connectToStorageAreaSync(IPC::Connection&, WebCore::StorageType, StorageAreaMapIdentifier, StorageNamespaceIdentifier, const WebCore::ClientOrigin&, CompletionHandler<void(StorageAreaIdentifier, HashMap<String, String>, uint64_t)>&&);
void disconnectFromStorageArea(IPC::Connection&, StorageAreaIdentifier);
void cloneSessionStorageNamespace(IPC::Connection&, StorageNamespaceIdentifier, StorageNamespaceIdentifier);
- void setItem(IPC::Connection&, StorageAreaIdentifier, StorageAreaImplIdentifier, uint64_t seed, String&& key, String&& value, String&& urlString);
- void removeItem(IPC::Connection&, StorageAreaIdentifier, StorageAreaImplIdentifier, uint64_t seed, String&& key, String&& urlString);
- void clear(IPC::Connection&, StorageAreaIdentifier, StorageAreaImplIdentifier, uint64_t seed, String&& urlString);
+ void setItem(IPC::Connection&, StorageAreaIdentifier, StorageAreaImplIdentifier, String&& key, String&& value, String&& urlString, CompletionHandler<void(bool)>&&);
+ void removeItem(IPC::Connection&, StorageAreaIdentifier, StorageAreaImplIdentifier, String&& key, String&& urlString, CompletionHandler<void()>&&);
+ void clear(IPC::Connection&, StorageAreaIdentifier, StorageAreaImplIdentifier, String&& urlString, CompletionHandler<void()>&&);
PAL::SessionID m_sessionID;
Ref<SuspendableWorkQueue> m_queue;
diff --git a/Source/WebKit/NetworkProcess/storage/NetworkStorageManager.messages.in b/Source/WebKit/NetworkProcess/storage/NetworkStorageManager.messages.in
index ffc86f9..50b9670 100644
--- a/Source/WebKit/NetworkProcess/storage/NetworkStorageManager.messages.in
+++ b/Source/WebKit/NetworkProcess/storage/NetworkStorageManager.messages.in
@@ -44,7 +44,7 @@
ConnectToStorageAreaSync(WebCore::StorageType type, WebKit::StorageAreaMapIdentifier sourceIdentifier, WebKit::StorageNamespaceIdentifier namespaceIdentifier, struct WebCore::ClientOrigin origin) -> (WebKit::StorageAreaIdentifier identifier, HashMap<String, String> items, uint64_t messageIdentifier) Synchronous WantsConnection
DisconnectFromStorageArea(WebKit::StorageAreaIdentifier identifier) WantsConnection
CloneSessionStorageNamespace(WebKit::StorageNamespaceIdentifier fromStorageNamespaceID, WebKit::StorageNamespaceIdentifier toStorageNamespaceID) WantsConnection
- SetItem(WebKit::StorageAreaIdentifier identifier, WebKit::StorageAreaImplIdentifier implIdentifier, uint64_t storageMapSeed, String key, String value, String urlString) WantsConnection
- RemoveItem(WebKit::StorageAreaIdentifier identifier, WebKit::StorageAreaImplIdentifier implIdentifier, uint64_t storageMapSeed, String key, String urlString) WantsConnection
- Clear(WebKit::StorageAreaIdentifier identifier, WebKit::StorageAreaImplIdentifier implIdentifier, uint64_t storageMapSeed, String urlString) WantsConnection
+ SetItem(WebKit::StorageAreaIdentifier identifier, WebKit::StorageAreaImplIdentifier implIdentifier, String key, String value, String urlString) -> (bool quotaException) Async WantsConnection
+ RemoveItem(WebKit::StorageAreaIdentifier identifier, WebKit::StorageAreaImplIdentifier implIdentifier, String key, String urlString) -> () Async WantsConnection
+ Clear(WebKit::StorageAreaIdentifier identifier, WebKit::StorageAreaImplIdentifier implIdentifier, String urlString) -> () Async WantsConnection
}
diff --git a/Source/WebKit/WebProcess/WebStorage/StorageAreaMap.cpp b/Source/WebKit/WebProcess/WebStorage/StorageAreaMap.cpp
index df4c26f..5a417c4 100644
--- a/Source/WebKit/WebProcess/WebStorage/StorageAreaMap.cpp
+++ b/Source/WebKit/WebProcess/WebStorage/StorageAreaMap.cpp
@@ -97,10 +97,17 @@
m_pendingValueChanges.add(key);
- if (m_remoteAreaIdentifier)
- WebProcess::singleton().ensureNetworkProcessConnection().connection().send(Messages::NetworkStorageManager::SetItem(*m_remoteAreaIdentifier, sourceArea->identifier(), m_currentSeed, key, value, sourceFrame->document()->url().string()), 0);
- else
+ if (!m_remoteAreaIdentifier) {
RELEASE_LOG_ERROR(Storage, "StorageAreaMap::setItem failed because storage map ID is invalid");
+ return;
+ }
+
+ auto callback = [weakThis = WeakPtr { *this }, seed = m_currentSeed, key](bool hasQuotaException) mutable {
+ if (weakThis)
+ weakThis->didSetItem(seed, key, hasQuotaException);
+ };
+ auto& connection = WebProcess::singleton().ensureNetworkProcessConnection().connection();
+ connection.sendWithAsyncReply(Messages::NetworkStorageManager::SetItem(*m_remoteAreaIdentifier, sourceArea->identifier(), key, value, sourceFrame->document()->url().string()), WTFMove(callback));
}
void StorageAreaMap::removeItem(WebCore::Frame* sourceFrame, StorageAreaImpl* sourceArea, const String& key)
@@ -116,10 +123,16 @@
m_pendingValueChanges.add(key);
- if (m_remoteAreaIdentifier)
- WebProcess::singleton().ensureNetworkProcessConnection().connection().send(Messages::NetworkStorageManager::RemoveItem(*m_remoteAreaIdentifier, sourceArea->identifier(), m_currentSeed, key, sourceFrame->document()->url().string()), 0);
- else
+ if (!m_remoteAreaIdentifier) {
RELEASE_LOG_ERROR(Storage, "StorageAreaMap::removeItem failed because storage map ID is invalid");
+ return;
+ }
+
+ auto callback = [weakThis = WeakPtr { *this }, seed = m_currentSeed, key]() mutable {
+ if (weakThis)
+ weakThis->didRemoveItem(seed, key);
+ };
+ WebProcess::singleton().ensureNetworkProcessConnection().connection().sendWithAsyncReply(Messages::NetworkStorageManager::RemoveItem(*m_remoteAreaIdentifier, sourceArea->identifier(), key, sourceFrame->document()->url().string()), WTFMove(callback));
}
void StorageAreaMap::clear(WebCore::Frame* sourceFrame, StorageAreaImpl* sourceArea)
@@ -130,10 +143,16 @@
m_hasPendingClear = true;
m_map = makeUnique<StorageMap>(m_quotaInBytes);
- if (m_remoteAreaIdentifier)
- WebProcess::singleton().ensureNetworkProcessConnection().connection().send(Messages::NetworkStorageManager::Clear(*m_remoteAreaIdentifier, sourceArea->identifier(), m_currentSeed, sourceFrame->document()->url().string()), 0);
- else
+ if (!m_remoteAreaIdentifier) {
RELEASE_LOG_ERROR(Storage, "StorageAreaMap::clear failed because storage map ID is invalid");
+ return;
+ }
+
+ auto callback = [weakThis = WeakPtr { *this }, seed = m_currentSeed]() mutable {
+ if (weakThis)
+ weakThis->didClear(seed);
+ };
+ WebProcess::singleton().ensureNetworkProcessConnection().connection().sendWithAsyncReply(Messages::NetworkStorageManager::Clear(*m_remoteAreaIdentifier, sourceArea->identifier(), sourceFrame->document()->url().string()), WTFMove(callback));
}
bool StorageAreaMap::contains(const String& key)
diff --git a/Source/WebKit/WebProcess/WebStorage/StorageAreaMap.h b/Source/WebKit/WebProcess/WebStorage/StorageAreaMap.h
index c6271d9..9653f76 100644
--- a/Source/WebKit/WebProcess/WebStorage/StorageAreaMap.h
+++ b/Source/WebKit/WebProcess/WebStorage/StorageAreaMap.h
@@ -104,7 +104,7 @@
std::unique_ptr<WebCore::StorageMap> m_map;
std::optional<StorageAreaIdentifier> m_remoteAreaIdentifier;
HashCountedSet<String> m_pendingValueChanges;
- uint64_t m_currentSeed { 0 };
+ uint64_t m_currentSeed { 1 };
unsigned m_quotaInBytes;
WebCore::StorageType m_type;
uint64_t m_useCount { 0 };
diff --git a/Source/WebKit/WebProcess/WebStorage/StorageAreaMap.messages.in b/Source/WebKit/WebProcess/WebStorage/StorageAreaMap.messages.in
index 6dee8e8..60a6347 100644
--- a/Source/WebKit/WebProcess/WebStorage/StorageAreaMap.messages.in
+++ b/Source/WebKit/WebProcess/WebStorage/StorageAreaMap.messages.in
@@ -21,10 +21,6 @@
# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
messages -> StorageAreaMap NotRefCounted {
- DidSetItem(uint64_t storageMapSeed, String key, bool quotaException)
- DidRemoveItem(uint64_t storageMapSeed, String key)
- DidClear(uint64_t storageMapSeed)
-
DispatchStorageEvent(std::optional<WebKit::StorageAreaImplIdentifier> storageAreaImplID, String key, String oldValue, String newValue, String urlString, uint64_t messageIdentifier)
ClearCache(uint64_t messageIdentifier)
}
