<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[245649] trunk</title>
</head>
<body>

<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt;  }
#msg dl a { font-weight: bold}
#msg dl a:link    { color:#fc3; }
#msg dl a:active  { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff  {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/245649">245649</a></dd>
<dt>Author</dt> <dd>sihui_liu@apple.com</dd>
<dt>Date</dt> <dd>2019-05-22 15:03:50 -0700 (Wed, 22 May 2019)</dd>
</dl>

<h3>Log Message</h3>
<pre>API Test landed in <a href="http://trac.webkit.org/projects/webkit/changeset/245540">r245540</a> [Mac WK2] TestWebKitAPI.WKWebView.LocalStorageProcessCrashes is a flaky failure
https://bugs.webkit.org/show_bug.cgi?id=198090
<rdar://problem/51003644>

Reviewed by Youenn Fablet.

Source/WebKit:

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 <a href="http://trac.webkit.org/projects/webkit/changeset/245540">r245540</a>, 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::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:

Tools:

WebView was wrongly loaded multiple times.

* TestWebKitAPI/Tests/WebKitCocoa/LocalStoragePersistence.mm:
(TEST):</pre>

<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceWebKitChangeLog">trunk/Source/WebKit/ChangeLog</a></li>
<li><a href="#trunkSourceWebKitNetworkProcessNetworkConnectionToWebProcesscpp">trunk/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp</a></li>
<li><a href="#trunkSourceWebKitNetworkProcessNetworkProcesscpp">trunk/Source/WebKit/NetworkProcess/NetworkProcess.cpp</a></li>
<li><a href="#trunkSourceWebKitNetworkProcessWebStorageStorageManagercpp">trunk/Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp</a></li>
<li><a href="#trunkSourceWebKitNetworkProcessWebStorageStorageManagerh">trunk/Source/WebKit/NetworkProcess/WebStorage/StorageManager.h</a></li>
<li><a href="#trunkToolsChangeLog">trunk/Tools/ChangeLog</a></li>
<li><a href="#trunkToolsTestWebKitAPITestsWebKitCocoaLocalStoragePersistencemm">trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStoragePersistence.mm</a></li>
</ul>

