From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=0.2 required=5.0 tests=DKIM_INVALID,DKIM_SIGNED autolearn=no autolearn_force=no version=3.4.4 Received: (qmail 9654 invoked from network); 17 Jun 2021 14:08:33 -0000 Received: from 1ess.inri.net (216.126.196.35) by inbox.vuxu.org with ESMTPUTF8; 17 Jun 2021 14:08:33 -0000 Received: from mail.9lab.org ([168.119.8.41]) by 1ess; Thu Jun 17 09:59:09 -0400 2021 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=9lab.org; s=20210803; t=1623938340; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=aS46M6fhV7LV+CHR4drBAtAEtvgCHmE38uoeNI+aNMY=; b=hDmXdaW85R7PxI5sdDiZzMZ/1ga5CFk5fliRRBpZEj4tBB51BvdVU7u7KTNUwUNAzzbstw ONuSoGojox6PWDvZyZRtuENfE8m6g3zaAwUZogQy2R4R5pKiBe6UnWgLRI6BsJvq3tkNk8 2FsGVUMbqu9/VvchXYqpq66eOQDmwD0= Received: from pjw (host-185-64-155-70.ecsnet.at [185.64.155.70]) by mail.9lab.org (OpenSMTPD) with ESMTPSA id 3e2bcb11 (TLSv1.2:ECDHE-RSA-CHACHA20-POLY1305:256:NO); Thu, 17 Jun 2021 15:59:00 +0200 (CEST) Message-ID: To: 9front@9front.org CC: igor@9lab.org Date: Thu, 17 Jun 2021 15:58:30 +0200 From: igor@9lab.org MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit List-ID: <9front.9front.org> List-Help: X-Glyph: ➈ X-Bullshit: virtual abstract DOM-oriented NoSQL API-based lifecycle control Subject: [9front] drawterm/gui-cocoa/screen.m: fix SEGFAULT during cursor updates (patch) Reply-To: 9front@9front.org Precedence: bulk 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 +@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 -@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