Bug 89346 – [Cairo] Fix memory leaks in GLContextGLX.cpp

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Sudarsana Nagineni (babu)]

The following memory leaks found in GLContextGLX with different scenarios.

1) The last element in the ActiveContextList is not getting deleted, so pbuffer returned by glXCreatePbuffer is leaking.
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/glx/GLContextGLX.cpp#L74

==27018== 264 (208 direct, 56 indirect) bytes in 1 blocks are definitely lost in loss record 13,415 of 14,618
==27018==    at 0x4C2B6CD: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==27018==    by 0x1091340A: ??? (in /usr/lib/x86_64-linux-gnu/mesa/libGL.so.1.2)
==27018==    by 0x1090D513: ??? (in /usr/lib/x86_64-linux-gnu/mesa/libGL.so.1.2)
==27018==    by 0x1090DAC2: ??? (in /usr/lib/x86_64-linux-gnu/mesa/libGL.so.1.2)
==27018==    by 0x8276603: WebCore::GLContextGLX::createPbufferContext(__GLXcontextRec*) (GLContextGLX.cpp:135)
==27018==    by 0x82769A1: WebCore::GLContextGLX::createContext(unsigned long, WebCore::GLContext*) (GLContextGLX.cpp:212)
==27018==    by 0x8277AFA: WebCore::GLContext::createOffscreenContext(WebCore::GLContext*) (GLContext.cpp:54)
==27018==    by 0x7CE3C64: WebCore::GraphicsContext3DPrivate::GraphicsContext3DPrivate(WebCore::GraphicsContext3D*) (GraphicsContext3DPrivate.cpp:41)
==27018==    by 0x7CE3BF8: WebCore::GraphicsContext3DPrivate::create(WebCore::GraphicsContext3D*) (GraphicsContext3DPrivate.cpp:36)
==27018==    by 0x7CE26E7: WebCore::GraphicsContext3D::GraphicsContext3D(WebCore::GraphicsContext3D::Attributes, WebCore::HostWindow*, bool) (GraphicsContext3DCairo.cpp:79)
==27018==    by 0x7CE254A: WebCore::GraphicsContext3D::create(WebCore::GraphicsContext3D::Attributes, WebCore::HostWindow*, WebCore::GraphicsContext3D::RenderStyle) (GraphicsContext3DCairo.cpp:62)
==27018==    by 0x7CAD268: WebCore::WebGLRenderingContext::create(WebCore::HTMLCanvasElement*, WebCore::WebGLContextAttributes*) (WebGLRenderingContext.cpp:414)
==27018==    by 0x737AE71: WebCore::HTMLCanvasElement::getContext(WTF::String const&, WebCore::CanvasContextAttributes*) (HTMLCanvasElement.cpp:201)
==27018==    by 0x6F640BF: WebCore::JSHTMLCanvasElement::getContext(JSC::ExecState*) (JSHTMLCanvasElementCustom.cpp:77)
==27018==    by 0x7E54616: WebCore::jsHTMLCanvasElementPrototypeFunctionGetContext(JSC::ExecState*) (JSHTMLCanvasElement.cpp:210)

2) pbuffer is leaking when the context is null.
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/glx/GLContextGLX.cpp#L141

==11123== 1,488 (208 direct, 1,280 indirect) bytes in 1 blocks are definitely lost in loss record 13,955 of 14,422
==11123==    at 0x4C2B6CD: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==11123==    by 0x1091340A: ??? (in /usr/lib/x86_64-linux-gnu/mesa/libGL.so.1.2)
==11123==    by 0x1090D513: ??? (in /usr/lib/x86_64-linux-gnu/mesa/libGL.so.1.2)
==11123==    by 0x1090DAC2: ??? (in /usr/lib/x86_64-linux-gnu/mesa/libGL.so.1.2)
==11123==    by 0x8276603: WebCore::GLContextGLX::createPbufferContext(__GLXcontextRec*) (GLContextGLX.cpp:135)
==11123==    by 0x8276995: WebCore::GLContextGLX::createContext(unsigned long, WebCore::GLContext*) (GLContextGLX.cpp:212)
==11123==    by 0x8277AEE: WebCore::GLContext::createOffscreenContext(WebCore::GLContext*) (GLContext.cpp:54)
==11123==    by 0x8277A09: WebCore::GLContext::sharingContext() (GLContext.cpp:35)
==11123==    by 0x7CE3C52: WebCore::GraphicsContext3DPrivate::GraphicsContext3DPrivate(WebCore::GraphicsContext3D*) (GraphicsContext3DPrivate.cpp:41)
==11123==    by 0x7CE3BF8: WebCore::GraphicsContext3DPrivate::create(WebCore::GraphicsContext3D*) (GraphicsContext3DPrivate.cpp:36)
==11123==    by 0x7CE26E7: WebCore::GraphicsContext3D::GraphicsContext3D(WebCore::GraphicsContext3D::Attributes, WebCore::HostWindow*, bool) (GraphicsContext3DCairo.cpp:79)
==11123==    by 0x7CE254A: WebCore::GraphicsContext3D::create(WebCore::GraphicsContext3D::Attributes, WebCore::HostWindow*, WebCore::GraphicsContext3D::RenderStyle) (GraphicsContext3DCairo.cpp:62)
==11123==    by 0x7CAD268: WebCore::WebGLRenderingContext::create(WebCore::HTMLCanvasElement*, WebCore::WebGLContextAttributes*) (WebGLRenderingContext.cpp:414)
==11123==    by 0x737AE71: WebCore::HTMLCanvasElement::getContext(WTF::String const&, WebCore::CanvasContextAttributes*) (HTMLCanvasElement.cpp:201)
==11123==    by 0x6F640BF: WebCore::JSHTMLCanvasElement::getContext(JSC::ExecState*) (JSHTMLCanvasElementCustom.cpp:77)
==11123==    by 0x7E54616: WebCore::jsHTMLCanvasElementPrototypeFunctionGetContext(JSC::ExecState*) (JSHTMLCanvasElement.cpp:210)

