[Qemu-devel] [PATCH] Fix for crashes and non-responsive UI on macOS Mojave

[Berkus Decker]

It seems that Cocoa checks are stricter on Mojave and some callbacks that 
worked from non-GUI thread on High Sierra are no longer working.

The fixes included here are:

* Deferring qemu_main() to another thread so that the actual main thread is 
reserved for the Cocoa UI; it also removes blocking from 
applicationDidFinishLoading: delegate. I beleive this alone caused complete UI 
blockage on Mojave.
* Deferring UI-related updates in callbacks to the UI thread using 
invokeOnMainThread helper function. This function uses DDInvocationGrabber 
object courtesy of Dave Dribin, licensed under MIT license.
Here’s relevant blog post for his code: 
https://www.dribin.org/dave/blog/archives/2008/05/22/invoke_on_main_thread/

NSInvocation is used here instead of plain 
performSelectorOnMainThread:withObject:waitUntilDone: because we want to be 
able to pass non-id types to the handlers.

These changes are ought to work on OSX 10.6, although I don’t have a machine 
handy to test it.

Fixes https://bugs.launchpad.net/qemu/+bug/1802684

From 8f86e30f3710d782d78dccdbe7a1564ae79220c7 Mon Sep 17 00:00:00 2001
From: Berkus Decker <address@hidden>
Date: Sun, 11 Nov 2018 21:58:17 +0200
Subject: [PATCH 1/2] ui/cocoa: Defer qemu to another thread, leaving main
 thread for the UI

This prevents blocking in applicationDidFinishLoading: which is
not recommended and now causes complete UI lock on macOS Mojave.

Signed-off-by: Berkus Decker <address@hidden>
---
 ui/cocoa.m | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/ui/cocoa.m b/ui/cocoa.m
