[webkit-changes] [276405] branches/safari-611-branch/Source/WebKit

Wed, 21 Apr 2021 16:39:47 -0700

Title: [276405] branches/safari-611-branch/Source/WebKit
Revision
276405
Author
[email protected]
Date
2021-04-21 16:39:37 -0700 (Wed, 21 Apr 2021)

Log Message

Cherry-pick r275805. rdar://problem/76963040

    Crash under WebProcessProxy::shouldSendPendingMessage()
    https://bugs.webkit.org/show_bug.cgi?id=224377
    <rdar://75329251>

    Reviewed by David Kilzer.

    We are crashing with a null-dereference of pendingMessage.encoder inside WebProcessProxy::shouldSendPendingMessage().
    However, pendingMessage.encoder is a UniqueRef<> and thus cannot be null. Also, we know that the WebProcessProxy
    is alive because WebProcessProxy::didFinishLaunching() has a protector.

    One thing that I believe could theoretically happen and would not be safe though is AuxiliaryProcessProxy::sendMessage()
    being called on a non-main thread. Sending IPC off the main thread is safe in general and something we commonly do with
    an IPC::Connection. To make this safe, IPC::Connection uses a Lock to protect its vector of messages. However, sending
    IPC via an AuxiliaryProcessProxy is currently not thread safe as it relies on the process state (which gets updated on
    the main thread) and access to the m_pendingMessages is not synchronized.

    As a speculative fix, I have added logic in AuxiliaryProcessProxy::sendMessage() to dispatch to the main thread if
    we're not already on it. I have also used WTF::DestructionThread::MainRunLoop to make sure all AuxiliaryProcessProxy
    objects get destroyed on the main thread.

    In a follow-up, I am planning to add a release assertion in AuxiliaryProcessProxy::sendMessage() to make sure we're
    on a main thread. We'll then be able to drop the "dispatching the main thread" logic. For now though, I think we
    should start with the "dispatch to main thread" logic, so that we have a patch that we can cherry-pick to a branch.

    * UIProcess/AuxiliaryProcessProxy.cpp:
    (WebKit::AuxiliaryProcessProxy::sendMessage):
    (WebKit::AuxiliaryProcessProxy::didFinishLaunching):
    (WebKit::AuxiliaryProcessProxy::replyToPendingMessages):
    * UIProcess/AuxiliaryProcessProxy.h:
    * UIProcess/GPU/GPUProcessProxy.h:
    * UIProcess/Network/NetworkProcessProxy.h:
    * UIProcess/Plugins/PluginProcessProxy.h:
    * UIProcess/WebAuthentication/WebAuthnProcessProxy.cpp:
    (WebKit::WebAuthnProcessProxy::singleton):
    * UIProcess/WebProcessProxy.h:

    git-svn-id: https://svn.webkit.org/repository/webkit/trunk@275805 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Modified Paths

Diff

Modified: branches/safari-611-branch/Source/WebKit/ChangeLog (276404 => 276405)


