Skip to content

Commit

Permalink
Finally, added the XML weak_ptr memory leak fix
Browse files Browse the repository at this point in the history
  • Loading branch information
DennisPopovDn committed Aug 22, 2015
1 parent b1bf8ab commit 28e90bb
Show file tree
Hide file tree
Showing 4 changed files with 58 additions and 23 deletions.
34 changes: 23 additions & 11 deletions ePub3/xml/tree/node.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,8 @@ Node::Node(const string & name, NodeType type, const string & content, const cla
Node::Node(Node && o) : _xml(o._xml) {
typedef LibXML2Private<Node> _Private;
_Private* priv = reinterpret_cast<_Private*>(_xml->_private);
priv->__ptr.reset(this);
//priv->__ptr.reset(this);
priv->__ptr=std::shared_ptr<Node>(this);
o._xml = NULL;
}
Node::~Node()
Expand All @@ -181,14 +182,17 @@ Node::~Node()
return;

_Private* priv = reinterpret_cast<_Private*>(_xml->_private);
if ( priv->__sig != _READIUM_XML_SIGNATURE || priv->__ptr.get() != this )
if (priv->__sig != _READIUM_XML_SIGNATURE || (priv->__ptr.lock()!=nullptr && priv->__ptr.lock().get() != this))
return;

// free the underlying node if *and only if* it is detached
if (!bool(priv->__ptr.lock()))
{
_xml->_private = nullptr;
delete priv;
}
// free the underlying node if *and only if* it is detached
if ( _xml->parent == nullptr && _xml->prev == nullptr && _xml->next == nullptr )
{
_xml->_private = nullptr;
delete priv;
xmlFreeNode(_xml);
}
}
Expand All @@ -213,7 +217,9 @@ string Node::Content() const
const xmlChar* ch = xmlNodeGetContent(_xml);
if (ch == nullptr)
return string::EmptyString;
return ch;
string result(ch);
xmlFree((void*)ch);
return result;
}
void Node::SetContent(const string &content)
{
Expand Down Expand Up @@ -251,7 +257,9 @@ string Node::Language() const
const xmlChar * ch = xmlNodeGetLang(_xml);
if ( ch == nullptr )
return string();
return ch;
string result(ch);
xmlFree((void*)ch);
return result;
}
void Node::SetLanguage(const string &language)
{
Expand All @@ -270,7 +278,9 @@ string Node::BaseURL() const
const xmlChar * ch = xmlNodeGetBase(_xml->doc, _xml);
if ( ch == nullptr )
return string();
return ch;
string result(ch);
xmlFree((void*)ch);
return result;
}
void Node::SetBaseURL(const string &baseURL)
{
Expand Down Expand Up @@ -352,7 +362,9 @@ string Node::StringValue() const
const xmlChar * content = xmlNodeGetContent(_xml);
if ( content == nullptr )
return string();
return content;
string result(content);
xmlFree((void*)content);
return result;
}
int Node::IntValue() const
{
Expand Down Expand Up @@ -634,7 +646,7 @@ void Node::Unwrap(_xmlNode *aNode)
NsPrivate* ptr = reinterpret_cast<NsPrivate*>(__ns->_private);
if (ptr->__sig == _READIUM_XML_SIGNATURE)
{
ptr->__ptr->release();
//ptr->__ptr->release();
delete ptr;
}
__ns->_private = nullptr;
Expand All @@ -645,7 +657,7 @@ void Node::Unwrap(_xmlNode *aNode)
NodePrivate* ptr = reinterpret_cast<NodePrivate*>(aNode->_private);
if (ptr->__sig == _READIUM_XML_SIGNATURE)
{
ptr->__ptr->release();
//ptr->__ptr->release();
delete ptr;
}
aNode->_private = nullptr;
Expand Down
34 changes: 27 additions & 7 deletions ePub3/xml/utilities/base.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ struct LibXML2Private
: __sig(_READIUM_XML_SIGNATURE), __ptr(nullptr)
{}
LibXML2Private(_Tp* __p)
: __sig(_READIUM_XML_SIGNATURE), __ptr(__p)
: __sig(_READIUM_XML_SIGNATURE), __ptr(std::shared_ptr<_Tp>(__p))
{}
LibXML2Private(std::shared_ptr<_Tp>& __p)
: __sig(_READIUM_XML_SIGNATURE), __ptr(__p)
Expand All @@ -133,7 +133,7 @@ struct LibXML2Private

// data member-- used to determine if this is a Readium-made pointer
unsigned int __sig;
std::shared_ptr<_Tp> __ptr;
std::weak_ptr<_Tp> __ptr;
};
#endif

