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 <berkus+github@metta.systems>
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 <berkus+github@metta.systems>
---
 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 <berkus+github@metta.systems>
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 <berkus+github@metta.systems>
---
 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];
+
+ */
+@interface 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;
+
+@end
+
+@interface 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;
+
+@end
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"
+
+
+@implementation 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];
+    }
+}
+
+@end
+
+#pragma mark -
+
+@implementation DDInvocationGrabber (DDnvocationGrabber_Conveniences)
+
+- (id)prepareWithInvocationTarget:(id)inTarget
+{
+    [self setTarget:inTarget];
+    return(self);
+}
+
+@end
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

From 8f86e30f3710d782d78dccdbe7a1564ae79220c7 Mon Sep 17 00:00:00 2001
From: Berkus Decker <berkus+github@metta.systems>
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 <berkus+github@metta.systems>
---
 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 <berkus+github@metta.systems>
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 <berkus+github@metta.systems>
---
 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];
+
+ */
+@interface 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;
+
+@end
+
+@interface 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;
+
+@end
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"
+
+
+@implementation 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];
+    }
+}
+
+@end
+
+#pragma mark -
+
+@implementation DDInvocationGrabber (DDnvocationGrabber_Conveniences)
+
+- (id)prepareWithInvocationTarget:(id)inTarget
+{
+    [self setTarget:inTarget];
+    return(self);
+}
+
+@end
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

On 11 November 2018 at 21:24, Berkus Decker <berkus@gmail.com> wrote:
> 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

Hi; thanks for these patches. I haven't tried running them yet
(I should be able to get to that tomorrow) I have done a compile
test on High Sierra. I have some code review comments below:

> From 8f86e30f3710d782d78dccdbe7a1564ae79220c7 Mon Sep 17 00:00:00 2001
> From: Berkus Decker <berkus+github@metta.systems>
> 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 <berkus+github@metta.systems>

If you could send your patchset in git format-patch style as
multiple emails (a cover letter, and each patch in the series
a followup to the cover letter) that would be helpful, as
our patch-handling tooling assumes that setup. But if not
I can manually sort things out at my end.

> ---
>  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);

I suspect this won't compile on 10.6:
https://developer.apple.com/documentation/dispatch/1453030-dispatch_queue_create
says that DISPATCH_QUEUE_SERIAL is 10.7 or later and earlier
versions need to pass NULL.

> +
> +    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 <berkus+github@metta.systems>
> 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 <berkus+github@metta.systems>
> ---
>  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

I think it would be good to separate out this commit into two:
 * ui/cocoa: Add Dave Drivbin's DDInvocationGrabber
   (which would add the two new files and change ui/Makefile.objs
   to compile the .m file)
 * ui/cocoa: Fix UI crashes on macOS Mojave
   (which just has the ui/cocoa.m changes)

Then we have one patch whose review is just "check the license"
and one patch which is the parts that require review of the code.

(If we were willing to drop support for OSX before 10.10
we could use dispatch_sync(dispatch_get_main_queue(), ...).
Thanks for finding this bit of code to keep things working
for 10.6. It does feel a little heavyweight but on the other
hand it's existing working code.)

> 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];

Isn't there a race condition here? You've left the code
which calculates the rectangle size in the cocoa_update()
function, but it looks at properties like [cocoaView cdx]
which can be changed by user interactions like window resizing.
I think it would be better to push everything into the main
thread, so that "update screen" is atomic with respect to other
UI operations that might affect the screen coordinates.

We had this race before, of course, since the old cocoa_update()
ran on a different thread to the main event loop; but we have
the opportunity to fix it since we're pushing the code into
the main thread now.

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

thanks
-- PMM

(Hi Gerd -- I have a question at the bottom of this email
about the thread/locking semantics of the UI midlayer...)

On 20 November 2018 at 12:59, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 11 November 2018 at 21:24, Berkus Decker <berkus@gmail.com> wrote:
>> 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
>
> Hi; thanks for these patches. I haven't tried running them yet
> (I should be able to get to that tomorrow)

