9front - general discussion about 9front
 help / color / mirror / Atom feed
* [9front] drawterm/gui-cocoa/screen.m: fix SEGFAULT during cursor updates (patch)
@ 2021-06-17 13:58 igor
  2021-06-28 19:32 ` Sigrid Solveig Haflínudóttir
  0 siblings, 1 reply; 2+ messages in thread
From: igor @ 2021-06-17 13:58 UTC (permalink / raw)
  To: 9front; +Cc: igor

The symptom of the issue is a SEGFAULT (drawterm crashing) that occurs
when the Plan9 cursor is updated frequently.  Running netsurf on a
webpage with many links whilst hovering over the links reliably
triggers the crash (OSX Catalina and Big Sur) as that results in
frequent cursor updates.

The root cause of the issue is a misdeclaration of the following
variable:

drawterm/gui-cocoa/screen.m:/^static NSCursor \*currentCursor;

This causes issues with Objective-C's reference counting based memory
management.  By not `retaining` the NSCursor instance, there is a
chance that it is deallocated whilst the event loop responsible for
rendering graphics is still using it.

One solution to fix the issue is to declare the `currentCursor` as a
property of the `DrawtermView` using the `retain` attribute:

```
@interface DrawtermView : NSOpenGLView
@property (nonatomic, retain) NSCursor *currentCursor;
                      ^^^^^^
