1187032 - Resource leak in Blit9::copy2D

Status: NEW

(Core :: Graphics, defect)

Description

[q1]

Blit9::copy2D (gfx\angle\src\libGLESv2\renderer\d3d\d3d9\Blit9.cpp) leaks an IDirect3DSurface9 object if its call to getSurfaceLevel fails:

...
250:    IDirect3DSurface9 *source = renderTarget9->getSurface();
251:    ASSERT(source);
252:
253:    IDirect3DSurface9 *destSurface = NULL;
254:    TextureStorage9_2D *storage9 = TextureStorage9_2D::makeTextureStorage9_2D(storage);
255:    error = storage9->getSurfaceLevel(level, true, &destSurface);
256:    if (error.isError())
257:    {
258:        return error;
259:    }
260:    ASSERT(destSurface);
261:
262:    gl::Error result = copy(source, sourceRect, destFormat, xoffset, yoffset, destSurface);
263:
264:    SafeRelease(destSurface);
265:    SafeRelease(source);
266:
267:    return result;
268:}

Line 250 |addref|s source, and line 265 properly |release|es it, but the |if| statement on lines 256-259 does not.

Comment 1

[q1]

There is a similar bug in Blit9::copyCube lines 295-98 in the same module.

Comment 2

[Sotaro Ikeda [:sotaro]]

jgilbert, can you check this bug?

Comment 3

[Kelsey Gilbert [:jgilbert]]

This looks likely.

Comment 4

[Kelsey Gilbert [:jgilbert]]

Oh god, they're doing manual addref/release.

Comment 5

[Kelsey Gilbert [:jgilbert]]

This is even true on top-of-tree ANGLE master.

Comment 6

[Kelsey Gilbert [:jgilbert]]

Fortunately, this should only be a problem for us for OOMs, which are the only errors we don't catch before issuing commands to the driver.

Comment 7

[Kelsey Gilbert [:jgilbert]]

ANGLE bug:
https://code.google.com/p/angleproject/issues/detail?id=1111

Comment 8

[q1]

(In reply to Jeff Gilbert [:jgilbert] from comment #6)
> Fortunately, this should only be a problem for us for OOMs, which are the
> only errors we don't catch before issuing commands to the driver.

I guess that the only non-logic-error reason getSurfaceLevel can fail is OOM. 'Twould be nicer if this code used the RAII pattern. In a somewhat-related vein, do you know if there are any plans to use exceptions more widely in Mozilla code?