On High Sierra this does run for me, but seems to
introduce a bug:

 * Run qemu-system-x86_64 with no arguments
 * Let the BIOS get to the "No bootable devices" message
 * Press a key in the QEMU window

QEMU hits an assert:
ERROR:/Users/pm215/src/qemu/accel/tcg/tcg-all.c:42:tcg_handle_interrupt:
assertion failed: (qemu_mutex_iothread_locked())

with this backtrace:

* thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGABRT
  * frame #0: 0x00007fff7b123b66 libsystem_kernel.dylib`__pthread_kill + 10
    frame #1: 0x00007fff7b2ee080 libsystem_pthread.dylib`pthread_kill + 333
    frame #2: 0x00007fff7b07f1ae libsystem_c.dylib`abort + 127
    frame #3: 0x00000001014f7f19 libglib-2.0.0.dylib`g_assertion_message + 407
    frame #4: 0x00000001014f7f77
libglib-2.0.0.dylib`g_assertion_message_expr + 94
    frame #5: 0x00000001000a3509
qemu-system-x86_64`tcg_handle_interrupt(cpu=0x0000000103883200,
mask=2) at tcg-all.c:42
    frame #6: 0x00000001000e8222
qemu-system-x86_64`cpu_interrupt(cpu=0x0000000103883200, mask=2) at
cpu.h:855
    frame #7: 0x00000001000e70ca
qemu-system-x86_64`apic_local_deliver(s=0x0000000101e40010, vector=3)
at apic.c:156
    frame #8: 0x00000001000e6f71
qemu-system-x86_64`apic_deliver_pic_intr(dev=0x0000000101e40010,
level=1) at apic.c:173
    frame #9: 0x000000010010ff9e
qemu-system-x86_64`pic_irq_request(opaque=0x0000000000000000, irq=0,
level=1) at pc.c:195
    frame #10: 0x0000000100297f3b
qemu-system-x86_64`qemu_set_irq(irq=0x0000000101cdac30, level=1) at
irq.c:45
    frame #11: 0x000000010030073a
qemu-system-x86_64`qemu_irq_raise(irq=0x0000000101cdac30) at irq.h:16
    frame #12: 0x00000001003006ad
qemu-system-x86_64`pic_update_irq(s=0x0000000101cdada0) at i8259.c:111
    frame #13: 0x0000000100300d1c
qemu-system-x86_64`pic_set_irq(opaque=0x0000000101cdada0, irq=1,
level=1) at i8259.c:153
    frame #14: 0x0000000100297f3b
qemu-system-x86_64`qemu_set_irq(irq=0x0000000101cdbf50, level=1) at
irq.c:45
    frame #15: 0x000000010010cad8
qemu-system-x86_64`gsi_handler(opaque=0x0000000101cc7760, n=1,
level=1) at pc.c:118
    frame #16: 0x0000000100297f3b
qemu-system-x86_64`qemu_set_irq(irq=0x0000000101cc7410, level=1) at
irq.c:45
    frame #17: 0x00000001002fa162
qemu-system-x86_64`kbd_update_irq(s=0x0000000101e5cef8) at pckbd.c:181
    frame #18: 0x00000001002f967f
qemu-system-x86_64`kbd_update_kbd_irq(opaque=0x0000000101e5cef8,
level=1) at pckbd.c:193
    frame #19: 0x00000001002fa603
qemu-system-x86_64`ps2_queue(s=0x0000000101e5e490, b=22) at ps2.c:213
    frame #20: 0x00000001002fadd4
qemu-system-x86_64`ps2_put_keycode(opaque=0x0000000101e5e490,
keycode=60) at ps2.c:267
    frame #21: 0x00000001002fd10f
qemu-system-x86_64`ps2_keyboard_event(dev=0x0000000101e5e490,
src=0x0000000000000000, evt=0x0000000101af02c0) at ps2.c:470
    frame #22: 0x0000000100491e2c
qemu-system-x86_64`qemu_input_event_send_impl(src=0x0000000000000000,
evt=0x0000000101af02c0) at input.c:346
    frame #23: 0x000000010046c8d5
qemu-system-x86_64`replay_input_event(src=0x0000000000000000,
evt=0x0000000101af02c0) at replay-input.c:128
    frame #24: 0x0000000100491d5b