```

Declaring `currentCursor` with the `retain` attribute prevents it
from being deallocated by the OSX graphical subsystem event
loop while drawterm is still busy using it.

Crashes caused by this issue typically look like below.  Notice the
crash in `objc_retain`

```
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   libobjc.A.dylib               	0x00007fff6d9de6e8 objc_retain + 24
1   com.apple.AppKit              	0x00007fff31f4382e -[NSWindow(NSCursorRects) _addCursorRect:cursor:forView:] + 711
2   com.apple.AppKit              	0x00007fff31f432f0 -[NSView addCursorRect:cursor:] + 1036
3   drawterm                      	0x00000001051b09cb -[DrawtermView resetCursorRects] + 107 (screen.m:490)
4   com.apple.AppKit              	0x00007fff31e55d58 -[NSView(NSInternal) _updateTrackingAreasWithInvalidCursorRects:] + 678
5   com.apple.AppKit              	0x00007fff31e5634a -[NSView(NSInternal) _updateTrackingAreasWithInvalidCursorRects:] + 2200
6   com.apple.AppKit              	0x00007fff31e5634a -[NSView(NSInternal) _updateTrackingAreasWithInvalidCursorRects:] + 2200
7   com.apple.AppKit              	0x00007fff31e5583d _NSWindowDisplayCycleUpdateStructuralRegions + 319
8   com.apple.AppKit              	0x00007fff31e50064 __NSWindowGetDisplayCycleObserverForUpdateStructuralRegions_block_invoke + 420
9   com.apple.AppKit              	0x00007fff31e481f2 NSDisplayCycleObserverInvoke + 155
10  com.apple.AppKit              	0x00007fff31e47d7c NSDisplayCycleFlush + 937
11  com.apple.QuartzCore          	0x00007fff405d4e40 CA::Transaction::run_commit_handlers(CATransactionPhase) + 106
12  com.apple.QuartzCore          	0x00007fff405d3b52 CA::Transaction::commit() + 230
13  com.apple.AppKit              	0x00007fff31f03da1 __62+[CATransaction(NSCATransaction) NS_setFlushesWithDisplayLink]_block_invoke + 266
14  com.apple.AppKit              	0x00007fff32623080 ___NSRunLoopObserverCreateWithHandler_block_invoke + 41
15  com.apple.CoreFoundation      	0x00007fff34af3335 __CFRUNLOOP_IS_CALLING_OUT_TO_AN_OBSERVER_CALLBACK_FUNCTION__ + 23
16  com.apple.CoreFoundation      	0x00007fff34af3267 __CFRunLoopDoObservers + 457
17  com.apple.CoreFoundation      	0x00007fff34af2805 __CFRunLoopRun + 874
18  com.apple.CoreFoundation      	0x00007fff34af1e3e CFRunLoopRunSpecific + 462
19  com.apple.HIToolbox           	0x00007fff3371eabd RunCurrentEventLoopInMode + 292
20  com.apple.HIToolbox           	0x00007fff3371e7d5 ReceiveNextEventCommon + 584
21  com.apple.HIToolbox           	0x00007fff3371e579 _BlockUntilNextEventMatchingListInModeWithFilter + 64
22  com.apple.AppKit              	0x00007fff31d64039 _DPSNextEvent + 883
23  com.apple.AppKit              	0x00007fff31d62880 -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 1352
24  com.apple.AppKit              	0x00007fff31d5458e -[NSApplication run] + 658
25  com.apple.AppKit              	0x00007fff31d26396 NSApplicationMain + 777
26  drawterm                      	0x000000010514a78c cpumain + 1660 (cpu.c:339)
27  drawterm                      	0x000000010514980c main + 412 (main.c:67)
28  libdyld.dylib                 	0x00007fff6eb93cc9 start + 1
```

```
Application Specific Information:
=================================================================
==46501==ERROR: AddressSanitizer: SEGV on unknown address 0x7fff7c800150 (pc 0x7fff6d9de6e8 bp 0x7ffee2f416f0 sp 0x7ffee2f41648 T0)
==46501==The signal is caused by a READ memory access.
    #0 0x7fff6d9de6e8 in objc_retain+0x18 (libobjc.A.dylib:x86_64h+0x66e8)
    #1 0x7fff31f432ef in -[NSView addCursorRect:cursor:]+0x40b (AppKit:x86_64+0x2202ef)
    #2 0x10cf175c6 in -[DrawtermView resetCursorRects] screen.m:495
    #3 0x7fff31e55d57 in -[NSView(NSInternal) _updateTrackingAreasWithInvalidCursorRects:]+0x2a5 (AppKit:x86_64+0x132d57)
    #4 0x7fff31e56349 in -[NSView(NSInternal) _updateTrackingAreasWithInvalidCursorRects:]+0x897 (AppKit:x86_64+0x133349)
    #5 0x7fff31e56349 in -[NSView(NSInternal) _updateTrackingAreasWithInvalidCursorRects:]+0x897 (AppKit:x86_64+0x133349)
    #6 0x7fff31e5583c in _NSWindowDisplayCycleUpdateStructuralRegions+0x13e (AppKit:x86_64+0x13283c)
    #7 0x7fff31e50063 in __NSWindowGetDisplayCycleObserverForUpdateStructuralRegions_block_invoke+0x1a3 (AppKit:x86_64+0x12d063)
    #8 0x7fff31e481f1 in NSDisplayCycleObserverInvoke+0x9a (AppKit:x86_64+0x1251f1)
    #9 0x7fff31e47d7b in NSDisplayCycleFlush+0x3a8 (AppKit:x86_64+0x124d7b)
    #10 0x7fff405d4e3f in CA::Transaction::run_commit_handlers(CATransactionPhase)+0x69 (QuartzCore:x86_64+0x3e3f)
    #11 0x7fff405d3b51 in CA::Transaction::commit()+0xe5 (QuartzCore:x86_64+0x2b51)
    #12 0x7fff31f03da0 in __62+[CATransaction(NSCATransaction) NS_setFlushesWithDisplayLink]_block_invoke+0x109 (AppKit:x86_64+0x1e0da0)
    #13 0x7fff3262307f in ___NSRunLoopObserverCreateWithHandler_block_invoke+0x28 (AppKit:x86_64+0x90007f)
    #14 0x7fff34af3334 in __CFRUNLOOP_IS_CALLING_OUT_TO_AN_OBSERVER_CALLBACK_FUNCTION__+0x16 (CoreFoundation:x86_64h+0x83334)
    #15 0x7fff34af3266 in __CFRunLoopDoObservers+0x1c8 (CoreFoundation:x86_64h+0x83266)
    #16 0x7fff34af2804 in __CFRunLoopRun+0x369 (CoreFoundation:x86_64h+0x82804)
    #17 0x7fff34af1e3d in CFRunLoopRunSpecific+0x1cd (CoreFoundation:x86_64h+0x81e3d)
    #18 0x7fff3371eabc in RunCurrentEventLoopInMode+0x123 (HIToolbox:x86_64+0x2fabc)
    #19 0x7fff3371e7d4 in ReceiveNextEventCommon+0x247 (HIToolbox:x86_64+0x2f7d4)
    #20 0x7fff3371e578 in _BlockUntilNextEventMatchingListInModeWithFilter+0x3f (HIToolbox:x86_64+0x2f578)
    #21 0x7fff31d64038 in _DPSNextEvent+0x372 (AppKit:x86_64+0x41038)
    #22 0x7fff31d6287f in -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:]+0x547 (AppKit:x86_64+0x3f87f)
    #23 0x7fff31d5458d in -[NSApplication run]+0x291 (AppKit:x86_64+0x3158d)
    #24 0x7fff31d26395 in NSApplicationMain+0x308 (AppKit:x86_64+0x3395)
    #25 0x10cf0cd14 in guimain screen.m:32
    #26 0x10ccc249e in cpumain cpu.c:336
    #27 0x10ccbf341 in main main.c:66
    #28 0x7fff6eb93cc8 in start+0x0 (libdyld.dylib:x86_64+0x1acc8)
