Bug 235281

Summary: REGRESSION (r286603): rx::ProgramMtl::getSpecializedShader() leaks MTLFunctionConstantValues object due early return when an error occurs in ANGLE_TRY() macro
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: ANGLEAssignee: David Kilzer (:ddkilzer) <ddkilzer>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, dino, ews-watchlist, geofflang, gman, jonahr, kbr, kkinnunen, kondapallykalyan, kpiddington, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=235282
Bug Depends on: 220896, 229128    
Bug Blocks:    
Attachments:
Description Flags
Patch v1
darin: review+, ews-feeder: commit-queue-
Patch for landing none

Description David Kilzer (:ddkilzer) 2022-01-16 13:59:51 PST
rx::angle::Result ProgramMtl::getSpecializedShader() leaks MTLFunctionConstantValues object due early return when an error occurs in ANGLE_TRY() macro.

The early return occurs before the object is autoreleased:

    // Create Metal shader object
    ANGLE_MTL_OBJC_SCOPE
    {
        ANGLE_TRY(CreateMslShader(context, translatedMslInfo->metalLibrary, SHADER_ENTRY_NAME,
                                  funcConstants, &shaderVariant->metalShader));
        [funcConstants ANGLE_MTL_AUTORELEASE];
    }

Found by clang static analyzer.

Regressed with this commit:

    Roll ANGLE to include upstreamed Metal backend
    <https://bugs.webkit.org/show_bug.cgi?id=220896>
    <rdar://problem/73539682>
    <https://commits.webkit.org/r286603>
Comment 1 Radar WebKit Bug Importer 2022-01-16 14:00:20 PST
<rdar://problem/87656387>
Comment 2 David Kilzer (:ddkilzer) 2022-01-16 14:18:20 PST
(In reply to David Kilzer (:ddkilzer) from comment #0)
> Regressed with this commit:
> 
>     Roll ANGLE to include upstreamed Metal backend
>     <https://bugs.webkit.org/show_bug.cgi?id=220896>
>     <rdar://problem/73539682>
>     <https://commits.webkit.org/r286603>

Huh, that commit actually reverted some changes I made in r281160 to fix this leak earlier!

    [Metal ANGLE] Fix over-autorelease of rx::DisplayMtl::getMetalDeviceMatchingAttribute() and various Objective-C leak
s
    <https://webkit.org/b/229128>
    <rdar://problem/81964007>
    <https://commits.webkit.org/r281160>
Comment 3 David Kilzer (:ddkilzer) 2022-01-16 14:22:00 PST
Created attachment 449288 [details]
Patch v1
Comment 4 EWS Watchlist 2022-01-16 14:23:18 PST
Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE
Comment 5 Darin Adler 2022-01-16 15:43:35 PST
Comment on attachment 449288 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=449288&action=review

We can all agree that autorelease is the worst

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/ProgramMtl.mm:643
> +            [funcConstants.get() setConstantValue:&emulateDiscard

I don’t think this .get() is needed, same for the 5 more below, unless mtl::AutoObjCObj is different from RetainPtr in this respect.
Comment 6 David Kilzer (:ddkilzer) 2022-01-17 10:23:18 PST
Created attachment 449341 [details]
Patch for landing
Comment 7 David Kilzer (:ddkilzer) 2022-01-17 10:24:19 PST
Comment on attachment 449288 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=449288&action=review

>> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/ProgramMtl.mm:643
>> +            [funcConstants.get() setConstantValue:&emulateDiscard
> 
> I don’t think this .get() is needed, same for the 5 more below, unless mtl::AutoObjCObj is different from RetainPtr in this respect.

Yes!  Will remove and land a follow-up fix for Bug 235278.
Comment 8 EWS 2022-01-17 16:57:21 PST
Committed r288105 (246119@main): <https://commits.webkit.org/246119@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 449341 [details].
Comment 9 Kenneth Russell 2022-01-18 16:08:44 PST
David, Darin - FYI Kyle, myself and several other colleagues are actively working on resolving divergence between WebKit's copy of ANGLE, and upstream ANGLE. Filed:

Upstream fix for MTLFunctionConstantValues leak in rx::ProgramMtl::getSpecializedShader()
https://bugs.chromium.org/p/angleproject/issues/detail?id=6920