884057 - On GLX, the pixmaps used for offscreen GL contexts are leaked (regression from surfacestream big patch)

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: revert to destroying our pixmaps

The big patch in bug 716859 had a typo that escaped all of its reviewers: the |deleteDrawable| parameter passed by CreateOffscreenPixmapContext to CreateGLContext was accidentally changed from true to false, causing these pixmaps to be leaked. This was discovered by some valgrinding while working on bug 874768. Tested, works, green on tbpl.

Comment 1

[Justin Lebar (not reading bugmail)]

Do you happen to have a Valgrind log of what this leak looks like?  That would help me understand if we're seeing this leak elsewhere (and catching it via DMD).

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

"Elsewhere"? pixmaps are a X-specific concept.

Argh, I had a valgrind.log, until I overwrote it yesterday night with another one... I still have a DMD log for you:

  http://people.mozilla.org/~bjacob/dmd-processed.log.gz

The reports look like this:

Unreported: 1 block in stack trace record 6 of 5,354
 9,457,664 bytes (9,453,584 requested / 4,080 slop)
 1.60% of the heap (36.52% cumulative);  1.70% of unreported (38.73% cumulative)
 Allocated at
   replace_malloc (/hack/mozilla-graphics/memory/replace/dmd/DMD.cpp:1225) 0x7fa347cb2fbc
   malloc (/hack/mozilla-graphics/memory/build/replace_malloc.c:152) 0x41b289
   _swrast_CreateContext (/usr/lib/x86_64-linux-gnu/dri/libdricore.so) 0x7fa31fa86172
   intelInitContext (/usr/lib/x86_64-linux-gnu/dri/i965_dri.so) 0x7fa31fda1d3c
   brwCreateContext (/usr/lib/x86_64-linux-gnu/dri/i965_dri.so) 0x7fa31fdb8455
   ??? (/usr/lib/x86_64-linux-gnu/dri/i965_dri.so) 0x7fa31fdaa23d
   ??? (/usr/lib/x86_64-linux-gnu/dri/i965_dri.so) 0x7fa31fdefd65
   ??? (/usr/lib/x86_64-linux-gnu/dri/i965_dri.so) 0x7fa31fdefe7d
   ??? (/usr/lib/x86_64-linux-gnu/mesa/libGL.so.1) 0x7fa320ae4bd3
   ??? (/usr/lib/x86_64-linux-gnu/mesa/libGL.so.1) 0x7fa320abe33e
   glXCreateNewContext (/usr/lib/x86_64-linux-gnu/mesa/libGL.so.1) 0x7fa320abe56a
   mozilla::gl::GLXLibrary::xCreateNewContext(_XDisplay*, __GLXFBConfigRec*, int, __GLXcontextRec*, int) (/hack/mozilla-graphics/gfx/gl/GLContextProvide
rGLX.cpp:591) 0x7fa3434624e8
   mozilla::gl::GLContextGLX::CreateGLContext(mozilla::gfx::SurfaceCaps const&, mozilla::gl::GLContextGLX*, bool, _XDisplay*, unsigned long, __GLXFBConf
igRec*, bool, mozilla::gl::GLXLibrary::LibraryType, gfxXlibSurface*) (/hack/mozilla-graphics/gfx/gl/GLContextProviderGLX.cpp:801) 0x7fa343462f83
   CreateOffscreenPixmapContext (/hack/mozilla-graphics/gfx/gl/GLContextProviderGLX.cpp:1392) 0x7fa343464aac
   mozilla::gl::GLContextProviderGLX::GetGlobalContext(mozilla::gl::GLContext::ContextFlags) (/hack/mozilla-graphics/gfx/gl/GLContextProviderGLX.cpp:1452) 0x7fa343464d0e
   GetGlobalContextGLX (/hack/mozilla-graphics/gfx/gl/GLContextProviderGLX.cpp:1156) 0x7fa343463f5d
   mozilla::gl::GLContextProviderGLX::CreateForWindow(nsIWidget*) (/hack/mozilla-graphics/gfx/gl/GLContextProviderGLX.cpp:1264) 0x7fa34346440c
   mozilla::layers::LayerManagerOGL::CreateContext() (/hack/mozilla-graphics/gfx/layers/opengl/LayerManagerOGL.cpp:225) 0x7fa34342ab66
   mozilla::layers::LayerManagerOGL::Initialize(bool) (/hack/mozilla-graphics/gfx/layers/opengl/LayerManagerOGL.cpp:264) 0x7fa34342ad9d
   nsBaseWidget::GetLayerManager(mozilla::layers::PLayerTransactionChild*, mozilla::layers::LayersBackend, nsIWidget::LayerManagerPersistence, bool*) (/hack/mozilla-graphics/widget/xpwidgets/nsBaseWidget.cpp:967) 0x7fa342b2350a
   nsWindow::GetLayerManager(mozilla::layers::PLayerTransactionChild*, mozilla::layers::LayersBackend, nsIWidget::LayerManagerPersistence, bool*) (/hack/mozilla-graphics/widget/gtk2/nsWindow.cpp:6179) 0x7fa342afe6f3
   nsIWidget::GetLayerManager(bool*) (/hack/mozilla-graphics/obj-firefox-debug-dmd/embedding/browser/webBrowser/../../../dist/include/nsIWidget.h:1142) 0x7fa3417cfd60
   PresShell::GetLayerManager() (/hack/mozilla-graphics/layout/base/nsPresShell.cpp:5029) 0x7fa34183c95b
   nsRefreshDriver::Tick(long, mozilla::TimeStamp) (/hack/mozilla-graphics/layout/base/nsRefreshDriver.cpp:1217) 0x7fa341853701

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Oops, sorry, the report I showed in comment 2 is the wrong one (we fixed that leak in a different bug). The correct one is:

