Bug 235284 - REGRESSION (r249823): gl::Context::onProgramLink() leaks gl::Framebuffer object due early return when an error occurs in ANGLE_TRY() macro
Summary: REGRESSION (r249823): gl::Context::onProgramLink() leaks gl::Framebuffer obje...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: ANGLE (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on: 201156
Blocks:
  Show dependency treegraph
 
Reported: 2022-01-16 14:33 PST by David Kilzer (:ddkilzer)
Modified: 2022-04-05 12:24 PDT (History)
8 users (show)

See Also:


Attachments
Patch v1 (3.20 KB, patch)
2022-01-16 14:39 PST, David Kilzer (:ddkilzer)
darin: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (5.62 KB, patch)
2022-01-17 11:32 PST, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2022-01-16 14:33:20 PST
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>
Comment 1 Radar WebKit Bug Importer 2022-01-16 14:34:02 PST
<rdar://problem/87656747>
Comment 2 David Kilzer (:ddkilzer) 2022-01-16 14:39:24 PST
Created attachment 449290 [details]
Patch v1
Comment 3 EWS Watchlist 2022-01-16 14:40:26 PST
Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE
Comment 4 Darin Adler 2022-01-16 16:12:16 PST
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?
Comment 5 Darin Adler 2022-01-16 16:12:28 PST
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
Comment 6 Darin Adler 2022-01-16 16:13:21 PST
Not sure this should be related to bug 235282 about ARC, since ARC doesn’t help us with C++ new/delete.
Comment 7 David Kilzer (:ddkilzer) 2022-01-17 10:20:35 PST
(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.
Comment 8 David Kilzer (:ddkilzer) 2022-01-17 10:44:29 PST
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.
Comment 9 David Kilzer (:ddkilzer) 2022-01-17 10:48:41 PST
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.
Comment 10 David Kilzer (:ddkilzer) 2022-01-17 11:32:21 PST
Created attachment 449342 [details]
Patch for landing
Comment 11 David Kilzer (:ddkilzer) 2022-01-17 11:36:59 PST
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.
Comment 12 David Kilzer (:ddkilzer) 2022-01-17 12:04:46 PST
Comment on attachment 449342 [details]
Patch for landing

The removed method must be used on other platforms.  Investigating.
Comment 13 David Kilzer (:ddkilzer) 2022-01-17 12:10:30 PST
(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.
Comment 14 David Kilzer (:ddkilzer) 2022-01-17 17:26:02 PST
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.
Comment 15 EWS 2022-01-17 17:40:14 PST
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].