Opened on 06/15/2016 at 11:31:18 AM

Closed on 09/26/2017 at 06:48:59 AM

#4153 closed defect (fixed)

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.

Attachments (0)

Change History (2)

comment:1 Changed on 08/30/2016 at 12:44:11 PM 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.

comment:2 Changed on 09/26/2017 at 06:48:59 AM by asmirnov

  • Resolution set to fixed
  • Status changed from new to closed

Add Comment

Modify Ticket

Change Properties
Action
as closed .
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from (none).
 
Note: See TracTickets for help on using tickets.