--- branches/safari-611-branch/Source/WebKit/ChangeLog	2021-04-21 23:39:33 UTC (rev 276404)
+++ branches/safari-611-branch/Source/WebKit/ChangeLog	2021-04-21 23:39:37 UTC (rev 276405)
@@ -1,3 +1,83 @@
+2021-04-21  Alan Coon  <[email protected]>
+
+        Cherry-pick r275805. rdar://problem/76963040
+
+    Crash under WebProcessProxy::shouldSendPendingMessage()
+    https://bugs.webkit.org/show_bug.cgi?id=224377
+    <rdar://75329251>
+    
+    Reviewed by David Kilzer.
+    
+    We are crashing with a null-dereference of pendingMessage.encoder inside WebProcessProxy::shouldSendPendingMessage().
+    However, pendingMessage.encoder is a UniqueRef<> and thus cannot be null. Also, we know that the WebProcessProxy
+    is alive because WebProcessProxy::didFinishLaunching() has a protector.
+    
+    One thing that I believe could theoretically happen and would not be safe though is AuxiliaryProcessProxy::sendMessage()
+    being called on a non-main thread. Sending IPC off the main thread is safe in general and something we commonly do with
+    an IPC::Connection. To make this safe, IPC::Connection uses a Lock to protect its vector of messages. However, sending
+    IPC via an AuxiliaryProcessProxy is currently not thread safe as it relies on the process state (which gets updated on
+    the main thread) and access to the m_pendingMessages is not synchronized.
+    
+    As a speculative fix, I have added logic in AuxiliaryProcessProxy::sendMessage() to dispatch to the main thread if
+    we're not already on it. I have also used WTF::DestructionThread::MainRunLoop to make sure all AuxiliaryProcessProxy
+    objects get destroyed on the main thread.
+    
+    In a follow-up, I am planning to add a release assertion in AuxiliaryProcessProxy::sendMessage() to make sure we're
+    on a main thread. We'll then be able to drop the "dispatching the main thread" logic. For now though, I think we
+    should start with the "dispatch to main thread" logic, so that we have a patch that we can cherry-pick to a branch.
+    
+    * UIProcess/AuxiliaryProcessProxy.cpp:
+    (WebKit::AuxiliaryProcessProxy::sendMessage):
+    (WebKit::AuxiliaryProcessProxy::didFinishLaunching):
+    (WebKit::AuxiliaryProcessProxy::replyToPendingMessages):
+    * UIProcess/AuxiliaryProcessProxy.h:
+    * UIProcess/GPU/GPUProcessProxy.h:
+    * UIProcess/Network/NetworkProcessProxy.h:
+    * UIProcess/Plugins/PluginProcessProxy.h:
+    * UIProcess/WebAuthentication/WebAuthnProcessProxy.cpp:
+    (WebKit::WebAuthnProcessProxy::singleton):
+    * UIProcess/WebProcessProxy.h:
+    
+    git-svn-id: https://svn.webkit.org/repository/webkit/trunk@275805 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+    2021-04-10  Chris Dumez  <[email protected]>
+
+            Crash under WebProcessProxy::shouldSendPendingMessage()
+            https://bugs.webkit.org/show_bug.cgi?id=224377
+            <rdar://75329251>
+
+            Reviewed by David Kilzer.
+
+            We are crashing with a null-dereference of pendingMessage.encoder inside WebProcessProxy::shouldSendPendingMessage().
+            However, pendingMessage.encoder is a UniqueRef<> and thus cannot be null. Also, we know that the WebProcessProxy
+            is alive because WebProcessProxy::didFinishLaunching() has a protector.
+
+            One thing that I believe could theoretically happen and would not be safe though is AuxiliaryProcessProxy::sendMessage()
+            being called on a non-main thread. Sending IPC off the main thread is safe in general and something we commonly do with
+            an IPC::Connection. To make this safe, IPC::Connection uses a Lock to protect its vector of messages. However, sending
+            IPC via an AuxiliaryProcessProxy is currently not thread safe as it relies on the process state (which gets updated on
+            the main thread) and access to the m_pendingMessages is not synchronized.
+
+            As a speculative fix, I have added logic in AuxiliaryProcessProxy::sendMessage() to dispatch to the main thread if
+            we're not already on it. I have also used WTF::DestructionThread::MainRunLoop to make sure all AuxiliaryProcessProxy
+            objects get destroyed on the main thread.
+
+            In a follow-up, I am planning to add a release assertion in AuxiliaryProcessProxy::sendMessage() to make sure we're
+            on a main thread. We'll then be able to drop the "dispatching the main thread" logic. For now though, I think we
+            should start with the "dispatch to main thread" logic, so that we have a patch that we can cherry-pick to a branch.
+
+            * UIProcess/AuxiliaryProcessProxy.cpp:
+            (WebKit::AuxiliaryProcessProxy::sendMessage):
+            (WebKit::AuxiliaryProcessProxy::didFinishLaunching):
+            (WebKit::AuxiliaryProcessProxy::replyToPendingMessages):
+            * UIProcess/AuxiliaryProcessProxy.h:
+            * UIProcess/GPU/GPUProcessProxy.h:
+            * UIProcess/Network/NetworkProcessProxy.h:
+            * UIProcess/Plugins/PluginProcessProxy.h:
+            * UIProcess/WebAuthentication/WebAuthnProcessProxy.cpp:
+            (WebKit::WebAuthnProcessProxy::singleton):
+            * UIProcess/WebProcessProxy.h:
+
 2021-04-21  Ruben Turcios  <[email protected]>
 
         Cherry-pick r275487. rdar://problem/76962948

Modified: branches/safari-611-branch/Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp (276404 => 276405)


