Document leaks memory when stored as Value.

[GoogleCodeExporter]

What steps will reproduce the problem?
1. rapidjson::Value* value = new rapidjson::Document;
2. delete value;
3. // leak memory.

What is the expected output? What do you see instead?
Expected all memory to be freed, but Document::stack_ is not freed.

What version of the product are you using? On what operating system?
Tip of the tree. Linux.

Please provide any additional information below.
rapidjson::Value has a non-virtual destructor. rapidjson::Document does not 
have a destructor at all. When a pointer to rapidjson::Value is deleted, the 
destructor for rapidjson::Document does not get called. This causes the leak of 
Document::stack_. The solution is to mark the destructor virtual in Value and 
also add a virtual destructor to Document.

Original issue reported on code.google.com by rous...@chromium.org on 19 May 2014 at 8:00

[GoogleCodeExporter]

The current implementation of Value and Document (and even the rest of the 
library) are not designed for run-time polymorphism.  Therefore, the above code 
example could be seen as user error.

I agree that this may be surprising and should be guarded against, e.g. by 
having Document inheriting privately from Value and exposing only the allowed 
interface (but disallowing the pointer conversion).  Such a change can still 
break existing code, though.

On the other hand, adding virtual functions (like a destrcutor) to Value will 
break the existing implementation in various places.  The in-place 
onstruction/destruction used for the move semantics of Value will no longer 
work in the presence of virtual functions (or at least cause significantly 
undefined behaviour in C++).

My 2¢,
Philipp

Original comment by philipp....@gmail.com on 28 May 2014 at 11:30

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Agree with Philipp.
Adding virtual functions in general would also hurt performance, compromise the 
"rapid" part of it.

Just to read data from json, the way I understand it, only one 'delete' is 
required, that is of the whole document. Documentation should probably outrule 
deletion of the document in any other way except with 
rapidjson::Document::~Document()

I tested rapidjson in such read-only usage model for the memory leaks, and 
couldn't find any leaks.

Original comment by yuriv...@gmail.com on 6 Jun 2014 at 1:42

[GoogleCodeExporter]

Original comment by milo...@gmail.com on 20 Jun 2014 at 11:19

• Changed state: Invalid