3) The reason for this leak is same as 1), when we call createPixmapContext().

==11123== 32 bytes in 1 blocks are definitely lost in loss record 4,574 of 14,422
==11123==    at 0x4C2B6CD: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==11123==    by 0x108EBAE9: glXCreateGLXPixmap (in /usr/lib/x86_64-linux-gnu/mesa/libGL.so.1.2)
==11123==    by 0x8276808: WebCore::GLContextGLX::createPixmapContext(__GLXcontextRec*) (GLContextGLX.cpp:184)
==11123==    by 0x82769D7: WebCore::GLContextGLX::createContext(unsigned long, WebCore::GLContext*) (GLContextGLX.cpp:214)
==11123==    by 0x8277AEE: WebCore::GLContext::createOffscreenContext(WebCore::GLContext*) (GLContext.cpp:54)
==11123==    by 0x7CE3C64: WebCore::GraphicsContext3DPrivate::GraphicsContext3DPrivate(WebCore::GraphicsContext3D*) (GraphicsContext3DPrivate.cpp:41)
==11123==    by 0x7CE3BF8: WebCore::GraphicsContext3DPrivate::create(WebCore::GraphicsContext3D*) (GraphicsContext3DPrivate.cpp:36)
==11123==    by 0x7CE26E7: WebCore::GraphicsContext3D::GraphicsContext3D(WebCore::GraphicsContext3D::Attributes, WebCore::HostWindow*, bool) (GraphicsContext3DCairo.cpp:79)
==11123==    by 0x7CE254A: WebCore::GraphicsContext3D::create(WebCore::GraphicsContext3D::Attributes, WebCore::HostWindow*, WebCore::GraphicsContext3D::RenderStyle) (GraphicsContext3DCairo.cpp:62)
==11123==    by 0x7CAD268: WebCore::WebGLRenderingContext::create(WebCore::HTMLCanvasElement*, WebCore::WebGLContextAttributes*) (WebGLRenderingContext.cpp:414)
==11123==    by 0x737AE71: WebCore::HTMLCanvasElement::getContext(WTF::String const&, WebCore::CanvasContextAttributes*) (HTMLCanvasElement.cpp:201)
==11123==    by 0x6F640BF: WebCore::JSHTMLCanvasElement::getContext(JSC::ExecState*) (JSHTMLCanvasElementCustom.cpp:77)
==11123==    by 0x7E54616: WebCore::jsHTMLCanvasElementPrototypeFunctionGetContext(JSC::ExecState*) (JSHTMLCanvasElement.cpp:210)

4) visualInfo is leaking here.
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/glx/GLContextGLX.cpp#L185