--- branches/safari-611-branch/Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp	2021-04-21 23:39:33 UTC (rev 276404)
+++ branches/safari-611-branch/Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp	2021-04-21 23:39:37 UTC (rev 276405)
@@ -158,6 +158,15 @@
 
 bool AuxiliaryProcessProxy::sendMessage(std::unique_ptr<IPC::Encoder> encoder, OptionSet<IPC::SendOption> sendOptions, Optional<std::pair<CompletionHandler<void(IPC::Decoder*)>, uint64_t>>&& asyncReplyInfo, ShouldStartProcessThrottlerActivity shouldStartProcessThrottlerActivity)
 {
+    // FIXME: We should turn this into a RELEASE_ASSERT().
+    ASSERT(isMainRunLoop());
+    if (!isMainRunLoop()) {
+        callOnMainRunLoop([protectedThis = makeRef(*this), encoder = WTFMove(encoder), sendOptions, asyncReplyInfo = WTFMove(asyncReplyInfo), shouldStartProcessThrottlerActivity]() mutable {
+            protectedThis->sendMessage(WTFMove(encoder), sendOptions, WTFMove(asyncReplyInfo), shouldStartProcessThrottlerActivity);
+        });
+        return true;
+    }
+
     if (asyncReplyInfo && canSendMessage() && shouldStartProcessThrottlerActivity == ShouldStartProcessThrottlerActivity::Yes) {
         auto completionHandler = std::exchange(asyncReplyInfo->first, nullptr);
         asyncReplyInfo->first = [activity = throttler().backgroundActivity(ASCIILiteral::null()), completionHandler = WTFMove(completionHandler)](IPC::Decoder* decoder) mutable {
@@ -224,6 +233,7 @@
 void AuxiliaryProcessProxy::didFinishLaunching(ProcessLauncher*, IPC::Connection::Identifier connectionIdentifier)
 {
     ASSERT(!m_connection);
+    ASSERT(isMainRunLoop());
 
     if (!IPC::Connection::identifierIsValid(connectionIdentifier))
         return;
@@ -236,11 +246,9 @@
     for (auto&& pendingMessage : std::exchange(m_pendingMessages, { })) {
         if (!shouldSendPendingMessage(pendingMessage))
             continue;
-        auto encoder = WTFMove(pendingMessage.encoder);
-        auto sendOptions = pendingMessage.sendOptions;
         if (pendingMessage.asyncReplyInfo)
             IPC::addAsyncReplyHandler(*connection(), pendingMessage.asyncReplyInfo->second, WTFMove(pendingMessage.asyncReplyInfo->first));
-        m_connection->sendMessage(WTFMove(encoder), sendOptions);
+        m_connection->sendMessage(WTFMove(pendingMessage.encoder), pendingMessage.sendOptions);
     }
 }

Modified: branches/safari-611-branch/Source/WebKit/UIProcess/AuxiliaryProcessProxy.h (276404 => 276405)


--- branches/safari-611-branch/Source/WebKit/UIProcess/AuxiliaryProcessProxy.h	2021-04-21 23:39:33 UTC (rev 276404)
+++ branches/safari-611-branch/Source/WebKit/UIProcess/AuxiliaryProcessProxy.h	2021-04-21 23:39:37 UTC (rev 276405)
@@ -38,7 +38,7 @@
 
 class ProcessThrottler;
 
-class AuxiliaryProcessProxy : ProcessLauncher::Client, public IPC::Connection::Client {
+class AuxiliaryProcessProxy : public ThreadSafeRefCounted<AuxiliaryProcessProxy, WTF::DestructionThread::MainRunLoop>, private ProcessLauncher::Client, public IPC::Connection::Client {
     WTF_MAKE_NONCOPYABLE(AuxiliaryProcessProxy);
 
 protected:

Modified: branches/safari-611-branch/Source/WebKit/UIProcess/GPU/GPUProcessProxy.h (276404 => 276405)


--- branches/safari-611-branch/Source/WebKit/UIProcess/GPU/GPUProcessProxy.h	2021-04-21 23:39:33 UTC (rev 276404)
+++ branches/safari-611-branch/Source/WebKit/UIProcess/GPU/GPUProcessProxy.h	2021-04-21 23:39:37 UTC (rev 276405)
@@ -47,7 +47,7 @@
 class WebsiteDataStore;
 struct GPUProcessCreationParameters;
 
-class GPUProcessProxy final : public AuxiliaryProcessProxy, private ProcessThrottlerClient, public CanMakeWeakPtr<GPUProcessProxy>, public RefCounted<GPUProcessProxy> {
+class GPUProcessProxy final : public AuxiliaryProcessProxy, private ProcessThrottlerClient, public CanMakeWeakPtr<GPUProcessProxy> {
     WTF_MAKE_FAST_ALLOCATED;
     WTF_MAKE_NONCOPYABLE(GPUProcessProxy);
     friend LazyNeverDestroyed<GPUProcessProxy>;

Modified: branches/safari-611-branch/Source/WebKit/UIProcess/Network/NetworkProcessProxy.h (276404 => 276405)


--- branches/safari-611-branch/Source/WebKit/UIProcess/Network/NetworkProcessProxy.h	2021-04-21 23:39:33 UTC (rev 276404)
+++ branches/safari-611-branch/Source/WebKit/UIProcess/Network/NetworkProcessProxy.h	2021-04-21 23:39:37 UTC (rev 276405)
@@ -93,7 +93,7 @@
 struct WebsiteData;
 struct WebsiteDataStoreParameters;
 
-class NetworkProcessProxy final : public AuxiliaryProcessProxy, private ProcessThrottlerClient, public CanMakeWeakPtr<NetworkProcessProxy>, public RefCounted<NetworkProcessProxy> {
+class NetworkProcessProxy final : public AuxiliaryProcessProxy, private ProcessThrottlerClient, public CanMakeWeakPtr<NetworkProcessProxy> {
     WTF_MAKE_FAST_ALLOCATED;
 public:
     using RegistrableDomain = WebCore::RegistrableDomain;

Modified: branches/safari-611-branch/Source/WebKit/UIProcess/Plugins/PluginProcessProxy.h (276404 => 276405)


--- branches/safari-611-branch/Source/WebKit/UIProcess/Plugins/PluginProcessProxy.h	2021-04-21 23:39:33 UTC (rev 276404)
+++ branches/safari-611-branch/Source/WebKit/UIProcess/Plugins/PluginProcessProxy.h	2021-04-21 23:39:37 UTC (rev 276405)
@@ -63,7 +63,7 @@
 int pluginProcessThroughputQOS();
 #endif
 
-class PluginProcessProxy final : public AuxiliaryProcessProxy, public ThreadSafeRefCounted<PluginProcessProxy>, private ProcessThrottlerClient {
+class PluginProcessProxy final : public AuxiliaryProcessProxy, private ProcessThrottlerClient {
 public:
     static Ref<PluginProcessProxy> create(PluginProcessManager*, const PluginProcessAttributes&, uint64_t pluginProcessToken);
     ~PluginProcessProxy();

Modified: branches/safari-611-branch/Source/WebKit/UIProcess/WebAuthentication/WebAuthnProcessProxy.cpp (276404 => 276405)


--- branches/safari-611-branch/Source/WebKit/UIProcess/WebAuthentication/WebAuthnProcessProxy.cpp	2021-04-21 23:39:33 UTC (rev 276404)
+++ branches/safari-611-branch/Source/WebKit/UIProcess/WebAuthentication/WebAuthnProcessProxy.cpp	2021-04-21 23:39:37 UTC (rev 276405)
@@ -56,16 +56,16 @@
     ASSERT(RunLoop::isMain());
 
     static std::once_flag onceFlag;
-    static LazyNeverDestroyed<WebAuthnProcessProxy> webAuthnProcess;
+    static LazyNeverDestroyed<Ref<WebAuthnProcessProxy>> webAuthnProcess;
 
     std::call_once(onceFlag, [] {
-        webAuthnProcess.construct();
+        webAuthnProcess.construct(adoptRef(*new WebAuthnProcessProxy));
 
         WebAuthnProcessCreationParameters parameters;
 
         // Initialize the WebAuthn process.
-        webAuthnProcess->send(Messages::WebAuthnProcess::InitializeWebAuthnProcess(parameters), 0);
-        webAuthnProcess->updateProcessAssertion();
+        webAuthnProcess.get()->send(Messages::WebAuthnProcess::InitializeWebAuthnProcess(parameters), 0);
+        webAuthnProcess.get()->updateProcessAssertion();
     });
 
     return webAuthnProcess.get();

Modified: branches/safari-611-branch/Source/WebKit/UIProcess/WebProcessProxy.h (276404 => 276405)


--- branches/safari-611-branch/Source/WebKit/UIProcess/WebProcessProxy.h	2021-04-21 23:39:33 UTC (rev 276404)
+++ branches/safari-611-branch/Source/WebKit/UIProcess/WebProcessProxy.h	2021-04-21 23:39:37 UTC (rev 276405)
@@ -119,7 +119,7 @@
 using WebProcessWithAudibleMediaToken = WebProcessWithAudibleMediaCounter::Token;
 enum class CheckBackForwardList : bool { No, Yes };
 
-class WebProcessProxy : public AuxiliaryProcessProxy, public ResponsivenessTimer::Client, public ThreadSafeRefCounted<WebProcessProxy>, public CanMakeWeakPtr<WebProcessProxy>, private ProcessThrottlerClient {
+class WebProcessProxy : public AuxiliaryProcessProxy, public ResponsivenessTimer::Client, public CanMakeWeakPtr<WebProcessProxy>, private ProcessThrottlerClient {
 public:
     typedef HashMap<WebCore::FrameIdentifier, RefPtr<WebFrameProxy>> WebFrameProxyMap;
     typedef HashMap<WebPageProxyIdentifier, WebPageProxy*> WebPageProxyMap;

_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to