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

[Laurent Pinchart]

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).

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;

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 :-)

[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);
> >>>>    		}
> >>>>    

-- 
Regards,

Laurent Pinchart