</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceWebKitChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebKit/ChangeLog (245648 => 245649)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebKit/ChangeLog    2019-05-22 21:52:58 UTC (rev 245648)
+++ trunk/Source/WebKit/ChangeLog       2019-05-22 22:03:50 UTC (rev 245649)
</span><span class="lines">@@ -1,3 +1,46 @@
</span><ins>+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 Youenn Fablet.
+
+        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::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:
+
</ins><span class="cx"> 2019-05-22  Antoine Quint  <graouts@apple.com>
</span><span class="cx"> 
</span><span class="cx">         [iOS] Compatibility mouse events aren't prevented by calling preventDefault() on pointerdown
</span></span></pre></div>
<a id="trunkSourceWebKitNetworkProcessNetworkConnectionToWebProcesscpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp (245648 => 245649)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp     2019-05-22 21:52:58 UTC (rev 245648)
+++ trunk/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp        2019-05-22 22:03:50 UTC (rev 245649)
</span><span class="lines">@@ -227,7 +227,7 @@
</span><span class="cx"> 
</span><span class="cx">     if (decoder.messageReceiverName() == Messages::StorageManager::messageReceiverName()) {
</span><span class="cx">         if (auto* session = m_networkProcess->networkSessionByConnection(connection)) {
</span><del>-            session->storageManager().dispatchMessageToQueue(connection, decoder);
</del><ins>+            session->storageManager().didReceiveMessage(connection, decoder);
</ins><span class="cx">             return;
</span><span class="cx">         }
</span><span class="cx">     }
</span><span class="lines">@@ -273,7 +273,7 @@
</span><span class="cx"> 
</span><span class="cx">     if (decoder.messageReceiverName() == Messages::StorageManager::messageReceiverName()) {
</span><span class="cx">         if (auto* session = m_networkProcess->networkSessionByConnection(connection)) {
</span><del>-            session->storageManager().dispatchSyncMessageToQueue(connection, decoder, reply);
</del><ins>+            session->storageManager().didReceiveSyncMessage(connection, decoder, reply);
</ins><span class="cx">             return;
</span><span class="cx">         }
</span><span class="cx">     }
</span></span></pre></div>
<a id="trunkSourceWebKitNetworkProcessNetworkProcesscpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebKit/NetworkProcess/NetworkProcess.cpp (245648 => 245649)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebKit/NetworkProcess/NetworkProcess.cpp    2019-05-22 21:52:58 UTC (rev 245648)
+++ trunk/Source/WebKit/NetworkProcess/NetworkProcess.cpp       2019-05-22 22:03:50 UTC (rev 245649)
</span><span class="lines">@@ -2635,7 +2635,6 @@
</span><span class="cx"> 
</span><span class="cx">     auto connectionID = connection.uniqueID();
</span><span class="cx">     m_sessionByConnection.ensure(connectionID, [&]() {
</span><del>-        storageManager.processWillOpenConnection(connection);
</del><span class="cx">         return sessionID;
</span><span class="cx">     });
</span><span class="cx"> 
</span></span></pre></div>
<a id="trunkSourceWebKitNetworkProcessWebStorageStorageManagercpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp (245648 => 245649)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp 2019-05-22 21:52:58 UTC (rev 245648)
+++ trunk/Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp    2019-05-22 22:03:50 UTC (rev 245649)
</span><span class="lines">@@ -551,15 +551,8 @@
</span><span class="cx">     });
</span><span class="cx"> }
</span><span class="cx"> 
</span><del>-void StorageManager::processWillOpenConnection(IPC::Connection& connection)
-{
-    connection.addWorkQueueMessageReceiver(Messages::StorageManager::messageReceiverName(), m_queue.get(), this);
-}
-
</del><span class="cx"> void StorageManager::processDidCloseConnection(IPC::Connection& connection)
</span><span class="cx"> {
</span><del>-    connection.removeWorkQueueMessageReceiver(Messages::StorageManager::messageReceiverName());
-
</del><span class="cx">     m_queue->dispatch([this, protectedThis = makeRef(*this), connectionID = connection.uniqueID()]() mutable {
</span><span class="cx">         Vector<std::pair<IPC::Connection::UniqueID, uint64_t>> connectionAndStorageMapIDPairsToRemove;
</span><span class="cx">         for (auto& storageArea : m_storageAreasByConnection) {
</span><span class="lines">@@ -718,213 +711,219 @@
</span><span class="cx"> 
</span><span class="cx"> void StorageManager::createLocalStorageMap(IPC::Connection& connection, uint64_t storageMapID, uint64_t storageNamespaceID, SecurityOriginData&& securityOriginData)
</span><span class="cx"> {
</span><del>-    // 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);
</del><ins>+    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);
</ins><span class="cx"> 
</span><del>-    // FIXME: This should be a message check.
-    ASSERT((HashMap<std::pair<IPC::Connection::UniqueID, uint64_t>, RefPtr<StorageArea>>::isValidKey(connectionAndStorageMapIDPair)));
</del><ins>+        // FIXME: This should be a message check.
+        ASSERT((HashMap<std::pair<IPC::Connection::UniqueID, uint64_t>, RefPtr<StorageArea>>::isValidKey(connectionAndStorageMapIDPair)));
</ins><span class="cx"> 
</span><del>-    auto result = m_storageAreasByConnection.add(connectionAndStorageMapIDPair, nullptr);
</del><ins>+        auto result = m_storageAreasByConnection.add(connectionAndStorageMapIDPair, nullptr);
</ins><span class="cx"> 
</span><del>-    // FIXME: These should be a message checks.
-    ASSERT(result.isNewEntry);
-    ASSERT((HashMap<uint64_t, RefPtr<LocalStorageNamespace>>::isValidKey(storageNamespaceID)));
</del><ins>+        // FIXME: These should be a message checks.
+        ASSERT(result.isNewEntry);
+        ASSERT((HashMap<uint64_t, RefPtr<LocalStorageNamespace>>::isValidKey(storageNamespaceID)));
</ins><span class="cx"> 
</span><del>-    LocalStorageNamespace* localStorageNamespace = getOrCreateLocalStorageNamespace(storageNamespaceID);
</del><ins>+        LocalStorageNamespace* localStorageNamespace = getOrCreateLocalStorageNamespace(storageNamespaceID);
</ins><span class="cx"> 
</span><del>-    // FIXME: This should be a message check.
-    ASSERT(localStorageNamespace);
</del><ins>+        // FIXME: This should be a message check.
+        ASSERT(localStorageNamespace);
</ins><span class="cx"> 
</span><del>-    auto storageArea = localStorageNamespace->getOrCreateStorageArea(WTFMove(securityOriginData));
-    storageArea->addListener(connection.uniqueID(), storageMapID);
</del><ins>+        auto storageArea = localStorageNamespace->getOrCreateStorageArea(WTFMove(securityOriginData));
+        storageArea->addListener(connectionID, storageMapID);
</ins><span class="cx"> 
</span><del>-    result.iterator->value = WTFMove(storageArea);
</del><ins>+        result.iterator->value = WTFMove(storageArea);
+    });
</ins><span class="cx"> }
</span><span class="cx"> 
</span><span class="cx"> void StorageManager::createTransientLocalStorageMap(IPC::Connection& connection, uint64_t storageMapID, uint64_t storageNamespaceID, SecurityOriginData&& topLevelOriginData, SecurityOriginData&& origin)
</span><span class="cx"> {
</span><del>-    ASSERT(!RunLoop::isMain());
</del><ins>+    m_queue->dispatch([this, protectedThis = makeRef(*this), connectionID = connection.uniqueID(), storageMapID, storageNamespaceID, topLevelOriginData = topLevelOriginData.isolatedCopy(), origin = origin.isolatedCopy()]() mutable {
+        // FIXME: This should be a message check.
+        ASSERT(m_storageAreasByConnection.isValidKey({ connectionID, storageMapID }));
</ins><span class="cx"> 
</span><del>-    // FIXME: This should be a message check.
-    ASSERT(m_storageAreasByConnection.isValidKey({ connection.uniqueID(), storageMapID }));
</del><ins>+        // 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 != connectionID)
+                continue;
+            Ref<StorageArea> area = *it->value;
+            if (!area->isSessionStorage())
+                continue;
+            if (!origin.securityOrigin()->isSameSchemeHostPort(area->securityOrigin().securityOrigin().get()))
+                continue;
+            area->addListener(connectionID, 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(connectionID, it->key.second))
+                m_storageAreasByConnection.remove(it);
+            m_storageAreasByConnection.add({ connectionID, storageMapID }, WTFMove(area));
+            return;
+        }
</ins><span class="cx"> 
</span><del>-    // 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;
-    }
</del><ins>+        auto& slot = m_storageAreasByConnection.add({ connectionID, storageMapID }, nullptr).iterator->value;
</ins><span class="cx"> 
</span><del>-    auto& slot = m_storageAreasByConnection.add({ connection.uniqueID(), storageMapID }, nullptr).iterator->value;
</del><ins>+        // FIXME: This should be a message check.
+        ASSERT(!slot);
</ins><span class="cx"> 
</span><del>-    // FIXME: This should be a message check.
-    ASSERT(!slot);
</del><ins>+        auto* transientLocalStorageNamespace = getOrCreateTransientLocalStorageNamespace(storageNamespaceID, WTFMove(topLevelOriginData));
</ins><span class="cx"> 
</span><del>-    auto* transientLocalStorageNamespace = getOrCreateTransientLocalStorageNamespace(storageNamespaceID, WTFMove(topLevelOriginData));
</del><ins>+        auto storageArea = transientLocalStorageNamespace->getOrCreateStorageArea(WTFMove(origin));
+        storageArea->addListener(connectionID, storageMapID);
</ins><span class="cx"> 
</span><del>-    auto storageArea = transientLocalStorageNamespace->getOrCreateStorageArea(WTFMove(origin));
-    storageArea->addListener(connection.uniqueID(), storageMapID);
-
-    slot = WTFMove(storageArea);
</del><ins>+        slot = WTFMove(storageArea);
+    });
</ins><span class="cx"> }
</span><span class="cx"> 
</span><span class="cx"> void StorageManager::createSessionStorageMap(IPC::Connection& connection, uint64_t storageMapID, uint64_t storageNamespaceID, SecurityOriginData&& securityOriginData)
</span><span class="cx"> {
</span><del>-    ASSERT(!RunLoop::isMain());
</del><ins>+    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));
</ins><span class="cx"> 
</span><del>-    if (m_isEphemeral) {
-        m_ephemeralStorage.add(securityOriginData, WebCore::StorageMap::create(localStorageDatabaseQuotaInBytes));
-        return;
-    }
-    // FIXME: This should be a message check.
-    ASSERT(m_sessionStorageNamespaces.isValidKey(storageNamespaceID));
</del><ins>+        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;
+        }
</ins><span class="cx"> 
</span><del>-    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;
-    }
</del><ins>+        // FIXME: This should be a message check.
+        ASSERT(m_storageAreasByConnection.isValidKey({ connectionID, storageMapID }));
</ins><span class="cx"> 
</span><del>-    // FIXME: This should be a message check.
-    ASSERT(m_storageAreasByConnection.isValidKey({ connection.uniqueID(), storageMapID }));
</del><ins>+        auto& slot = m_storageAreasByConnection.add({ connectionID, storageMapID }, nullptr).iterator->value;
</ins><span class="cx"> 
</span><del>-    auto& slot = m_storageAreasByConnection.add({ connection.uniqueID(), storageMapID }, nullptr).iterator->value;
</del><ins>+        // FIXME: This should be a message check.
+        ASSERT(!slot);
</ins><span class="cx"> 
</span><del>-    // FIXME: This should be a message check.
-    ASSERT(!slot);
</del><ins>+        // FIXME: This should be a message check.
+        ASSERT(sessionStorageNamespace->allowedConnections().contains(connectionID));
</ins><span class="cx"> 
</span><del>-    // FIXME: This should be a message check.
-    ASSERT(sessionStorageNamespace->allowedConnections().contains(connection.uniqueID()));
</del><ins>+        auto storageArea = sessionStorageNamespace->getOrCreateStorageArea(WTFMove(securityOriginData));
+        storageArea->addListener(connectionID, storageMapID);
</ins><span class="cx"> 
</span><del>-    auto storageArea = sessionStorageNamespace->getOrCreateStorageArea(WTFMove(securityOriginData));
-    storageArea->addListener(connection.uniqueID(), storageMapID);
-
-    slot = WTFMove(storageArea);
</del><ins>+        slot = WTFMove(storageArea);
+    });
</ins><span class="cx"> }
</span><span class="cx"> 
</span><span class="cx"> void StorageManager::destroyStorageMap(IPC::Connection& connection, uint64_t storageMapID)
</span><span class="cx"> {
</span><del>-    ASSERT(!RunLoop::isMain());
</del><ins>+    m_queue->dispatch([this, protectedThis = makeRef(*this), connectionID = connection.uniqueID(), storageMapID]() mutable {
+        std::pair<IPC::Connection::UniqueID, uint64_t> connectionAndStorageMapIDPair(connectionID, storageMapID);
</ins><span class="cx"> 
</span><del>-    std::pair<IPC::Connection::UniqueID, uint64_t> connectionAndStorageMapIDPair(connection.uniqueID(), storageMapID);
</del><ins>+        // FIXME: This should be a message check.
+        ASSERT(m_storageAreasByConnection.isValidKey(connectionAndStorageMapIDPair));
</ins><span class="cx"> 
</span><del>-    // FIXME: This should be a message check.
-    ASSERT(m_storageAreasByConnection.isValidKey(connectionAndStorageMapIDPair));
</del><ins>+        auto it = m_storageAreasByConnection.find(connectionAndStorageMapIDPair);
+        if (it == m_storageAreasByConnection.end()) {
+            // The connection has been removed because the last page was closed.
+            return;
+        }
</ins><span class="cx"> 
</span><del>-    auto it = m_storageAreasByConnection.find(connectionAndStorageMapIDPair);
-    if (it == m_storageAreasByConnection.end()) {
-        // The connection has been removed because the last page was closed.
-        return;
-    }
</del><ins>+        it->value->removeListener(connectionID, storageMapID);
</ins><span class="cx"> 
</span><del>-    it->value->removeListener(connection.uniqueID(), storageMapID);
</del><ins>+        // Don't remove session storage maps. The web process may reconnect and expect the data to still be around.
+        if (it->value->isSessionStorage())
+            return;
</ins><span class="cx"> 
</span><del>-    // Don't remove session storage maps. The web process may reconnect and expect the data to still be around.
-    if (it->value->isSessionStorage())
-        return;
</del><ins>+        m_storageAreasByConnection.remove(connectionAndStorageMapIDPair);
+    });
+}
</ins><span class="cx"> 
</span><del>-    m_storageAreasByConnection.remove(connectionAndStorageMapIDPair);
</del><ins>+static void didGetValues(IPC::Connection& connection, uint64_t storageMapID, const HashMap<String, String>& items, GetValuesCallback&& completionHandler)
+{
+    RunLoop::main().dispatch([items = crossThreadCopy(items), completionHandler = WTFMove(completionHandler)]() mutable {
+        completionHandler(items);
+    });
</ins><span class="cx"> }
</span><span class="cx"> 
</span><del>-void StorageManager::getValues(IPC::Connection& connection, WebCore::SecurityOriginData&& securityOriginData, uint64_t storageMapID, uint64_t storageMapSeed, CompletionHandler<void(const HashMap<String, String>&)>&& completionHandler)
</del><ins>+void StorageManager::getValues(IPC::Connection& connection, WebCore::SecurityOriginData&& securityOriginData, uint64_t storageMapID, uint64_t storageMapSeed, GetValuesCallback&& completionHandler)
</ins><span class="cx"> {
</span><del>-    ASSERT(!RunLoop::isMain());
-
-    auto* storageArea = findStorageArea(connection, storageMapID);
-    if (!storageArea) {
-        if (m_isEphemeral) {
-            if (auto storageMap = m_ephemeralStorage.get(securityOriginData))
-                return completionHandler(storageMap->items());
</del><ins>+    m_queue->dispatch([this, protectedThis = makeRef(*this), connection = makeRef(connection), securityOriginData = securityOriginData.isolatedCopy(), storageMapID, storageMapSeed, completionHandler = WTFMove(completionHandler)]() 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(), WTFMove(completionHandler));
+            }
+            // This is a session storage area for a page that has already been closed. Ignore it.
+            return didGetValues(connection.get(), storageMapID, { }, WTFMove(completionHandler));
</ins><span class="cx">         }
</span><del>-        // 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);
</del><ins>+        didGetValues(connection.get(), storageMapID, storageArea->items(), WTFMove(completionHandler));
+        connection->send(Messages::StorageAreaMap::DidGetValues(storageMapSeed), storageMapID);
+    });
</ins><span class="cx"> }
</span><span class="cx"> 
</span><span class="cx"> 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)
</span><span class="cx"> {
</span><del>-    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);
</del><ins>+    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);
+                }
</ins><span class="cx">             }
</span><ins>+            // This is a session storage area for a page that has already been closed. Ignore it.
+            return;
</ins><span class="cx">         }
</span><del>-        // This is a session storage area for a page that has already been closed. Ignore it.
-        return;
-    }
</del><span class="cx"> 
</span><del>-    bool quotaError;
-    storageArea->setItem(connection.uniqueID(), sourceStorageAreaID, key, value, urlString, quotaError);
-    connection.send(Messages::StorageAreaMap::DidSetItem(storageMapSeed, key, quotaError), storageMapID);
</del><ins>+        bool quotaError;
+        storageArea->setItem(connection->uniqueID(), sourceStorageAreaID, key, value, urlString, quotaError);
+        connection->send(Messages::StorageAreaMap::DidSetItem(storageMapSeed, key, quotaError), storageMapID);
+    });
</ins><span class="cx"> }
</span><span class="cx"> 
</span><span class="cx"> void StorageManager::setItems(IPC::Connection& connection, uint64_t storageMapID, const HashMap<String, String>& items)
</span><span class="cx"> {
</span><del>-    ASSERT(!RunLoop::isMain());
-
-    if (auto* storageArea = findStorageArea(connection, storageMapID))
-        storageArea->setItems(items);
</del><ins>+    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);
+    });
</ins><span class="cx"> }
</span><span class="cx"> 
</span><span class="cx"> void StorageManager::removeItem(IPC::Connection& connection, WebCore::SecurityOriginData&& securityOriginData, uint64_t storageMapID, uint64_t sourceStorageAreaID, uint64_t storageMapSeed, const String& key, const String& urlString)
</span><span class="cx"> {
</span><del>-    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);
</del><ins>+    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);
+                }
</ins><span class="cx">             }
</span><ins>+            // This is a session storage area for a page that has already been closed. Ignore it.
+            return;
</ins><span class="cx">         }
</span><del>-        // This is a session storage area for a page that has already been closed. Ignore it.
-        return;
-    }
</del><span class="cx"> 
</span><del>-    storageArea->removeItem(connection.uniqueID(), sourceStorageAreaID, key, urlString);
-    connection.send(Messages::StorageAreaMap::DidRemoveItem(storageMapSeed, key), storageMapID);
</del><ins>+        storageArea->removeItem(connection->uniqueID(), sourceStorageAreaID, key, urlString);
+        connection->send(Messages::StorageAreaMap::DidRemoveItem(storageMapSeed, key), storageMapID);
+    });
</ins><span class="cx"> }
</span><span class="cx"> 
</span><span class="cx"> void StorageManager::clear(IPC::Connection& connection, WebCore::SecurityOriginData&& securityOriginData, uint64_t storageMapID, uint64_t sourceStorageAreaID, uint64_t storageMapSeed, const String& urlString)
</span><span class="cx"> {
</span><del>-    ASSERT(!RunLoop::isMain());
</del><ins>+    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;
+        }
</ins><span class="cx"> 
</span><del>-    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;
-    }
-
-    storageArea->clear(connection.uniqueID(), sourceStorageAreaID, urlString);
-    connection.send(Messages::StorageAreaMap::DidClear(storageMapSeed), storageMapID);
</del><ins>+        storageArea->clear(connection->uniqueID(), sourceStorageAreaID, urlString);
+        connection->send(Messages::StorageAreaMap::DidClear(storageMapSeed), storageMapID);
+    });
</ins><span class="cx"> }
</span><span class="cx"> 
</span><span class="cx"> void StorageManager::waitUntilWritesFinished()
</span><span class="lines">@@ -975,18 +974,4 @@
</span><span class="cx">     }).iterator->value.get();
</span><span class="cx"> }
</span><span class="cx"> 
</span><del>-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);
-    });
-}
-
</del><span class="cx"> } // namespace WebKit
</span></span></pre></div>
<a id="trunkSourceWebKitNetworkProcessWebStorageStorageManagerh"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebKit/NetworkProcess/WebStorage/StorageManager.h (245648 => 245649)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebKit/NetworkProcess/WebStorage/StorageManager.h   2019-05-22 21:52:58 UTC (rev 245648)
+++ trunk/Source/WebKit/NetworkProcess/WebStorage/StorageManager.h      2019-05-22 22:03:50 UTC (rev 245649)
</span><span class="lines">@@ -43,6 +43,8 @@
</span><span class="cx"> class LocalStorageDatabaseTracker;
</span><span class="cx"> class WebProcessProxy;
</span><span class="cx"> 
</span><ins>+using GetValuesCallback = CompletionHandler<void(const HashMap<String, String>&)>;
+
</ins><span class="cx"> class StorageManager : public IPC::Connection::WorkQueueMessageReceiver {
</span><span class="cx"> public:
</span><span class="cx">     static Ref<StorageManager> create(const String& localStorageDirectory);
</span><span class="lines">@@ -54,7 +56,6 @@
</span><span class="cx">     void removeAllowedSessionStorageNamespaceConnection(uint64_t storageNamespaceID, IPC::Connection&);
</span><span class="cx">     void cloneSessionStorageNamespace(uint64_t storageNamespaceID, uint64_t newStorageNamespaceID);
</span><span class="cx"> 
</span><del>-    void processWillOpenConnection(IPC::Connection&);
</del><span class="cx">     void processDidCloseConnection(IPC::Connection&);
</span><span class="cx">     void waitUntilWritesFinished();
</span><span class="cx"> 
</span><span class="lines">@@ -70,16 +71,13 @@
</span><span class="cx"> 
</span><span class="cx">     void getLocalStorageOriginDetails(Function<void(Vector<LocalStorageDatabaseTracker::OriginDetails>&&)>&& completionHandler);
</span><span class="cx"> 
</span><del>-    void dispatchMessageToQueue(IPC::Connection&, IPC::Decoder&);
-    void dispatchSyncMessageToQueue(IPC::Connection&, IPC::Decoder&, std::unique_ptr<IPC::Encoder>& replyEncoder);
</del><ins>+    // IPC::Connection::WorkQueueMessageReceiver.
+    void didReceiveMessage(IPC::Connection&, IPC::Decoder&) override;
+    void didReceiveSyncMessage(IPC::Connection&, IPC::Decoder&, std::unique_ptr<IPC::Encoder>& replyEncoder) override;
</ins><span class="cx"> 
</span><span class="cx"> private:
</span><span class="cx">     explicit StorageManager(const String& localStorageDirectory);
</span><span class="cx"> 
</span><del>-    // IPC::Connection::WorkQueueMessageReceiver.
-    void didReceiveMessage(IPC::Connection&, IPC::Decoder&) override;
-    void didReceiveSyncMessage(IPC::Connection&, IPC::Decoder&, std::unique_ptr<IPC::Encoder>& replyEncoder) override;
-
</del><span class="cx">     // Message handlers.
</span><span class="cx">     void createLocalStorageMap(IPC::Connection&, uint64_t storageMapID, uint64_t storageNamespaceID, WebCore::SecurityOriginData&&);
</span><span class="cx">     void createTransientLocalStorageMap(IPC::Connection&, uint64_t storageMapID, uint64_t storageNamespaceID, WebCore::SecurityOriginData&& topLevelOriginData, WebCore::SecurityOriginData&&);
</span><span class="lines">@@ -86,7 +84,7 @@
</span><span class="cx">     void createSessionStorageMap(IPC::Connection&, uint64_t storageMapID, uint64_t storageNamespaceID, WebCore::SecurityOriginData&&);
</span><span class="cx">     void destroyStorageMap(IPC::Connection&, uint64_t storageMapID);
</span><span class="cx"> 
</span><del>-    void getValues(IPC::Connection&, WebCore::SecurityOriginData&&, uint64_t storageMapID, uint64_t storageMapSeed, CompletionHandler<void(const HashMap<String, String>&)>&&);
</del><ins>+    void getValues(IPC::Connection&, WebCore::SecurityOriginData&&, uint64_t storageMapID, uint64_t storageMapSeed, GetValuesCallback&&);
</ins><span class="cx">     void setItem(IPC::Connection&, WebCore::SecurityOriginData&&, uint64_t storageMapID, uint64_t sourceStorageAreaID, uint64_t storageMapSeed, const String& key, const String& value, const String& urlString);
</span><span class="cx">     void setItems(IPC::Connection&, uint64_t storageMapID, const HashMap<String, String>& items);
</span><span class="cx">     void removeItem(IPC::Connection&, WebCore::SecurityOriginData&&, uint64_t storageMapID, uint64_t sourceStorageAreaID, uint64_t storageMapSeed, const String& key, const String& urlString);
</span><span class="lines">@@ -115,7 +113,6 @@
</span><span class="cx"> 
</span><span class="cx">     HashMap<WebCore::SecurityOriginData, Ref<WebCore::StorageMap>> m_ephemeralStorage;
</span><span class="cx">     bool m_isEphemeral { false };
</span><del>-
</del><span class="cx"> };
</span><span class="cx"> 
</span><span class="cx"> } // namespace WebKit
</span></span></pre></div>
<a id="trunkToolsChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Tools/ChangeLog (245648 => 245649)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Tools/ChangeLog    2019-05-22 21:52:58 UTC (rev 245648)
+++ trunk/Tools/ChangeLog       2019-05-22 22:03:50 UTC (rev 245649)
</span><span class="lines">@@ -1,3 +1,16 @@
</span><ins>+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 Youenn Fablet.
+
+        WebView was wrongly loaded multiple times.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/LocalStoragePersistence.mm:
+        (TEST):
+
</ins><span class="cx"> 2019-05-22  Jiewen Tan  <jiewen_tan@apple.com>
</span><span class="cx"> 
</span><span class="cx">         [WebAuthN] Support Attestation Conveyance Preference
</span></span></pre></div>
<a id="trunkToolsTestWebKitAPITestsWebKitCocoaLocalStoragePersistencemm"></a>
<div class="modfile"><h4>Modified: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStoragePersistence.mm (245648 => 245649)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStoragePersistence.mm   2019-05-22 21:52:58 UTC (rev 245648)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStoragePersistence.mm      2019-05-22 22:03:50 UTC (rev 245649)
</span><span class="lines">@@ -68,21 +68,18 @@
</span><span class="cx">     RetainPtr<WKWebView> webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:configuration.get()]);
</span><span class="cx">     NSURLRequest *request = [NSURLRequest requestWithURL:[[NSBundle mainBundle] URLForResource:@"local-storage-process-crashes" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"]];
</span><span class="cx">     [webView loadRequest:request];
</span><del>-    
</del><ins>+
</ins><span class="cx">     receivedScriptMessage = false;
</span><del>-    [webView loadRequest:request];
</del><span class="cx">     TestWebKitAPI::Util::run(&receivedScriptMessage);
</span><span class="cx">     EXPECT_WK_STREQ(@"local:storage", [lastScriptMessage body]);
</span><del>-    
</del><ins>+
</ins><span class="cx">     receivedScriptMessage = false;
</span><del>-    [webView loadRequest:request];
</del><span class="cx">     TestWebKitAPI::Util::run(&receivedScriptMessage);
</span><span class="cx">     EXPECT_WK_STREQ(@"session:storage", [lastScriptMessage body]);
</span><span class="cx"> 
</span><span class="cx">     [configuration.get().processPool _terminateNetworkProcess];
</span><del>-    
</del><ins>+
</ins><span class="cx">     receivedScriptMessage = false;
</span><del>-    [webView loadRequest:request];
</del><span class="cx">     TestWebKitAPI::Util::run(&receivedScriptMessage);
</span><span class="cx">     EXPECT_WK_STREQ(@"Network Process Crashed", [lastScriptMessage body]);
</span><span class="cx"> 
</span></span></pre>
</div>
</div>

</body>
</html>