Using Umbrello from Git/master: In file "component.cpp" at line 58 in the method "UMLComponent::saveToXMI1()": for (UMLObjectListIt objectsIt(m_objects); objectsIt.hasNext();) { UMLObject* obj = objectsIt.next(); uIgnoreZeroPointer(obj); obj->saveToXMI1 (writer); } What happens if "obj" == nullptr? A warning is emitted by "uIgnoreZeroPointer", but "obj" is dereferenced after that anyway, which causes undefined behavior according to the C++ standard. I found instances of similar behavior in about 60 other files. Which gives rise to the next question: Why are null pointers being stored in the "m_objects" collection? Instead of an error or warning, this should be handled by an assert() or something similar, causing the program to exit, preferably in code which appends the pointers to the collection.
Hi Robert, thanks for working on that issue. > Which gives rise to the next question: Why are null pointers being stored in the "m_objects" collection? > Instead of an error or warning, this should be handled by an assert() or something similar, causing the > program to exit, preferably in code which appends the pointers to the collection. When I started working on Umbrello, there were many crashes because various lists contained undefined pointers. This has been massively mitigated by using QPointer, so that there are zero pointers left, which are checked and ignored when accessing the lists. Why these null pointers are contained there has already been investigated and fixed in some cases in the past, when the cause was identifiable. One reason for this is probably the lack of referential integrity of the objects involved. Objects are destroyed although they are still registered in any lists.
(In reply to Ralf Habacker from comment #1) > When I started working on Umbrello, there were many crashes because various > lists contained undefined pointers. This has been massively mitigated by > using QPointer, so that there are zero pointers left, which are checked and > ignored when accessing the lists. Why these null pointers are contained > there has already been investigated and fixed in some cases in the past, > when the cause was identifiable. One reason for this is probably the lack > of referential integrity of the objects involved. Objects are destroyed > although they are still registered in any lists. I wonder if it would help to have each object containing a UMLObjectList member to connect the "destroyed()" signal, emitted whenever a QObject is about to be destroyed, to some cleanup function which would remove the QPointer from its list? This could also be a slot added to the UMLObjectList class (which otherwise doesn't seem to be doing much of anything on its own) instead of having the object containing the list as a member be responsible for its own cleanup?
I realize now, after looking through the source code and docs for QList and QPointer, that this would not work with signals and slots unless a completely different copying mechanism and/or redesign of UMLObjectList were implemented. Neither QList nor QPointer are descendents of QObject, so they emit no signals. UMLObjectList would most likely have to contain a QList<QPointer<UMLObject>> member instead of inheriting it directly. Read and write operations could be forwarded directly to the QList member, but adding and removing objects (pointers) should be controlled by UMLObjectList. What I can envision now, without knowing all of the details about how this class is used, is the following: 1. Class UMLObjectList should inherit from QObject, or possibly just implement signals and slots using the Q_OBJECT macro; 2. UMLObjectList should connect itself via slot to the "destroyed()" signal of each element added to its list which should only be permitted through its own "append()" function (and removal through a correlated "remove()" function); 3. Whenever a contained element is destroyed, the slot would have to find the pointer in its list and then remove it. Of course, the actual object deletion would be left to QPointer.
(In reply to Robert Hairgrove from comment #3) > What I can envision now, without knowing all of the details about how this > class is used, is the following: > > 1. Class UMLObjectList should inherit from QObject, or possibly just > implement signals and slots using the Q_OBJECT macro; > 2. UMLObjectList should connect itself via slot to the "destroyed()" signal > of each element added to its list which should only be permitted through its > own "append()" function (and removal through a correlated "remove()" > function); > 3. Whenever a contained element is destroyed, the slot would have to find > the pointer in its list and then remove it. Of course, the actual object > deletion would be left to QPointer. Feel free to open working branches in your private umbrello git repo on invent.kde.org where you can add and test appropriate things and result in a merge request. If such a merge request is not yet ready, a draft can be marked with the WIP: prefix. The easiest merge requests to test are those that consist of smaller, self-contained changes.
(In reply to Robert Hairgrove from comment #0) > Using Umbrello from Git/master: > > In file "component.cpp" at line 58 in the method > "UMLComponent::saveToXMI1()": > > for (UMLObjectListIt objectsIt(m_objects); objectsIt.hasNext();) { > UMLObject* obj = objectsIt.next(); > uIgnoreZeroPointer(obj); > obj->saveToXMI1 (writer); > } > > What happens if "obj" == nullptr? A warning is emitted by > "uIgnoreZeroPointer", but "obj" is dereferenced after that anyway, which > causes undefined behavior according to the C++ standard. Looking at the definition of uIgnoreZeroPointer, in umbrello/debug/debug_utils.h : #define uIgnoreZeroPointer(a) if (!a) { uDebug() << "zero pointer detected" << __FILE__ << __LINE__; continue; } The `continue` statement means that the null pointer will not be accessed. Just a minor point... not to detract from the question of why lists contain null pointers at all.
(In reply to Oliver Kellogg from comment #5) > (In reply to Robert Hairgrove from comment #0) > > Using Umbrello from Git/master: > > > > In file "component.cpp" at line 58 in the method > > "UMLComponent::saveToXMI1()": > > > > for (UMLObjectListIt objectsIt(m_objects); objectsIt.hasNext();) { > > UMLObject* obj = objectsIt.next(); > > uIgnoreZeroPointer(obj); > > obj->saveToXMI1 (writer); > > } > > > > What happens if "obj" == nullptr? A warning is emitted by > > "uIgnoreZeroPointer", but "obj" is dereferenced after that anyway, which > > causes undefined behavior according to the C++ standard. > > Looking at the definition of uIgnoreZeroPointer, in > umbrello/debug/debug_utils.h : > #define uIgnoreZeroPointer(a) if (!a) { uDebug() << "zero pointer detected" > << __FILE__ << __LINE__; continue; } > > The `continue` statement means that the null pointer will not be accessed. > > Just a minor point... not to detract from the question of why lists contain > null pointers at all. Thanks, Oliver -- I didn't see the "continue" statement.