| Message ID | 20200120002437.6633-9-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On Mon, Jan 20, 2020 at 02:24:26AM +0200, Laurent Pinchart wrote: > Add two tests that exercise the Signal::disconnect(Object *) and > Signal::disconnect() methods, to verify that they correctly remove the > signal from the connected object's list of signals. This triggers an > issue that was detected through manual code inspection, and is expected > to crash or at least generate valgrind warnings. > I can confirm the following is gone in the next commits ==25012== Invalid read of size 8 ==25012== at 0x112E8D: std::__cxx11::list<libcamera::BoundMethodBase*, std::allocator<libcamera::BoundMethodBase*> >::begin() (stl_list.h:942) ==25012== by 0x4A3253C: void libcamera::SignalBase::disconnect<libcamera::Object>(libcamera::Object*) (signal.h:25) ==25012== by 0x4A31A0B: libcamera::Object::~Object() (object.cpp:80) ==25012== by 0x11AA87: SlotObject::~SlotObject() (signal.cpp:31) ==25012== by 0x11AAAB: SlotObject::~SlotObject() (signal.cpp:31) ==25012== by 0x112536: SignalTest::run() (signal.cpp:233) ==25012== by 0x11B41D: Test::execute() (test.cpp:36) ==25012== by 0x1113E6: main (signal.cpp:317) ==25012== Address 0x50d3690 is 0 bytes inside a block of size 24 free'd ==25012== at 0x4839EAB: operator delete(void*) (vg_replace_malloc.c:586) ==25012== by 0x11250E: SignalTest::run() (signal.cpp:232) ==25012== by 0x11B41D: Test::execute() (test.cpp:36) ==25012== by 0x1113E6: main (signal.cpp:317) ==25012== Block was alloc'd at ==25012== at 0x4838DEF: operator new(unsigned long) (vg_replace_malloc.c:344) ==25012== by 0x11240D: SignalTest::run() (signal.cpp:228) ==25012== by 0x11B41D: Test::execute() (test.cpp:36) ==25012== by 0x1113E6: main (signal.cpp:317) > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > --- > test/signal.cpp | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/test/signal.cpp b/test/signal.cpp > index 0054ed5a380d..f83ceb05f091 100644 > --- a/test/signal.cpp > +++ b/test/signal.cpp > @@ -220,6 +220,30 @@ protected: > delete dynamicSignal; > delete slotObject; > > + /* > + * Test that signal manual disconnection from Object removes > + * the signal for the object. This shall not generate any > + * valgrind warning. > + */ > + dynamicSignal = new Signal<>(); > + slotObject = new SlotObject(); > + dynamicSignal->connect(slotObject, &SlotObject::slot); > + dynamicSignal->disconnect(slotObject); > + delete dynamicSignal; > + delete slotObject; > + > + /* > + * Test that signal manual disconnection from all slots removes > + * the signal for the object. This shall not generate any > + * valgrind warning. > + */ > + dynamicSignal = new Signal<>(); > + slotObject = new SlotObject(); > + dynamicSignal->connect(slotObject, &SlotObject::slot); > + dynamicSignal->disconnect(); > + delete dynamicSignal; > + delete slotObject; > + > /* Exercise the Object slot code paths. */ > slotObject = new SlotObject(); > signalVoid_.connect(slotObject, &SlotObject::slot); > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Laurent, Thanks for your test. On 2020-01-20 02:24:26 +0200, Laurent Pinchart wrote: > Add two tests that exercise the Signal::disconnect(Object *) and > Signal::disconnect() methods, to verify that they correctly remove the > signal from the connected object's list of signals. This triggers an > issue that was detected through manual code inspection, and is expected > to crash or at least generate valgrind warnings. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > test/signal.cpp | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/test/signal.cpp b/test/signal.cpp > index 0054ed5a380d..f83ceb05f091 100644 > --- a/test/signal.cpp > +++ b/test/signal.cpp > @@ -220,6 +220,30 @@ protected: > delete dynamicSignal; > delete slotObject; > > + /* > + * Test that signal manual disconnection from Object removes > + * the signal for the object. This shall not generate any > + * valgrind warning. > + */ > + dynamicSignal = new Signal<>(); > + slotObject = new SlotObject(); > + dynamicSignal->connect(slotObject, &SlotObject::slot); > + dynamicSignal->disconnect(slotObject); > + delete dynamicSignal; > + delete slotObject; > + > + /* > + * Test that signal manual disconnection from all slots removes > + * the signal for the object. This shall not generate any > + * valgrind warning. > + */ > + dynamicSignal = new Signal<>(); > + slotObject = new SlotObject(); > + dynamicSignal->connect(slotObject, &SlotObject::slot); > + dynamicSignal->disconnect(); > + delete dynamicSignal; > + delete slotObject; > + > /* Exercise the Object slot code paths. */ > slotObject = new SlotObject(); > signalVoid_.connect(slotObject, &SlotObject::slot); > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
diff --git a/test/signal.cpp b/test/signal.cpp index 0054ed5a380d..f83ceb05f091 100644 --- a/test/signal.cpp +++ b/test/signal.cpp @@ -220,6 +220,30 @@ protected: delete dynamicSignal; delete slotObject; + /* + * Test that signal manual disconnection from Object removes + * the signal for the object. This shall not generate any + * valgrind warning. + */ + dynamicSignal = new Signal<>(); + slotObject = new SlotObject(); + dynamicSignal->connect(slotObject, &SlotObject::slot); + dynamicSignal->disconnect(slotObject); + delete dynamicSignal; + delete slotObject; + + /* + * Test that signal manual disconnection from all slots removes + * the signal for the object. This shall not generate any + * valgrind warning. + */ + dynamicSignal = new Signal<>(); + slotObject = new SlotObject(); + dynamicSignal->connect(slotObject, &SlotObject::slot); + dynamicSignal->disconnect(); + delete dynamicSignal; + delete slotObject; + /* Exercise the Object slot code paths. */ slotObject = new SlotObject(); signalVoid_.connect(slotObject, &SlotObject::slot);
Add two tests that exercise the Signal::disconnect(Object *) and Signal::disconnect() methods, to verify that they correctly remove the signal from the connected object's list of signals. This triggers an issue that was detected through manual code inspection, and is expected to crash or at least generate valgrind warnings. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- test/signal.cpp | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)