==11123== 640 bytes in 1 blocks are definitely lost in loss record 13,620 of 14,422
==11123==    at 0x4C2B6CD: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==11123==    by 0x11DB7915: XGetVisualInfo (VisUtil.c:78)
==11123==    by 0x108EAE5F: glXChooseVisual (in /usr/lib/x86_64-linux-gnu/mesa/libGL.so.1.2)
==11123==    by 0x827671C: WebCore::GLContextGLX::createPixmapContext(__GLXcontextRec*) (GLContextGLX.cpp:168)
==11123==    by 0x82769D7: WebCore::GLContextGLX::createContext(unsigned long, WebCore::GLContext*) (GLContextGLX.cpp:214)
==11123==    by 0x8277AEE: WebCore::GLContext::createOffscreenContext(WebCore::GLContext*) (GLContext.cpp:54)
==11123==    by 0x8277A09: WebCore::GLContext::sharingContext() (GLContext.cpp:35)
==11123==    by 0x7CE3C52: WebCore::GraphicsContext3DPrivate::GraphicsContext3DPrivate(WebCore::GraphicsContext3D*) (GraphicsContext3DPrivate.cpp:41)
==11123==    by 0x7CE3BF8: WebCore::GraphicsContext3DPrivate::create(WebCore::GraphicsContext3D*) (GraphicsContext3DPrivate.cpp:36)
==11123==    by 0x7CE26E7: WebCore::GraphicsContext3D::GraphicsContext3D(WebCore::GraphicsContext3D::Attributes, WebCore::HostWindow*, bool) (GraphicsContext3DCairo.cpp:79)
==11123==    by 0x7CE254A: WebCore::GraphicsContext3D::create(WebCore::GraphicsContext3D::Attributes, WebCore::HostWindow*, WebCore::GraphicsContext3D::RenderStyle) (GraphicsContext3DCairo.cpp:62)
==11123==    by 0x7CAD268: WebCore::WebGLRenderingContext::create(WebCore::HTMLCanvasElement*, WebCore::WebGLContextAttributes*) (WebGLRenderingContext.cpp:414)
==11123==    by 0x737AE71: WebCore::HTMLCanvasElement::getContext(WTF::String const&, WebCore::CanvasContextAttributes*) (HTMLCanvasElement.cpp:201)
==11123==    by 0x6F640BF: WebCore::JSHTMLCanvasElement::getContext(JSC::ExecState*) (JSHTMLCanvasElementCustom.cpp:77)
==11123==    by 0x7E54616: WebCore::jsHTMLCanvasElementPrototypeFunctionGetContext(JSC::ExecState*) (JSHTMLCanvasElement.cpp:210)

[Sudarsana Nagineni (babu)]

Created attachment 148096 [details]
Patch

Fixed memory leaks.

[Martin Robinson]

Comment on attachment 148096 [details]
Patch

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

> Source/WebCore/platform/graphics/glx/GLContextGLX.cpp:75
> -    for (size_t i = 0; i < contextList.size(); ++i)
> -        delete contextList[i];
> +    for (size_t i = contextList.size(); i > 0; --i)
> +        delete contextList[i - 1];

I'm not sure I understand this. Why is the last element leaking here and why does iterating backward help?

[Sudarsana Nagineni (babu)]

Comment on attachment 148096 [details]
Patch

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

>> Source/WebCore/platform/graphics/glx/GLContextGLX.cpp:75
>> +        delete contextList[i - 1];
> 
> I'm not sure I understand this. Why is the last element leaking here and why does iterating backward help?

contextList.size() is getting updated after removing the element from the vector in GLContextGLX::removeActiveContext(). So, the for loop termination condition returns always false in the last iteration, when we have more than one element. For example, when we have two elements in the vector, after the first iteration the size becomes 1 and the loop exits, so the second element leaks. 

I'm iterating the vector backward so that the loop termination condition doesn't fail until the contextList.size() reaches 0.

[Martin Robinson]

Comment on attachment 148096 [details]
Patch

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

>>> Source/WebCore/platform/graphics/glx/GLContextGLX.cpp:75
>>> +        delete contextList[i - 1];
>> 
>> I'm not sure I understand this. Why is the last element leaking here and why does iterating backward help?
> 
> contextList.size() is getting updated after removing the element from the vector in GLContextGLX::removeActiveContext(). So, the for loop termination condition returns always false in the last iteration, when we have more than one element. For example, when we have two elements in the vector, after the first iteration the size becomes 1 and the loop exits, so the second element leaks. 
> 
> I'm iterating the vector backward so that the loop termination condition doesn't fail until the contextList.size() reaches 0.

This sort of thing deserves a comment, I think. :)

[Sudarsana Nagineni (babu)]

Comment on attachment 148096 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/glx/GLContextGLX.cpp:75
>>>> +        delete contextList[i - 1];
>>> 
>>> I'm not sure I understand this. Why is the last element leaking here and why does iterating backward help?
>> 
>> contextList.size() is getting updated after removing the element from the vector in GLContextGLX::removeActiveContext(). So, the for loop termination condition returns always false in the last iteration, when we have more than one element. For example, when we have two elements in the vector, after the first iteration the size becomes 1 and the loop exits, so the second element leaks. 
>> 
>> I'm iterating the vector backward so that the loop termination condition doesn't fail until the contextList.size() reaches 0.
> 
> This sort of thing deserves a comment, I think. :)

Sure. I will add a comment and update the changelog also.

[Sudarsana Nagineni (babu)]

Created attachment 148143 [details]
Patch

Updated patch taking Martin's feedback into consideration.

[Martin Robinson]

Comment on attachment 148143 [details]
Patch

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

> Source/WebCore/platform/graphics/glx/GLContextGLX.cpp:75
> +    // Iterate the vector backwards, so that the loop termination condition
> +    // doesn't fail until the size() reaches 0.

This still leaves the situation slightly obscure. The comment should specifically mention that delete contextList[i -1] removes the context from the vector as a side effect. In fact, maybe it's better to just do a check in GLContextGLX::removeActiveContext that we aren't cleaning up the list at exit.

[Martin Robinson]

If you like, I can make this change and land the patch or you can update the patch and use the cq. It's up to you.

[Martin Robinson]

Committed r120673: <http://trac.webkit.org/changeset/120673>