Bug 229128

Summary: [Metal ANGLE] Fix over-autorelease of rx::DisplayMtl::getMetalDeviceMatchingAttribute() and various Objective-C leaks
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: ANGLEAssignee: David Kilzer (:ddkilzer) <ddkilzer>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, dino, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, hi, joepeck, jonlee, kbr, kkinnunen, kondapallykalyan, kpiddington, macpherson, menard, pangle, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=235282
Bug Depends on:    
Bug Blocks: 235281    
Attachments:
Description Flags
Patch v1
none
Patch v2
none
Patch v1.2 - Resolve failing test in DEBUG, make sure all selectors make it to the frontend none

David Kilzer (:ddkilzer)
Reported 2021-08-15 19:46:45 PDT
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.
Attachments
Patch v1 (20.68 KB, patch)
2021-08-15 19:50 PDT, David Kilzer (:ddkilzer)
no flags
Patch v2 (21.50 KB, patch)
2021-08-17 07:55 PDT, David Kilzer (:ddkilzer)
no flags
Patch v1.2 - Resolve failing test in DEBUG, make sure all selectors make it to the frontend (40.91 KB, patch)
2021-08-18 14:53 PDT, Patrick Angle
no flags
Radar WebKit Bug Importer
Comment 1 2021-08-15 19:47:08 PDT
David Kilzer (:ddkilzer)
Comment 2 2021-08-15 19:50:07 PDT
Created attachment 435576 [details] Patch v1
EWS Watchlist
Comment 3 2021-08-15 19:50:58 PDT
Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE
Kenneth Russell
Comment 4 2021-08-16 15:57:45 PDT
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.
Alex Christensen
Comment 5 2021-08-16 18:18:43 PDT
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.
David Kilzer (:ddkilzer)
Comment 6 2021-08-17 04:05:10 PDT
Comment on attachment 435576 [details] Patch v1 Clearing r? to address feedback.
David Kilzer (:ddkilzer)
Comment 7 2021-08-17 07:53:48 PDT
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).
David Kilzer (:ddkilzer)
Comment 8 2021-08-17 07:55:36 PDT
Created attachment 435684 [details] Patch v2
David Kilzer (:ddkilzer)
Comment 9 2021-08-17 07:56:56 PDT
(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..
Kenneth Russell
Comment 10 2021-08-17 12:43:33 PDT
(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.
Kenneth Russell
Comment 11 2021-08-17 12:46:26 PDT
Comment on attachment 435684 [details] Patch v2 Still looks good to me but I defer to achristensen's review.
EWS
Comment 12 2021-08-17 13:32:37 PDT
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].
Patrick Angle
Comment 13 2021-08-18 14:53:03 PDT
Reopening to attach new patch.
Patrick Angle
Comment 14 2021-08-18 14:53:05 PDT
Created attachment 435800 [details] Patch v1.2 - Resolve failing test in DEBUG, make sure all selectors make it to the frontend
Patrick Angle
Comment 15 2021-08-18 14:54:22 PDT
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.
Note You need to log in before you can comment on or make changes to this bug.