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