search

dmurdoch / rgl

rgl is a 3D visualization system based on OpenGL. It provides a medium to high level interface for use in R, currently modelled on classic R graphics, with extensions to allow for interaction.
https://dmurdoch.github.io/rgl/
GNU General Public License v2.0
86 stars 21 forks source link

Potential memory leak #439

Closed szhorvat closed 4 days ago

szhorvat commented 1 week ago

In the igraph CI runs, LeakSanitizer reports a memory leak in rgl. Could you please have a quick look and let us know if this looks like a real leak or a false positive? This started happening after the 1.3.12 release.

https://github.com/igraph/rigraph/actions/runs/11671197963/job/32497326078#step:5:7873

LSan output ``` ==786==ERROR: LeakSanitizer: detected memory leaks Direct leak of 400 byte(s) in 1 object(s) allocated from: #0 0x55b8f586bf0d in operator new(unsigned long) (/usr/local/RDcsan/lib/R/bin/exec/R+0xddf0d) (BuildId: fbbe33b828d9063e4a0cd431c628028497551820) #1 0x7f195ae0b50a in rgl::Subscene::hideBackground(int) /tmp/RtmpHCFKmJ/R.INSTALL200b4c98f7a7/rgl/src/useNULL/subscene.cpp:254:36 #2 0x7f195ae07c88 in rgl::Scene::hide(int) /tmp/RtmpHCFKmJ/R.INSTALL200b4c98f7a7/rgl/src/useNULL/scene.cpp:141:52 #3 0x7f195ae07ab1 in rgl::Scene::pop(unsigned int, int) /tmp/RtmpHCFKmJ/R.INSTALL200b4c98f7a7/rgl/src/useNULL/scene.cpp:111:9 #4 0x7f195adfac39 in rgl::Device::pop(unsigned int, int) /tmp/RtmpHCFKmJ/R.INSTALL200b4c98f7a7/rgl/src/useNULL/device.cpp:127:23 #5 0x7f195adf38d4 in rgl_pop /tmp/RtmpHCFKmJ/R.INSTALL200b4c98f7a7/rgl/src/useNULL/api.cpp:240:38 #6 0x7f1974ce08cc in do_dotCode /tmp/r-source/src/main/dotcode.c:1966:2 #7 0x7f1974ea457b in bcEval_loop /tmp/r-source/src/main/eval.c:8118:14 #8 0x7f1974e2fce5 in bcEval /tmp/r-source/src/main/eval.c:7501:16 #9 0x7f1974e2d116 in Rf_eval /tmp/r-source/src/main/eval.c:1167:8 #10 0x7f1974e38b3e in R_execClosure /tmp/r-source/src/main/eval.c:2393:22 #11 0x7f1974e33d20 in applyClosure_core /tmp/r-source/src/main/eval.c:2306:16 #12 0x7f1974e32b3c in Rf_applyClosure /tmp/r-source/src/main/eval.c:2328:16 #13 0x7f1974e2f6b3 in Rf_eval /tmp/r-source/src/main/eval.c:1280:12 #14 0x7f1974b4d507 in do_docall /tmp/r-source/src/main/coerce.c:2764:12 #15 0x7f1974ea457b in bcEval_loop /tmp/r-source/src/main/eval.c:8118:14 #16 0x7f1974e2fce5 in bcEval /tmp/r-source/src/main/eval.c:7501:16 #17 0x7f1974e2d116 in Rf_eval /tmp/r-source/src/main/eval.c:1167:8 #18 0x7f1974e38b3e in R_execClosure /tmp/r-source/src/main/eval.c:2393:22 #19 0x7f1974e33d20 in applyClosure_core /tmp/r-source/src/main/eval.c:2306:16 #20 0x7f1974e32b3c in Rf_applyClosure /tmp/r-source/src/main/eval.c:2328:16 #21 0x7f1974e2f6b3 in Rf_eval /tmp/r-source/src/main/eval.c:1280:12 #22 0x7f1974e40317 in do_begin /tmp/r-source/src/main/eval.c:3000:10 #23 0x7f1974e2e642 in Rf_eval /tmp/r-source/src/main/eval.c:1232:12 #24 0x7f1974e38b3e in R_execClosure /tmp/r-source/src/main/eval.c:2393:22 #25 0x7f1974e33d20 in applyClosure_core /tmp/r-source/src/main/eval.c:2306:16 #26 0x7f1974e32b3c in Rf_applyClosure /tmp/r-source/src/main/eval.c:2328:16 #27 0x7f19750d0662 in applyMethod /tmp/r-source/src/main/objects.c:120:8 #28 0x7f19750c8492 in dispatchMethod /tmp/r-source/src/main/objects.c:473:16 #29 0x7f19750c652c in Rf_usemethod /tmp/r-source/src/main/objects.c:513:10 Indirect leak of 4 byte(s) in 1 object(s) allocated from: #0 0x55b8f5832156 in __interceptor_realloc (/usr/local/RDcsan/lib/R/bin/exec/R+0xa4156) (BuildId: fbbe33b828d9063e4a0cd431c628028497551820) #1 0x7f195ade2010 in rgl::ColorArray::recycle(unsigned int) /tmp/RtmpHCFKmJ/R.INSTALL200b4c98f7a7/rgl/src/useNULL/Color.cpp:258:33 #2 0x7f195ade00b8 in rgl::Background::Background(rgl::Material&, bool, int, float) /tmp/RtmpHCFKmJ/R.INSTALL200b4c98f7a7/rgl/src/useNULL/Background.cpp:61:28 #3 0x7f195ae0b539 in rgl::Subscene::hideBackground(int) /tmp/RtmpHCFKmJ/R.INSTALL200b4c98f7a7/rgl/src/useNULL/subscene.cpp:254:36 #4 0x7f195ae07c88 in rgl::Scene::hide(int) /tmp/RtmpHCFKmJ/R.INSTALL200b4c98f7a7/rgl/src/useNULL/scene.cpp:141:52 #5 0x7f195ae07ab1 in rgl::Scene::pop(unsigned int, int) /tmp/RtmpHCFKmJ/R.INSTALL200b4c98f7a7/rgl/src/useNULL/scene.cpp:111:9 #6 0x7f195adfac39 in rgl::Device::pop(unsigned int, int) /tmp/RtmpHCFKmJ/R.INSTALL200b4c98f7a7/rgl/src/useNULL/device.cpp:127:23 #7 0x7f195adf38d4 in rgl_pop /tmp/RtmpHCFKmJ/R.INSTALL200b4c98f7a7/rgl/src/useNULL/api.cpp:240:38 #8 0x7f1974ce08cc in do_dotCode /tmp/r-source/src/main/dotcode.c:1966:2 #9 0x7f1974ea457b in bcEval_loop /tmp/r-source/src/main/eval.c:8118:14 #10 0x7f1974e2fce5 in bcEval /tmp/r-source/src/main/eval.c:7501:16 #11 0x7f1974e2d116 in Rf_eval /tmp/r-source/src/main/eval.c:1167:8 #12 0x7f1974e38b3e in R_execClosure /tmp/r-source/src/main/eval.c:2393:22 #13 0x7f1974e33d20 in applyClosure_core /tmp/r-source/src/main/eval.c:2306:16 #14 0x7f1974e32b3c in Rf_applyClosure /tmp/r-source/src/main/eval.c:2328:16 #15 0x7f1974e2f6b3 in Rf_eval /tmp/r-source/src/main/eval.c:1280:12 #16 0x7f1974b4d507 in do_docall /tmp/r-source/src/main/coerce.c:2764:12 #17 0x7f1974ea457b in bcEval_loop /tmp/r-source/src/main/eval.c:8118:14 #18 0x7f1974e2fce5 in bcEval /tmp/r-source/src/main/eval.c:7501:16 #19 0x7f1974e2d116 in Rf_eval /tmp/r-source/src/main/eval.c:1167:8 #20 0x7f1974e38b3e in R_execClosure /tmp/r-source/src/main/eval.c:2393:22 #21 0x7f1974e33d20 in applyClosure_core /tmp/r-source/src/main/eval.c:2306:16 #22 0x7f1974e32b3c in Rf_applyClosure /tmp/r-source/src/main/eval.c:2328:16 #23 0x7f1974e2f6b3 in Rf_eval /tmp/r-source/src/main/eval.c:1280:12 #24 0x7f1974e40317 in do_begin /tmp/r-source/src/main/eval.c:3000:10 #25 0x7f1974e2e642 in Rf_eval /tmp/r-source/src/main/eval.c:1232:12 #26 0x7f1974e38b3e in R_execClosure /tmp/r-source/src/main/eval.c:2393:22 #27 0x7f1974e33d20 in applyClosure_core /tmp/r-source/src/main/eval.c:2306:16 #28 0x7f1974e32b3c in Rf_applyClosure /tmp/r-source/src/main/eval.c:2328:16 #29 0x7f19750d0662 in applyMethod /tmp/r-source/src/main/objects.c:120:8 SUMMARY: AddressSanitizer: 404 byte(s) leaked in 2 allocation(s). ```
dmurdoch commented 1 week ago

That does look like a real leak. I'll see if I can fix it.

dmurdoch commented 1 week ago

I think this is the issue: changing the background made scenes end up with multiple background nodes (only one of which was used), and I thought I fixed that. However, the fix was done at the wrong level, so an object (a "Subscene") that is only supposed to reference existing nodes actually created a new one in some cases. Then the object that managed nodes didn't know about it, so it leaked when everything was cleaned up.

dmurdoch commented 5 days ago

@szhorvat, would it be possible to check if the changes in the issue439 branch fix the leak? I think they should, but there may be more to do.

szhorvat commented 4 days ago

Thanks for the quick fix! Test seems to pass: https://github.com/igraph/rigraph/pull/1579 I hope I did this right. Can you have a look @krlmlr?

dmurdoch commented 4 days ago

Thanks for running the checks. It's in the main branch now.

krlmlr commented 4 days ago

Thanks for the quick fix. We'll watch out for an rgl update on CRAN.