Expand Down Expand Up @@ -162,7 +162,24 @@ static inline std::shared_ptr<_Tp> Wrapped(_Nm * __n)
_PrivatePtr __p = reinterpret_cast<_PrivatePtr>(__n->_private);
if (__p->__sig == _READIUM_XML_SIGNATURE)
{
return __p->__ptr;
std::shared_ptr<_Tp> toRet = __p->__ptr.lock();
if (!bool(toRet))// if there is no live reference to the readium wrapper, but
{
delete __p; // release unreferenced memory, if any
__n->_private = nullptr;

std::shared_ptr<_Tp> toRet = std::shared_ptr<_Tp>(new _Tp(__n));

//_PrivatePtr __p = new LibXML2Private<_Tp>(new _Tp(__n));
_PrivatePtr __p = new LibXML2Private<_Tp>(toRet);
__n->_private = __p;
//return __p->__ptr;
return toRet;
}
///////////////////////////////////////////////////
return toRet;
//return __p->__ptr;

}
else
{
Expand All @@ -175,11 +192,14 @@ static inline std::shared_ptr<_Tp> Wrapped(_Nm * __n)
if (__n->_private != nullptr)
throw std::logic_error("XML _private already carries a value!");
}

std::shared_ptr<_Tp> toRet = std::shared_ptr<_Tp>(new _Tp(__n));


_PrivatePtr __p = new LibXML2Private<_Tp>(new _Tp(__n));
//_PrivatePtr __p = new LibXML2Private<_Tp>(new _Tp(__n));
_PrivatePtr __p = new LibXML2Private<_Tp>(toRet);
__n->_private = __p;
return __p->__ptr;
//return __p->__ptr;
return toRet;
}

/**
Expand All @@ -198,7 +218,7 @@ static inline void Rewrap(_Nm* __n, std::shared_ptr<_Tp> __t)
if (IS_READIUM_WRAPPED_XML(__n))
{
_Private* __p = reinterpret_cast<_Private*>(__n->_private);
if (__p->__ptr == __t)
if (__p->__ptr.lock() && __p->__ptr.lock() == __t)
return;

delete __p;
Expand Down
8 changes: 5 additions & 3 deletions ePub3/xml/validation/ns.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@ EPUB3_XML_BEGIN_NAMESPACE
Namespace::Namespace(std::shared_ptr<Document> doc, const string &prefix, const string &uri)
{
xmlDocPtr d = doc->xml();

_xml = xmlNewNs(reinterpret_cast<xmlNodePtr>(d), uri.utf8(), prefix.utf8());
bShouldxmlFreeNs = true;
if (_xml->_private != nullptr)
Node::Unwrap((xmlNodePtr)_xml);

Expand All @@ -38,13 +40,13 @@ Namespace::~Namespace()
return;

LibXML2Private<Namespace>* priv = reinterpret_cast<LibXML2Private<Namespace>*>(_xml->_private);
if (priv->__sig == _READIUM_XML_SIGNATURE && priv->__ptr.get() == this)
if (priv->__sig == _READIUM_XML_SIGNATURE && priv->__ptr.lock() == nullptr/* && priv->__ptr.lock().get() == this*/)
{
delete priv;
_xml->_private = nullptr;
}
xmlFreeNs(_xml);
if (bShouldxmlFreeNs)
xmlFreeNs(_xml);
}

EPUB3_XML_END_NAMESPACE
5 changes: 3 additions & 2 deletions ePub3/xml/validation/ns.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,8 @@ typedef std::vector<std::shared_ptr<Namespace>> NamespaceList;
class Namespace : public WrapperBase<Namespace>
{
public:
explicit Namespace(_xmlNs * ns) : _xml(ns) {}
Namespace() : _xml(nullptr) {}
explicit Namespace(_xmlNs * ns) : _xml(ns), bShouldxmlFreeNs(false) {}
Namespace() : _xml(nullptr), bShouldxmlFreeNs(false) {}
Namespace(std::shared_ptr<Document> doc, const string & prefix, const string & uri);
virtual ~Namespace();

Expand All @@ -67,6 +67,7 @@ class Namespace : public WrapperBase<Namespace>
{ _xml = nullptr; }

protected:
bool bShouldxmlFreeNs;
_xmlNs * _xml;

};
Expand Down

1 comment on commit 28e90bb

@danielweck
Copy link
Member

@danielweck danielweck commented on 28e90bb Oct 28, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @DennisPopovDn I have included your non-contentious / obvious memory leak fixes (i.e. xmlFree() on char* and namespace string) in a feature branch we are currently working on:
efc71c2
Thank you!

I have also incorporated your slightly more "controversial" weak_ptr-related changes, behind an #ifdef preprocessor directive which is deactivated by default:
a33c979
I say "controversial" because of the untested status of these changes on multiple platforms, but otherwise I like the principle, as I commented in your Pull Request a while back.

CC @rkwright @clebeaupin @llemeurfr

Please sign in to comment.