AN-8945: Call reset() to delete ByteStream in ResourceStream

[ZakHsieh]

Original implementation will cause huge memory leakages when Java layer acquire a resource, the ByteStream is never been released due to calling release() won't delete the memory allocation. I'd change the _ptr.release() to _ptr.reset() to unlink the pointer and release the memory of ByteStream.

[danielweck]

Wow, something is indeed not right here, and there are several possible fixes, if I understand correctly:

Option 1: explicit delete

ResourceStream::~ResourceStream() {
    ePub3::ByteStream* reader = _ptr.get();
    reader->Close();
    _ptr.release();

        delete reader; // LINE ADDED
 }

...which can be compacted further:

ResourceStream::~ResourceStream() {
    ePub3::ByteStream* reader = _ptr.release(); // NO NEED FOR GET
    reader->Close();
        delete reader;
 }

...in fact, the Close() function is called automatically by the ByteStream destructor (to terminate the underlying file handle), so there is no need to invoke it explicitly:

ResourceStream::~ResourceStream() {
    ePub3::ByteStream* reader = _ptr.release();
        delete reader;
 }

See: https://github.com/readium/readium-sdk/blob/develop/ePub3/utilities/byte_stream.cpp#L747 ...and: https://github.com/readium/readium-sdk/blob/develop/ePub3/utilities/byte_stream.cpp#L775

Option 2: implicit "deleter"

ResourceStream::~ResourceStream() {
    ePub3::ByteStream* reader = _ptr.get();
    reader->Close();
    _ptr.reset(); // RESET INSTEAD OF RELEASE
 }

...which can be compacted to (for the same reasons described in above option 1):

ResourceStream::~ResourceStream() {
        _ptr.reset();
 }

Personally I have a preference for the explicit style (option 1), especially as reset() would otherwise be used without parameter (normally, it is used to swap pointer instances, with the expectation of automatic deletion of the previously-managed pointer).

What do you think?

[ZakHsieh]

Okay, make sense for me for option 1. Should I update a new patch for this pull request?

[danielweck]

I would prefer your contribution to be properly acknowledged, and I think if you update your existing PR with our commonly-agreed "destruction" code pattern, then we can simply merge your PR as-is (rather than clone it, adjust it, etc.)

[ZakHsieh]

Hi Daniel, I've pushed a new patch by your suggestion. Please help to review.

[danielweck]

Thanks for the updated PR! :)

By the way, although I agree with the return 0; for the Media Overlays / SMIL premature function exit in case of XML parsing error, this probably belongs to another Pull Request. Or at least if this PR is approved including this additional unrelated code change, we must document it properly with an associated, specific issue.

[ZakHsieh]

Ok, will create a new pull request for memory leak commits.