index ecf12bfc2e..f69f7105f2 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -1089,9 +1089,13 @@ QemuCocoaView *cocoaView;
 {
     COCOA_DEBUG("QemuCocoaAppController: startEmulationWithArgc\n");
 
-    int status;
-    status = qemu_main(argc, argv, *_NSGetEnviron());
-    exit(status);
+    dispatch_queue_t qemu_runner = dispatch_queue_create("qemu-runner", 
DISPATCH_QUEUE_SERIAL);
+
+    dispatch_async(qemu_runner, ^{
+        int status;
+        status = qemu_main(argc, argv, *_NSGetEnviron());
+        exit(status);
+    });
 }
 
 /* We abstract the method called by the Enter Fullscreen menu item
-- 
2.18.0


From 467b0f67d94616ef98d2ec1e8d16eeb5e9506b4e Mon Sep 17 00:00:00 2001
From: Berkus Decker <address@hidden>
Date: Sun, 11 Nov 2018 20:22:01 +0200
Subject: [PATCH 2/2] ui/cocoa: Fix UI crashes on macOS Mojave

Signed-off-by: Berkus Decker <address@hidden>
---
 ui/DDInvocationGrabber.h | 124 ++++++++++++++++++++++++++++
 ui/DDInvocationGrabber.m | 171 +++++++++++++++++++++++++++++++++++++++
 ui/Makefile.objs         |   2 +-
 ui/cocoa.m               |  57 ++++++++-----
 4 files changed, 333 insertions(+), 21 deletions(-)
 create mode 100644 ui/DDInvocationGrabber.h
 create mode 100644 ui/DDInvocationGrabber.m

diff --git a/ui/DDInvocationGrabber.h b/ui/DDInvocationGrabber.h
new file mode 100644
index 0000000000..7218421d74
--- /dev/null
+++ b/ui/DDInvocationGrabber.h
@@ -0,0 +1,124 @@
+/*
+ * Copyright (c) 2007-2008 Dave Dribin
+ *
+ * Permission is hereby granted, free of charge, to any person
+ * obtaining a copy of this software and associated documentation
+ * files (the "Software"), to deal in the Software without
+ * restriction, including without limitation the rights to use, copy,
+ * modify, merge, publish, distribute, sublicense, and/or sell copies
+ * of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be
+ * included in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+
+/*
+ *  This class is based on CInvocationGrabber:
+ *
+ *  Copyright (c) 2007, Toxic Software
+ *  All rights reserved.
+ *  Redistribution and use in source and binary forms, with or without
+ *  modification, are permitted provided that the following conditions are
+ *  met:
+ *
+ *  * Redistributions of source code must retain the above copyright notice,
+ *  this list of conditions and the following disclaimer.
+ *
+ *  * Redistributions in binary form must reproduce the above copyright
+ *  notice, this list of conditions and the following disclaimer in the
+ *  documentation and/or other materials provided with the distribution.
+ *
+ *  * Neither the name of the Toxic Software nor the names of its
+ *  contributors may be used to endorse or promote products derived from
+ *  this software without specific prior written permission.
+ *
+ *  THIS SOFTWARE IS PROVIDED BY THE REGENTS AND CONTRIBUTORS ``AS IS'' AND
+ *  ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ *  IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ *  PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE
+ *  LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ *  CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ *  SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ *  INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ *  CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ *  ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
+ *  THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ */
+
+#import <Foundation/Foundation.h>
+
+/**
+ * @class DDInvocationGrabber
+ * @discussion DDInvocationGrabber is a helper object that makes it very easy
+ * to construct instances of NSInvocation for later use. The object is
+ * inspired by NSUndoManager's prepareWithInvocationTarget method. To use
+ * a DDInvocationGrabber object, you set its target to some object, then send
+ * it a message as if it were the target object (the DDInvocationGrabber object
+ * acts as a proxy), if the target message understands the message the
+ * DDInvocationGrabber object stores the message invocation.
+
+ DDInvocationGrabber *theGrabber = [DDInvocationGrabber invocationGrabber];
+ [theGrabber setTarget:someObject]
+ // Send messages to 'theGrabber' as if it were 'someObject'
+ [theGrabber doSomethingWithParameter:someParameter];
+ NSInvocation *theInvocation = [theGrabber invocation];
+
+ A slightly more concise version (using the covenience category) follows:
+
+ DDInvocationGrabber *theGrabber = [DDInvocationGrabber invocationGrabber];
+ [[theGrabber prepareWithInvocationTarget:someObject]
+    doSomethingWithParameter:someParameter];
+ NSInvocation *theInvocation = [theGrabber invocation];
+
+ */
address@hidden DDInvocationGrabber : NSProxy
+{
+    id _target;
+    NSInvocation * _invocation;
+    BOOL _forwardInvokesOnMainThread;
+    BOOL _waitUntilDone;
+}
+
+/**
+ * @method invocationGrabber
+ * @abstract Returns a newly allocated, inited, autoreleased
+ * DDInvocationGrabber object.
+ */
++ (id) invocationGrabber;
+
+- (id) target;
+- (void) setTarget:(id)inTarget;
+
+- (NSInvocation *) invocation;
+- (void) setInvocation:(NSInvocation *)inInvocation;
+
+- (BOOL) forwardInvokesOnMainThread;
+- (void) setForwardInvokesOnMainThread:(BOOL)forwardInvokesOnMainThread;
+
+- (BOOL) waitUntilDone;
+- (void) setWaitUntilDone:(BOOL)waitUntilDone;
+
address@hidden
+
address@hidden DDInvocationGrabber (DDInvocationGrabber_Conveniences)
+
+/**
+ * @method prepareWithInvocationTarget:
+ * @abstract Sets the target object of the receiver and returns itself.
+ * The sender can then send a message to the receiver.
+ */
+- (id)prepareWithInvocationTarget:(id)inTarget;
+
address@hidden
diff --git a/ui/DDInvocationGrabber.m b/ui/DDInvocationGrabber.m
new file mode 100644
index 0000000000..297a0b41f5
--- /dev/null
+++ b/ui/DDInvocationGrabber.m
@@ -0,0 +1,171 @@
+/*
+ * Copyright (c) 2007-2008 Dave Dribin
+ *
+ * Permission is hereby granted, free of charge, to any person
+ * obtaining a copy of this software and associated documentation
+ * files (the "Software"), to deal in the Software without
+ * restriction, including without limitation the rights to use, copy,
+ * modify, merge, publish, distribute, sublicense, and/or sell copies
+ * of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be
+ * included in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+
+/*
+ *  This class is based on CInvocationGrabber:
+ *
+ *  Copyright (c) 2007, Toxic Software
+ *  All rights reserved.
+ *  Redistribution and use in source and binary forms, with or without
+ *  modification, are permitted provided that the following conditions are
+ *  met:
+ *
+ *  * Redistributions of source code must retain the above copyright notice,
+ *  this list of conditions and the following disclaimer.
+ *
+ *  * Redistributions in binary form must reproduce the above copyright
+ *  notice, this list of conditions and the following disclaimer in the
+ *  documentation and/or other materials provided with the distribution.
+ *
+ *  * Neither the name of the Toxic Software nor the names of its
+ *  contributors may be used to endorse or promote products derived from
+ *  this software without specific prior written permission.
+ *
+ *  THIS SOFTWARE IS PROVIDED BY THE REGENTS AND CONTRIBUTORS ``AS IS'' AND
+ *  ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ *  IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ *  PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE
+ *  LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ *  CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ *  SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ *  INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ *  CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ *  ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
+ *  THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ */
+
+#import "DDInvocationGrabber.h"
+
+
address@hidden DDInvocationGrabber
+
++ (id)invocationGrabber
+{
+    return([[[self alloc] init] autorelease]);
+}
+
+- (id)init
+{
+    _target = nil;
+    _invocation = nil;
+    _forwardInvokesOnMainThread = NO;
+    _waitUntilDone = NO;
+
+    return self;
+}
+
+- (void)dealloc
+{
+    [self setTarget:NULL];
+    [self setInvocation:NULL];
+    //
+    [super dealloc];
+}
+
+#pragma mark -
+
+- (id)target
+{
+    return _target;
+}
+
+- (void)setTarget:(id)inTarget
+{
+    if (_target != inTarget)
+       {
+        [_target autorelease];
+        _target = [inTarget retain];
+       }
+}
+
+- (NSInvocation *)invocation
+{
+    return _invocation;
+}
+
+- (void)setInvocation:(NSInvocation *)inInvocation
+{
+    if (_invocation != inInvocation)
+       {
+        [_invocation autorelease];
+        _invocation = [inInvocation retain];
+       }
+}
+
+- (BOOL)forwardInvokesOnMainThread;
+{
+    return _forwardInvokesOnMainThread;
+}
+
+- (void)setForwardInvokesOnMainThread:(BOOL)forwardInvokesOnMainThread;
+{
+    _forwardInvokesOnMainThread = forwardInvokesOnMainThread;
+}
+
+- (BOOL)waitUntilDone;
+{
+    return _waitUntilDone;
+}
+
+- (void)setWaitUntilDone:(BOOL)waitUntilDone;
+{
+    _waitUntilDone = waitUntilDone;
+}
+
+#pragma mark -
+
+- (NSMethodSignature *)methodSignatureForSelector:(SEL)selector
+{
+    return [[self target] methodSignatureForSelector:selector];
+}
+
+- (void)forwardInvocation:(NSInvocation *)ioInvocation
+{
+    [ioInvocation setTarget:[self target]];
+    [self setInvocation:ioInvocation];
+    if (_forwardInvokesOnMainThread)
+    {
+        if (!_waitUntilDone)
+            [_invocation retainArguments];
+        [_invocation performSelectorOnMainThread:@selector(invoke)
+                                      withObject:nil
+                                   waitUntilDone:_waitUntilDone];
+    }
+}
+
address@hidden
+
+#pragma mark -
+
address@hidden DDInvocationGrabber (DDnvocationGrabber_Conveniences)
+
+- (id)prepareWithInvocationTarget:(id)inTarget
+{
+    [self setTarget:inTarget];
+    return(self);
+}
+
address@hidden
diff --git a/ui/Makefile.objs b/ui/Makefile.objs
index 00f6976c30..bd9983232f 100644
--- a/ui/Makefile.objs
+++ b/ui/Makefile.objs
@@ -11,7 +11,7 @@ common-obj-y += keymaps.o console.o cursor.o qemu-pixman.o
 common-obj-y += input.o input-keymap.o input-legacy.o
 common-obj-$(CONFIG_LINUX) += input-linux.o
 common-obj-$(CONFIG_SPICE) += spice-core.o spice-input.o spice-display.o
