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.

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 =

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

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 =
+		DocumentClassConstPtr oldClass = buffer_.params().documentClassPtr();
-			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 =
+		DocumentClassConstPtr oldDocClass = buffer_.params().documentClassPtr();
-			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);

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_;
 	///

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 = 
+	DocumentClassConstPtr newDocClass = buffer.params().documentClassPtr();
-		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
+	// This used to need to be static to avoid a memory leak. It no longer needs
-	// constructed, it constructs a BufferParams, which in turn constructs
+	// to be so, but the alternative is to construct a new one of these (with a
-	// a DocumentClass, via new, that is never deleted. If we were to go to
+	// new temporary directory, etc) every time, and then to destroy it. So maybe
-	// some kind of garbage collection there, or a shared_ptr, then this
+	// it's worth just keeping this one around.
-	// would not be needed.
 	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, dc));
-	cutstack.push(make_pair(copy_pars, d));
 }
 
 } // namespace anon
@@ -634,8 +631,8 @@ bool multipleCellsSelected(Cursor const & cur)
 }
 
 
-void switchBetweenClasses(DocumentClass const * const oldone, 
+void switchBetweenClasses(DocumentClassConstPtr  oldone,
-		DocumentClass const * const newone, InsetText & in, ErrorList & errorlist)
+		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();

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, 
+void switchBetweenClasses(DocumentClassConstPtr c1,
-			DocumentClass const * const c2, InsetText & in, ErrorList &);
+			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.

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

src/Makefile.am

@@ -216,6 +216,7 @@ HEADERFILESCORE = \
 	DepTable.h \
 	DispatchResult.h \
 	DocIterator.h \
+	DocumentClassPtr.h \
 	Encoding.h \
 	ErrorList.h \
 	Exporter.h \

src/TextClass.cpp

@@ -1431,38 +1431,11 @@ Layout TextClass::createBasicLayout(docstring const & name, bool unknown) const
 }
 
 
-/////////////////////////////////////////////////////////////////////////
+DocumentClassPtr getDocumentClass(
-//
-// 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(
 		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);

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
+/// The only way to make a DocumentClass is to call this function.
-/// they stay in memory for use by Insets, CutAndPaste, and the like, even
+/// The shared_ptr is needed because DocumentClass objects can be kept
-/// when their associated Buffers are destroyed.
+/// in memory long after their associated Buffer is destroyed, mostly
-/// FIXME Some sort of garbage collection or reference counting wouldn't
+/// on the CutStack.
-/// be a bad idea here. It might be enough to check when a Buffer is closed
+DocumentClassPtr getDocumentClass(LayoutFile const & baseClass,
-/// (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,
 			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);

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

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);