Deal with memory issue reported some time ago in connection with DocumentClass

objects. The problem that led to the leak is that these objects can be held in
memory long after the Buffer that created them is gone, mostly due to their
use in the CutStack. So they were previously held in a storage facility, the
DocumentClassBundle. Unfortunately, they were now being created too often,
especially by cloning. It's not really a leak, because they're accessible, but
we weren't ever destroying them.

This new approach uses a shared_ptr instead.

Thanks to Vincent for pointing out const_pointer_cast.

diff --git a/src/BufferParams.cpp b/src/BufferParams.cpp
index 7fdba576e7c706a969badd9cfa91f88523ead699..817403f0def86b7c53c2ed56bc34392ccac32cb9 100644 (file)
--- a/src/BufferParams.cpp
+++ b/src/BufferParams.cpp
@@ -1953,20 +1953,20 @@ bool BufferParams::hasClassDefaults() const
 
 DocumentClass const & BufferParams::documentClass() const
 {
-       return *doc_class_;
+       return *doc_class_.get();
 }
 
 
-DocumentClass const * BufferParams::documentClassPtr() const
+DocumentClassConstPtr BufferParams::documentClassPtr() const
 {
        return doc_class_;
 }
 
 
-void BufferParams::setDocumentClass(DocumentClass const * const tc)
+void BufferParams::setDocumentClass(DocumentClassConstPtr tc)
 {
        // evil, but this function is evil
-       doc_class_ = const_cast<DocumentClass *>(tc);
+       doc_class_ = const_pointer_cast<DocumentClass>(tc);
 }
 
 
@@ -2037,7 +2037,7 @@ void BufferParams::makeDocumentClass()
        en = cite_engine_.end();
        for (; it != en; ++it)
                mods.push_back(*it);
-       doc_class_ = &(DocumentClassBundle::get().makeDocumentClass(*baseClass(), mods));
+       doc_class_ = getDocumentClass(*baseClass(), mods);
 
        if (!local_layout.empty()) {
                TextClass::ReturnValues success =

diff --git a/src/BufferParams.h b/src/BufferParams.h
index 772f42de84924d4d97e0c05e6433f76e9a66e0c2..f4f9bef8e0ea068db854a26bda9eaa950b99df1f 100644 (file)
--- a/src/BufferParams.h
+++ b/src/BufferParams.h
@@ -16,6 +16,7 @@
 #define BUFFERPARAMS_H
 
 #include "Citation.h"
+#include "DocumentClassPtr.h"
 #include "Format.h"
 #include "LayoutModuleList.h"
 #include "OutputParams.h"
@@ -129,11 +130,11 @@ public:
        DocumentClass const & documentClass() const;
        /// \return A pointer to the DocumentClass currently in use: the BaseClass
        /// as modified by modules.
-       DocumentClass const * documentClassPtr() const;
+       DocumentClassConstPtr  documentClassPtr() const;
        /// This bypasses the baseClass and sets the textClass directly.
        /// Should be called with care and would be better not being here,
        /// but it seems to be needed by CutAndPaste::putClipboard().
-       void setDocumentClass(DocumentClass const * const);
+       void setDocumentClass(DocumentClassConstPtr);
        /// List of modules in use
        LayoutModuleList const & getModules() const { return layout_modules_; }
        /// List of default modules the user has removed
@@ -500,7 +501,7 @@ private:
        /// the type of cite engine (authoryear or numerical)
        CiteEngineType cite_engine_type_;
        ///
-       DocumentClass * doc_class_;
+       DocumentClassPtr doc_class_;
        ///
        LayoutModuleList layout_modules_;
        /// this is for modules that are required by the document class but that

diff --git a/src/BufferView.cpp b/src/BufferView.cpp
index 9b6c6b3c18c2724b0c62e09743cbe926e20755e3..6c9aa366b8730b2a0bfd80a69952ff7b5f1ad1f4 100644 (file)
--- a/src/BufferView.cpp
+++ b/src/BufferView.cpp
@@ -941,7 +941,7 @@ bool BufferView::scrollToCursor(DocIterator const & dit, bool recenter)
 }
 
 
