[libcamera-devel] [PATCH] qcam: Fix camera reference leak on hot-unplug

[Umang Jain]

Hi Laurent,

On 7/28/20 4:39 AM, Laurent Pinchart wrote:
> Hi Umang,
>
> On Mon, Jul 27, 2020 at 08:20:40PM +0000, Umang Jain wrote:
>> On 7/27/20 9:20 PM, Laurent Pinchart wrote:
>>> On Mon, Jul 27, 2020 at 03:38:00PM +0000, Umang Jain wrote:
>>>> On 7/27/20 8:35 PM, Laurent Pinchart wrote:
>>>>> On Mon, Jul 27, 2020 at 12:32:59PM +0000, Umang Jain wrote:
>>>>>> If the currently streaming camera is hot-unplugged,
>>>>>> a camera reference was still held by MainWindow::camera_,
>>>>>> preventing it to be destructed, until qcam window is
>>>>>> closed. Plug the leak in the hot-unplug handler itself.
>>>>>>
>>>>>> Signed-off-by: Umang Jain <email at uajain.com>
>>>>> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>>>>
>>>>> Does this fix all the shared pointer refcount issues you've been chasing
>>>>> ?
>>>> The refcount issues are sorted here. I spent extensive time on it (both for hotplug
>>>> and un-plug refcount).
>>>>
>>>> However, the other issue I am chasing (earlier believed was caused by refcount issue)
>>>> is still present - which primarily deals with event loop and how event-notifiers are processed.
>>>>
>>>> I can still trigger ASSERT(iter != notifiers_.end()); in EventDispatcherPoll::processNotifiers(),
>>>> on hot-unplugging a currently streaming camera. This is probably due to the fact that, when
>>>> destroying a Camera object, it always destroys V4L2VideoDevice() - which in turn, "delete"s
>>>> two EventNotifiers in it's destructor() and then processNotifiers() isn't aware about the deletion
>>>> and still trying to find a EventNotifier with concerned pollfd and fails  :-(
>>> The issue is that the camera object is meant to be destroyed in the
>>> thread it belongs to, as explained in a previous e-mail (see [1]). Is
>>> that something you can try fixing, with the pointers in that e-mail, or
>>> would you like to discuss it further first ?
>>>
>>> [1] https://lists.libcamera.org/pipermail/libcamera-devel/2020-July/010951.html
>> hmm, I figured that I need to call the destructor via
>> Object::invokeMethod() with
>> connection type ConnectionTypeQueued to ensure the destructor is called
>> within
>> it's own thread. Something on the lines:
>>
>> void Object::deleteLater()
>> {
>> 	this->invokeMethod(&(typeid(this).name()::customDeletorFunc),
>> 			   ConnectionTypeQueued);
> You can drop the 'this->', calling a member function of an object from
> within the same object doesn't require explicitly accessing 'this'.
>
>> }
>>
>> and
>>
>> void className::customDeletorFunc()
>> {
>> 	~className();
>> }
>>
>> which obviously does not compile for me.
>>
>> How would one go about calling a destructor(in this case Camera's) from it's one
>> of the parent class(i.e. Object)? I am very cloudy around the syntax formation for
>> this->invokeMethod() above. Any help is appreciated.
> This indeed won't compile. The good news is that it can be simplified,
> fixing compilation at the same time :-)
>
> Your code, calling ~className(), would indeed call the destructor, but
> wouldn't free memory. Destruction of an object involves two steps, first
> the destructors are called (in order from the most derived class to the
> parent class) to perform cleanups specific to the object. Then, memory
> is freed. When you allocate an object on the stack with
>
> 	{
> 		Object o(...);
> 		...
> 	} <-- 1
>
> at point 1 the object becomes out of scope and is destroyed. The
> compiler generates code to call the destructors, and then simply updates
> the stack pointer to free the memory.
>
> When you allocate an object on the heap with
>
> 	Object *o = new Object(...);
> 	...
> 	delete o; <-- 1
>
> at point 1 the object is explicitly destroyed. The destructors are
> called, as in the stack case, but to free the memory, the compiler calls
> the delete operator (to be pedantic, a delete expression, formed by the
> 'delete' keyword followed by an expression, is compiled as an invocation
> of the destructor, followed by a call to the delete operator to free the
> memory - see [1] if you're interested in the details).
Thanks for the explaination. I indeed was missing these basics concepts :(
>
> Freeing memory is thus independent of the destructor, so calling the
> destructor manually isn't enough. The good news is that we don't need to
> do so manually. The Object class has a virtual destructor, so deleting
> an object using an Object pointer will call the destructor of the
> derived class. You don't need to get hold of a pointer to the derived
> class. You can thus simply implement
>
> void Object::customDeletorFunc()
> {
> 	delete this;
> }
>
> void Object::deleteLater()
> {
> 	invokeMethod(&Object::customDeletorFunc, ConnectionTypeQueued);
> }
>
> However, I think we can optimize the implementation a little bit to
> avoid going through the complex machinery of invokeMethod(). I would
> recommend adding a new DeferredDelete entry to the Message::Type
> enumeration, and simply posting the message with
>
> void Object::deleteLater()
> {
> 	postMessage(std::make_unique<Message>(Message::DeferredDelete));
> }
>
> and to handle that, in Object::message(), add
>
> 	case Message::DeferredDelete:
> 		delete this;
> 		break;
Yeah, this quite comes close to what Qt does for it's 
QObject::deleteLater().
It posts an event to defer delete.
>
> Finally, you'll have to wire that with a custom deleter when creating
> the camera shared pointer. Fortunately we have one already in
> Camera::create() :-) It should thus just be a matter of replacing
>
> 	struct Deleter : std::default_delete<Camera> {
> 		void operator()(Camera *camera)
> 		{
> 			delete camera;
> 		}
> 	};
>
> with
>
> 	struct Deleter : std::default_delete<Camera> {
> 		void operator()(Camera *camera)
> 		{
> 			camera->deleteLater();
> 		}
> 	};
>
> and of course crossing fingers :-)

Thanks for the input.
This is really simplified now and does not look as scary as I thought 
initially :)
I have pushed patches for review right now on the list.

>
> [1] https://en.cppreference.com/w/cpp/language/delete
>
>>>>>> ---
>>>>>>     src/qcam/main_window.cpp | 2 ++
>>>>>>     1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
>>>>>> index 7bc1360..c8298ec 100644
>>>>>> --- a/src/qcam/main_window.cpp
>>>>>> +++ b/src/qcam/main_window.cpp
>>>>>> @@ -587,6 +587,8 @@ void MainWindow::processHotplug(HotplugEvent *e)
>>>>>>     		/* Check if the currently-streaming camera is removed. */
>>>>>>     		if (camera == camera_.get()) {
>>>>>>     			toggleCapture(false);
>>>>>> +			camera_->release();
>>>>>> +			camera_.reset();
>>>>>>     			cameraCombo_->setCurrentIndex(0);
>>>>>>     		}
>>>>>>