Opened 2 years ago

Closed 2 years ago

#5031 closed defect (fixed)

Avoid double deletion of C++ objects

Reported by: sergz Assignee:
Priority: Unknown Milestone:
Module: Libadblockplus-Android Keywords:
Cc: asmirnov, fhd, vickyyu Blocked By:
Blocking: #5026 Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29397600/

Description

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.

Change History (9)

comment:1 Changed 2 years ago by sergz

  • Cc asmirnov added; anton removed

comment:2 Changed 2 years ago by 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.

comment:3 Changed 2 years ago by 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.

comment:4 Changed 2 years ago by 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).


Last edited 2 years ago by asmirnov (previous) (diff)

comment:5 Changed 2 years ago by asmirnov

  • Cc fhd added

comment:6 Changed 2 years ago by vickyyu

  • Cc vickyyu added

comment:7 Changed 2 years ago by asmirnov

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:8 Changed 2 years ago by abpbot

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

comment:9 Changed 2 years ago by asmirnov

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.