```

The patch contains the following modifications:

  1. Fix declaration of `currentCursor` declaring it as a
     property of the `DrawtermView` using the `retain`
     attribute. This prevents drawterm from crashing as
     the OSX graphical event loop refrains from deallocating
     it while it is still in use.

  2. Modification of gui-cocoa/screen.m:/^setcursor to use
     'getBitmapDataPlanes' API function instead of
     'static unsigned char data[64]' container.
     (https://developer.apple.com/documentation/appkit/nsbitmapimagerep/1395490-getbitmapdataplanes?language=objc)
     The idea is to avoid using static variables for memory
     managed/supplied by OSX APIs.

  3. Replace 'static GLuint tex;' and declare it as a
     property of the `DrawtermView` as it is part of the
     `DrawtermView`.

  4. Move up declaration of 'DrawtermView' within the file.
     Once the static variables like 'currentCursor'
     and 'tex' have been moved to be properties of the DrawtermView
     this change made sense.

This patch has been tested on OSX Catalina and Big Sur.  Let me know
if there is something you dislike and want me to change.

Cheers,
Igor


term% cd $home/src/drawterm
term% git/diff
--- /usr/igor/src/drawterm/.git/fs/object/0e9b2f46e70a3c775eb541a754d4402c1e454221/tree/gui-cocoa/screen.m
+++ gui-cocoa/screen.m
@@ -16,14 +16,44 @@
 #include "screen.h"
 #include "keyboard.h"
 
-Memimage *gscreen;
+@interface AppDelegate : NSObject <NSApplicationDelegate>
+@end
 
-static NSOpenGLView *myview;
-static NSSize winsize;
-static NSCursor *currentCursor;
+@interface AppDelegate ()
+@property (assign) IBOutlet NSWindow *window;
+@property (assign) IBOutlet NSOpenGLView *view;
+@end
 
-static GLuint tex;
+@interface DrawtermView : NSOpenGLView
+@property (nonatomic, retain) NSCursor *currentCursor;
+@property (nonatomic, assign) GLuint tex;
 
+- (void) drawRect:(NSRect)rect;
+- (void) keyDown:(NSEvent*)event;
+- (void) flagsChanged:(NSEvent*)event;
+- (void) keyUp:(NSEvent*)event;
+- (void) mouseDown:(NSEvent*)event;
+- (void) mouseDragged:(NSEvent*)event;
+- (void) mouseUp:(NSEvent*)event;
+- (void) mouseMoved:(NSEvent*)event;
+- (void) rightMouseDown:(NSEvent*)event;
+- (void) rightMouseDragged:(NSEvent*)event;
+- (void) rightMouseUp:(NSEvent*)event;
+- (void) otherMouseDown:(NSEvent*)event;
+- (void) otherMouseDragged:(NSEvent*)event;
+- (void) otherMouseUp:(NSEvent*)event;
+- (void) scrollWheel:(NSEvent*)event;
+- (BOOL) acceptsFirstResponder;
+- (void) reshape;
+- (BOOL) acceptsMouseMovedEvents;
+- (void) prepareOpenGL;
+- (void) resetCursorRects;
+@end
+
+Memimage *gscreen;
+static DrawtermView *myview;
+static NSSize winsize;
+
 void
 guimain(void)
 {
@@ -102,7 +132,7 @@
 	unloadmemimage(gscreen, r, buf, sz);
 	dispatch_async(dispatch_get_main_queue(), ^(void){
 		[[myview openGLContext] makeCurrentContext];
-		glBindTexture(GL_TEXTURE_2D, tex);
+		glBindTexture(GL_TEXTURE_2D, myview.tex);
 		glTexSubImage2D(GL_TEXTURE_2D, 0, r.min.x, r.min.y, Dx(r), Dy(r), GL_RGBA, GL_UNSIGNED_BYTE, buf);
 		free(buf);
 		[NSOpenGLContext clearCurrentContext];
@@ -123,17 +153,13 @@
 void
 setcursor(void)
 {
-	static unsigned char data[64];
-	unsigned char *planes[2] = {&data[0], &data[32]};
-	int i;
+	NSPoint p;
+	NSBitmapImageRep *r;
+	unsigned char *plane[5];
+	int b;
 
-	lock(&cursor.lk);
-	for(i = 0; i < 32; i++){
-		data[i] = ~cursor.set[i] & cursor.clr[i];
-		data[i+32] = cursor.set[i] | cursor.clr[i];
-	}
-	NSBitmapImageRep *rep = [[NSBitmapImageRep alloc]
-		initWithBitmapDataPlanes:planes
+	r = [[NSBitmapImageRep alloc]
+		initWithBitmapDataPlanes:nil
 		pixelsWide:16
 		pixelsHigh:16
 		bitsPerSample:1
@@ -144,11 +170,22 @@
 		bitmapFormat:0
 		bytesPerRow:2
 		bitsPerPixel:0];
-	NSImage *img = [[NSImage alloc] initWithSize:NSMakeSize(16, 16)];
-	[img addRepresentation:rep];
-	currentCursor = [[NSCursor alloc] initWithImage:img hotSpot:NSMakePoint(-cursor.offset.x, -cursor.offset.y)];
+	[r getBitmapDataPlanes:plane];
+	assert(plane[0]!=nil && plane[1]!=nil);
+
+	lock(&cursor.lk);
+	for(b=0; b < nelem(cursor.set); b++){
+		plane[0][b] = ~cursor.set[b] & cursor.clr[b];
+		plane[1][b] =  cursor.set[b] | cursor.clr[b];
+	}
+	p = NSMakePoint(-cursor.offset.x, -cursor.offset.y);
 	unlock(&cursor.lk);
 
+	NSImage *i = [[NSImage alloc] initWithSize:NSMakeSize(16, 16)];
+	[i addRepresentation:r];
+
+	myview.currentCursor = [[NSCursor alloc] initWithImage:i hotSpot:p];
+
 	dispatch_async(dispatch_get_main_queue(), ^(void){
 		[[myview window] invalidateCursorRectsForView:myview];
 	});
@@ -169,14 +206,7 @@
 	});
 }
 
-@interface AppDelegate : NSObject <NSApplicationDelegate>
-@end
 
-@interface AppDelegate ()
-@property (assign) IBOutlet NSWindow *window;
-@property (assign) IBOutlet NSOpenGLView *view;
-@end
-
 @implementation AppDelegate
 
 void
@@ -204,34 +234,12 @@
 
 @end
 
-@interface DrawtermView : NSOpenGLView
-- (void) drawRect:(NSRect)rect;
-- (void) keyDown:(NSEvent*)event;
-- (void) flagsChanged:(NSEvent*)event;
-- (void) keyUp:(NSEvent*)event;
-- (void) mouseDown:(NSEvent*)event;
-- (void) mouseDragged:(NSEvent*)event;
-- (void) mouseUp:(NSEvent*)event;
-- (void) mouseMoved:(NSEvent*)event;
-- (void) rightMouseDown:(NSEvent*)event;
-- (void) rightMouseDragged:(NSEvent*)event;
-- (void) rightMouseUp:(NSEvent*)event;
-- (void) otherMouseDown:(NSEvent*)event;
-- (void) otherMouseDragged:(NSEvent*)event;
-- (void) otherMouseUp:(NSEvent*)event;
-- (void) scrollWheel:(NSEvent*)event;                                                                                                                                                                                                                                                                                                                                                                                                   
-- (BOOL) acceptsFirstResponder;
-- (void) reshape;
-- (BOOL) acceptsMouseMovedEvents;
-- (void) prepareOpenGL;
-- (void) resetCursorRects;
-@end
 
 @implementation DrawtermView
 
 - (void) prepareOpenGL {
-	glGenTextures(1, &tex);
-	glBindTexture(GL_TEXTURE_2D, tex);
+	glGenTextures(1, &_tex);
+	glBindTexture(GL_TEXTURE_2D, _tex);
 	glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
 	glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST);
 	glEnable(GL_TEXTURE_2D);
@@ -244,7 +252,7 @@
 
 - (void) drawRect:(NSRect)rect
 {
-	glBindTexture(GL_TEXTURE_2D, tex);
+	glBindTexture(GL_TEXTURE_2D, _tex);
 	glClearColor(0, 0, 0, 0);
 	glClear(GL_COLOR_BUFFER_BIT);
 	glColor4f(1.0, 1.0, 1.0, 1.0);
@@ -456,10 +464,11 @@
 }
 
 - (void) reshape {
+	[super reshape];
 	winsize = self.frame.size;
 	NSOpenGLContext *ctxt = [NSOpenGLContext currentContext];
 	[[myview openGLContext] makeCurrentContext];
-	glBindTexture(GL_TEXTURE_2D, tex);
+	glBindTexture(GL_TEXTURE_2D, _tex);
 	glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, self.frame.size.width, self.frame.size.height, 0, GL_RGBA, GL_UNSIGNED_BYTE, NULL);
 	if(ctxt == nil)
 		[NSOpenGLContext clearCurrentContext];
@@ -477,6 +486,7 @@
 
 - (void)resetCursorRects {
 	[super resetCursorRects];
-	[self addCursorRect:self.bounds cursor:currentCursor];
+	if (self.currentCursor != nil)
+		[self addCursorRect:self.bounds cursor:self.currentCursor];
 }
 @end


^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [9front] drawterm/gui-cocoa/screen.m: fix SEGFAULT during cursor updates (patch)
  2021-06-17 13:58 [9front] drawterm/gui-cocoa/screen.m: fix SEGFAULT during cursor updates (patch) igor
@ 2021-06-28 19:32 ` Sigrid Solveig Haflínudóttir
  0 siblings, 0 replies; 2+ messages in thread
From: Sigrid Solveig Haflínudóttir @ 2021-06-28 19:32 UTC (permalink / raw)
  To: 9front

This is merged now, thanks.


^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2021-06-28 19:40 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-17 13:58 [9front] drawterm/gui-cocoa/screen.m: fix SEGFAULT during cursor updates (patch) igor
2021-06-28 19:32 ` Sigrid Solveig Haflínudóttir

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).