GIT HEAD (commit 3409db0b671a61440cb7e89dd024adf961b21cee) Crash when I 1) create new component diagram 2) create component 3) create two pins for the component 4) select component and remove it from diagram using Delete key 5) select pins and remove them from diagram using Delete key Backtrace #0 0x0000000000608123 in WidgetBase::id() const (this=0xf51c90) at /home/umbrello/umbrello/src/umbrello/umlwidgets/widgetbase.cpp:171 #1 0x00000000005f5b3d in PinPortBase::saveToXMI(QDomDocument&, QDomElement&) (this=0xefa390, qDoc=..., qElement=...) at /home/umbrello/umbrello/src/umbrello/umlwidgets/pinportbase.cpp:323 element = {<QDomNode> = {impl = 0xc17f80}, <No data fields>} #2 0x00000000005a48c4 in Uml::CmdRemoveWidget::CmdRemoveWidget(UMLWidget*) (this=0xf1d130, widget=0xefa390) at /home/umbrello/umbrello/src/umbrello/cmds/widget/cmdremovewidget.cpp:33 doc = {<QDomNode> = {impl = 0xf98850}, <No data fields>} #3 0x0000000000476c11 in UMLScene::removeWidget(UMLWidget*) (this=this@entry=0xeefb50, o=o@entry=0xefa390) at /home/umbrello/umbrello/src/umbrello/umlscene.cpp:1167 #4 0x000000000047afab in UMLScene::deleteSelection() (this=0xeefb50) at /home/umbrello/umbrello/src/umbrello/umlscene.cpp:1401 widget = 0xefa390 _container_ = {c = {{p = {static shared_null = {ref = {_q_value = 3408}, alloc = 0, begin = 0, end = 0, sharable = 1, array = {0x0}}, d = 0xde7840}, d = 0xde7840}}, brk = 0, i = {i = 0xde7858}, e = {i = 0xde7868}} selectionCount = 2 #5 0x000000000045082a in UMLApp::slotDeleteSelected() (this=0xba8670) at /home/umbrello/umbrello/src/umbrello/uml.cpp:2994 f = <optimized out> __PRETTY_FUNCTION__ = "void UMLApp::slotDeleteSelected()" #6 0x00007ffff449c0ba in QMetaObject::activate(QObject*, QMetaObject const*, int, void**) () at /usr/lib64/libQtCore.so.4 #7 0x00007ffff4c0d5b2 in QAction::triggered(bool) () at /usr/lib64/libQtGui.so.4 #8 0x00007ffff4c0ef63 in QAction::activate(QAction::ActionEvent) () at /usr/lib64/libQtGui.so.4 #9 0x00007ffff4c0f0bc in QAction::event(QEvent*) () at /usr/lib64/libQtGui.so.4 #10 0x00007ffff640c33f in KAction::event(QEvent*) () at /usr/lib64/libkdeui.so.5 #11 0x00007ffff4c136fc in QApplicationPrivate::notify_helper(QObject*, QEvent*) () at /usr/lib64/libQtGui.so.4 #12 0x00007ffff4c19c3d in QApplication::notify(QObject*, QEvent*) () at /usr/lib64/libQtGui.so.4 #13 0x00007ffff64e1e9a in KApplication::notify(QObject*, QEvent*) () at /usr/lib64/libkdeui.so.5 #14 0x00007ffff448814d in QCoreApplication::notifyInternal(QObject*, QEvent*) () at /usr/lib64/libQtCore.so.4 #15 0x00007ffff4c44986 in () at /usr/lib64/libQtGui.so.4 #16 0x00007ffff4c44abc in () at /usr/lib64/libQtGui.so.4 #17 0x00007ffff4c1af73 in QApplication::notify(QObject*, QEvent*) () at /usr/lib64/libQtGui.so.4 #18 0x00007ffff64e1e9a in KApplication::notify(QObject*, QEvent*) () at /usr/lib64/libkdeui.so.5 #19 0x00007ffff448814d in QCoreApplication::notifyInternal(QObject*, QEvent*) () at /usr/lib64/libQtCore.so.4 #20 0x00007ffff4cada57 in () at /usr/lib64/libQtGui.so.4 #21 0x00007ffff4caddf9 in () at /usr/lib64/libQtGui.so.4 #22 0x00007ffff4c89a0f in QApplication::x11ProcessEvent(_XEvent*) () at /usr/lib64/libQtGui.so.4 #23 0x00007ffff4cb0552 in () at /usr/lib64/libQtGui.so.4 #24 0x00007ffff0546c84 in g_main_context_dispatch () at /usr/lib64/libglib-2.0.so.0 #25 0x00007ffff0546ed8 in () at /usr/lib64/libglib-2.0.so.0 #26 0x00007ffff0546f7c in g_main_context_iteration () at /usr/lib64/libglib-2.0.so.0 #27 0x00007ffff44b4f7e in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () at /usr/lib64/libQtCore.so.4 #28 0x00007ffff4cb0606 in () at /usr/lib64/libQtGui.so.4 #29 0x00007ffff4486d0f in QEventLoop::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () at /usr/lib64/libQtCore.so.4 Root cause seems to be that PinPortBase stores pointer to owner widget, but does not get notified if owner gets removed from diagram. Please review and apply attached patch that fixes the crash.
Created attachment 101978 [details] Patch to fix crash: Remove PinPortBase objects when owner is removed.
Comment on attachment 101978 [details] Patch to fix crash: Remove PinPortBase objects when owner is removed. I can confirm that the patch fixes the crash. Looking at the patch I have a few questions/notes: 1. The owner management of the PinWidgets is done by the private member m_pOw. UMLWidgets is derived from QGraphicsItem which also has a owner/parent management with parentItem(). In principal it looks possible to use that instead of m_pOw. Any idea how much the related effort would be to fix the issue that way ? 2. The usage of Q_ASSERT(m_baseType >= wt_UMLWidget && m_baseType <= wt_Port); is good, but also introduces in further development to miss some added enum values. It would be better to introduce enum constants for the upper and lower limit and to check against them. 3. Redoing has issues: I removed one pin, then the component and tried to redo both with results into incomplete redo. 4. For detailed review there is reviewboard.kde.org
Regarding 1.: I'm not sure how much effort it will be, but it's definitely better than the current approach. I'll give it a try, but might take some days. Regarding 2.: You're right. I've created a separate patch for this improvement to shorten the remaining changes. See new attachment. Regarding 3.: Yes, I've encountered this too. Delete component with pins cannot be undone, pins don't appear back. I think a proper solution might be to extend the cmdremovewidget to maintain list of child widgets in addition to the parent widget. What do you think?
Created attachment 102013 [details] Enforce valid baseType
Git commit 2ed4f7baedb1766452a5b08ffaf882ab09dcdbcf by Ralf Habacker, on behalf of Carsten Clever. Committed on 04/11/2016 at 07:42. Pushed by habacker into branch 'Applications/16.08'. Add wrapper for WidgetBase::m_baseType and enforce valid range. Reviewed by Ralf Habacker. M +4 -4 umbrello/umlwidgets/classifierwidget.cpp M +2 -1 umbrello/umlwidgets/pinportbase.cpp M +1 -0 umbrello/umlwidgets/portwidget.cpp M +4 -4 umbrello/umlwidgets/umlwidget.cpp M +12 -0 umbrello/umlwidgets/widgetbase.cpp M +7 -2 umbrello/umlwidgets/widgetbase.h http://commits.kde.org/umbrello/2ed4f7baedb1766452a5b08ffaf882ab09dcdbcf
Comment on attachment 102013 [details] Enforce valid baseType Thanks for your contribution which is a good addition. You made m_baseType private to force clients to use the wrapper. I applied that fix into umbrello git repo which requires that you rebase your patch against umbrello-master. Because this is a crash bug I would apply the fix to Applications/16.08 to get it into next release 2.20.3. Unfortunally 2.20.3 will be released at November 10, so there is not much time left. If you have problems to update your patch using the mentioned QGraphicsItems proposal in that time frame, I will use the recent (and rebased) patch for that release. The QGraphicsItem based fix would then be in the next release series 2.21 (Applications/16.12 branch)
Thank's for commit the first part already. I've rebased and retested the fix for the crash, and found another issue when selecting both component and attached pin. That's fixed as well with the new patch. I tried the suggested child/parent cleanup, but encounter several problems with this. Briefly, the PinPortBase is a subclass of UMLWidget, but this class is meant to represent root graphic items. The PinPortBase would be a child widget, and needs to behave differently, e.g. when moving the parent component. I'll continue to work on this.
Created attachment 102037 [details] Patch to fix crash: Remove PinPortBase objects when owner is removed - rebased
Comment on attachment 101978 [details] Patch to fix crash: Remove PinPortBase objects when owner is removed. superseeded by attachment 102037 [details]
Git commit 536e52fe6f71145cdcee16b5833f1c73b64e61fa by Ralf Habacker. Committed on 08/11/2016 at 07:39. Pushed by habacker into branch 'Applications/16.08'. Fix 'Crash when removing pin widgets after owning component has been removed before'. The fix from clushkou@gmx.net is designed to remove PinPortBase objects when owner is remove. FIXED-IN:2.20.3 (KDE Applications 16.08.3) M +41 -0 umbrello/umlscene.cpp M +6 -0 umbrello/umlscene.h M +32 -1 umbrello/umlwidgets/pinportbase.cpp M +6 -1 umbrello/umlwidgets/pinportbase.h M +4 -4 umbrello/umlwidgets/portwidget.cpp http://commits.kde.org/umbrello/536e52fe6f71145cdcee16b5833f1c73b64e61fa
Created attachment 102196 [details] Turn PinPortBase into child item Attached patch cleans up m_pOw in PinPortBase, and fixes all issues arising due to this change.
Comment on attachment 102037 [details] Patch to fix crash: Remove PinPortBase objects when owner is removed - rebased applied to Application/16.08
Comment on attachment 102196 [details] Turn PinPortBase into child item I checked if this it works using the following use cases: 1. Add component diagram 2. Add component 3. add two pins 4. save file 5. load file -> okay 6. delete second pin 7. delete first pin 8. undo both delete -> okay 9 set different color for component and each pin 10. save and reload -> okay Looks good :-) 11. delete second ping 12. delete component 13. undo component delete 14. undo -> regardless of number of undo the first pin is not restored and need to be readded manual (may be a different bug) Running after creating a new file 1. Add component diagram 2. Add component 3. add two pins 4. delete second ping 5. delete component 6. undo component delete 7. repeated undo -> I get Q_ASSERT in CmdBaseWidgetCommand::widget()
Created attachment 102311 [details] Restrict write access to UMLScene::widgetList to track NULL-pointer list entries. (1/2) PinPortBase as child widget: Restrict write access to UMLScene::widgetList to track NULL-pointer list entries.
Created attachment 102312 [details] Turn PinPortBase into real child widget. (2/2) PinPortBase as child widget: Turn PinPortBase into real child widget.
I could reproduce the crash. It's due to remove undo not restoring the child widgets. Subsequent undo commands assume the child widget exists and access a deleted widget. I've splitted and updated the patch to fix this issue as well.
Comment on attachment 102312 [details] Turn PinPortBase into real child widget. A quick look shows that the following code + foreach(QGraphicsItem* item, widget->childItems()) { + UMLWidget* child = dynamic_cast<UMLWidget*>(item); + QDomDocument doc; + m_children.append(doc.createElement(QLatin1String("child"))); + child->saveToXMI(doc, m_children.back()); + } + needs to goto into ComponentWidget::saveToXMI() and this + foreach(QDomElement childElement, m_children) { + widgetElement = childElement.firstChild().toElement(); + widget = umlScene->loadWidgetFromXMI(widgetElement); + if (0 != widget) { + addWidgetToScene(widget); + } + } into ComponentWidget::loadToXMI() to support also saving to and loading from file. If this is fixed I'm going to test this implementation. Thanks for contribution.
Git commit c7250ebcb0886f47f4e694a6b4d2ea369adc0a89 by Ralf Habacker, on behalf of Carsten Clever. Committed on 26/11/2016 at 22:07. Pushed by habacker into branch 'Applications/16.12'. Restrict write access to UMLScene::widgetList to track NULL-pointer list entries. M +1 -1 umbrello/clipboard/umlclipboard.cpp M +1 -1 umbrello/clipboard/umlclipboard.h M +1 -1 umbrello/cmds/widget/cmdbasewidgetcommand.cpp M +2 -2 umbrello/codeimport/import_utils.cpp M +1 -1 umbrello/toolbarstatemessages.cpp M +12 -6 umbrello/umlscene.cpp M +4 -1 umbrello/umlscene.h M +2 -2 umbrello/umlwidgets/combinedfragmentwidget.cpp https://commits.kde.org/umbrello/c7250ebcb0886f47f4e694a6b4d2ea369adc0a89
Comment on attachment 102311 [details] Restrict write access to UMLScene::widgetList to track NULL-pointer list entries. Applied to Applications/16.12 branch with CCBUG: key instead of BUG: to not close the bug.
I tried to fulfill your request in comment #17, but do not understand how to achieve this. If I simply move the calls to ComponentWidget, I get a crash on undo because restoring of child element does not work. In addition, if the code needs to be added to ComponentWidget, the same adaption is needed in all widgets that allow child Pins / Ports.
Created attachment 102527 [details] Another patch proposal. CompoentWidget::loadFromXmi and ::saveToXmi adapted, but crashing.
(In reply to clushkou from comment #20) > I tried to fulfill your request in comment #17, but do not understand how to > achieve this. If I simply move the calls to ComponentWidget, I get a crash > on undo because restoring of child element does not work. > In addition, if the code needs to be added to ComponentWidget, the same > adaption is needed in all widgets that allow child Pins / Ports. I see. For the record: umbrello uses saveToXMI and loadFromXMI() to handle xmi file save/load, object (de)serialisation on copy/paste between same and other umbrello instances and undo framework so it would be nice to hold related stuff together and to implement stuff in the same way if possible to reduce req What I did: I created a component widget with ports and saved it into a xmi file. Inspecting this file shows that the component widget and the pin widgets does not have any direct association. The association is performed by the related UML:Component which has UML:Port as child. The in-memory representation changes this behavior by adding a direct association former with m_pow and now this QGraphicsItem child api.
(In reply to Ralf Habacker from comment #22) > I see. For the record: umbrello uses saveToXMI and loadFromXMI() to handle > xmi file save/load, object (de)serialisation on copy/paste between same and > other umbrello instances and undo framework so it would be nice to hold > related stuff together and to implement stuff in the same way if possible to > reduce req .. required support efforts and further extensions.
(In reply to Ralf Habacker from comment #22) >... The in-memory > representation changes this behavior by adding a direct association former > with m_pow and now this QGraphicsItem child api. This is used by the undo framework, but not by the copy/paste and load/save framework. It would be nice to have a way to keep handling of port widgets in sync in all mentioned frameworks.
Git commit 703b25a024380dcb81895c9700c3a8d4d72cfb74 by Ralf Habacker. Committed on 03/12/2016 at 17:00. Pushed by habacker into branch 'master'. Turn PinPortBase into real child widget. M +17 -1 umbrello/cmds/widget/cmdremovewidget.cpp M +2 -1 umbrello/cmds/widget/cmdremovewidget.h M +15 -44 umbrello/umlscene.cpp M +126 -127 umbrello/umlwidgets/pinportbase.cpp M +26 -11 umbrello/umlwidgets/pinportbase.h M +3 -12 umbrello/umlwidgets/pinwidget.cpp M +0 -1 umbrello/umlwidgets/pinwidget.h M +3 -15 umbrello/umlwidgets/portwidget.cpp M +1 -2 umbrello/umlwidgets/portwidget.h M +20 -4 umbrello/umlwidgets/umlwidget.cpp M +1 -0 umbrello/umlwidgets/umlwidget.h M +8 -8 umbrello/umlwidgets/widget_factory.cpp https://commits.kde.org/umbrello/703b25a024380dcb81895c9700c3a8d4d72cfb74
Comment on attachment 102312 [details] Turn PinPortBase into real child widget. committed to master.
Created attachment 102601 [details] Turn PinPortBase into real child widget. CCBUG:371990
Comment on attachment 102527 [details] Another patch proposal. CompoentWidget::loadFromXmi and ::saveToXmi adapted, but crashing. Rebased last patch
Created attachment 102602 [details] Turn PinPortBase into child widget. BUG:371990
Created attachment 102603 [details] Another patch proposal. CompoentWidget::loadFromXmi and ::saveToXmi adapted, but crashing. (rebased)
(In reply to clushkou from comment #20) > I tried to fulfill your request in comment #17, but do not understand how to > achieve this. I applied your first approach to git master. Thanks for that contribution. > If I simply move the calls to ComponentWidget, I get a crash > on undo because restoring of child element does not work. It looks that this would require more refactoring :-( See also my comment below. > In addition, if the code needs to be added to ComponentWidget, the same > adaption is needed in all widgets that allow child Pins / Ports. From a internet research I got the impression that ports are used used with Components (https://en.wikipedia.org/wiki/Component_diagram) and pins with activities (https://de.wikipedia.org/wiki/Aktion_(UML)). The load/save implementation differs also between pins and ports in that way: activities and pins are not backed up by a related uml model objects, they are only widgets. The relation from the pin widget to the activity widget is handled with the <pinwidget> tag attribute "widgetaid". The same is true for the relation port widget to component widget. Currently I do not have any idea how the refactoring could be done. With that we have two choices: 1. leave this bug open as reminder for that open task or 2. close this bug because the initial crash has been fixed and a basic refactoring has been implemented and to open a new bug describing the left issue as a reminder.
(In reply to Ralf Habacker from comment #31) > From a internet research I got the impression that ports are used used with > Components (https://en.wikipedia.org/wiki/Component_diagram) Looking at this page and having your updated implementation of PinPortWidget it looks now possible to place the ports on the component border (---[]---) instead of outside ( ____[]______)
Thank you for accepting and committing the changes. I can continue to work on this topic including refactorings. For actual fixes of differences between Components and Activities, I would need further insight that might come along with working on the code base. For simplicity, I suggest we continue to use this ticket for now.
I just tried to connect a pin on an activity diagram with another activity. The result is that one end of the connection ends on the "another activity", the other end does *not* ends on the pin. It ends somewhere in empty diagram space. My guess it that the connection end point calculation seems to be wrong now.
(In reply to Ralf Habacker from comment #34) > I just tried to connect a pin on an activity diagram with another activity. > The result is that one end of the connection ends on the "another activity", > the other end does *not* ends on the pin. It ends somewhere in empty diagram > space. fixed with bug 373308
Comparing the new implementation with the old one from 2.20 shows a little remaining issue: In the old implementation the position of the pin/port been added depends on the current mouse position. If the mouse is in the upper half of the related widget, the pin/port is added on top of the widget and at the horizontal mouse position. If the mouse is in the lower half of the widget the pin/port is added on the bottom and at the horizontal mouse position.
Git commit f389d9d163d5755fb57db9a834e28d4e3b8bd9fa by Ralf Habacker. Committed on 08/12/2016 at 20:06. Pushed by habacker into branch 'master'. Local fix for using deprecated Qt5 method QGraphicsScene::itemAt(QPointF). M +9 -0 umbrello/umlscene.h https://commits.kde.org/umbrello/f389d9d163d5755fb57db9a834e28d4e3b8bd9fa
(In reply to Ralf Habacker from comment #36) This is not straight forward to fix because for Ports, the parent widget is not set at the required point in time. A PortWidget first creates its parent PinPortBase, and afterwards sets the parent widget. But PinPortBase c'tor calls its init() function which would then try to place the widget relative to the parent. The code should be refactored first to set the parent widget as early as possible.
At https://mail.kde.org/pipermail/umbrello-devel/2016-December/020658.html it looks that coverity detected implementation issues with your code (see CID 173354 and 173353). It would be nice to have a fix the for the mentioned issues.
Created attachment 102921 [details] Fix NULL pointer derefences found by Coverity - CIDs 173354 and 173353.
Git commit 9ed47597b5506956e1ca9f4bced7ece3efe12d3e by Ralf Habacker, on behalf of Carsten Clever. Committed on 21/12/2016 at 14:15. Pushed by habacker into branch 'master'. Fix coverity check CID 173354 and 173353: NULL pointer derefences Related: bug 340646 M +5 -3 umbrello/cmds/widget/cmdremovewidget.cpp M +3 -2 umbrello/umlscene.cpp https://commits.kde.org/umbrello/9ed47597b5506956e1ca9f4bced7ece3efe12d3e
Comment on attachment 102921 [details] Fix NULL pointer derefences found by Coverity - CIDs 173354 and 173353. Patch with coverity fixes has been applied to git master. Many thanks for contributing.
Remaining issue has been reported with bug 374879.