-common-obj-$(CONFIG_COCOA) += cocoa.o
+common-obj-$(CONFIG_COCOA) += cocoa.o DDInvocationGrabber.o
 common-obj-$(CONFIG_VNC) += $(vnc-obj-y)
 common-obj-$(call lnot,$(CONFIG_VNC)) += vnc-stubs.o
 
diff --git a/ui/cocoa.m b/ui/cocoa.m
index f69f7105f2..41aa7524d8 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -38,6 +38,8 @@
 #include <Carbon/Carbon.h>
 #include "qom/cpu.h"
 
+#import "DDInvocationGrabber.h"
+
 #ifndef MAC_OS_X_VERSION_10_5
 #define MAC_OS_X_VERSION_10_5 1050
 #endif
@@ -314,6 +316,8 @@ static void handleAnyDeviceErrors(Error * err)
 - (float) cdy;
 - (QEMUScreen) gscreen;
 - (void) raiseAllKeys;
+- (void) refresh;
+- (id) invokeOnMainThread;
 @end
 
 QemuCocoaView *cocoaView;
@@ -345,6 +349,14 @@ QemuCocoaView *cocoaView;
     [super dealloc];
 }
 
+- (id) invokeOnMainThread
+{
+    DDInvocationGrabber * grabber = [DDInvocationGrabber invocationGrabber];
+    [grabber setForwardInvokesOnMainThread:YES];
+    [grabber setWaitUntilDone:YES];
+    return [grabber prepareWithInvocationTarget:self];
+}
+
 - (BOOL) isOpaque
 {
     return YES;
@@ -509,6 +521,28 @@ QemuCocoaView *cocoaView;
     }
 }
 
