Attachment 369509 Details for Bug 197747 – Patch

[patch] Patch

bug-197747-20190509120519.patch (text/plain), 14.91 KB, created by Alex Christensen on 2019-05-09 12:05:20 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Alex Christensen

Created: 2019-05-09 12:05:20 PDT

Size: 14.91 KB

patch

obsolete

>Index: Source/WebKit/ChangeLog
>===================================================================
>--- Source/WebKit/ChangeLog	(revision 245147)
>+++ Source/WebKit/ChangeLog	(working copy)
>@@ -1,3 +1,30 @@
>+2019-05-09  Alex Christensen  <achristensen@webkit.org>
>+
>+        Remove now-unnecessary Connection::sendMessageWithReply
>+        https://bugs.webkit.org/show_bug.cgi?id=197747
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        The WebProcess messages that use it already do the right thing with CompletionHandlers and can be replaced by sendWithAsyncReply.
>+        The SecItemShim message that uses it can be replaced by sendSync.
>+
>+        This patch only increases responsiveness when clearing website data and removes the need for a WorkQueue dedicated entirely to the SecItemShim.
>+
>+        * Platform/IPC/Connection.cpp:
>+        (IPC::Connection::sendMessageWithReply): Deleted.
>+        * Platform/IPC/Connection.h:
>+        (IPC::Connection::send):
>+        (IPC::Connection::sendWithReply): Deleted.
>+        * Shared/mac/SecItemResponseData.h:
>+        * Shared/mac/SecItemShim.cpp:
>+        (WebKit::sendSecItemRequest):
>+        (WebKit::workQueue): Deleted.
>+        * UIProcess/WebProcessProxy.cpp:
>+        (WebKit::WebProcessProxy::fetchWebsiteData):
>+        (WebKit::WebProcessProxy::deleteWebsiteData):
>+        (WebKit::WebProcessProxy::deleteWebsiteDataForOrigins):
>+        * WebProcess/WebProcess.messages.in:
>+
> 2019-05-09  Daniel Bates  <dabates@apple.com>
> 
>         [iOS] Unable to commit search on MSN.com, qq.com, or sina.com.cn using enter key (hardware or software keyboard)
>Index: Source/WebKit/Platform/IPC/Connection.cpp
>===================================================================
>--- Source/WebKit/Platform/IPC/Connection.cpp	(revision 245056)
>+++ Source/WebKit/Platform/IPC/Connection.cpp	(working copy)
>@@ -49,11 +49,6 @@ namespace IPC {
> const size_t maxPendingIncomingMessagesKillingThreshold { 50000 };
> #endif
> 
>-struct Connection::ReplyHandler {
>-    RefPtr<FunctionDispatcher> dispatcher;
>-    Function<void (std::unique_ptr<Decoder>)> handler;
>-};
>-
> struct Connection::WaitForMessageState {
>     WaitForMessageState(StringReference messageReceiverName, StringReference messageName, uint64_t destinationID, OptionSet<WaitForOption> waitForOptions)
>         : messageReceiverName(messageReceiverName)
>@@ -371,17 +366,6 @@ void Connection::invalidate()
>     
>     m_isValid = false;
> 
>-    {
>-        std::lock_guard<Lock> lock(m_replyHandlersLock);
>-        for (auto& replyHandler : m_replyHandlers.values()) {
>-            replyHandler.dispatcher->dispatch([handler = WTFMove(replyHandler.handler)] {
>-                handler(nullptr);
>-            });
>-        }
>-
>-        m_replyHandlers.clear();
>-    }
>-
>     m_connectionQueue->dispatch([protectedThis = makeRef(*this)]() mutable {
>         protectedThis->platformInvalidate();
>     });
>@@ -437,25 +421,6 @@ bool Connection::sendMessage(std::unique
>     return true;
> }
> 
>-void Connection::sendMessageWithReply(uint64_t requestID, std::unique_ptr<Encoder> encoder, FunctionDispatcher& replyDispatcher, Function<void (std::unique_ptr<Decoder>)>&& replyHandler)
>-{
>-    {
>-        std::lock_guard<Lock> lock(m_replyHandlersLock);
>-
>-        if (!isValid()) {
>-            replyDispatcher.dispatch([replyHandler = WTFMove(replyHandler)] {
>-                replyHandler(nullptr);
>-            });
>-            return;
>-        }
>-
>-        ASSERT(!m_replyHandlers.contains(requestID));
>-        m_replyHandlers.set(requestID, ReplyHandler { &replyDispatcher, WTFMove(replyHandler) });
>-    }
>-
>-    sendMessage(WTFMove(encoder), { });
>-}
>-
> bool Connection::sendSyncReply(std::unique_ptr<Encoder> encoder)
> {
>     return sendMessage(WTFMove(encoder), { });
>@@ -658,22 +623,6 @@ void Connection::processIncomingSyncRepl
>         }
>     }
> 
>-    {
>-        LockHolder locker(m_replyHandlersLock);
>-
>-        auto replyHandler = m_replyHandlers.take(decoder->destinationID());
>-        if (replyHandler.dispatcher) {
>-            replyHandler.dispatcher->dispatch([protectedThis = makeRef(*this), handler = WTFMove(replyHandler.handler), decoder = WTFMove(decoder)] () mutable {
>-                if (!protectedThis->isValid()) {
>-                    handler(nullptr);
>-                    return;
>-                }
>-
>-                handler(WTFMove(decoder));
>-            });
>-        }
>-    }
>-
>     // If we get here, it means we got a reply for a message that wasn't in the sync request stack or map.
>     // This can happen if the send timed out, so it's fine to ignore.
> }
>@@ -808,17 +757,6 @@ void Connection::connectionDidClose()
>     platformInvalidate();
> 
>     {
>-        LockHolder locker(m_replyHandlersLock);
>-        for (auto& replyHandler : m_replyHandlers.values()) {
>-            replyHandler.dispatcher->dispatch([handler = WTFMove(replyHandler.handler)] {
>-                handler(nullptr);
>-            });
>-        }
>-
>-        m_replyHandlers.clear();
>-    }
>-
>-    {
>         LockHolder locker(m_syncReplyStateMutex);
> 
>         ASSERT(m_shouldWaitForSyncReplies);
>Index: Source/WebKit/Platform/IPC/Connection.h
>===================================================================
>--- Source/WebKit/Platform/IPC/Connection.h	(revision 245056)
>+++ Source/WebKit/Platform/IPC/Connection.h	(working copy)
>@@ -179,12 +179,10 @@ public:
> 
>     template<typename T, typename C> void sendWithAsyncReply(T&& message, C&& completionHandler, uint64_t destinationID = 0);
>     template<typename T> bool send(T&& message, uint64_t destinationID, OptionSet<SendOption> sendOptions = { });
>-    template<typename T> void sendWithReply(T&& message, uint64_t destinationID, FunctionDispatcher& replyDispatcher, Function<void(Optional<typename CodingType<typename T::Reply>::Type>)>&& replyHandler);
>     template<typename T> bool sendSync(T&& message, typename T::Reply&& reply, uint64_t destinationID, Seconds timeout = Seconds::infinity(), OptionSet<SendSyncOption> sendSyncOptions = { });
>     template<typename T> bool waitForAndDispatchImmediately(uint64_t destinationID, Seconds timeout, OptionSet<WaitForOption> waitForOptions = { });
> 
>     bool sendMessage(std::unique_ptr<Encoder>, OptionSet<SendOption> sendOptions);
>-    void sendMessageWithReply(uint64_t requestID, std::unique_ptr<Encoder>, FunctionDispatcher& replyDispatcher, Function<void(std::unique_ptr<Decoder>)>&& replyHandler);
>     std::unique_ptr<Encoder> createSyncMessageEncoder(StringReference messageReceiverName, StringReference messageName, uint64_t destinationID, uint64_t& syncRequestID);
>     std::unique_ptr<Decoder> sendSyncMessage(uint64_t syncRequestID, std::unique_ptr<Encoder>, Seconds timeout, OptionSet<SendSyncOption> sendSyncOptions);
>     bool sendSyncReply(std::unique_ptr<Encoder>);
>@@ -313,11 +311,6 @@ private:
>     Condition m_waitForMessageCondition;
>     Lock m_waitForMessageMutex;
> 
>-    struct ReplyHandler;
>-
>-    Lock m_replyHandlersLock;
>-    HashMap<uint64_t, ReplyHandler> m_replyHandlers;
>-
>     struct WaitForMessageState;
>     WaitForMessageState* m_waitingForMessage;
> 
>@@ -434,28 +427,6 @@ void Connection::sendWithAsyncReply(T&& 
>     });
> }
> 
>-template<typename T>
>-void Connection::sendWithReply(T&& message, uint64_t destinationID, FunctionDispatcher& replyDispatcher, Function<void(Optional<typename CodingType<typename T::Reply>::Type>)>&& replyHandler)
>-{
>-    uint64_t requestID = 0;
>-    std::unique_ptr<Encoder> encoder = createSyncMessageEncoder(T::receiverName(), T::name(), destinationID, requestID);
>-
>-    encoder->encode(message.arguments());
>-
>-    sendMessageWithReply(requestID, WTFMove(encoder), replyDispatcher, [replyHandler = WTFMove(replyHandler)](std::unique_ptr<Decoder> decoder) {
>-        if (decoder) {
>-            Optional<typename CodingType<typename T::Reply>::Type> reply;
>-            *decoder >> reply;
>-            if (reply) {
>-                replyHandler(WTFMove(*reply));
>-                return;
>-            }
>-        }
>-
>-        replyHandler(WTF::nullopt);
>-    });
>-}
>-
> template<size_t i, typename A, typename B> struct TupleMover {
>     static void move(A&& a, B& b)
>     {
>Index: Source/WebKit/Shared/mac/SecItemResponseData.h
>===================================================================
>--- Source/WebKit/Shared/mac/SecItemResponseData.h	(revision 245056)
>+++ Source/WebKit/Shared/mac/SecItemResponseData.h	(working copy)
>@@ -23,8 +23,7 @@
>  * THE POSSIBILITY OF SUCH DAMAGE.
>  */
> 
>-#ifndef SecItemResponseData_h
>-#define SecItemResponseData_h
>+#pragma once
> 
> #include <wtf/RetainPtr.h>
> 
>@@ -37,6 +36,7 @@ namespace WebKit {
>     
> class SecItemResponseData {
> public:
>+    SecItemResponseData() = default;
>     SecItemResponseData(OSStatus, RetainPtr<CFTypeRef>&& result);
> 
>     void encode(IPC::Encoder&) const;
>@@ -51,5 +51,3 @@ private:
> };
>     
> } // namespace WebKit
>-
>-#endif // SecItemResponseData_h
>Index: Source/WebKit/Shared/mac/SecItemShim.cpp
>===================================================================
>--- Source/WebKit/Shared/mac/SecItemShim.cpp	(revision 245056)
>+++ Source/WebKit/Shared/mac/SecItemShim.cpp	(working copy)
>@@ -64,37 +64,16 @@ static WeakPtr<NetworkProcess>& globalNe
>     return networkProcess.get();
> }
> 
>-static WorkQueue& workQueue()
>-{
>-    static WorkQueue* workQueue;
>-    static dispatch_once_t onceToken;
>-    dispatch_once(&onceToken, ^{
>-        workQueue = &WorkQueue::create("com.apple.WebKit.SecItemShim").leakRef();
>-
>-    });
>-
>-    return *workQueue;
>-}
>-
> static Optional<SecItemResponseData> sendSecItemRequest(SecItemRequestData::Type requestType, CFDictionaryRef query, CFDictionaryRef attributesToMatch = 0)
> {
>     if (!globalNetworkProcess())
>         return WTF::nullopt;
> 
>-    Optional<SecItemResponseData> response;
>-
>-    BinarySemaphore semaphore;
>-
>-    globalNetworkProcess()->parentProcessConnection()->sendWithReply(Messages::SecItemShimProxy::SecItemRequest(SecItemRequestData(requestType, query, attributesToMatch)), 0, workQueue(), [&response, &semaphore](auto reply) {
>-        if (reply)
>-            response = WTFMove(std::get<0>(*reply));
>-
>-        semaphore.signal();
>-    });
>-
>-    semaphore.wait();
>+    SecItemResponseData response;
>+    if (!globalNetworkProcess()->parentProcessConnection()->sendSync(Messages::SecItemShimProxy::SecItemRequest(SecItemRequestData(requestType, query, attributesToMatch)), Messages::SecItemShimProxy::SecItemRequest::Reply(response), 0))
>+        return WTF::nullopt;
> 
>-    return response;
>+    return WTFMove(response);
> }
> 
> static OSStatus webSecItemCopyMatching(CFDictionaryRef query, CFTypeRef* result)
>Index: Source/WebKit/UIProcess/WebProcessProxy.cpp
>===================================================================
>--- Source/WebKit/UIProcess/WebProcessProxy.cpp	(revision 245056)
>+++ Source/WebKit/UIProcess/WebProcessProxy.cpp	(working copy)
>@@ -936,17 +936,12 @@ void WebProcessProxy::fetchWebsiteData(P
>     auto token = throttler().backgroundActivityToken();
>     RELEASE_LOG_IF(sessionID.isAlwaysOnLoggingAllowed(), ProcessSuspension, "%p - WebProcessProxy is taking a background assertion because the Web process is fetching Website data", this);
> 
>-    connection()->sendWithReply(Messages::WebProcess::FetchWebsiteData(sessionID, dataTypes), 0, RunLoop::main(), [this, token, completionHandler = WTFMove(completionHandler), sessionID] (auto reply) mutable {
>+    connection()->sendWithAsyncReply(Messages::WebProcess::FetchWebsiteData(sessionID, dataTypes), [this, protectedThis = makeRef(*this), token, completionHandler = WTFMove(completionHandler), sessionID] (auto reply) mutable {
> #if RELEASE_LOG_DISABLED
>         UNUSED_PARAM(sessionID);
>         UNUSED_PARAM(this);
> #endif
>-        if (!reply) {
>-            completionHandler(WebsiteData { });
>-            return;
>-        }
>-
>-        completionHandler(WTFMove(std::get<0>(*reply)));
>+        completionHandler(WTFMove(reply));
>         RELEASE_LOG_IF(sessionID.isAlwaysOnLoggingAllowed(), ProcessSuspension, "%p - WebProcessProxy is releasing a background assertion because the Web process is done fetching Website data", this);
>     });
> }
>@@ -958,7 +953,7 @@ void WebProcessProxy::deleteWebsiteData(
>     auto token = throttler().backgroundActivityToken();
>     RELEASE_LOG_IF(sessionID.isAlwaysOnLoggingAllowed(), ProcessSuspension, "%p - WebProcessProxy is taking a background assertion because the Web process is deleting Website data", this);
> 
>-    connection()->sendWithReply(Messages::WebProcess::DeleteWebsiteData(sessionID, dataTypes, modifiedSince), 0, RunLoop::main(), [this, token, completionHandler = WTFMove(completionHandler), sessionID] (auto reply) mutable {
>+    connection()->sendWithAsyncReply(Messages::WebProcess::DeleteWebsiteData(sessionID, dataTypes, modifiedSince), [this, protectedThis = makeRef(*this), token, completionHandler = WTFMove(completionHandler), sessionID] () mutable {
> #if RELEASE_LOG_DISABLED
>         UNUSED_PARAM(this);
>         UNUSED_PARAM(sessionID);
>@@ -975,7 +970,7 @@ void WebProcessProxy::deleteWebsiteDataF
>     auto token = throttler().backgroundActivityToken();
>     RELEASE_LOG_IF(sessionID.isAlwaysOnLoggingAllowed(), ProcessSuspension, "%p - WebProcessProxy is taking a background assertion because the Web process is deleting Website data for several origins", this);
> 
>-    connection()->sendWithReply(Messages::WebProcess::DeleteWebsiteDataForOrigins(sessionID, dataTypes, origins), 0, RunLoop::main(), [this, token, completionHandler = WTFMove(completionHandler), sessionID] (auto reply) mutable {
>+    connection()->sendWithAsyncReply(Messages::WebProcess::DeleteWebsiteDataForOrigins(sessionID, dataTypes, origins), [this, protectedThis = makeRef(*this), token, completionHandler = WTFMove(completionHandler), sessionID] () mutable {
> #if RELEASE_LOG_DISABLED
>         UNUSED_PARAM(this);
>         UNUSED_PARAM(sessionID);
>Index: Source/WebKit/WebProcess/WebProcess.messages.in
>===================================================================
>--- Source/WebKit/WebProcess/WebProcess.messages.in	(revision 245056)
>+++ Source/WebKit/WebProcess/WebProcess.messages.in	(working copy)
>@@ -78,9 +78,9 @@ messages -> WebProcess LegacyReceiver {
> 
>     ReleasePageCache()
> 
>-    FetchWebsiteData(PAL::SessionID sessionID, OptionSet<WebKit::WebsiteDataType> websiteDataTypes) -> (struct WebKit::WebsiteData websiteData) Synchronous
>-    DeleteWebsiteData(PAL::SessionID sessionID, OptionSet<WebKit::WebsiteDataType> websiteDataTypes, WallTime modifiedSince) -> () Synchronous
>-    DeleteWebsiteDataForOrigins(PAL::SessionID sessionID, OptionSet<WebKit::WebsiteDataType> websiteDataTypes, Vector<WebCore::SecurityOriginData> origins) -> () Synchronous
>+    FetchWebsiteData(PAL::SessionID sessionID, OptionSet<WebKit::WebsiteDataType> websiteDataTypes) -> (struct WebKit::WebsiteData websiteData) Async
>+    DeleteWebsiteData(PAL::SessionID sessionID, OptionSet<WebKit::WebsiteDataType> websiteDataTypes, WallTime modifiedSince) -> () Async
>+    DeleteWebsiteDataForOrigins(PAL::SessionID sessionID, OptionSet<WebKit::WebsiteDataType> websiteDataTypes, Vector<WebCore::SecurityOriginData> origins) -> () Async
> 
>     SetHiddenPageDOMTimerThrottlingIncreaseLimit(int milliseconds)
> #if PLATFORM(COCOA)

Flags:

thorton: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 197747: 369505 | 369509