On Mon, Feb 17, 2020 at 02:26:17PM -0500, Richard Kimberly Heck wrote: > On 2/17/20 2:08 PM, Scott Kostyshak wrote: > > On Mon, Feb 17, 2020 at 12:35:19PM -0500, Richard Kimberly Heck wrote: > >> On 2/17/20 8:17 AM, Pavel Sanda wrote: > >>> On Sun, Feb 16, 2020 at 07:21:17PM -0500, Scott Kostyshak wrote: > >>>> On Sat, Feb 15, 2020 at 09:42:52PM +0100, Pavel Sanda wrote: > >>>>> On Fri, Feb 14, 2020 at 08:09:09PM -0500, Scott Kostyshak wrote: > >>>>>> Thanks to both of you for taking a look. I was hoping to investigate > >>>>>> this memory leak but it seems to be beyond my knowledge. > >>>>> Do you have recipy to reproduce valgrind's warning? > >>>> 1. I run the following command (you will need to adapt it): > >>>> > >>>> valgrind --leak-check=full --track-origins=yes --log-file=valgrind.log > >>>> ~/lyxbuilds/master/CMakeBuild/bin/lyx -userdir > >>>> ~/lyxbuilds/master/user-dir > >>>> > >>>> 2. Do "ctrl + n" to start a new document. > >>>> > >>>> 3. Type "abc". > >>>> > >>>> 4. Quit LyX and respond "no" when prompted whether to save the document. > >>>> > >>>> I attach the full log in case it is useful. The other warnings involve > >>>> external libraries. > >>>> > >>>> Can anyone else reproduce? > >>> I can reproduce. I looked now more carefully in the code, but couldn't > >>> find where in the code are delete operators corresponding to the new > >>> operator > >>> at TocModel.cpp:362. > >>> > >>> So this looks like we just forget to free memory at the end of TocModels > >>> life. As I know little about ToCXXX routines I would appreciate someone > >>> (Riki?) > >>> knowing the context ack my change before comitting. > >> I don't see how that could hurt. And it looks to me like the TocModels > >> objects are per-buffer. If so, then closing a file would leak, and I > >> think the patch would prevent that leak. I take it that it does silence > >> the warning? > > I'm wondering if I'm interpreting the above paragraph correctly. Here's > > my interpretation: if the TocModels were per-LyX session, then although > > it is still technically a memory leak, it would not have any benefit to > > fix it because LyX is being exited anyway so all of the memory LyX has > > will be given back to the OS. That is, the memory leak has a short life. > > On the other hand, since TocModels objects are per-buffer, these memory > > leaks could have longer lives because users can close buffers without > > exiting LyX. Did I understand the intuition correctly? > > Yes, precisely. And I think they are per-buffer. It may well be that > valgrind is complaining (on your script for reproducing) when the Buffer > and its associated TocModel are destroyed, which it is during the exit > process, at which point the memory is leaked---though not for very long, > in that case. But in other cases.... In any event, I think it is worth > fixing even if I'm wrong, if only because someone could come along and > make the TocModels per-buffer, and then we have a real leak.
Makes sense. Thanks for the explanation. I guess an additional reason to fix it is the same reason why we fix unimportant compiler warnings---it makes it easier to spot important warnings when they do come up. > Does Pavel's patch help? Yes, just tested with valgrind and that error is gone. Thanks, Pavel and Riki! Scott
signature.asc
Description: PGP signature
-- lyx-devel mailing list lyx-devel@lists.lyx.org http://lists.lyx.org/mailman/listinfo/lyx-devel