-void BufferView::updateDocumentClass(DocumentClass const * const olddc)
+void BufferView::updateDocumentClass(DocumentClassConstPtr olddc)
 {
        message(_("Converting document to new document class..."));
        
@@ -1234,7 +1234,7 @@ void BufferView::dispatch(FuncRequest const & cmd, DispatchResult & dr)
        switch (act) {
 
        case LFUN_BUFFER_PARAMS_APPLY: {
-               DocumentClass const * const oldClass = buffer_.params().documentClassPtr();
+               DocumentClassConstPtr oldClass = buffer_.params().documentClassPtr();
                cur.recordUndoFullDocument();
                istringstream ss(to_utf8(cmd.argument()));
                Lexer lex;
@@ -1256,8 +1256,7 @@ void BufferView::dispatch(FuncRequest const & cmd, DispatchResult & dr)
        }
                
        case LFUN_LAYOUT_MODULES_CLEAR: {
-               DocumentClass const * const oldClass =
-                       buffer_.params().documentClassPtr();
+               DocumentClassConstPtr oldClass = buffer_.params().documentClassPtr();
                cur.recordUndoFullDocument();
                buffer_.params().clearLayoutModules();
                buffer_.params().makeDocumentClass();
@@ -1275,7 +1274,7 @@ void BufferView::dispatch(FuncRequest const & cmd, DispatchResult & dr)
                                "conflicts with installed modules.");
                        break;
                }
-               DocumentClass const * const oldClass = params.documentClassPtr();
+               DocumentClassConstPtr oldClass = params.documentClassPtr();
                cur.recordUndoFullDocument();
                buffer_.params().addLayoutModule(argument);
                buffer_.params().makeDocumentClass();
@@ -1306,8 +1305,7 @@ void BufferView::dispatch(FuncRequest const & cmd, DispatchResult & dr)
                        break;
 
                // Save the old, possibly modular, layout for use in conversion.
-               DocumentClass const * const oldDocClass =
-                       buffer_.params().documentClassPtr();
+               DocumentClassConstPtr oldDocClass = buffer_.params().documentClassPtr();
                cur.recordUndoFullDocument();
                buffer_.params().setBaseClass(argument);
                buffer_.params().makeDocumentClass();
@@ -1332,7 +1330,7 @@ void BufferView::dispatch(FuncRequest const & cmd, DispatchResult & dr)
        }
 
        case LFUN_LAYOUT_RELOAD: {
-               DocumentClass const * const oldClass = buffer_.params().documentClassPtr();
+               DocumentClassConstPtr oldClass = buffer_.params().documentClassPtr();
                LayoutFileIndex bc = buffer_.params().baseClassID();
                LayoutFileList::get().reset(bc);
                buffer_.params().setBaseClass(bc);

diff --git a/src/BufferView.h b/src/BufferView.h
index f6d5b1894baeca201817065cc28514681e76b281..d164438ad57f6a5946c2b101ccfa4c4b030b35f6 100644 (file)
--- a/src/BufferView.h
+++ b/src/BufferView.h
@@ -15,8 +15,10 @@
 #ifndef BUFFER_VIEW_H
 #define BUFFER_VIEW_H
 
+#include "DocumentClassPtr.h"
 #include "update_flags.h"
 
+#include "support/shared_ptr.h"
 #include "support/strfwd.h"
 #include "support/types.h"
 
@@ -345,7 +347,7 @@ private:
        void updateHoveredInset() const;
 
        ///
-       void updateDocumentClass(DocumentClass const * const olddc);
+       void updateDocumentClass(DocumentClassConstPtr olddc);
        ///
        int width_;
        ///

diff --git a/src/CutAndPaste.cpp b/src/CutAndPaste.cpp
index 757e318b6e766517acaa217879051e87025e7ab6..b84e52fc52e174dd36e0f3617bd0f36383b12b97 100644 (file)
--- a/src/CutAndPaste.cpp
+++ b/src/CutAndPaste.cpp
@@ -79,7 +79,7 @@ namespace {
 
 typedef pair<pit_type, int> PitPosPair;
 
-typedef limited_stack<pair<ParagraphList, DocumentClass const *> > CutStack;
+typedef limited_stack<pair<ParagraphList, DocumentClassConstPtr> > CutStack;
 
 CutStack theCuts(10);
 // persistent selection, cleared until the next selection
@@ -99,7 +99,7 @@ bool checkPastePossible(int index)
 
 pair<PitPosPair, pit_type>
 pasteSelectionHelper(Cursor const & cur, ParagraphList const & parlist,
-                    DocumentClass const * const oldDocClass, ErrorList & errorlist)
+                    DocumentClassConstPtr oldDocClass, ErrorList & errorlist)
 {
        Buffer const & buffer = *cur.buffer();
        pit_type pit = cur.pit();
@@ -119,8 +119,7 @@ pasteSelectionHelper(Cursor const & cur, ParagraphList const & parlist,
 
        // Make a copy of the CaP paragraphs.
        ParagraphList insertion = parlist;
-       DocumentClass const * const newDocClass = 
-               buffer.params().documentClassPtr();
+       DocumentClassConstPtr newDocClass = buffer.params().documentClassPtr();
 
        // Now remove all out of the pars which is NOT allowed in the
        // new environment and set also another font if that is required.
@@ -461,15 +460,14 @@ PitPosPair eraseSelectionHelper(BufferParams const & params,
 
 
 void putClipboard(ParagraphList const & paragraphs, 
-       DocumentClass const * const docclass, docstring const & plaintext)
+       DocumentClassConstPtr docclass, docstring const & plaintext)
 {
        // For some strange reason gcc 3.2 and 3.3 do not accept
        // Buffer buffer(string(), false);
-       // This needs to be static to avoid a memory leak. When a Buffer is
-       // constructed, it constructs a BufferParams, which in turn constructs
-       // a DocumentClass, via new, that is never deleted. If we were to go to
-       // some kind of garbage collection there, or a shared_ptr, then this
-       // would not be needed.
+       // This used to need to be static to avoid a memory leak. It no longer needs
+       // to be so, but the alternative is to construct a new one of these (with a
+       // new temporary directory, etc) every time, and then to destroy it. So maybe
+       // it's worth just keeping this one around.
        static Buffer * buffer = theBufferList().newInternalBuffer(
                FileName::tempName("clipboard.internal").absFileName());
        buffer->setUnnamed(true);
@@ -503,7 +501,7 @@ static bool isFullyDeleted(ParagraphList const & pars)
 
 void copySelectionHelper(Buffer const & buf, Text const & text,
        pit_type startpit, pit_type endpit,
-       int start, int end, DocumentClass const * const dc, CutStack & cutstack)
+       int start, int end, DocumentClassConstPtr dc, CutStack & cutstack)
 {
        ParagraphList const & pars = text.paragraphs();
 
@@ -558,8 +556,7 @@ void copySelectionHelper(Buffer const & buf, Text const & text,
                it->setInsetOwner(0);
        }
 
-       DocumentClass * d = const_cast<DocumentClass *>(dc);
-       cutstack.push(make_pair(copy_pars, d));
+       cutstack.push(make_pair(copy_pars, dc));
 }
 
 } // namespace anon
@@ -634,8 +631,8 @@ bool multipleCellsSelected(Cursor const & cur)
 }
 
 
-void switchBetweenClasses(DocumentClass const * const oldone, 
-               DocumentClass const * const newone, InsetText & in, ErrorList & errorlist)
+void switchBetweenClasses(DocumentClassConstPtr  oldone,
+               DocumentClassConstPtr newone, InsetText & in, ErrorList & errorlist)
 {
        errorlist.clear();
 
@@ -975,7 +972,7 @@ docstring selection(size_t sel_index)
 
 
 void pasteParagraphList(Cursor & cur, ParagraphList const & parlist,
-                       DocumentClass const * const docclass, ErrorList & errorList)
+                       DocumentClassConstPtr docclass, ErrorList & errorList)
 {
        if (cur.inTexted()) {
                Text * text = cur.text();

diff --git a/src/CutAndPaste.h b/src/CutAndPaste.h
index c5c7aa4334c8841929ea2921b1e560cfcbf084d7..2453601157106b63b1ec60faebf0e1d43347d2e7 100644 (file)
--- a/src/CutAndPaste.h
+++ b/src/CutAndPaste.h
@@ -14,6 +14,8 @@
 #ifndef CUTANDPASTE_H
 #define CUTANDPASTE_H
 
+#include "DocumentClassPtr.h"
+
 #include "support/docstring.h"
 
 #include "frontends/Clipboard.h"
@@ -100,15 +102,15 @@ void pasteSimpleText(Cursor & cur, bool asParagraphs);
 /// Paste the paragraph list \p parlist at the position given by \p cur.
 /// Does not handle undo. Does only work in text, not mathed.
 void pasteParagraphList(Cursor & cur, ParagraphList const & parlist,
-                       DocumentClass const * const textclass, ErrorList & errorList);
+                       DocumentClassConstPtr textclass, ErrorList & errorList);
 
 
 /** Needed to switch between different classes. This works
  *  for a list of paragraphs beginning with the specified par.
  *  It changes layouts and character styles.
  */
-void switchBetweenClasses(DocumentClass const * const c1, 
-                       DocumentClass const * const c2, InsetText & in, ErrorList &);
+void switchBetweenClasses(DocumentClassConstPtr c1,
+                       DocumentClassConstPtr c2, InsetText & in, ErrorList &);
 
 /// Get the current selection as a string. Does not change the selection.
 /// Does only work if the whole selection is in mathed.

diff --git a/src/DocumentClassPtr.h b/src/DocumentClassPtr.h
new file mode 100644 (file)
index 0000000..eef9ac2
--- /dev/null
+++ b/src/DocumentClassPtr.h
@@ -0,0 +1,24 @@
+// -*- C++ -*-
+/**
+ * \file DocumentClassPtr.h
+ * This file is part of LyX, the document processor.
+ * Licence details can be found in the file COPYING.
+ *
+ * \author Richard Heck
+ *
+ * Full author contact details are available in file CREDITS.
+ */
+
+#ifndef DOCUMENT_CLASS_PTR_H
+#define DOCUMENT_CLASS_PTR_H
+
+#include "support/shared_ptr.h"
+
+namespace lyx {
+class DocumentClass;
+
+typedef shared_ptr<DocumentClass> DocumentClassPtr;
+typedef shared_ptr<DocumentClass const> DocumentClassConstPtr;
+}
+
+#endif // DISPATCH_RESULT_H

diff --git a/src/Makefile.am b/src/Makefile.am
index bfd510ce86e9faf0c6a2d08a16812e3f26015cbb..02a878d56f9825cab33fb26aeb8f2ebef38b681e 100644 (file)
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -216,6 +216,7 @@ HEADERFILESCORE = \
        DepTable.h \
        DispatchResult.h \
        DocIterator.h \
+       DocumentClassPtr.h \
        Encoding.h \
        ErrorList.h \
        Exporter.h \

diff --git a/src/TextClass.cpp b/src/TextClass.cpp
index 65d5ef84d441518e3b0e9702d28939ee511ad864..17e4f09c48ead4e67c95b13531e04ab6af348944 100644 (file)
--- a/src/TextClass.cpp
+++ b/src/TextClass.cpp
@@ -1431,38 +1431,11 @@ Layout TextClass::createBasicLayout(docstring const & name, bool unknown) const
 }
 
 
-/////////////////////////////////////////////////////////////////////////
-//
-// DocumentClassBundle
-//
-/////////////////////////////////////////////////////////////////////////
-
-DocumentClassBundle::~DocumentClassBundle()
-{
-       for (size_t i = 0; i != documentClasses_.size(); ++i)
-               delete documentClasses_[i];
-       documentClasses_.clear();
-}
-
-DocumentClass & DocumentClassBundle::newClass(LayoutFile const & baseClass)
-{
-       DocumentClass * dc = new DocumentClass(baseClass);
-       documentClasses_.push_back(dc);
-       return *documentClasses_.back();
-}
-
-
-DocumentClassBundle & DocumentClassBundle::get()
-{
-       static DocumentClassBundle singleton;
-       return singleton;
-}
-
-
-DocumentClass & DocumentClassBundle::makeDocumentClass(
+DocumentClassPtr getDocumentClass(
                LayoutFile const & baseClass, LayoutModuleList const & modlist)
 {
-       DocumentClass & doc_class = newClass(baseClass);
+       DocumentClassPtr doc_class =
+           DocumentClassPtr(new DocumentClass(baseClass));
        LayoutModuleList::const_iterator it = modlist.begin();
        LayoutModuleList::const_iterator en = modlist.end();
        for (; it != en; ++it) {
@@ -1490,7 +1463,7 @@ DocumentClass & DocumentClassBundle::makeDocumentClass(
                        frontend::Alert::warning(_("Package not available"), msg, true);
                }
                FileName layout_file = libFileSearch("layouts", lm->getFilename());
-               if (!doc_class.read(layout_file, TextClass::MODULE)) {
+               if (!doc_class->read(layout_file, TextClass::MODULE)) {
                        docstring const msg =
                                                bformat(_("Error reading module %1$s\n"), from_utf8(modName));
                        frontend::Alert::warning(_("Read Error"), msg);

diff --git a/src/TextClass.h b/src/TextClass.h
index a1de81a516d0c2b9cd62f550c7dfe10fa31f32ad..fd8011328bfd160750d571180c188eb2f4458078 100644 (file)
--- a/src/TextClass.h
+++ b/src/TextClass.h
@@ -13,6 +13,7 @@
 #include "Citation.h"
 #include "ColorCode.h"
 #include "Counters.h"
+#include "DocumentClassPtr.h"
 #include "FloatList.h"
 #include "FontInfo.h"
 #include "Layout.h"
@@ -24,8 +25,6 @@
 #include "support/docstring.h"
 #include "support/types.h"
 
-#include <boost/noncopyable.hpp>
-
 #include <list>
 #include <map>
 #include <set>
@@ -369,7 +368,7 @@ private:
 /// In the main LyX code, DocumentClass objects are created only by
 /// DocumentClassBundle, for which see below.
 ///
-class DocumentClass : public TextClass, boost::noncopyable {
+class DocumentClass : public TextClass {
 public:
        ///
        virtual ~DocumentClass() {}
@@ -475,41 +474,19 @@ protected:
 private:
        /// The only class that can create a DocumentClass is
        /// DocumentClassBundle, which calls the protected constructor.
-       friend class DocumentClassBundle;
+       friend DocumentClassPtr
+               getDocumentClass(LayoutFile const &, LayoutModuleList const &);
        ///
        static InsetLayout plain_insetlayout_;
 };
 
 
-/// DocumentClassBundle is a container for DocumentClass objects, so that
-/// they stay in memory for use by Insets, CutAndPaste, and the like, even
-/// when their associated Buffers are destroyed.
-/// FIXME Some sort of garbage collection or reference counting wouldn't
-/// be a bad idea here. It might be enough to check when a Buffer is closed
-/// (or makeDocumentClass is called) whether the old DocumentClass is in use
-/// anywhere.
-///
-/// This is a singleton class. Its sole instance is accessed via
-/// DocumentClassBundle::get().
-class DocumentClassBundle : boost::noncopyable {
-public:
-       /// \return The sole instance of this class.
-       static DocumentClassBundle & get();
-       /// \return A new DocumentClass based on baseClass, with info added
-       /// from the modules in modlist.
-       DocumentClass & makeDocumentClass(LayoutFile const & baseClass,
+/// The only way to make a DocumentClass is to call this function.
+/// The shared_ptr is needed because DocumentClass objects can be kept
+/// in memory long after their associated Buffer is destroyed, mostly
+/// on the CutStack.
+DocumentClassPtr getDocumentClass(LayoutFile const & baseClass,
                        LayoutModuleList const & modlist);
-private:
-       /// control instantiation
-       DocumentClassBundle() {}
-       /// clean up
-       ~DocumentClassBundle();
-       /// \return Reference to a new DocumentClass equal to baseClass
-       DocumentClass & newClass(LayoutFile const & baseClass);
-       ///
-       std::vector<DocumentClass *> documentClasses_;
-};
-
 
 /// convert page sides option to text 1 or 2
 std::ostream & operator<<(std::ostream & os, PageSides p);

diff --git a/src/support/shared_ptr.h b/src/support/shared_ptr.h
index 28ac3d7a2357a8f7d216313eb47254c8f60e72ca..69e42da23c216a96732d2a57c52772fee520df2e 100644 (file)
--- a/src/support/shared_ptr.h
+++ b/src/support/shared_ptr.h
@@ -23,6 +23,7 @@
 namespace lyx
 {
        using std::tr1::shared_ptr;
+       using std::tr1::const_pointer_cast;
 }
 
 #else
@@ -32,6 +33,7 @@ namespace lyx
 namespace lyx
 {
        using boost::shared_ptr;
+       using boost::const_pointer_cast;
 }
 
 #endif

diff --git a/src/tex2lyx/tex2lyx.cpp b/src/tex2lyx/tex2lyx.cpp
index 94e099a66c934b7eec74dd29124b399a4686364d..d890130b6bed7bb44836a7a29cf5ba390a7b89bf 100644 (file)
--- a/src/tex2lyx/tex2lyx.cpp
+++ b/src/tex2lyx/tex2lyx.cpp
@@ -254,12 +254,11 @@ bool checkModule(string const & name, bool command)
        // This is needed since a module cannot be read on its own, only as
        // part of a document class.
        LayoutFile const & baseClass = LayoutFileList::get()[textclass.name()];
-       typedef map<string, DocumentClass *> ModuleMap;
+       typedef map<string, DocumentClassPtr > ModuleMap;
        static ModuleMap modules;
        static bool init = true;
        if (init) {
                baseClass.load();
-               DocumentClassBundle & bundle = DocumentClassBundle::get();
                LyXModuleList::const_iterator const end = theModuleList.end();
                LyXModuleList::const_iterator it = theModuleList.begin();
                for (; it != end; ++it) {
@@ -269,7 +268,7 @@ bool checkModule(string const & name, bool command)
                        if (!m.moduleCanBeAdded(module, &baseClass))
                                continue;
                        m.push_back(module);
-                       modules[module] = &bundle.makeDocumentClass(baseClass, m);
+                       modules[module] = getDocumentClass(baseClass, m);
                }
                init = false;
        }
@@ -290,7 +289,7 @@ bool checkModule(string const & name, bool command)
                        continue;
                if (findInsetLayoutWithoutModule(textclass, name, command))
                        continue;
-               DocumentClass const * c = it->second;
+               DocumentClassConstPtr  c = it->second;
                Layout const * layout = findLayoutWithoutModule(*c, name, command);
                InsetLayout const * insetlayout = layout ? 0 :
                        findInsetLayoutWithoutModule(*c, name, command);