WebKit Bugzilla
Attachment 370424 Details for
Bug 198090
: API Test landed in r245540 [Mac WK2] TestWebKitAPI.WKWebView.LocalStorageProcessCrashes is a flaky failure
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[patch]
Patch
bug-198090-20190522110433.patch (text/plain), 32.18 KB, created by
Sihui Liu
on 2019-05-22 11:04:34 PDT
(
hide
)
Description:
Patch
Filename:
MIME Type:
Creator:
Sihui Liu
Created:
2019-05-22 11:04:34 PDT
Size:
32.18 KB
patch
obsolete
>Subversion Revision: 245599 >diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog >index 4754cc9d11306bb8b6d99ee8a227b8c3b5a35eea..6730da83bafadd86540a146828d4fe6881d6ffa5 100644 >--- a/Source/WebKit/ChangeLog >+++ b/Source/WebKit/ChangeLog >@@ -1,3 +1,46 @@ >+2019-05-22 Sihui Liu <sihui_liu@apple.com> >+ >+ API Test landed in r245540 [Mac WK2] TestWebKitAPI.WKWebView.LocalStorageProcessCrashes is a flaky failure >+ https://bugs.webkit.org/show_bug.cgi?id=198090 >+ <rdar://problem/51003644> >+ >+ Reviewed by NOBODY (OOPS!). >+ >+ We used to dispatch StorageManager message to StorageManager's work queue, which required message handler to be >+ added to queue before receiving first StorageManager message, otherwise network process would not know how to >+ decode the message. >+ >+ After r245540, when netork process crashes and dom storage is accessed from web process after that, web process >+ re-establishes its connection to network process, asks network process to add message handler, and then sends >+ StorageManager message immediately. Because work queue message receiver is added on a background thread in >+ network process, it is possible the StorageManager message is received before that. >+ >+ A safe and easy resolution is to not dispatch StorageManager message to work queue, so that we don't need to >+ wait for the message receiver to be added. Handling message on the main thread also allows us to untying the >+ knot that binds StorageManager and connection, which may be a preferred design in the future. >+ >+ * NetworkProcess/NetworkConnectionToWebProcess.cpp: >+ (WebKit::NetworkConnectionToWebProcess::didReceiveMessage): >+ (WebKit::NetworkConnectionToWebProcess::didReceiveSyncMessage): >+ * NetworkProcess/NetworkProcess.cpp: >+ (WebKit::NetworkProcess::webPageWasAdded): >+ * NetworkProcess/WebStorage/StorageManager.cpp: >+ (WebKit::StorageManager::processDidCloseConnection): >+ (WebKit::StorageManager::createLocalStorageMap): >+ (WebKit::StorageManager::createTransientLocalStorageMap): >+ (WebKit::StorageManager::createSessionStorageMap): >+ (WebKit::StorageManager::destroyStorageMap): >+ (WebKit::StorageManager::didGetValues): >+ (WebKit::StorageManager::getValues): >+ (WebKit::StorageManager::setItem): >+ (WebKit::StorageManager::setItems): >+ (WebKit::StorageManager::removeItem): >+ (WebKit::StorageManager::clear): >+ (WebKit::StorageManager::processWillOpenConnection): Deleted. >+ (WebKit::StorageManager::dispatchMessageToQueue): Deleted. >+ (WebKit::StorageManager::dispatchSyncMessageToQueue): Deleted. >+ * NetworkProcess/WebStorage/StorageManager.h: >+ > 2019-05-21 Per Arne Vollan <pvollan@apple.com> > > Sandbox violation is making the WebContent process crash >diff --git a/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp b/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp >index 18db0da8698cff020995903f93791a74e26326f2..3d71f8f6b1b6c26db0550f88aa5ee1144210a906 100644 >--- a/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp >+++ b/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp >@@ -227,7 +227,7 @@ void NetworkConnectionToWebProcess::didReceiveMessage(IPC::Connection& connectio > > if (decoder.messageReceiverName() == Messages::StorageManager::messageReceiverName()) { > if (auto* session = m_networkProcess->networkSessionByConnection(connection)) { >- session->storageManager().dispatchMessageToQueue(connection, decoder); >+ session->storageManager().didReceiveMessage(connection, decoder); > return; > } > } >@@ -273,7 +273,7 @@ void NetworkConnectionToWebProcess::didReceiveSyncMessage(IPC::Connection& conne > > if (decoder.messageReceiverName() == Messages::StorageManager::messageReceiverName()) { > if (auto* session = m_networkProcess->networkSessionByConnection(connection)) { >- session->storageManager().dispatchSyncMessageToQueue(connection, decoder, reply); >+ session->storageManager().didReceiveSyncMessage(connection, decoder, reply); > return; > } > } >diff --git a/Source/WebKit/NetworkProcess/NetworkProcess.cpp b/Source/WebKit/NetworkProcess/NetworkProcess.cpp >index bcd5ab6c028e98c25529771071ff0efab36fcbaa..a5d6f5cfcc7c4325e9ad73136f36fda299d1de0b 100644 >--- a/Source/WebKit/NetworkProcess/NetworkProcess.cpp >+++ b/Source/WebKit/NetworkProcess/NetworkProcess.cpp >@@ -2635,7 +2635,6 @@ void NetworkProcess::webPageWasAdded(IPC::Connection& connection, PAL::SessionID > > auto connectionID = connection.uniqueID(); > m_sessionByConnection.ensure(connectionID, [&]() { >- storageManager.processWillOpenConnection(connection); > return sessionID; > }); > >diff --git a/Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp b/Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp >index 97157877e840becd258c06e697a4de8f6c023316..b254b414d89e2e46276bd2e5fec21de05db785ff 100644 >--- a/Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp >+++ b/Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp >@@ -551,15 +551,9 @@ void StorageManager::cloneSessionStorageNamespace(uint64_t storageNamespaceID, u > }); > } > >-void StorageManager::processWillOpenConnection(IPC::Connection& connection) >-{ >- connection.addWorkQueueMessageReceiver(Messages::StorageManager::messageReceiverName(), m_queue.get(), this); >-} >- > void StorageManager::processDidCloseConnection(IPC::Connection& connection) > { >- connection.removeWorkQueueMessageReceiver(Messages::StorageManager::messageReceiverName()); >- >+ m_getValuesCallbacks.remove(&connection); > m_queue->dispatch([this, protectedThis = makeRef(*this), connectionID = connection.uniqueID()]() mutable { > Vector<std::pair<IPC::Connection::UniqueID, uint64_t>> connectionAndStorageMapIDPairsToRemove; > for (auto& storageArea : m_storageAreasByConnection) { >@@ -718,213 +712,231 @@ void StorageManager::deleteLocalStorageEntriesForOrigins(const Vector<WebCore::S > > void StorageManager::createLocalStorageMap(IPC::Connection& connection, uint64_t storageMapID, uint64_t storageNamespaceID, SecurityOriginData&& securityOriginData) > { >- // FIXME: Replace this check if https://bugs.webkit.org/show_bug.cgi?id=198048 is done. >- ASSERT(!RunLoop::isMain()); >- ASSERT(!m_isEphemeral); >- std::pair<IPC::Connection::UniqueID, uint64_t> connectionAndStorageMapIDPair(connection.uniqueID(), storageMapID); >+ m_queue->dispatch([this, protectedThis = makeRef(*this), connectionID = connection.uniqueID(), storageMapID, storageNamespaceID, securityOriginData = securityOriginData.isolatedCopy()]() mutable { >+ ASSERT(!m_isEphemeral); >+ std::pair<IPC::Connection::UniqueID, uint64_t> connectionAndStorageMapIDPair(connectionID, storageMapID); > >- // FIXME: This should be a message check. >- ASSERT((HashMap<std::pair<IPC::Connection::UniqueID, uint64_t>, RefPtr<StorageArea>>::isValidKey(connectionAndStorageMapIDPair))); >+ // FIXME: This should be a message check. >+ ASSERT((HashMap<std::pair<IPC::Connection::UniqueID, uint64_t>, RefPtr<StorageArea>>::isValidKey(connectionAndStorageMapIDPair))); > >- auto result = m_storageAreasByConnection.add(connectionAndStorageMapIDPair, nullptr); >+ auto result = m_storageAreasByConnection.add(connectionAndStorageMapIDPair, nullptr); > >- // FIXME: These should be a message checks. >- ASSERT(result.isNewEntry); >- ASSERT((HashMap<uint64_t, RefPtr<LocalStorageNamespace>>::isValidKey(storageNamespaceID))); >+ // FIXME: These should be a message checks. >+ ASSERT(result.isNewEntry); >+ ASSERT((HashMap<uint64_t, RefPtr<LocalStorageNamespace>>::isValidKey(storageNamespaceID))); > >- LocalStorageNamespace* localStorageNamespace = getOrCreateLocalStorageNamespace(storageNamespaceID); >+ LocalStorageNamespace* localStorageNamespace = getOrCreateLocalStorageNamespace(storageNamespaceID); > >- // FIXME: This should be a message check. >- ASSERT(localStorageNamespace); >+ // FIXME: This should be a message check. >+ ASSERT(localStorageNamespace); > >- auto storageArea = localStorageNamespace->getOrCreateStorageArea(WTFMove(securityOriginData)); >- storageArea->addListener(connection.uniqueID(), storageMapID); >+ auto storageArea = localStorageNamespace->getOrCreateStorageArea(WTFMove(securityOriginData)); >+ storageArea->addListener(connectionID, storageMapID); > >- result.iterator->value = WTFMove(storageArea); >+ result.iterator->value = WTFMove(storageArea); >+ }); > } > > void StorageManager::createTransientLocalStorageMap(IPC::Connection& connection, uint64_t storageMapID, uint64_t storageNamespaceID, SecurityOriginData&& topLevelOriginData, SecurityOriginData&& origin) > { >- ASSERT(!RunLoop::isMain()); >- >- // FIXME: This should be a message check. >- ASSERT(m_storageAreasByConnection.isValidKey({ connection.uniqueID(), storageMapID })); >- >- // See if we already have session storage for this connection/origin combo. >- // If so, update the map with the new ID, otherwise keep on trucking. >- for (auto it = m_storageAreasByConnection.begin(), end = m_storageAreasByConnection.end(); it != end; ++it) { >- if (it->key.first != connection.uniqueID()) >- continue; >- Ref<StorageArea> area = *it->value; >- if (!area->isSessionStorage()) >- continue; >- if (!origin.securityOrigin()->isSameSchemeHostPort(area->securityOrigin().securityOrigin().get())) >- continue; >- area->addListener(connection.uniqueID(), storageMapID); >- // If the storageMapID used as key in m_storageAreasByConnection is no longer one of the StorageArea's listeners, then this means >- // that destroyStorageMap() was already called for that storageMapID but it decided not to remove it from m_storageAreasByConnection >- // so that we could reuse it later on for the same connection/origin combo. In this case, it is safe to remove the previous >- // storageMapID from m_storageAreasByConnection. >- if (!area->hasListener(connection.uniqueID(), it->key.second)) >- m_storageAreasByConnection.remove(it); >- m_storageAreasByConnection.add({ connection.uniqueID(), storageMapID }, WTFMove(area)); >- return; >- } >+ m_queue->dispatch([this, protectedThis = makeRef(*this), &connection, storageMapID, storageNamespaceID, topLevelOriginData = topLevelOriginData.isolatedCopy(), origin = origin.isolatedCopy()]() mutable { >+ // FIXME: This should be a message check. >+ ASSERT(m_storageAreasByConnection.isValidKey({ connection.uniqueID(), storageMapID })); >+ >+ // See if we already have session storage for this connection/origin combo. >+ // If so, update the map with the new ID, otherwise keep on trucking. >+ for (auto it = m_storageAreasByConnection.begin(), end = m_storageAreasByConnection.end(); it != end; ++it) { >+ if (it->key.first != connection.uniqueID()) >+ continue; >+ Ref<StorageArea> area = *it->value; >+ if (!area->isSessionStorage()) >+ continue; >+ if (!origin.securityOrigin()->isSameSchemeHostPort(area->securityOrigin().securityOrigin().get())) >+ continue; >+ area->addListener(connection.uniqueID(), storageMapID); >+ // If the storageMapID used as key in m_storageAreasByConnection is no longer one of the StorageArea's listeners, then this means >+ // that destroyStorageMap() was already called for that storageMapID but it decided not to remove it from m_storageAreasByConnection >+ // so that we could reuse it later on for the same connection/origin combo. In this case, it is safe to remove the previous >+ // storageMapID from m_storageAreasByConnection. >+ if (!area->hasListener(connection.uniqueID(), it->key.second)) >+ m_storageAreasByConnection.remove(it); >+ m_storageAreasByConnection.add({ connection.uniqueID(), storageMapID }, WTFMove(area)); >+ return; >+ } > >- auto& slot = m_storageAreasByConnection.add({ connection.uniqueID(), storageMapID }, nullptr).iterator->value; >+ auto& slot = m_storageAreasByConnection.add({ connection.uniqueID(), storageMapID }, nullptr).iterator->value; > >- // FIXME: This should be a message check. >- ASSERT(!slot); >+ // FIXME: This should be a message check. >+ ASSERT(!slot); > >- auto* transientLocalStorageNamespace = getOrCreateTransientLocalStorageNamespace(storageNamespaceID, WTFMove(topLevelOriginData)); >+ auto* transientLocalStorageNamespace = getOrCreateTransientLocalStorageNamespace(storageNamespaceID, WTFMove(topLevelOriginData)); > >- auto storageArea = transientLocalStorageNamespace->getOrCreateStorageArea(WTFMove(origin)); >- storageArea->addListener(connection.uniqueID(), storageMapID); >+ auto storageArea = transientLocalStorageNamespace->getOrCreateStorageArea(WTFMove(origin)); >+ storageArea->addListener(connection.uniqueID(), storageMapID); > >- slot = WTFMove(storageArea); >+ slot = WTFMove(storageArea); >+ }); > } > > void StorageManager::createSessionStorageMap(IPC::Connection& connection, uint64_t storageMapID, uint64_t storageNamespaceID, SecurityOriginData&& securityOriginData) > { >- ASSERT(!RunLoop::isMain()); >- >- if (m_isEphemeral) { >- m_ephemeralStorage.add(securityOriginData, WebCore::StorageMap::create(localStorageDatabaseQuotaInBytes)); >- return; >- } >- // FIXME: This should be a message check. >- ASSERT(m_sessionStorageNamespaces.isValidKey(storageNamespaceID)); >+ m_queue->dispatch([this, protectedThis = makeRef(*this), connectionID = connection.uniqueID(), storageMapID, storageNamespaceID, securityOriginData = securityOriginData.isolatedCopy()]() mutable { >+ if (m_isEphemeral) { >+ m_ephemeralStorage.add(securityOriginData, WebCore::StorageMap::create(localStorageDatabaseQuotaInBytes)); >+ return; >+ } >+ // FIXME: This should be a message check. >+ ASSERT(m_sessionStorageNamespaces.isValidKey(storageNamespaceID)); > >- SessionStorageNamespace* sessionStorageNamespace = m_sessionStorageNamespaces.get(storageNamespaceID); >- if (!sessionStorageNamespace) { >- // We're getting an incoming message from the web process that's for session storage for a web page >- // that has already been closed, just ignore it. >- return; >- } >+ SessionStorageNamespace* sessionStorageNamespace = m_sessionStorageNamespaces.get(storageNamespaceID); >+ if (!sessionStorageNamespace) { >+ // We're getting an incoming message from the web process that's for session storage for a web page >+ // that has already been closed, just ignore it. >+ return; >+ } > >- // FIXME: This should be a message check. >- ASSERT(m_storageAreasByConnection.isValidKey({ connection.uniqueID(), storageMapID })); >+ // FIXME: This should be a message check. >+ ASSERT(m_storageAreasByConnection.isValidKey({ connectionID, storageMapID })); > >- auto& slot = m_storageAreasByConnection.add({ connection.uniqueID(), storageMapID }, nullptr).iterator->value; >+ auto& slot = m_storageAreasByConnection.add({ connectionID, storageMapID }, nullptr).iterator->value; > >- // FIXME: This should be a message check. >- ASSERT(!slot); >+ // FIXME: This should be a message check. >+ ASSERT(!slot); > >- // FIXME: This should be a message check. >- ASSERT(sessionStorageNamespace->allowedConnections().contains(connection.uniqueID())); >+ // FIXME: This should be a message check. >+ ASSERT(sessionStorageNamespace->allowedConnections().contains(connectionID)); > >- auto storageArea = sessionStorageNamespace->getOrCreateStorageArea(WTFMove(securityOriginData)); >- storageArea->addListener(connection.uniqueID(), storageMapID); >+ auto storageArea = sessionStorageNamespace->getOrCreateStorageArea(WTFMove(securityOriginData)); >+ storageArea->addListener(connectionID, storageMapID); > >- slot = WTFMove(storageArea); >+ slot = WTFMove(storageArea); >+ }); > } > > void StorageManager::destroyStorageMap(IPC::Connection& connection, uint64_t storageMapID) > { >- ASSERT(!RunLoop::isMain()); >+ m_queue->dispatch([this, protectedThis = makeRef(*this), connectionID = connection.uniqueID(), storageMapID]() mutable { >+ std::pair<IPC::Connection::UniqueID, uint64_t> connectionAndStorageMapIDPair(connectionID, storageMapID); > >- std::pair<IPC::Connection::UniqueID, uint64_t> connectionAndStorageMapIDPair(connection.uniqueID(), storageMapID); >+ // FIXME: This should be a message check. >+ ASSERT(m_storageAreasByConnection.isValidKey(connectionAndStorageMapIDPair)); > >- // FIXME: This should be a message check. >- ASSERT(m_storageAreasByConnection.isValidKey(connectionAndStorageMapIDPair)); >+ auto it = m_storageAreasByConnection.find(connectionAndStorageMapIDPair); >+ if (it == m_storageAreasByConnection.end()) { >+ // The connection has been removed because the last page was closed. >+ return; >+ } > >- auto it = m_storageAreasByConnection.find(connectionAndStorageMapIDPair); >- if (it == m_storageAreasByConnection.end()) { >- // The connection has been removed because the last page was closed. >- return; >- } >+ it->value->removeListener(connectionID, storageMapID); > >- it->value->removeListener(connection.uniqueID(), storageMapID); >+ // Don't remove session storage maps. The web process may reconnect and expect the data to still be around. >+ if (it->value->isSessionStorage()) >+ return; > >- // Don't remove session storage maps. The web process may reconnect and expect the data to still be around. >- if (it->value->isSessionStorage()) >- return; >+ m_storageAreasByConnection.remove(connectionAndStorageMapIDPair); >+ }); >+} >+ >+void StorageManager::didGetValues(IPC::Connection& connection, uint64_t storageMapID, const HashMap<String, String>& items) >+{ >+ RunLoop::main().dispatch([this, protectedThis = makeRef(*this), &connection, storageMapID, items = crossThreadCopy(items)] { >+ auto it = m_getValuesCallbacks.find(&connection); >+ if (it == m_getValuesCallbacks.end()) >+ return; > >- m_storageAreasByConnection.remove(connectionAndStorageMapIDPair); >+ ASSERT(it->value.contains(storageMapID)); >+ if (auto completionHandler = it->value.take(storageMapID)) >+ completionHandler(items); >+ >+ if (it->value.isEmpty()) >+ m_getValuesCallbacks.remove(&connection); >+ }); > } > > void StorageManager::getValues(IPC::Connection& connection, WebCore::SecurityOriginData&& securityOriginData, uint64_t storageMapID, uint64_t storageMapSeed, CompletionHandler<void(const HashMap<String, String>&)>&& completionHandler) > { >- ASSERT(!RunLoop::isMain()); >- >- auto* storageArea = findStorageArea(connection, storageMapID); >- if (!storageArea) { >- if (m_isEphemeral) { >- if (auto storageMap = m_ephemeralStorage.get(securityOriginData)) >- return completionHandler(storageMap->items()); >+ auto callbacksForConnection = m_getValuesCallbacks.add(&connection, GetValuesCallback()); >+ auto result = callbacksForConnection.iterator->value.add(storageMapID, WTFMove(completionHandler)); >+ ASSERT(result.isNewEntry); >+ m_queue->dispatch([this, protectedThis = makeRef(*this), connection = makeRef(connection), securityOriginData = securityOriginData.isolatedCopy(), storageMapID, storageMapSeed]() mutable { >+ auto* storageArea = findStorageArea(connection.get(), storageMapID); >+ if (!storageArea) { >+ if (m_isEphemeral) { >+ if (auto storageMap = m_ephemeralStorage.get(securityOriginData)) >+ return didGetValues(connection.get(), storageMapID, storageMap->items()); >+ } >+ // This is a session storage area for a page that has already been closed. Ignore it. >+ return didGetValues(connection.get(), storageMapID, { }); > } >- // This is a session storage area for a page that has already been closed. Ignore it. >- return completionHandler({ }); >- } >- >- completionHandler(storageArea->items()); >- connection.send(Messages::StorageAreaMap::DidGetValues(storageMapSeed), storageMapID); >+ didGetValues(connection.get(), storageMapID, storageArea->items()); >+ connection->send(Messages::StorageAreaMap::DidGetValues(storageMapSeed), storageMapID); >+ }); > } > > void StorageManager::setItem(IPC::Connection& connection, WebCore::SecurityOriginData&& securityOriginData, uint64_t storageMapID, uint64_t sourceStorageAreaID, uint64_t storageMapSeed, const String& key, const String& value, const String& urlString) > { >- ASSERT(!RunLoop::isMain()); >- >- auto* storageArea = findStorageArea(connection, storageMapID); >- if (!storageArea) { >- if (m_isEphemeral) { >- if (auto storageMap = m_ephemeralStorage.get(securityOriginData)) { >- String oldValue; >- bool quotaException; >- storageMap->setItem(key, value, oldValue, quotaException); >+ m_queue->dispatch([this, protectedThis = makeRef(*this), connection = makeRef(connection), securityOriginData = securityOriginData.isolatedCopy(), storageMapID, sourceStorageAreaID, storageMapSeed, key = key.isolatedCopy(), value = value.isolatedCopy(), urlString = urlString.isolatedCopy()]() mutable { >+ auto* storageArea = findStorageArea(connection.get(), storageMapID); >+ if (!storageArea) { >+ if (m_isEphemeral) { >+ if (auto storageMap = m_ephemeralStorage.get(securityOriginData)) { >+ String oldValue; >+ bool quotaException; >+ storageMap->setItem(key, value, oldValue, quotaException); >+ } > } >+ // This is a session storage area for a page that has already been closed. Ignore it. >+ return; > } >- // This is a session storage area for a page that has already been closed. Ignore it. >- return; >- } > >- bool quotaError; >- storageArea->setItem(connection.uniqueID(), sourceStorageAreaID, key, value, urlString, quotaError); >- connection.send(Messages::StorageAreaMap::DidSetItem(storageMapSeed, key, quotaError), storageMapID); >+ bool quotaError; >+ storageArea->setItem(connection->uniqueID(), sourceStorageAreaID, key, value, urlString, quotaError); >+ connection->send(Messages::StorageAreaMap::DidSetItem(storageMapSeed, key, quotaError), storageMapID); >+ }); > } > > void StorageManager::setItems(IPC::Connection& connection, uint64_t storageMapID, const HashMap<String, String>& items) > { >- ASSERT(!RunLoop::isMain()); >- >- if (auto* storageArea = findStorageArea(connection, storageMapID)) >- storageArea->setItems(items); >+ m_queue->dispatch([this, protectedThis = makeRef(*this), connection = makeRef(connection), storageMapID, items = crossThreadCopy(items)]() mutable { >+ if (auto* storageArea = findStorageArea(connection.get(), storageMapID)) >+ storageArea->setItems(items); >+ }); > } > > void StorageManager::removeItem(IPC::Connection& connection, WebCore::SecurityOriginData&& securityOriginData, uint64_t storageMapID, uint64_t sourceStorageAreaID, uint64_t storageMapSeed, const String& key, const String& urlString) > { >- ASSERT(!RunLoop::isMain()); >- >- auto* storageArea = findStorageArea(connection, storageMapID); >- if (!storageArea) { >- if (m_isEphemeral) { >- if (auto storageMap = m_ephemeralStorage.get(securityOriginData)) { >- String oldValue; >- storageMap->removeItem(key, oldValue); >+ m_queue->dispatch([this, protectedThis = makeRef(*this), connection = makeRef(connection), securityOriginData = securityOriginData.isolatedCopy(), storageMapID, sourceStorageAreaID, storageMapSeed, key = key.isolatedCopy(), urlString = urlString.isolatedCopy()]() mutable { >+ auto* storageArea = findStorageArea(connection.get(), storageMapID); >+ if (!storageArea) { >+ if (m_isEphemeral) { >+ if (auto storageMap = m_ephemeralStorage.get(securityOriginData)) { >+ String oldValue; >+ storageMap->removeItem(key, oldValue); >+ } > } >+ // This is a session storage area for a page that has already been closed. Ignore it. >+ return; > } >- // This is a session storage area for a page that has already been closed. Ignore it. >- return; >- } > >- storageArea->removeItem(connection.uniqueID(), sourceStorageAreaID, key, urlString); >- connection.send(Messages::StorageAreaMap::DidRemoveItem(storageMapSeed, key), storageMapID); >+ storageArea->removeItem(connection->uniqueID(), sourceStorageAreaID, key, urlString); >+ connection->send(Messages::StorageAreaMap::DidRemoveItem(storageMapSeed, key), storageMapID); >+ }); > } > > void StorageManager::clear(IPC::Connection& connection, WebCore::SecurityOriginData&& securityOriginData, uint64_t storageMapID, uint64_t sourceStorageAreaID, uint64_t storageMapSeed, const String& urlString) > { >- ASSERT(!RunLoop::isMain()); >- >- auto* storageArea = findStorageArea(connection, storageMapID); >- if (!storageArea) { >- if (m_isEphemeral) >- m_ephemeralStorage.remove(securityOriginData); >- // This is a session storage area for a page that has already been closed. Ignore it. >- return; >- } >+ m_queue->dispatch([this, protectedThis = makeRef(*this), connection = makeRef(connection), securityOriginData = securityOriginData.isolatedCopy(), storageMapID, sourceStorageAreaID, storageMapSeed, urlString = urlString.isolatedCopy()]() mutable { >+ auto* storageArea = findStorageArea(connection.get(), storageMapID); >+ if (!storageArea) { >+ if (m_isEphemeral) >+ m_ephemeralStorage.remove(securityOriginData); >+ // This is a session storage area for a page that has already been closed. Ignore it. >+ return; >+ } > >- storageArea->clear(connection.uniqueID(), sourceStorageAreaID, urlString); >- connection.send(Messages::StorageAreaMap::DidClear(storageMapSeed), storageMapID); >+ storageArea->clear(connection->uniqueID(), sourceStorageAreaID, urlString); >+ connection->send(Messages::StorageAreaMap::DidClear(storageMapSeed), storageMapID); >+ }); > } > > void StorageManager::waitUntilWritesFinished() >@@ -975,18 +987,4 @@ StorageManager::TransientLocalStorageNamespace* StorageManager::getOrCreateTrans > }).iterator->value.get(); > } > >-void StorageManager::dispatchMessageToQueue(IPC::Connection& connection, IPC::Decoder& decoder) >-{ >- m_queue->dispatch([this, protectedThis = makeRef(*this), &connection, &decoder] { >- didReceiveMessage(connection, decoder); >- }); >-} >- >-void StorageManager::dispatchSyncMessageToQueue(IPC::Connection& connection, IPC::Decoder& decoder, std::unique_ptr<IPC::Encoder>& replyEncoder) >-{ >- m_queue->dispatch([this, protectedThis = makeRef(*this), &connection, &decoder, replyEncoder = WTFMove(replyEncoder)] () mutable { >- didReceiveSyncMessage(connection, decoder, replyEncoder); >- }); >-} >- > } // namespace WebKit >diff --git a/Source/WebKit/NetworkProcess/WebStorage/StorageManager.h b/Source/WebKit/NetworkProcess/WebStorage/StorageManager.h >index 4a5b7186f5832a81f67006f32bde3c5e44c5196e..bedf7ef7a9b6b3dc2c3d13f6980f5bf7449b477f 100644 >--- a/Source/WebKit/NetworkProcess/WebStorage/StorageManager.h >+++ b/Source/WebKit/NetworkProcess/WebStorage/StorageManager.h >@@ -54,7 +54,6 @@ public: > void removeAllowedSessionStorageNamespaceConnection(uint64_t storageNamespaceID, IPC::Connection&); > void cloneSessionStorageNamespace(uint64_t storageNamespaceID, uint64_t newStorageNamespaceID); > >- void processWillOpenConnection(IPC::Connection&); > void processDidCloseConnection(IPC::Connection&); > void waitUntilWritesFinished(); > >@@ -70,16 +69,13 @@ public: > > void getLocalStorageOriginDetails(Function<void(Vector<LocalStorageDatabaseTracker::OriginDetails>&&)>&& completionHandler); > >- void dispatchMessageToQueue(IPC::Connection&, IPC::Decoder&); >- void dispatchSyncMessageToQueue(IPC::Connection&, IPC::Decoder&, std::unique_ptr<IPC::Encoder>& replyEncoder); >- >-private: >- explicit StorageManager(const String& localStorageDirectory); >- > // IPC::Connection::WorkQueueMessageReceiver. > void didReceiveMessage(IPC::Connection&, IPC::Decoder&) override; > void didReceiveSyncMessage(IPC::Connection&, IPC::Decoder&, std::unique_ptr<IPC::Encoder>& replyEncoder) override; > >+private: >+ explicit StorageManager(const String& localStorageDirectory); >+ > // Message handlers. > void createLocalStorageMap(IPC::Connection&, uint64_t storageMapID, uint64_t storageNamespaceID, WebCore::SecurityOriginData&&); > void createTransientLocalStorageMap(IPC::Connection&, uint64_t storageMapID, uint64_t storageNamespaceID, WebCore::SecurityOriginData&& topLevelOriginData, WebCore::SecurityOriginData&&); >@@ -92,6 +88,8 @@ private: > void removeItem(IPC::Connection&, WebCore::SecurityOriginData&&, uint64_t storageMapID, uint64_t sourceStorageAreaID, uint64_t storageMapSeed, const String& key, const String& urlString); > void clear(IPC::Connection&, WebCore::SecurityOriginData&&, uint64_t storageMapID, uint64_t sourceStorageAreaID, uint64_t storageMapSeed, const String& urlString); > >+ void didGetValues(IPC::Connection&, uint64_t storageMapID, const HashMap<String, String>&); >+ > class StorageArea; > StorageArea* findStorageArea(IPC::Connection&, uint64_t) const; > >@@ -116,6 +114,8 @@ private: > HashMap<WebCore::SecurityOriginData, Ref<WebCore::StorageMap>> m_ephemeralStorage; > bool m_isEphemeral { false }; > >+ typedef HashMap<uint64_t, CompletionHandler<void(const HashMap<String, String>&)>> GetValuesCallback; >+ HashMap<RefPtr<IPC::Connection>, GetValuesCallback> m_getValuesCallbacks; > }; > > } // namespace WebKit >diff --git a/Tools/ChangeLog b/Tools/ChangeLog >index 81eabc75e282715dfd8c4ee93533903066283a7f..af9ba7cfffb568014a3747a22eace5caec421bde 100644 >--- a/Tools/ChangeLog >+++ b/Tools/ChangeLog >@@ -1,3 +1,16 @@ >+2019-05-22 Sihui Liu <sihui_liu@apple.com> >+ >+ API Test landed in r245540 [Mac WK2] TestWebKitAPI.WKWebView.LocalStorageProcessCrashes is a flaky failure >+ https://bugs.webkit.org/show_bug.cgi?id=198090 >+ <rdar://problem/51003644> >+ >+ Reviewed by NOBODY (OOPS!). >+ >+ WebView was wrongly loaded multiple times. >+ >+ * TestWebKitAPI/Tests/WebKitCocoa/LocalStoragePersistence.mm: >+ (TEST): >+ > 2019-05-21 Carlos Garcia Campos <cgarcia@igalia.com> > > Unreviewed. Fix the build with HAVE(ACCESSIBILITY) disabled >diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStoragePersistence.mm b/Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStoragePersistence.mm >index 0a5c70324cb7fe1533a39dfdb1dfb6d65fc052d5..02a55f490c00eeb0d40d1c72b3f0dbc6a3c88853 100644 >--- a/Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStoragePersistence.mm >+++ b/Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStoragePersistence.mm >@@ -68,21 +68,18 @@ TEST(WKWebView, LocalStorageProcessCrashes) > RetainPtr<WKWebView> webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:configuration.get()]); > NSURLRequest *request = [NSURLRequest requestWithURL:[[NSBundle mainBundle] URLForResource:@"local-storage-process-crashes" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"]]; > [webView loadRequest:request]; >- >+ > receivedScriptMessage = false; >- [webView loadRequest:request]; > TestWebKitAPI::Util::run(&receivedScriptMessage); > EXPECT_WK_STREQ(@"local:storage", [lastScriptMessage body]); >- >+ > receivedScriptMessage = false; >- [webView loadRequest:request]; > TestWebKitAPI::Util::run(&receivedScriptMessage); > EXPECT_WK_STREQ(@"session:storage", [lastScriptMessage body]); > > [configuration.get().processPool _terminateNetworkProcess]; >- >+ > receivedScriptMessage = false; >- [webView loadRequest:request]; > TestWebKitAPI::Util::run(&receivedScriptMessage); > EXPECT_WK_STREQ(@"Network Process Crashed", [lastScriptMessage body]); >
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=370424">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 198090
:
370382
|
370424
|
370430
|
370439
|
370445