Bug 229128 – [Metal ANGLE] Fix over-autorelease of rx::DisplayMtl::getMetalDeviceMatchingAttribute() and various Objective-C leaks

• Status: RESOLVED
• Resolution: FIXED
• Component: ANGLE

[David Kilzer (:ddkilzer)]

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.

[Radar WebKit Bug Importer]

<rdar://problem/81964007>

[David Kilzer (:ddkilzer)]

Created attachment 435576 [details]
Patch v1

[EWS Watchlist]

Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE

[Kenneth Russell]

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 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 on attachment 435576 [details]
Patch v1

Clearing r? to address feedback.

[David Kilzer (:ddkilzer)]

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

Created attachment 435684 [details]
Patch v2

[David Kilzer (:ddkilzer)]

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

(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 on attachment 435684 [details]
Patch v2

Still looks good to me but I defer to achristensen's review.

[EWS]

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]

Reopening to attach new patch.

[Patrick Angle]

Created attachment 435800 [details]
Patch v1.2 - Resolve failing test in DEBUG, make sure all selectors make it to the frontend

[Patrick Angle]

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.