WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
89346
[Cairo] Fix memory leaks in GLContextGLX.cpp
https://bugs.webkit.org/show_bug.cgi?id=89346
Summary
[Cairo] Fix memory leaks in GLContextGLX.cpp
Sudarsana Nagineni (babu)
Reported
2012-06-18 06:51:27 PDT
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)
Attachments
Patch
(2.38 KB, patch)
2012-06-18 07:34 PDT
,
Sudarsana Nagineni (babu)
no flags
Details
Formatted Diff
Diff
Patch
(2.69 KB, patch)
2012-06-18 11:49 PDT
,
Sudarsana Nagineni (babu)
mrobinson
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Sudarsana Nagineni (babu)
Comment 1
2012-06-18 07:34:12 PDT
Created
attachment 148096
[details]
Patch Fixed memory leaks.
Martin Robinson
Comment 2
2012-06-18 07:58:52 PDT
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 3
2012-06-18 08:57:56 PDT
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 4
2012-06-18 09:27:59 PDT
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 5
2012-06-18 11:42:30 PDT
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)
Comment 6
2012-06-18 11:49:02 PDT
Created
attachment 148143
[details]
Patch Updated patch taking Martin's feedback into consideration.
Martin Robinson
Comment 7
2012-06-18 11:55:04 PDT
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
Comment 8
2012-06-18 12:06:42 PDT
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
Comment 9
2012-06-18 22:34:45 PDT
Committed
r120673
: <
http://trac.webkit.org/changeset/120673
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug