Bug 240321 – [Cocoa] ProcessLauncher instance was not released well on the error case.

• Status: ASSIGNED
• Component: Platform

[Basuke Suzuki]

It's very rare case but when xpc is failing to establish the connection to the sub process, the error handler is called after executing xpc_connection_send_message_with_reply():

https://github.com/WebKit/WebKit/blob/main/Source/WebKit/UIProcess/Launcher/cocoa/ProcessLauncherCocoa.mm#L277

and ProcessLauncher is failing its responsibility. At the end the owner of ProcessLauncher is terminated and releases the ownership of ProcessLauncher at the destructor of AuxiliaryProcessProxy.

But just before calling xpc_connection_send_message_with_reply(), the process is ref()ed. There's no chance to deref() the instance because the callback of  xpc_connection_send_message_with_reply() won't be called if error happens.

[Basuke Suzuki]

Created attachment 459189 [details]
PoC patch

[Basuke Suzuki]

I confirmed the dummy destructor was successfully called with this patch.

[Basuke Suzuki]

Created attachment 459192 [details]
patch

[Basuke Suzuki]

I'm not confident that this behavior is os version dependent, but it happens on macOS Monterey 12.3.1.

[Kimmo Kinnunen]

(In reply to Basuke Suzuki from comment #2)
> I confirmed the dummy destructor was successfully called with this patch.

Do you have a suggestion for a good method of making the subprocess startup fail when you confirming this?

[Basuke Suzuki]

(In reply to Kimmo Kinnunen from comment #5)
> (In reply to Basuke Suzuki from comment #2)
> > I confirmed the dummy destructor was successfully called with this patch.
> 
> Do you have a suggestion for a good method of making the subprocess startup
> fail when you confirming this?

Check this thread: 
https://webkit.slack.com/archives/CU64U6FDW/p1652306563278849

Originally I had a build issue with Xcode and that caused the half-baked build artifacts. I didn't notice about that and found this issue.

Steps to reproduce this:
1. Delete WebKitBuild
2. Open WebKit.xcworkspace
3. Before doing anything, just change the scheme to MiniBrowser <My Mac>
4. Build and run

In my environment (macOS 12.3.1), I see xpc failure and falling into infinite loop of relaunching.

[Chris Dumez]

Comment on attachment 459192 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=459192&action=review

> Source/WebKit/UIProcess/Launcher/cocoa/ProcessLauncherCocoa.mm:275
> +        processLauncher->deref();

This looks dangerous given this comment below:
```
    xpc_connection_send_message_with_reply(m_xpcConnection.get(), bootstrapMessage.get(), dispatch_get_main_queue(), ^(xpc_object_t reply) {
        // Errors are handled in the event handler.
        // It is possible for this block to be called after the error event handler, in which case we're no longer
        // launching and we already took care of cleaning things up.
```

It says that the block below (which calls deref()), may get called after this error event handler block, which now also calls deref(). If that comment is true, then it seems like your patch might result in crashes.

The current model of explicit ref / deref seems so fragile. It would be nice to make it less error-prone.

[Chris Dumez]

Would this work?

```
diff --git a/Source/WebKit/UIProcess/Launcher/cocoa/ProcessLauncherCocoa.mm b/Source/WebKit/UIProcess/Launcher/cocoa/ProcessLauncherCocoa.mm
index 972de63c1313..cc87c7e5a11b 100644
--- a/Source/WebKit/UIProcess/Launcher/cocoa/ProcessLauncherCocoa.mm
+++ b/Source/WebKit/UIProcess/Launcher/cocoa/ProcessLauncherCocoa.mm
@@ -38,6 +38,7 @@
 #import <spawn.h>
 #import <sys/param.h>
 #import <sys/stat.h>
+#import <wtf/BlockPtr.h>
 #import <wtf/MachSendRight.h>
 #import <wtf/RunLoop.h>
 #import <wtf/SoftLinking.h>
@@ -299,8 +300,7 @@ void ProcessLauncher::launchProcess()
         return;
     }
 
-    ref();
-    xpc_connection_send_message_with_reply(m_xpcConnection.get(), bootstrapMessage.get(), dispatch_get_main_queue(), ^(xpc_object_t reply) {
+    xpc_connection_send_message_with_reply(m_xpcConnection.get(), bootstrapMessage.get(), dispatch_get_main_queue(), makeBlockPtr([protectedThis = Ref { *this }](xpc_object_t reply) {
         // Errors are handled in the event handler.
         // It is possible for this block to be called after the error event handler, in which case we're no longer
         // launching and we already took care of cleaning things up.
@@ -328,9 +328,7 @@ void ProcessLauncher::launchProcess()
             didFinishLaunchingProcess(processIdentifier, IPC::Connection::Identifier(listeningPort, m_xpcConnection));
             m_xpcConnection = nullptr;
         }
-
-        deref();
-    });
+    }).get());
 }
 
 void ProcessLauncher::terminateProcess()
```

[Radar WebKit Bug Importer]

<rdar://problem/93536496>