Bug 235281 – REGRESSION (r286603): rx::ProgramMtl::getSpecializedShader() leaks MTLFunctionConstantValues object due early return when an error occurs in ANGLE_TRY() macro

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

[David Kilzer (:ddkilzer)]

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>

[Radar WebKit Bug Importer]

<rdar://problem/87656387>

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

Created attachment 449288 [details]
Patch v1

[EWS Watchlist]

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

[Darin Adler]

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.

[David Kilzer (:ddkilzer)]

Created attachment 449341 [details]
Patch for landing

[David Kilzer (:ddkilzer)]

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.

[EWS]

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

[Kenneth Russell]

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