Opened 14 months ago

Last modified 12 months ago

#4153 new defect

Replace all naked pointer returns to Java with wrapped versions

Reported by: asmirnov Assignee:
Priority: P2 Milestone:
Module: Libadblockplus-Android Keywords:
Cc: rjeschke, sergz Blocked By:
Blocking: Platform: Android
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description

Due to memory allocation issue https://issues.adblockplus.org/ticket/4080#comment:6 all the raw pointers should be wrapped into shared_ptr.

Change History (1)

comment:1 Changed 12 months ago by sergz

I would like to add into this issue to check that all intermediate C++ objects are deleted and marshal the object lifetimes. May be it should become a meta issue for smaller ones.

  1. There is a comment https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-android/jni/Utils.h#l140
    However I have noticed another usage, we are casting shared_ptr<T:JsValue>* to shared_ptr<JsValue> using reinterpret_cast, that's actually wrong.
  1. A new object is created on a heap https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-android/jni/Utils.cpp#l106, and it seems it's not always deleted. E.g. JniCtor creates a copy of it on the heap and does not delete the argument.
    Should not it be simply &value instead of new T(value)?
  1. I find it good when we allocate and de-allocate in one place, for instance, we create something on the heap in java wrapper then we delete it in dispose method (dtor).
    In general sometimes it's convenient to create an object outside and pass the pointer to it into a constructor. In that case the instance of newly created wrapper should take over the responsibility for deleting of the argument. However there should be an agreement for that approach or at least it should be documented. Currently nothing of it is visible.
  1. Just in case check that all wrappers has dtor/dispose/whatever which releases the resource.
Note: See TracTickets for help on using tickets.