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

RESOLVED FIXED in Firefox 23

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bjacob, Assigned: bjacob)

Tracking

({regression})

Trunk
mozilla25
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox22 wontfix, firefox23 fixed, firefox24 fixed, firefox25 fixed)

Details

(Whiteboard: [MemShrink:P2][qa-])

Attachments

(1 attachment)

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.
Attachment #763808 - Flags: review?(jgilbert)
Blocks: 716859
Whiteboard: [MemShrink]
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).
"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
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
Attachment #763808 - Flags: review?(jgilbert) → review+
Whiteboard: [MemShrink] → [MemShrink:P2]
Justin: yes, bool args are evil.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7431d7b9751f
Assignee: nobody → bjacob
Target Milestone: --- → mozilla25
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
Attachment #763808 - Flags: approval-mozilla-beta?
Attachment #763808 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/7431d7b9751f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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.
Attachment #763808 - Flags: approval-mozilla-beta?
Attachment #763808 - Flags: approval-mozilla-beta+
Attachment #763808 - Flags: approval-mozilla-aurora?
Attachment #763808 - Flags: approval-mozilla-aurora+
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.
Whiteboard: [MemShrink:P2] → [MemShrink:P2][qa-]
You need to log in before you can comment on or make changes to this bug.