qemu-system-x86_64`qemu_input_event_send(src=0x0000000000000000,
evt=0x0000000101af02c0) at input.c:375
    frame #25: 0x0000000100492297
qemu-system-x86_64`qemu_input_event_send_key(src=0x0000000000000000,
key=0x00000001159037f0, down=true) at input.c:419
    frame #26: 0x0000000100491c71
qemu-system-x86_64`qemu_input_event_send_key_qcode(src=0x0000000000000000,
q=Q_KEY_CODE_U, down=true) at input.c:441
    frame #27: 0x0000000100496a0b qemu-system-x86_64`-[QemuCocoaView
handleEvent:](self=0x0000000101c2eef0, _cmd="handleEvent:",
event=0x0000000101cebdf0) at cocoa.m:767
    frame #28: 0x0000000100495b4e qemu-system-x86_64`-[QemuCocoaView
refresh](self=0x0000000101c2eef0, _cmd="refresh") at cocoa.m:541
    frame #29: 0x00007fff5301139c CoreFoundation`__invoking___ + 140
    frame #30: 0x00007fff53011270 CoreFoundation`-[NSInvocation invoke] + 320
    frame #31: 0x00007fff551730b5 Foundation`__NSThreadPerformPerform + 334
    frame #32: 0x00007fff53031be1
CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__
+ 17
    frame #33: 0x00007fff530e94bc CoreFoundation`__CFRunLoopDoSource0 + 108
    frame #34: 0x00007fff53014b90 CoreFoundation`__CFRunLoopDoSources0 + 208
    frame #35: 0x00007fff5301400d CoreFoundation`__CFRunLoopRun + 1293
    frame #36: 0x00007fff53013867 CoreFoundation`CFRunLoopRunSpecific + 487
    frame #37: 0x00007fff522f3d96 HIToolbox`RunCurrentEventLoopInMode + 286
    frame #38: 0x00007fff522f3b06 HIToolbox`ReceiveNextEventCommon + 613
    frame #39: 0x00007fff522f3884
HIToolbox`_BlockUntilNextEventMatchingListInModeWithFilter + 64
    frame #40: 0x00007fff505a3a73 AppKit`_DPSNextEvent + 2085
    frame #41: 0x00007fff50d39e34 AppKit`-[NSApplication(NSEvent)
_nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 3044
    frame #42: 0x00007fff50598885 AppKit`-[NSApplication run] + 764
    frame #43: 0x000000010049aac1 qemu-system-x86_64`main(argc=1,
argv=0x00007ffeefbffa40) at cocoa.m:1575
    frame #44: 0x00007fff7afd3015 libdyld.dylib`start + 1

Something somewhere in this call chain should have taken
the iothread lock, I assume, but I'm not sure where.

Gerd -- what are the rules of the UI subsystem regarding
multiple threads, and what threads are allowed to call
UI functions like qemu_input_event_send_key_qcode(),
from which threads, and whether they need to eg hold
the iothread lock when they do so? There's no
documentation on these functions in include/ui/input.h.

(To fix a problem on OSX Mojave this patchset has moved
which thread the UI executes on, so it is now always
the main thread and not the thread which calls
the QemuDisplay 'init' callback. That seems to break
an implicit assumption in the UI layer.)

thanks
-- PMM

  Hi,

> Something somewhere in this call chain should have taken
> the iothread lock, I assume, but I'm not sure where.
> 
> Gerd -- what are the rules of the UI subsystem regarding
> multiple threads, and what threads are allowed to call
> UI functions like qemu_input_event_send_key_qcode(),
> from which threads, and whether they need to eg hold
> the iothread lock when they do so? There's no
> documentation on these functions in include/ui/input.h.

UI event handling code typically runs in iothread context.  So, yes,
when calling qemu_input_* the UI code holds the iothread lock.