Unreported: ~267 blocks in stack trace record 59 of 5,354
 ~1,092,831 bytes (~1,092,831 requested / ~0 slop)
 0.19% of the heap (60.40% cumulative);  0.20% of unreported (64.06% cumulative)
 Allocated at
   replace_calloc (/hack/mozilla-graphics/memory/replace/dmd/DMD.cpp:1244) 0x7fa347cb3067
   calloc (/hack/mozilla-graphics/memory/build/replace_malloc.c:182) 0x41b3be
   ??? (/usr/lib/x86_64-linux-gnu/dri/i965_dri.so) 0x7fa31fdaa04f
   ??? (/usr/lib/x86_64-linux-gnu/dri/i965_dri.so) 0x7fa31fdeff14
   ??? (/usr/lib/x86_64-linux-gnu/mesa/libGL.so.1) 0x7fa320ae54d4
   ??? (/usr/lib/x86_64-linux-gnu/mesa/libGL.so.1) 0x7fa320adf524
   ??? (/usr/lib/x86_64-linux-gnu/mesa/libGL.so.1) 0x7fa320adf8fa
   mozilla::gl::GLXLibrary::xCreatePixmap(_XDisplay*, __GLXFBConfigRec*, unsigned long, int const*) (/hack/mozilla-graphics/gfx/gl/GLContextProviderGLX.cpp:655) 0x7fa3434628ac
   CreateOffscreenPixmapContext (/hack/mozilla-graphics/gfx/gl/GLContextProviderGLX.cpp:1358) 0x7fa343464932
   mozilla::gl::GLContextProviderGLX::CreateOffscreen(nsIntSize const&, mozilla::gfx::SurfaceCaps const&, mozilla::gl::GLContext::ContextFlags) (/hack/mozilla-graphics/gfx/gl/GLContextProviderGLX.cpp:1408) 0x7fa343464b74
   mozilla::dom::CanvasRenderingContext2D::EnsureTarget() (/hack/mozilla-graphics/content/canvas/src/CanvasRenderingContext2D.cpp:808) 0x7fa341cc36e7
   mozilla::dom::CanvasRenderingContext2D::FillRect(double, double, double, double) (/hack/mozilla-graphics/content/canvas/src/CanvasRenderingContext2D.cpp:1577) 0x7fa341cc6435
   fillRect (/hack/mozilla-graphics/obj-firefox-debug-dmd/dom/bindings/CanvasRenderingContext2DBinding.cpp:1644) 0x7fa342e6f7a7
   genericMethod (/hack/mozilla-graphics/obj-firefox-debug-dmd/dom/bindings/CanvasRenderingContext2DBinding.cpp:4021) 0x7fa342e77806
   js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (/hack/mozilla-graphics/js/src/jscntxtinlines.h:349) 0x7fa343fadb12
   js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (/hack/mozilla-graphics/js/src/vm/Interpreter.cpp:388) 0x7fa343fba29f
   js::Interpret(JSContext*, js::StackFrame*, js::InterpMode, bool) (/hack/mozilla-graphics/js/src/vm/Interpreter.cpp:2212) 0x7fa343fc19f4
   js::RunScript(JSContext*, js::StackFrame*) (/hack/mozilla-graphics/js/src/vm/Interpreter.cpp:345) 0x7fa343fb9e5d
   js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (/hack/mozilla-graphics/js/src/vm/Interpreter.cpp:401) 0x7fa343fba364
   js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::Value*) (/hack/mozilla-graphics/js/src/vm/Interpreter.cpp:434) 0x7fa343fba657
   JS_CallFunctionValue(JSContext*, JSObject*, JS::Value, unsigned int, JS::Value*, JS::Value*) (/hack/mozilla-graphics/js/src/jsapi.cpp:5885) 0x7fa3440a51c7
   mozilla::dom::EventHandlerNonNull::Call(JSContext*, JS::Handle<JSObject*>, nsDOMEvent&, mozilla::ErrorResult&) (/hack/mozilla-graphics/obj-firefox-debug-dmd/dom/bindings/EventHandlerBinding.cpp:40) 0x7fa342ef3dc3
   JS::Value mozilla::dom::EventHandlerNonNull::Call<nsISupports*>(nsISupports* const&, nsDOMEvent&, mozilla::ErrorResult&, mozilla::dom::CallbackObject::ExceptionHandling) (/hack/mozilla-graphics/obj-firefox-debug-dmd/dom/src/events/../../../dist/include/mozilla/dom/EventHandlerBinding.h:58) 0x7fa3421a65bb
   nsJSEventListener::HandleEvent(nsIDOMEvent*) (/hack/mozilla-graphics/dom/src/events/nsJSEventListener.cpp:247) 0x7fa3421a5a46

Comment 4

[Justin Lebar (not reading bugmail)]

Thanks!

Comment 5

[Justin Lebar (not reading bugmail)]

http://jlebar.com/2011/12/16/Boolean_parameters_to_API_functions_considered_harmful..html

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

Justin: yes, bool args are evil.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7431d7b9751f

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 763808 [details] [diff] [review]
revert to destroying our pixmaps

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 716859 regressed it
User impact if declined: Memory leak on desktop Linux when using WebGL (after destroying a WebGL context, some X11 resource is leaked)
Testing completed (on m-c, etc.): m-i
Risk to taking this patch (and alternatives if risky): very low risk -- just reverting a boolean change that was traced back to an accident/typo in bug 716859.
String or IDL/UUID changes made by this patch: none

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7431d7b9751f

Comment 9

[Alex Keybl [:akeybl]]

Comment on attachment 763808 [details] [diff] [review]
revert to destroying our pixmaps

Early enough in the cycle to take a fix for this FF22 regression.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4fbeaf9b6c76
https://hg.mozilla.org/releases/mozilla-beta/rev/384a7ae08ab1

Comment 11

[u279076]

De-prioritizing QA verification of this fix since it's very low risk. Please remove the [qa-] tag and add the verifyme keyword if this needs prioritization.