+- (void) refresh
+{
+    if (qemu_input_is_absolute()) {
+        if (![self isAbsoluteEnabled]) {
+            if ([self isMouseGrabbed]) {
+                [self ungrabMouse];
+            }
+        }
+        [self setAbsoluteEnabled:YES];
+    }
+
+    NSDate *distantPast = [NSDate distantPast];
+    NSEvent *event;
+    do {
+        event = [NSApp nextEventMatchingMask:NSEventMaskAny 
untilDate:distantPast
+                        inMode: NSDefaultRunLoopMode dequeue:YES];
+        if (event != nil) {
+            [self handleEvent:event];
+        }
+    } while(event != nil);
+}
+
 - (void) toggleFullScreen:(id)sender
 {
     COCOA_DEBUG("QemuCocoaView: toggleFullScreen\n");
@@ -1566,7 +1600,7 @@ static void cocoa_update(DisplayChangeListener *dcl,
             w * [cocoaView cdx],
             h * [cocoaView cdy]);
     }
-    [cocoaView setNeedsDisplayInRect:rect];
+    [[cocoaView invokeOnMainThread] setNeedsDisplayInRect:rect];
 
     [pool release];
 }
@@ -1577,7 +1611,7 @@ static void cocoa_switch(DisplayChangeListener *dcl,
     NSAutoreleasePool * pool = [[NSAutoreleasePool alloc] init];
 
     COCOA_DEBUG("qemu_cocoa: cocoa_switch\n");
-    [cocoaView switchSurface:surface];
+    [[cocoaView invokeOnMainThread] switchSurface:surface];
     [pool release];
 }
 
@@ -1588,25 +1622,8 @@ static void cocoa_refresh(DisplayChangeListener *dcl)
     COCOA_DEBUG("qemu_cocoa: cocoa_refresh\n");
     graphic_hw_update(NULL);
 
-    if (qemu_input_is_absolute()) {
-        if (![cocoaView isAbsoluteEnabled]) {
-            if ([cocoaView isMouseGrabbed]) {
-                [cocoaView ungrabMouse];
-            }
-        }
-        [cocoaView setAbsoluteEnabled:YES];
-    }
+    [[cocoaView invokeOnMainThread] refresh];
 
-    NSDate *distantPast;
-    NSEvent *event;
-    distantPast = [NSDate distantPast];
-    do {
-        event = [NSApp nextEventMatchingMask:NSEventMaskAny 
untilDate:distantPast
-                        inMode: NSDefaultRunLoopMode dequeue:YES];
-        if (event != nil) {
-            [cocoaView handleEvent:event];
-        }
-    } while(event != nil);
     [pool release];
 }
 
-- 
2.18.0

0001-ui-cocoa-Defer-qemu-to-another-thread-leaving-main-t.patch
Description: Binary data

0002-ui-cocoa-Fix-UI-crashes-on-macOS-Mojave.patch
Description: Binary data