Avoid double deletion of C++ objects

[sergz]

When a pointer to JniLogSystemCallback is passed into JsEngine an instance of std::shared_ptr<LogSystem> is constructed (it's actually done explicitly here ​https://hg.adblockplus.org/libadblockplus-android/file/447a4bb3cb28/libadblockplus-android/jni/JniJsEngine.cpp#l171) and now JniLogSystemCallback is deleted twice, by std::shared_ptr which is held by JsEngine and in ​https://hg.adblockplus.org/libadblockplus-android/file/447a4bb3cb28/libadblockplus-android/jni/JniLogSystem.cpp#l49.

I think that it's a normal way when the client prepares some object, like implementation of WebRequest or LogSystem, and then transfers the ownership to another object. Therefore that C++ implementation in our case should keep a strong reference to java object until the former is deleted.

[asmirnov]

Libadblockplus-android releases that instances because it created them.
This is simple following of a rule: "who created the object, releases it".
I think it's correct and we had this behaviour in libadblockplus-android from the beginning.

However transferring of ownership is a subject for discussion and it seems it was not yet discussed.

[sergz]

Well, if you create std::shared_ptr<T> from a raw pointer p you should never call delete p afterwards and be prepared that now the lifetime of object pointed by p is determined by all users of the shared pointer. I think there is nothing to discuss.

In our particular case if you want to clean some resources used by implementation of an injected interface there are two ways to achieve it.

• The current approach, somehow ensure that there are no users of it anymore and then clean it, though #5030 and the issue with leaking JsEngine should be fixed.
• your implementation of an interface should support the cleaning of internally held resources. Afterwards the injected implementation could have some basic functionality like always failing.

In addition I would say here that having such methods like SetWebRequest can be pretty complicated, I would rather prefer to inject required implementations only once at the construction phase and eliminate a possibility to change the implementation on the fly. If someone needs such functionality it can be done in the implementation of an interface (the second item in the list above), not in libadblockplus.

The current approach with manual disposing is fine for me, though this issue and the rest should be fixed.

[asmirnov]

We should separate the things.

1) JsEngine and FilterEngine are returned with std::shared_ptr<> and i'm talking about LogSystem (callback) in this particular case.
It is created by the android JNI layer without any smart pointers: ​https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-android/jni/JniLogSystem.cpp#l42

2) JNI layer does know that there are no more users of created LogSystem instance as it passes it to JsEngine instance only.
I agree we should release JnEngine before LogSystem is released because the probability that it's used by JsEngine while it's already released it extremely small, but > 0.

3) I'm absolutely sure that it's up to JNI layer to release LogSystem as it's just following the rule "who created it destroys it". Particular LogSystem can depend on current android context (let's say it's shows Android alert) and it's working only when the app is running. In Android app we do know when it should be destroyed. So the solution is to release it by JNI layer too (as currently) and let all the clients know that it's not available anymore (i believe we should just call setLogSystem(null) or provide some callback for callback to release (which is just ridiculous).

So, wrap-up:
1) i don't think we should introduce any significant changes with ownership
2) both JsEngine and FilterEngine should NOT release any callbacks (not like is asked by this task).
3) JNI layer should notify JsEngine or FilterEngine that passed callbacks are not available any more (passing 'nullptr' to setLogSystem or any similar).

[abpbot]

A commit referencing this issue has landed:
​Issue 5031 - Avoid double deletion of C++ objects