Fix over-autorelease of rx::DisplayMtl::getMetalDeviceMatchingAttribute() and various Objective-C leaks. The clang static analyzer flags several leaks in Metal ANGLE code, and I noticed an over-autorelease of the return value of rx::DisplayMtl::getMetalDeviceMatchingAttribute() in the process. To make this easier/cleaner to fix, I wrote an rx::mtl::adoptObjCObj<>() helper function, which was modeled after WTF::RetainPtr<> in WebKit.
<rdar://problem/81964007>
Created attachment 435576 [details] Patch v1
Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE
Comment on attachment 435576 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=435576&action=review This looks good - and correct - to me overall, but it would be better if kpiddington, kkinnunen or dino reviewed as well. One small comment. > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/IOSurfaceSurfaceMtl.mm:190 > + if (![captureManager startCaptureWithDescriptor:captureDescriptor.get() error:&error]) Note, the example at https://developer.apple.com/documentation/metal/frame_capture_debugging_tools/capturing_gpu_command_data_programmatically?language=objc looks like startCaptureWithDescriptor:error: releases the MTLCaptureDescriptor even if an error occurred.
Comment on attachment 435576 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=435576&action=review Looks fine. I'll approve tomorrow if nobody else has yet. We should be moving towards ARC. I wonder if upstream ANGLE would have any appetite for something like this at some point: #if !__has_feature(objc_arc) #error "ANGLE requires ARC" #endif >> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/IOSurfaceSurfaceMtl.mm:190 >> + if (![captureManager startCaptureWithDescriptor:captureDescriptor.get() error:&error]) > > Note, the example at https://developer.apple.com/documentation/metal/frame_capture_debugging_tools/capturing_gpu_command_data_programmatically?language=objc looks like startCaptureWithDescriptor:error: releases the MTLCaptureDescriptor even if an error occurred. It looks to me like that example assumes you're using ARC. > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_state_cache.mm:1049 > + [newState ANGLE_MTL_RELEASE]; ditto to my comment below. > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_state_cache.mm:1242 > + [newState ANGLE_MTL_RELEASE]; This function returns a AutoObjCPtr. Why don't we just adopt the value returned by newComputePipelineStateWithFunction then remove the autorelease below? That would have the added benefit of reducing the autorelease pool size.
Comment on attachment 435576 [details] Patch v1 Clearing r? to address feedback.
Comment on attachment 435576 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=435576&action=review >>> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/IOSurfaceSurfaceMtl.mm:190 >>> + if (![captureManager startCaptureWithDescriptor:captureDescriptor.get() error:&error]) >> >> Note, the example at https://developer.apple.com/documentation/metal/frame_capture_debugging_tools/capturing_gpu_command_data_programmatically?language=objc looks like startCaptureWithDescriptor:error: releases the MTLCaptureDescriptor even if an error occurred. > > It looks to me like that example assumes you're using ARC. Yes, the coding example assumes ARC. >> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_state_cache.mm:1242 >> + [newState ANGLE_MTL_RELEASE]; > > This function returns a AutoObjCPtr. Why don't we just adopt the value returned by newComputePipelineStateWithFunction then remove the autorelease below? That would have the added benefit of reducing the autorelease pool size. LOL...good catch. Honestly, this was the last thing I fixed, and I didn't scroll up far enough to see the return type (just thought it returned a bare, autoreleased NS object pointer).
Created attachment 435684 [details] Patch v2
(In reply to David Kilzer (:ddkilzer) from comment #8) > Created attachment 435684 [details] > Patch v2 Fixed methods in mtl_state_cache.mm to remove the -autorelease and use adoptObjCObj<>() instead..
(In reply to Alex Christensen from comment #5) > We should be moving towards ARC. I wonder if upstream ANGLE would have any > appetite for something like this at some point: > #if !__has_feature(objc_arc) > #error "ANGLE requires ARC" > #endif After talking with some other Chromium engineers, we'd be glad to work toward this. Let's wait until Apple's changes to ANGLE's Metal backend are fully upstreamed - please see https://bugs.chromium.org/p/angleproject/issues/detail?id=5505 - and current ANGLE is rolled back into WebKit. Currently we're resolving 1+ year of parallel development.
Comment on attachment 435684 [details] Patch v2 Still looks good to me but I defer to achristensen's review.
Committed r281160 (240610@main): <https://commits.webkit.org/240610@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 435684 [details].
Reopening to attach new patch.
Created attachment 435800 [details] Patch v1.2 - Resolve failing test in DEBUG, make sure all selectors make it to the frontend
Comment on attachment 435800 [details] Patch v1.2 - Resolve failing test in DEBUG, make sure all selectors make it to the frontend Type in bug number, this patch doesn't belong here. Sorry.