> (To fix a problem on OSX Mojave this patchset has moved
> which thread the UI executes on, so it is now always
> the main thread and not the thread which calls
> the QemuDisplay 'init' callback. That seems to break
> an implicit assumption in the UI layer.)

Hmm, I guess the options are (a) grab the iothread lock before calling
input layer functions, or (b) queue the event and schedule a bottom half
which processes the queue (which will then be called from iothread
context, with the lock already taken).

cheers,
  Gerd

On Thu, 22 Nov 2018 at 07:32, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > Something somewhere in this call chain should have taken
> > the iothread lock, I assume, but I'm not sure where.
> >
> > Gerd -- what are the rules of the UI subsystem regarding
> > multiple threads, and what threads are allowed to call
> > UI functions like qemu_input_event_send_key_qcode(),
> > from which threads, and whether they need to eg hold
> > the iothread lock when they do so? There's no
> > documentation on these functions in include/ui/input.h.
>
> UI event handling code typically runs in iothread context.  So, yes,
> when calling qemu_input_* the UI code holds the iothread lock.
>
> > (To fix a problem on OSX Mojave this patchset has moved
> > which thread the UI executes on, so it is now always
> > the main thread and not the thread which calls
> > the QemuDisplay 'init' callback. That seems to break
> > an implicit assumption in the UI layer.)
>
> Hmm, I guess the options are (a) grab the iothread lock before calling
> input layer functions, or (b) queue the event and schedule a bottom half
> which processes the queue (which will then be called from iothread
> context, with the lock already taken).

I was thinking about this today, and I realized that if
we just make the OSX UI thread code grab the iothread lock,
then there is a potential deadlock with the changes (also
in this patchset) which defer the DisplayChangeListener
update/switch/refresh operations to the UI thread. The
current patchset has those be synchronous deferrals, so
you could get a deadlock if the UI thread is waiting
for the iothread lock, but the code in the UI midlayer
is holding the iothread lock and waiting for a DCL
operation to be run on the main thread.

What are the required semantics for update/switch/refresh?
These don't seem to be documented. Can we validly make
those be asychronous, or does the midlayer require that
the operation has completed and been reflected onscreen
before the update/switch/refresh callback returns ?

If those have to be synchronous, then we'll need to do
something more complicated (eg the queuing of events
that you suggest), which I'd prefer it if we can avoid,
because that implies memory allocation or a fixed
length queue (plus some fairly significant restructuring
of the cocoa frontend).

thanks
-- PMM

  Hi,

> What are the required semantics for update/switch/refresh?
> These don't seem to be documented. Can we validly make
> those be asychronous, or does the midlayer require that
> the operation has completed and been reflected onscreen
> before the update/switch/refresh callback returns ?


refresh
-------

refresh() is called at regular intervals, for guest display refresh.  By
default it is called every 30 ms (GUI_REFRESH_INTERVAL_DEFAULT), UIs can
ask for a different interval using update_displaychangelistener() though.

Typically the refresh() callback will first call
graphic_hw_update(vd->dcl.con), which in turn calls into the display
device emulation for display update checks.  vga emulation will check
dirty tracking here to figure whenever the guest has updated the
display.

Next is actually updating the UI (but see below).


switch
------

Notifies the UI that the DisplaySurface has changed.  Will happen when
the guest switches the video mode.  Can also happen on pageflips.

Can happen during the graphic_hw_update() call, but can also happen
independant from that.

Qemu will free the old DisplaySurface after the switch callback returns,
so you must make sure you don't keep a reference.  These days the
DisplaySurface is just a thin wrapper around a pixman image though, and
pixman images are reference counted.  So if you want handle display
updates outside the iothread the best way to do that is to grab the
pixman image (DisplaySurface->image), get a reference to make sure it is
not released under your feet (pixman_image_ref()) and run with that in
your UI thread.


update
------

Notifies the UI that the guest has updated a specific display rectangle
and the UI should redraw it.

Can happen during the graphic_hw_update() call, but can also happen
independant from that.

You can do the UI update right here, or you can just record the
rectangles, then do a batched update in the refresh() callback.  Or
you can pass on the rectangle to your UI thread for processing.

HTH,
  Gerd