Bug 240321

Created attachment 459189 [details]
PoC patch

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

Created attachment 459192 [details]
patch

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

(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?

(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.

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.

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()
```

<rdar://problem/93536496>