Changeset 245649 in webkit


Ignore:
Timestamp:
May 22, 2019 3:03:50 PM (5 years ago)
Author:
sihui_liu@apple.com
Message:

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.

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 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:

Tools:

WebView was wrongly loaded multiple times.

  • TestWebKitAPI/Tests/WebKitCocoa/LocalStoragePersistence.mm:

(TEST):

Location:
trunk
Files:
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebKit/ChangeLog

    r245639 r245649  
     12019-05-22  Sihui Liu  <sihui_liu@apple.com>
     2
     3        API Test landed in r245540 [Mac WK2] TestWebKitAPI.WKWebView.LocalStorageProcessCrashes is a flaky failure
     4        https://bugs.webkit.org/show_bug.cgi?id=198090
     5        <rdar://problem/51003644>
     6
     7        Reviewed by Youenn Fablet.
     8
     9        We used to dispatch StorageManager message to StorageManager's work queue, which required message handler to be
     10        added to queue before receiving first StorageManager message, otherwise network process would not know how to
     11        decode the message.
     12
     13        After r245540, when netork process crashes and dom storage is accessed from web process after that, web process
     14        re-establishes its connection to network process, asks network process to add message handler, and then sends
     15        StorageManager message immediately. Because work queue message receiver is added on a background thread in
     16        network process, it is possible the StorageManager message is received before that.
     17
     18        A safe and easy resolution is to not dispatch StorageManager message to work queue, so that we don't need to
     19        wait for the message receiver to be added. Handling message on the main thread also allows us to untying the
     20        knot that binds StorageManager and connection, which may be a preferred design in the future.
     21
     22        * NetworkProcess/NetworkConnectionToWebProcess.cpp:
     23        (WebKit::NetworkConnectionToWebProcess::didReceiveMessage):
     24        (WebKit::NetworkConnectionToWebProcess::didReceiveSyncMessage):
     25        * NetworkProcess/NetworkProcess.cpp:
     26        (WebKit::NetworkProcess::webPageWasAdded):
     27        * NetworkProcess/WebStorage/StorageManager.cpp:
     28        (WebKit::StorageManager::processDidCloseConnection):
     29        (WebKit::StorageManager::createLocalStorageMap):
     30        (WebKit::StorageManager::createTransientLocalStorageMap):
     31        (WebKit::StorageManager::createSessionStorageMap):
     32        (WebKit::StorageManager::destroyStorageMap):
     33        (WebKit::didGetValues):
     34        (WebKit::StorageManager::getValues):
     35        (WebKit::StorageManager::setItem):
     36        (WebKit::StorageManager::setItems):
     37        (WebKit::StorageManager::removeItem):
     38        (WebKit::StorageManager::clear):
     39        (WebKit::StorageManager::processWillOpenConnection): Deleted.
     40        (WebKit::StorageManager::dispatchMessageToQueue): Deleted.
     41        (WebKit::StorageManager::dispatchSyncMessageToQueue): Deleted.
     42        * NetworkProcess/WebStorage/StorageManager.h:
     43
    1442019-05-22  Antoine Quint  <graouts@apple.com>
    245

  • trunk/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp

    r245540 r245649  
    228228    if (decoder.messageReceiverName() == Messages::StorageManager::messageReceiverName()) {
    229229        if (auto* session = m_networkProcess->networkSessionByConnection(connection)) {
    230             session->storageManager().dispatchMessageToQueue(connection, decoder);
     230            session->storageManager().didReceiveMessage(connection, decoder);
    231231            return;
    232232        }
    … …  
    274274    if (decoder.messageReceiverName() == Messages::StorageManager::messageReceiverName()) {
    275275        if (auto* session = m_networkProcess->networkSessionByConnection(connection)) {
    276             session->storageManager().dispatchSyncMessageToQueue(connection, decoder, reply);
     276            session->storageManager().didReceiveSyncMessage(connection, decoder, reply);
    277277            return;
    278278        }

  • trunk/Source/WebKit/NetworkProcess/NetworkProcess.cpp

    r245540 r245649  
    26362636    auto connectionID = connection.uniqueID();
    26372637    m_sessionByConnection.ensure(connectionID, [&]() {
    2638         storageManager.processWillOpenConnection(connection);
    26392638        return sessionID;
    26402639    });

  • trunk/Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp

    r245540 r245649  
    552552}
    553553
    554 void StorageManager::processWillOpenConnection(IPC::Connection& connection)
    555 {
    556     connection.addWorkQueueMessageReceiver(Messages::StorageManager::messageReceiverName(), m_queue.get(), this);
    557 }
    558 
    559554void StorageManager::processDidCloseConnection(IPC::Connection& connection)
    560555{
    561     connection.removeWorkQueueMessageReceiver(Messages::StorageManager::messageReceiverName());
    562 
    563556    m_queue->dispatch([this, protectedThis = makeRef(*this), connectionID = connection.uniqueID()]() mutable {
    564557        Vector<std::pair<IPC::Connection::UniqueID, uint64_t>> connectionAndStorageMapIDPairsToRemove;
    … …  
    719712void StorageManager::createLocalStorageMap(IPC::Connection& connection, uint64_t storageMapID, uint64_t storageNamespaceID, SecurityOriginData&& securityOriginData)
    720713{
    721     // FIXME: Replace this check if https://bugs.webkit.org/show_bug.cgi?id=198048 is done.
    722     ASSERT(!RunLoop::isMain());
    723     ASSERT(!m_isEphemeral);
    724     std::pair<IPC::Connection::UniqueID, uint64_t> connectionAndStorageMapIDPair(connection.uniqueID(), storageMapID);
    725 
    726     // FIXME: This should be a message check.
    727     ASSERT((HashMap<std::pair<IPC::Connection::UniqueID, uint64_t>, RefPtr<StorageArea>>::isValidKey(connectionAndStorageMapIDPair)));
    728 
    729     auto result = m_storageAreasByConnection.add(connectionAndStorageMapIDPair, nullptr);
    730 
    731     // FIXME: These should be a message checks.
    732     ASSERT(result.isNewEntry);
    733     ASSERT((HashMap<uint64_t, RefPtr<LocalStorageNamespace>>::isValidKey(storageNamespaceID)));
    734 
    735     LocalStorageNamespace* localStorageNamespace = getOrCreateLocalStorageNamespace(storageNamespaceID);
    736 
    737     // FIXME: This should be a message check.
    738     ASSERT(localStorageNamespace);
    739 
    740     auto storageArea = localStorageNamespace->getOrCreateStorageArea(WTFMove(securityOriginData));
    741     storageArea->addListener(connection.uniqueID(), storageMapID);
    742 
    743     result.iterator->value = WTFMove(storageArea);
     714    m_queue->dispatch([this, protectedThis = makeRef(*this), connectionID = connection.uniqueID(), storageMapID, storageNamespaceID, securityOriginData = securityOriginData.isolatedCopy()]() mutable {
     715        ASSERT(!m_isEphemeral);
     716        std::pair<IPC::Connection::UniqueID, uint64_t> connectionAndStorageMapIDPair(connectionID, storageMapID);
     717
     718        // FIXME: This should be a message check.
     719        ASSERT((HashMap<std::pair<IPC::Connection::UniqueID, uint64_t>, RefPtr<StorageArea>>::isValidKey(connectionAndStorageMapIDPair)));
     720
     721        auto result = m_storageAreasByConnection.add(connectionAndStorageMapIDPair, nullptr);
     722
     723        // FIXME: These should be a message checks.
     724        ASSERT(result.isNewEntry);
     725        ASSERT((HashMap<uint64_t, RefPtr<LocalStorageNamespace>>::isValidKey(storageNamespaceID)));
     726
     727        LocalStorageNamespace* localStorageNamespace = getOrCreateLocalStorageNamespace(storageNamespaceID);
     728
     729        // FIXME: This should be a message check.
     730        ASSERT(localStorageNamespace);
     731
     732        auto storageArea = localStorageNamespace->getOrCreateStorageArea(WTFMove(securityOriginData));
     733        storageArea->addListener(connectionID, storageMapID);
     734
     735        result.iterator->value = WTFMove(storageArea);
     736    });
    744737}
    745738
    746739void StorageManager::createTransientLocalStorageMap(IPC::Connection& connection, uint64_t storageMapID, uint64_t storageNamespaceID, SecurityOriginData&& topLevelOriginData, SecurityOriginData&& origin)
    747740{
    748     ASSERT(!RunLoop::isMain());
    749 
    750     // FIXME: This should be a message check.
    751     ASSERT(m_storageAreasByConnection.isValidKey({ connection.uniqueID(), storageMapID }));
    752 
    753     // See if we already have session storage for this connection/origin combo.
    754     // If so, update the map with the new ID, otherwise keep on trucking.
    755     for (auto it = m_storageAreasByConnection.begin(), end = m_storageAreasByConnection.end(); it != end; ++it) {
    756         if (it->key.first != connection.uniqueID())
    757             continue;
    758         Ref<StorageArea> area = *it->value;
    759         if (!area->isSessionStorage())
    760             continue;
    761         if (!origin.securityOrigin()->isSameSchemeHostPort(area->securityOrigin().securityOrigin().get()))
    762             continue;
    763         area->addListener(connection.uniqueID(), storageMapID);
    764         // If the storageMapID used as key in m_storageAreasByConnection is no longer one of the StorageArea's listeners, then this means
    765         // that destroyStorageMap() was already called for that storageMapID but it decided not to remove it from m_storageAreasByConnection
    766         // so that we could reuse it later on for the same connection/origin combo. In this case, it is safe to remove the previous
    767         // storageMapID from m_storageAreasByConnection.
    768         if (!area->hasListener(connection.uniqueID(), it->key.second))
    769             m_storageAreasByConnection.remove(it);
    770         m_storageAreasByConnection.add({ connection.uniqueID(), storageMapID }, WTFMove(area));
    771         return;
    772     }
    773 
    774     auto& slot = m_storageAreasByConnection.add({ connection.uniqueID(), storageMapID }, nullptr).iterator->value;
    775 
    776     // FIXME: This should be a message check.
    777     ASSERT(!slot);
    778 
    779     auto* transientLocalStorageNamespace = getOrCreateTransientLocalStorageNamespace(storageNamespaceID, WTFMove(topLevelOriginData));
    780 
    781     auto storageArea = transientLocalStorageNamespace->getOrCreateStorageArea(WTFMove(origin));
    782     storageArea->addListener(connection.uniqueID(), storageMapID);
    783 
    784     slot = WTFMove(storageArea);
     741    m_queue->dispatch([this, protectedThis = makeRef(*this), connectionID = connection.uniqueID(), storageMapID, storageNamespaceID, topLevelOriginData = topLevelOriginData.isolatedCopy(), origin = origin.isolatedCopy()]() mutable {
     742        // FIXME: This should be a message check.
     743        ASSERT(m_storageAreasByConnection.isValidKey({ connectionID, storageMapID }));
     744
     745        // See if we already have session storage for this connection/origin combo.
     746        // If so, update the map with the new ID, otherwise keep on trucking.
     747        for (auto it = m_storageAreasByConnection.begin(), end = m_storageAreasByConnection.end(); it != end; ++it) {
     748            if (it->key.first != connectionID)
     749                continue;
     750            Ref<StorageArea> area = *it->value;
     751            if (!area->isSessionStorage())
     752                continue;
     753            if (!origin.securityOrigin()->isSameSchemeHostPort(area->securityOrigin().securityOrigin().get()))
     754                continue;
     755            area->addListener(connectionID, storageMapID);
     756            // If the storageMapID used as key in m_storageAreasByConnection is no longer one of the StorageArea's listeners, then this means
     757            // that destroyStorageMap() was already called for that storageMapID but it decided not to remove it from m_storageAreasByConnection
     758            // so that we could reuse it later on for the same connection/origin combo. In this case, it is safe to remove the previous
     759            // storageMapID from m_storageAreasByConnection.
     760            if (!area->hasListener(connectionID, it->key.second))
     761                m_storageAreasByConnection.remove(it);
     762            m_storageAreasByConnection.add({ connectionID, storageMapID }, WTFMove(area));
     763            return;
     764        }
     765
     766        auto& slot = m_storageAreasByConnection.add({ connectionID, storageMapID }, nullptr).iterator->value;
     767
     768        // FIXME: This should be a message check.
     769        ASSERT(!slot);
     770
     771        auto* transientLocalStorageNamespace = getOrCreateTransientLocalStorageNamespace(storageNamespaceID, WTFMove(topLevelOriginData));
     772
     773        auto storageArea = transientLocalStorageNamespace->getOrCreateStorageArea(WTFMove(origin));
     774        storageArea->addListener(connectionID, storageMapID);
     775
     776        slot = WTFMove(storageArea);
     777    });
    785778}
    786779
    787780void StorageManager::createSessionStorageMap(IPC::Connection& connection, uint64_t storageMapID, uint64_t storageNamespaceID, SecurityOriginData&& securityOriginData)
    788781{
    789     ASSERT(!RunLoop::isMain());
    790 
    791     if (m_isEphemeral) {
    792         m_ephemeralStorage.add(securityOriginData, WebCore::StorageMap::create(localStorageDatabaseQuotaInBytes));
    793         return;
    794     }
    795     // FIXME: This should be a message check.
    796     ASSERT(m_sessionStorageNamespaces.isValidKey(storageNamespaceID));
    797 
    798     SessionStorageNamespace* sessionStorageNamespace = m_sessionStorageNamespaces.get(storageNamespaceID);
    799     if (!sessionStorageNamespace) {
    800         // We're getting an incoming message from the web process that's for session storage for a web page
    801         // that has already been closed, just ignore it.
    802         return;
    803     }
    804 
    805     // FIXME: This should be a message check.
    806     ASSERT(m_storageAreasByConnection.isValidKey({ connection.uniqueID(), storageMapID }));
    807 
    808     auto& slot = m_storageAreasByConnection.add({ connection.uniqueID(), storageMapID }, nullptr).iterator->value;
    809 
    810     // FIXME: This should be a message check.
    811     ASSERT(!slot);
    812 
    813     // FIXME: This should be a message check.
    814     ASSERT(sessionStorageNamespace->allowedConnections().contains(connection.uniqueID()));
    815 
    816     auto storageArea = sessionStorageNamespace->getOrCreateStorageArea(WTFMove(securityOriginData));
    817     storageArea->addListener(connection.uniqueID(), storageMapID);
    818 
    819     slot = WTFMove(storageArea);
     782    m_queue->dispatch([this, protectedThis = makeRef(*this), connectionID = connection.uniqueID(), storageMapID, storageNamespaceID, securityOriginData = securityOriginData.isolatedCopy()]() mutable {
     783        if (m_isEphemeral) {
     784            m_ephemeralStorage.add(securityOriginData, WebCore::StorageMap::create(localStorageDatabaseQuotaInBytes));
     785            return;
     786        }
     787        // FIXME: This should be a message check.
     788        ASSERT(m_sessionStorageNamespaces.isValidKey(storageNamespaceID));
     789
     790        SessionStorageNamespace* sessionStorageNamespace = m_sessionStorageNamespaces.get(storageNamespaceID);
     791        if (!sessionStorageNamespace) {
     792            // We're getting an incoming message from the web process that's for session storage for a web page
     793            // that has already been closed, just ignore it.
     794            return;
     795        }
     796
     797        // FIXME: This should be a message check.
     798        ASSERT(m_storageAreasByConnection.isValidKey({ connectionID, storageMapID }));
     799
     800        auto& slot = m_storageAreasByConnection.add({ connectionID, storageMapID }, nullptr).iterator->value;
     801
     802        // FIXME: This should be a message check.
     803        ASSERT(!slot);
     804
     805        // FIXME: This should be a message check.
     806        ASSERT(sessionStorageNamespace->allowedConnections().contains(connectionID));
     807
     808        auto storageArea = sessionStorageNamespace->getOrCreateStorageArea(WTFMove(securityOriginData));
     809        storageArea->addListener(connectionID, storageMapID);
     810
     811        slot = WTFMove(storageArea);
     812    });
    820813}
    821814
    822815void StorageManager::destroyStorageMap(IPC::Connection& connection, uint64_t storageMapID)
    823816{
    824     ASSERT(!RunLoop::isMain());
    825 
    826     std::pair<IPC::Connection::UniqueID, uint64_t> connectionAndStorageMapIDPair(connection.uniqueID(), storageMapID);
    827 
    828     // FIXME: This should be a message check.
    829     ASSERT(m_storageAreasByConnection.isValidKey(connectionAndStorageMapIDPair));
    830 
    831     auto it = m_storageAreasByConnection.find(connectionAndStorageMapIDPair);
    832     if (it == m_storageAreasByConnection.end()) {
    833         // The connection has been removed because the last page was closed.
    834         return;
    835     }
    836 
    837     it->value->removeListener(connection.uniqueID(), storageMapID);
    838 
    839     // Don't remove session storage maps. The web process may reconnect and expect the data to still be around.
    840     if (it->value->isSessionStorage())
    841         return;
    842 
    843     m_storageAreasByConnection.remove(connectionAndStorageMapIDPair);
    844 }
    845 
    846 void StorageManager::getValues(IPC::Connection& connection, WebCore::SecurityOriginData&& securityOriginData, uint64_t storageMapID, uint64_t storageMapSeed, CompletionHandler<void(const HashMap<String, String>&)>&& completionHandler)
    847 {
    848     ASSERT(!RunLoop::isMain());
    849 
    850     auto* storageArea = findStorageArea(connection, storageMapID);
    851     if (!storageArea) {
    852         if (m_isEphemeral) {
    853             if (auto storageMap = m_ephemeralStorage.get(securityOriginData))
    854                 return completionHandler(storageMap->items());
    855         }
    856         // This is a session storage area for a page that has already been closed. Ignore it.
    857         return completionHandler({ });
    858     }
    859 
    860     completionHandler(storageArea->items());
    861     connection.send(Messages::StorageAreaMap::DidGetValues(storageMapSeed), storageMapID);
     817    m_queue->dispatch([this, protectedThis = makeRef(*this), connectionID = connection.uniqueID(), storageMapID]() mutable {
     818        std::pair<IPC::Connection::UniqueID, uint64_t> connectionAndStorageMapIDPair(connectionID, storageMapID);
     819
     820        // FIXME: This should be a message check.
     821        ASSERT(m_storageAreasByConnection.isValidKey(connectionAndStorageMapIDPair));
     822
     823        auto it = m_storageAreasByConnection.find(connectionAndStorageMapIDPair);
     824        if (it == m_storageAreasByConnection.end()) {
     825            // The connection has been removed because the last page was closed.
     826            return;
     827        }
     828
     829        it->value->removeListener(connectionID, storageMapID);
     830
     831        // Don't remove session storage maps. The web process may reconnect and expect the data to still be around.
     832        if (it->value->isSessionStorage())
     833            return;
     834
     835        m_storageAreasByConnection.remove(connectionAndStorageMapIDPair);
     836    });
     837}
     838
     839static void didGetValues(IPC::Connection& connection, uint64_t storageMapID, const HashMap<String, String>& items, GetValuesCallback&& completionHandler)
     840{
     841    RunLoop::main().dispatch([items = crossThreadCopy(items), completionHandler = WTFMove(completionHandler)]() mutable {
     842        completionHandler(items);
     843    });
     844}
     845
     846void StorageManager::getValues(IPC::Connection& connection, WebCore::SecurityOriginData&& securityOriginData, uint64_t storageMapID, uint64_t storageMapSeed, GetValuesCallback&& completionHandler)
     847{
     848    m_queue->dispatch([this, protectedThis = makeRef(*this), connection = makeRef(connection), securityOriginData = securityOriginData.isolatedCopy(), storageMapID, storageMapSeed, completionHandler = WTFMove(completionHandler)]() mutable {
     849        auto* storageArea = findStorageArea(connection.get(), storageMapID);
     850        if (!storageArea) {
     851            if (m_isEphemeral) {
     852                if (auto storageMap = m_ephemeralStorage.get(securityOriginData))
     853                    return didGetValues(connection.get(), storageMapID, storageMap->items(), WTFMove(completionHandler));
     854            }
     855            // This is a session storage area for a page that has already been closed. Ignore it.
     856            return didGetValues(connection.get(), storageMapID, { }, WTFMove(completionHandler));
     857        }
     858        didGetValues(connection.get(), storageMapID, storageArea->items(), WTFMove(completionHandler));
     859        connection->send(Messages::StorageAreaMap::DidGetValues(storageMapSeed), storageMapID);
     860    });
    862861}
    863862
    864863void 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)
    865864{
    866     ASSERT(!RunLoop::isMain());
    867 
    868     auto* storageArea = findStorageArea(connection, storageMapID);
    869     if (!storageArea) {
    870         if (m_isEphemeral) {
    871             if (auto storageMap = m_ephemeralStorage.get(securityOriginData)) {
    872                 String oldValue;
    873                 bool quotaException;
    874                 storageMap->setItem(key, value, oldValue, quotaException);
     865    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 {
     866        auto* storageArea = findStorageArea(connection.get(), storageMapID);
     867        if (!storageArea) {
     868            if (m_isEphemeral) {
     869                if (auto storageMap = m_ephemeralStorage.get(securityOriginData)) {
     870                    String oldValue;
     871                    bool quotaException;
     872                    storageMap->setItem(key, value, oldValue, quotaException);
     873                }
    875874            }
    876         }
    877         // This is a session storage area for a page that has already been closed. Ignore it.
    878         return;
    879     }
    880 
    881     bool quotaError;
    882     storageArea->setItem(connection.uniqueID(), sourceStorageAreaID, key, value, urlString, quotaError);
    883     connection.send(Messages::StorageAreaMap::DidSetItem(storageMapSeed, key, quotaError), storageMapID);
     875            // This is a session storage area for a page that has already been closed. Ignore it.
     876            return;
     877        }
     878
     879        bool quotaError;
     880        storageArea->setItem(connection->uniqueID(), sourceStorageAreaID, key, value, urlString, quotaError);
     881        connection->send(Messages::StorageAreaMap::DidSetItem(storageMapSeed, key, quotaError), storageMapID);
     882    });
    884883}
    885884
    886885void StorageManager::setItems(IPC::Connection& connection, uint64_t storageMapID, const HashMap<String, String>& items)
    887886{
    888     ASSERT(!RunLoop::isMain());
    889 
    890     if (auto* storageArea = findStorageArea(connection, storageMapID))
    891         storageArea->setItems(items);
     887    m_queue->dispatch([this, protectedThis = makeRef(*this), connection = makeRef(connection), storageMapID, items = crossThreadCopy(items)]() mutable {
     888        if (auto* storageArea = findStorageArea(connection.get(), storageMapID))
     889            storageArea->setItems(items);
     890    });
    892891}
    893892
    894893void StorageManager::removeItem(IPC::Connection& connection, WebCore::SecurityOriginData&& securityOriginData, uint64_t storageMapID, uint64_t sourceStorageAreaID, uint64_t storageMapSeed, const String& key, const String& urlString)
    895894{
    896     ASSERT(!RunLoop::isMain());
    897 
    898     auto* storageArea = findStorageArea(connection, storageMapID);
    899     if (!storageArea) {
    900         if (m_isEphemeral) {
    901             if (auto storageMap = m_ephemeralStorage.get(securityOriginData)) {
    902                 String oldValue;
    903                 storageMap->removeItem(key, oldValue);
     895    m_queue->dispatch([this, protectedThis = makeRef(*this), connection = makeRef(connection), securityOriginData = securityOriginData.isolatedCopy(), storageMapID, sourceStorageAreaID, storageMapSeed, key = key.isolatedCopy(), urlString = urlString.isolatedCopy()]() mutable {
     896        auto* storageArea = findStorageArea(connection.get(), storageMapID);
     897        if (!storageArea) {
     898            if (m_isEphemeral) {
     899                if (auto storageMap = m_ephemeralStorage.get(securityOriginData)) {
     900                    String oldValue;
     901                    storageMap->removeItem(key, oldValue);
     902                }
    904903            }
    905         }
    906         // This is a session storage area for a page that has already been closed. Ignore it.
    907         return;
    908     }
    909 
    910     storageArea->removeItem(connection.uniqueID(), sourceStorageAreaID, key, urlString);
    911     connection.send(Messages::StorageAreaMap::DidRemoveItem(storageMapSeed, key), storageMapID);
     904            // This is a session storage area for a page that has already been closed. Ignore it.
     905            return;
     906        }
     907
     908        storageArea->removeItem(connection->uniqueID(), sourceStorageAreaID, key, urlString);
     909        connection->send(Messages::StorageAreaMap::DidRemoveItem(storageMapSeed, key), storageMapID);
     910    });
    912911}
    913912
    914913void StorageManager::clear(IPC::Connection& connection, WebCore::SecurityOriginData&& securityOriginData, uint64_t storageMapID, uint64_t sourceStorageAreaID, uint64_t storageMapSeed, const String& urlString)
    915914{
    916     ASSERT(!RunLoop::isMain());
    917 
    918     auto* storageArea = findStorageArea(connection, storageMapID);
    919     if (!storageArea) {
    920         if (m_isEphemeral)
    921             m_ephemeralStorage.remove(securityOriginData);
    922         // This is a session storage area for a page that has already been closed. Ignore it.
    923         return;
    924     }
    925 
    926     storageArea->clear(connection.uniqueID(), sourceStorageAreaID, urlString);
    927     connection.send(Messages::StorageAreaMap::DidClear(storageMapSeed), storageMapID);
     915    m_queue->dispatch([this, protectedThis = makeRef(*this), connection = makeRef(connection), securityOriginData = securityOriginData.isolatedCopy(), storageMapID, sourceStorageAreaID, storageMapSeed, urlString = urlString.isolatedCopy()]() mutable {
     916        auto* storageArea = findStorageArea(connection.get(), storageMapID);
     917        if (!storageArea) {
     918            if (m_isEphemeral)
     919                m_ephemeralStorage.remove(securityOriginData);
     920            // This is a session storage area for a page that has already been closed. Ignore it.
     921            return;
     922        }
     923
     924        storageArea->clear(connection->uniqueID(), sourceStorageAreaID, urlString);
     925        connection->send(Messages::StorageAreaMap::DidClear(storageMapSeed), storageMapID);
     926    });
    928927}
    929928
    … …  
    976975}
    977976
    978 void StorageManager::dispatchMessageToQueue(IPC::Connection& connection, IPC::Decoder& decoder)
    979 {
    980     m_queue->dispatch([this, protectedThis = makeRef(*this), &connection, &decoder] {
    981         didReceiveMessage(connection, decoder);
    982     });
    983 }
    984 
    985 void StorageManager::dispatchSyncMessageToQueue(IPC::Connection& connection, IPC::Decoder& decoder, std::unique_ptr<IPC::Encoder>& replyEncoder)
    986 {
    987     m_queue->dispatch([this, protectedThis = makeRef(*this), &connection, &decoder, replyEncoder = WTFMove(replyEncoder)] () mutable {
    988         didReceiveSyncMessage(connection, decoder, replyEncoder);
    989     });
    990 }
    991 
    992977} // namespace WebKit

  • trunk/Source/WebKit/NetworkProcess/WebStorage/StorageManager.h

    r245540 r245649  
    4444class WebProcessProxy;
    4545
     46using GetValuesCallback = CompletionHandler<void(const HashMap<String, String>&)>;
     47
    4648class StorageManager : public IPC::Connection::WorkQueueMessageReceiver {
    4749public:
    … …  
    5557    void cloneSessionStorageNamespace(uint64_t storageNamespaceID, uint64_t newStorageNamespaceID);
    5658
    57     void processWillOpenConnection(IPC::Connection&);
    5859    void processDidCloseConnection(IPC::Connection&);
    5960    void waitUntilWritesFinished();
    … …  
    7172    void getLocalStorageOriginDetails(Function<void(Vector<LocalStorageDatabaseTracker::OriginDetails>&&)>&& completionHandler);
    7273
    73     void dispatchMessageToQueue(IPC::Connection&, IPC::Decoder&);
    74     void dispatchSyncMessageToQueue(IPC::Connection&, IPC::Decoder&, std::unique_ptr<IPC::Encoder>& replyEncoder);
     74    // IPC::Connection::WorkQueueMessageReceiver.
     75    void didReceiveMessage(IPC::Connection&, IPC::Decoder&) override;
     76    void didReceiveSyncMessage(IPC::Connection&, IPC::Decoder&, std::unique_ptr<IPC::Encoder>& replyEncoder) override;
    7577
    7678private:
    7779    explicit StorageManager(const String& localStorageDirectory);
    78 
    79     // IPC::Connection::WorkQueueMessageReceiver.
    80     void didReceiveMessage(IPC::Connection&, IPC::Decoder&) override;
    81     void didReceiveSyncMessage(IPC::Connection&, IPC::Decoder&, std::unique_ptr<IPC::Encoder>& replyEncoder) override;
    8280
    8381    // Message handlers.
    … …  
    8785    void destroyStorageMap(IPC::Connection&, uint64_t storageMapID);
    8886
    89     void getValues(IPC::Connection&, WebCore::SecurityOriginData&&, uint64_t storageMapID, uint64_t storageMapSeed, CompletionHandler<void(const HashMap<String, String>&)>&&);
     87    void getValues(IPC::Connection&, WebCore::SecurityOriginData&&, uint64_t storageMapID, uint64_t storageMapSeed, GetValuesCallback&&);
    9088    void setItem(IPC::Connection&, WebCore::SecurityOriginData&&, uint64_t storageMapID, uint64_t sourceStorageAreaID, uint64_t storageMapSeed, const String& key, const String& value, const String& urlString);
    9189    void setItems(IPC::Connection&, uint64_t storageMapID, const HashMap<String, String>& items);
    … …  
    116114    HashMap<WebCore::SecurityOriginData, Ref<WebCore::StorageMap>> m_ephemeralStorage;
    117115    bool m_isEphemeral { false };
    118 
    119116};
    120117

  • trunk/Tools/ChangeLog

    r245638 r245649  
     12019-05-22  Sihui Liu  <sihui_liu@apple.com>
     2
     3        API Test landed in r245540 [Mac WK2] TestWebKitAPI.WKWebView.LocalStorageProcessCrashes is a flaky failure
     4        https://bugs.webkit.org/show_bug.cgi?id=198090
     5        <rdar://problem/51003644>
     6
     7        Reviewed by Youenn Fablet.
     8
     9        WebView was wrongly loaded multiple times.
     10
     11        * TestWebKitAPI/Tests/WebKitCocoa/LocalStoragePersistence.mm:
     12        (TEST):
     13
    1142019-05-22  Jiewen Tan  <jiewen_tan@apple.com>
    215

  • trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStoragePersistence.mm

    r245540 r245649  
    6969    NSURLRequest *request = [NSURLRequest requestWithURL:[[NSBundle mainBundle] URLForResource:@"local-storage-process-crashes" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"]];
    7070    [webView loadRequest:request];
    71    
     71
    7272    receivedScriptMessage = false;
    73     [webView loadRequest:request];
    7473    TestWebKitAPI::Util::run(&receivedScriptMessage);
    7574    EXPECT_WK_STREQ(@"local:storage", [lastScriptMessage body]);
    76    
     75
    7776    receivedScriptMessage = false;
    78     [webView loadRequest:request];
    7977    TestWebKitAPI::Util::run(&receivedScriptMessage);
    8078    EXPECT_WK_STREQ(@"session:storage", [lastScriptMessage body]);
    8179
    8280    [configuration.get().processPool _terminateNetworkProcess];
    83    
     81
    8482    receivedScriptMessage = false;
    85     [webView loadRequest:request];
    8683    TestWebKitAPI::Util::run(&receivedScriptMessage);
    8784    EXPECT_WK_STREQ(@"Network Process Crashed", [lastScriptMessage body]);
Note: See TracChangeset for help on using the changeset viewer.