Bug 235284 – REGRESSION (r249823): gl::Context::onProgramLink() leaks gl::Framebuffer object due early return when an error occurs in ANGLE_TRY() macro

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

[David Kilzer (:ddkilzer)]

gl::Context::onProgramLink() leaks gl::Framebuffer object due early return when an error occurs in ANGLE_TRY() macro.

The ANGLE_TRY() after `newDefaultFramebuffer` is set can return early, thus leaking the gl::Framebuffer object objects created in the previous if/else blocks:

    if (drawSurface != nullptr)
    {
        ANGLE_TRY(drawSurface->makeCurrent(this));
        newDefaultFramebuffer = drawSurface->createDefaultFramebuffer(this, readSurface);
    }
    else
    {
        newDefaultFramebuffer = new Framebuffer(this, mImplementation.get(), readSurface);
    }
    ASSERT(newDefaultFramebuffer);

    if (readSurface && (drawSurface != readSurface))
    {
        ANGLE_TRY(readSurface->makeCurrent(this));  // Leak of gl::Framebuffer on early return.
    }

Found by clang static analyzer.

Regressed with this commit:

    Update ANGLE
    <https://bugs.webkit.org/show_bug.cgi?id=201156>
    <rdar://problem/55288132>
    <https://commits.webkit.org/r249823>

[Radar WebKit Bug Importer]

<rdar://problem/87656747>

[David Kilzer (:ddkilzer)]

Created attachment 449290 [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 449290 [details]
Patch v1

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

> Source/ThirdParty/ANGLE/src/libANGLE/Context.cpp:9110
> +        newDefaultFramebuffer = std::unique_ptr<gl::Framebuffer>(drawSurface->createDefaultFramebuffer(this, readSurface));

Can createDefaultFramebuffer be changed to return a std::unique_buffer?

[Darin Adler]

Comment on attachment 449290 [details]
Patch v1

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

>> Source/ThirdParty/ANGLE/src/libANGLE/Context.cpp:9110
>> +        newDefaultFramebuffer = std::unique_ptr<gl::Framebuffer>(drawSurface->createDefaultFramebuffer(this, readSurface));
> 
> Can createDefaultFramebuffer be changed to return a std::unique_buffer?

unique_ptr

[Darin Adler]

Not sure this should be related to bug 235282 about ARC, since ARC doesn’t help us with C++ new/delete.

[David Kilzer (:ddkilzer)]

(In reply to Darin Adler from comment #6)
> Not sure this should be related to bug 235282 about ARC, since ARC doesn’t
> help us with C++ new/delete.

Fixed.

[David Kilzer (:ddkilzer)]

Comment on attachment 449290 [details]
Patch v1

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

>>> Source/ThirdParty/ANGLE/src/libANGLE/Context.cpp:9110
>>> +        newDefaultFramebuffer = std::unique_ptr<gl::Framebuffer>(drawSurface->createDefaultFramebuffer(this, readSurface));
>> 
>> Can createDefaultFramebuffer be changed to return a std::unique_buffer?
> 
> unique_ptr

It's possible to change, but it would be a non-trivial patch since it's used in so many places.  At least the function doesn't seem to be API, though.

$ nm libANGLE-shared.dylib | grep createDefaultFramebuffer | c++filt
00000000002305d2 t rx::SurfaceMtl::createDefaultFramebuffer(gl::Context const*, gl::FramebufferState const&)
000000000029e24c t rx::WindowSurfaceCGL::createDefaultFramebuffer(gl::Context const*, gl::FramebufferState const&)
0000000000231a2a t rx::WindowSurfaceMtl::createDefaultFramebuffer(gl::Context const*, gl::FramebufferState const&)
000000000018ed0c t rx::PbufferSurfaceCGL::createDefaultFramebuffer(gl::Context const*, gl::FramebufferState const&)
000000000013bb1c t rx::IOSurfaceSurfaceCGL::createDefaultFramebuffer(gl::Context const*, gl::FramebufferState const&)

At minimum, this should be done in a separate patch.

[David Kilzer (:ddkilzer)]

Comment on attachment 449290 [details]
Patch v1

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

>>>> Source/ThirdParty/ANGLE/src/libANGLE/Context.cpp:9110
>>>> +        newDefaultFramebuffer = std::unique_ptr<gl::Framebuffer>(drawSurface->createDefaultFramebuffer(this, readSurface));
>>> 
>>> Can createDefaultFramebuffer be changed to return a std::unique_buffer?
>> 
>> unique_ptr
> 
> It's possible to change, but it would be a non-trivial patch since it's used in so many places.  At least the function doesn't seem to be API, though.
> 
> $ nm libANGLE-shared.dylib | grep createDefaultFramebuffer | c++filt
> 00000000002305d2 t rx::SurfaceMtl::createDefaultFramebuffer(gl::Context const*, gl::FramebufferState const&)
> 000000000029e24c t rx::WindowSurfaceCGL::createDefaultFramebuffer(gl::Context const*, gl::FramebufferState const&)
> 0000000000231a2a t rx::WindowSurfaceMtl::createDefaultFramebuffer(gl::Context const*, gl::FramebufferState const&)
> 000000000018ed0c t rx::PbufferSurfaceCGL::createDefaultFramebuffer(gl::Context const*, gl::FramebufferState const&)
> 000000000013bb1c t rx::IOSurfaceSurfaceCGL::createDefaultFramebuffer(gl::Context const*, gl::FramebufferState const&)
> 
> At minimum, this should be done in a separate patch.

On second look, it seems like egl::Surface isn't tied to the interface of the other methods, so it may be possible to fix this.  Will investigate.

[David Kilzer (:ddkilzer)]

Created attachment 449342 [details]
Patch for landing

[David Kilzer (:ddkilzer)]

Comment on attachment 449342 [details]
Patch for landing

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

> Source/ThirdParty/ANGLE/src/libANGLE/Surface.h:-218
> -    gl::Framebuffer *createDefaultFramebuffer(const Display *display);

BTW, this method was not implemented (or used), so I removed it.

[David Kilzer (:ddkilzer)]

Comment on attachment 449342 [details]
Patch for landing

The removed method must be used on other platforms.  Investigating.

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #12)
> Comment on attachment 449342 [details]
> Patch for landing
> 
> The removed method must be used on other platforms.  Investigating.

Couldn't find any other implementation.

[David Kilzer (:ddkilzer)]

Comment on attachment 449342 [details]
Patch for landing

Adding cq+ since this isn't materially different from "Patch v1" (Attachment #449288 [details]), and the `win` bot passed on that patch.

[EWS]

Committed r288106 (246120@main): <https://commits.webkit.org/246120@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 449342 [details].

[Kimmo Kinnunen]

https://bugs.chromium.org/p/angleproject/issues/detail?id=6920