Bug 341709

>#6 UMLListViewItem::updateObject (this=0x0) at /usr/src/debug/umbrello-4.14.3/umbrello/umllistviewitem.cpp:338 
>#7 0x0000000000509dae in Import_Utils::createUMLObject (type=type@entry=UMLObject::ot_Class, inName=..., parentPkg=<optimized out>, comment=..., stereotype=..., searchInParentPackageOnly=searchInParentPackageOnly@entry=false) at /usr/src/debug/umbrello-4.14.3/umbrello/codeimport/import_utils.cpp:246

The listed backtrace belongs to the following code fragment from umbrello/codeimport/import_utils.cpp:

UMLObject *createUMLObject(UMLObject::ObjectType type,
                           const QString& inName,
                           UMLPackage *parentPkg,
                           const QString& comment,
                           const QString& stereotype,
                           bool searchInParentPackageOnly)
... 
246             UMLListViewItem *item = UMLApp::app()->listView()->findUMLObject(o);
                    item->updateObject();

The related entry in the list view is created by a signal/slot connection. It looks that the related class is not created in the list view yet.

Git commit c359c26780de7463750166b0f73c565fce52a59e by Ralf Habacker.
Committed on 10/12/2014 at 07:17.
Pushed by habacker into branch 'Applications/14.12'.

Fix 'Crash while importing C++ code from existing project'.

On importing code UML objects are created on the fly. The
related list view item is afterwards created by a signal/slot
connection. There are cases where this seems to be delayed,
so we need to check if the list view item is really present.
FIXED-IN:2.15.0 (KDE 14.12.0)

M  +2    -1    umbrello/codeimport/import_utils.cpp

http://commits.kde.org/umbrello/c359c26780de7463750166b0f73c565fce52a59e

On 10-12-14 08:27, Ralf Habacker wrote:
> https://bugs.kde.org/show_bug.cgi?id=341709
>
> Ralf Habacker <ralf.habacker@freenet.de> changed:
>
>             What    |Removed                     |Added
> ----------------------------------------------------------------------------
>     Version Fixed In|                            |2.15.0 (KDE 14.12.0)
>           Resolution|---                         |FIXED
>        Latest Commit|                            |http://commits.kde.org/umbr
>                     |                            |ello/c359c26780de7463750166
>                     |                            |b0f73c565fce52a59e
>               Status|CONFIRMED                   |RESOLVED
>
> --- Comment #2 from Ralf Habacker <ralf.habacker@freenet.de> ---
> Git commit c359c26780de7463750166b0f73c565fce52a59e by Ralf Habacker.
> Committed on 10/12/2014 at 07:17.
> Pushed by habacker into branch 'Applications/14.12'.
>
> Fix 'Crash while importing C++ code from existing project'.
>
> On importing code UML objects are created on the fly. The
> related list view item is afterwards created by a signal/slot
> connection. There are cases where this seems to be delayed,
> so we need to check if the list view item is really present.
> FIXED-IN:2.15.0 (KDE 14.12.0)

Hey Ralf,

While reading up on Qt I saw somewhere that Qt delays signals until they 
can be processed in their own thread. Could this explain the delay we 
are experiencing here? Probably the application would have to be 
multithreaded for that to happen.
Maybe this explains the delay.

>
> M  +2    -1    umbrello/codeimport/import_utils.cpp
>
> http://commits.kde.org/umbrello/c359c26780de7463750166b0f73c565fce52a59e
>

(In reply to Guus from comment #3)
> While reading up on Qt I saw somewhere that Qt delays signals until they 
> can be processed in their own thread. Could this explain the delay we 
> are experiencing here? Probably the application would have to be 
> multithreaded for that to happen.
> Maybe this explains the delay.
> 
From the related code path the following happens:

Object_Factory::createUMLObject() 
        calls Object_Factory::createNewUMLObject() 
        which 
               1. creates the object
               2. add it to the undo list
               3. signals object creation (UMLApp::app()->document()->signalUMLObjectCreated(o);)
               4. call qApp->processEvents() to handle signals/slots  
               5. return

The list view is connected to the signal mentioned in 3.  It creates the list view entry and adds it to the list view. The question is if this is completly handled by 4. 

To be sure about that, a test case header file is required.

On 10-12-14 12:16, Ralf Habacker wrote:
> https://bugs.kde.org/show_bug.cgi?id=341709
>
>  From the related code path the following happens:
>
> Object_Factory::createUMLObject()
>          calls Object_Factory::createNewUMLObject()
>          which
>                 1. creates the object
>                 2. add it to the undo list
>                 3. signals object creation
> (UMLApp::app()->document()->signalUMLObjectCreated(o);)
>                 4. call qApp->processEvents() to handle signals/slots
>                 5. return
>
> The list view is connected to the signal mentioned in 3.  It creates the list
> view entry and adds it to the list view. The question is if this is completly
> handled by 4.
>
> To be sure about that, a test case header file is required.
>
Do you need anything from me?

(In reply to Guus from comment #5)
> > To be sure about that, a test case header file is required.
> >
> Do you need anything from me?

You wrote: 
> I loaded the source of a project someone else had been writing. While importing the code (it is in
> C++), the handler showed progress in the status bar. Somewhere during the processing it
> crashed. When repeating the action, the result is the same. 
One of the imported files triggers this issue, so if it would be possible to get this related file, the problem could be reproduced in the debugger. 
In the case that you are not allowed to publish this files, it would be nice to have a stripped down test file with which the behavior could be reproduced. 
Another option would be, that you compile umbrello from master or Applications/14.12 branch by yourself and check if the issue is fixed. The issue has been fixed with the commit mentioned in the "Latest commit" field.

Created attachment 89902 [details]
checker.tar.gz

On 10-12-14 12:31, Ralf Habacker wrote:
> https://bugs.kde.org/show_bug.cgi?id=341709
>
> --- Comment #6 from Ralf Habacker <ralf.habacker@freenet.de> ---
> (In reply to Guus from comment #5)
>>> To be sure about that, a test case header file is required.
>>>
>> Do you need anything from me?
> You wrote:
>> I loaded the source of a project someone else had been writing. While importing the code (it is in
>> C++), the handler showed progress in the status bar. Somewhere during the processing it
>> crashed. When repeating the action, the result is the same.
> One of the imported files triggers this issue, so if it would be possible to
> get this related file, the problem could be reproduced in the debugger.
> In the case that you are not allowed to publish this files, it would be nice to
> have a stripped down test file with which the behavior could be reproduced.
> Another option would be, that you compile umbrello from master or
> Applications/14.12 branch by yourself and check if the issue is fixed. The
> issue has been fixed with the commit mentioned in the "Latest commit" field.
>
I just obtained permission to upload. It appears the bulk of the source 
is Apache licensed, the lib part is GPLv3 licensed, according to the 
author. I also pointed out he did not cite GPL anywhere in the source 
code and he indicated this a "todo". So, I will copy the source code and 
attach it as tar.gz file.

Kind regards, Guus.

Thanks for providing the test case. The committed patch fixed one problem, but there are still more crash cases.

Git commit 6de2d9308680a7c7755e66eddb937db85dfc46a7 by Ralf Habacker.
Committed on 11/12/2014 at 10:48.
Pushed by habacker into branch 'Applications/14.12'.

Add dynamic cast test to be sure we can trust getting zero pointer for invalid casts.

M  +21   -0    unittests/TEST_basictypes.cpp

http://commits.kde.org/umbrello/6de2d9308680a7c7755e66eddb937db85dfc46a7

On 11-12-14 11:51, Ralf Habacker wrote:
> https://bugs.kde.org/show_bug.cgi?id=341709
>
> Ralf Habacker <ralf.habacker@freenet.de> changed:
>
>             What    |Removed                     |Added
> ----------------------------------------------------------------------------
>               Status|RESOLVED                    |REOPENED
>           Resolution|FIXED                       |---
>
> --- Comment #8 from Ralf Habacker <ralf.habacker@freenet.de> ---
> Thanks for providing the test case. The committed patch fixed one problem, but
> there are still more crash cases.
>
This does not come as a surprise: the problem of re-engineering from 
code to UML is a non-trivial problem to say the least: it may be very 
hard to solve. When I encountered it I suspected there would be more 
where that came from. I am very interested, because with the errors gone 
it is much easier to study code, starting from the UML. I have only 
started studying C++ (I come from a Java environment) and UML gives a 
great entry to complex code, if it is well designed.

Thanks for your efforts!

Kind regards, Guus.

(In reply to Ralf Habacker from comment #9)
...
> Add dynamic cast test to be sure we can trust getting zero pointer for
> invalid casts.

There is a surprising dynamic_cast problem inModel_Utils::findUMLObject()

At line 202 there is an access to the package child objects:
202:          UMLObjectList objectsInCurrentScope = pkg->containedObjects();

Child objects are supported from UMLPackage and higher derived classes.

UMLObject<- UMLCancasObject<-UMLPackage
                      <- UMLAssociation

In the crash case the package is an UMLAssociation instance, which does support objects.

I added a dedicated dynamic_cast to check this case 
+            UMLPackage *p = dynamic_cast<UMLPackage*>(pkg);
+           qDebug() << p << pkg
            UMLObjectList objectsInCurrentScope = pkg->containedObjects();

and expected to get p == 0 but got

          UMLAssociation(0x1954da0, name = "UMLObject")  UMLAssociation(0x1954da0, name = "UMLObject") 

which means p == 0x1954da0, which is *NOT* zero and indicates that dynamic_cast fails in this context and result into the crash accessing invalid class instance member.

In the opposite a simple testcase 

UMLObject *a = new UMLAssociation;
UMLObject *p = new UMLPackage;

UMLPackage *p1 = dynamic_cast<UMLPackage*>(a)
qDebug() << p1;
UMLPackage *p2 = dynamic_cast<UMLPackage*>(p)
qDebug() << p2;

returns p1 == 0 and p2 != 0 as expected

Git commit 713fe83d4a105e5abe734b95b2ef983954e50e04 by Ralf Habacker.
Committed on 11/12/2014 at 13:18.
Pushed by habacker into branch 'Applications/14.12'.

Fix another 'Crash while importing C++ code from existing project'.

Exclude non UMLPackage based uml objects from accessing not present
m_objects member in Model_Utils::findUMLObject().

A common solution would be to solve the dynamic_cast<UMLPackage*>()
failure (see bug) or to add a virtual bool canHaveObjects() method
to UMLObject, which returns false by default and true for classes
derived from UMLPackage to guard the access to m_objects.

M  +12   -0    umbrello/model_utils.cpp

http://commits.kde.org/umbrello/713fe83d4a105e5abe734b95b2ef983954e50e04

(In reply to Ralf Habacker from comment #11)
> There is a surprising dynamic_cast problem inModel_Utils::findUMLObject()
> 
> At line 202 there is an access to the package child objects:
> 202:          UMLObjectList objectsInCurrentScope = pkg->containedObjects();
> [...]
> In the crash case the package is an UMLAssociation instance, which does
> support objects.

I don't understand: UMLAssociation is not derived from UMLPackage.
In fact, in line 179:
        if (pkg == NULL || pkg->baseType() == UMLObject::ot_Association)
            pkg = currentObj->umlPackage();
the RHS of the "||" should never become true (?)

Git commit e9c3cfbd5c9cf9acd7343589e52f2c15e309c570 by Andi Fischer, on behalf of Ralf Habacker.
Committed on 10/12/2014 at 07:17.
Pushed by fischer into branch 'frameworks'.

Fix 'Crash while importing C++ code from existing project'.

On importing code UML objects are created on the fly. The
related list view item is afterwards created by a signal/slot
connection. There are cases where this seems to be delayed,
so we need to check if the list view item is really present.
FIXED-IN:2.15.0 (KDE 14.12.0)

M  +2    -1    umbrello/codeimport/import_utils.cpp

http://commits.kde.org/umbrello/e9c3cfbd5c9cf9acd7343589e52f2c15e309c570

(In reply to Ralf Habacker from comment #11)
> In the crash case the package is an UMLAssociation instance, which does support objects.
.. wording mistake, it should be 
> In the crash case the package is an UMLAssociation instance, which does *NOT* support objects.

(In reply to Oliver Kellogg from comment #13)
> (In reply to Ralf Habacker from comment #11)
> > There is a surprising dynamic_cast problem inModel_Utils::findUMLObject()
> > 
> > At line 202 there is an access to the package child objects:
> > 202:          UMLObjectList objectsInCurrentScope = pkg->containedObjects();
> > [...]
> > In the crash case the package is an UMLAssociation instance, which does
> > support objects.
> 
> I don't understand: UMLAssociation is not derived from UMLPackage.
I also do not understand the reason why a dynamic_cast<UMLPackage*> of an UMLAssociation instance does not return 0. :-(

> In fact, in line 179:
>         if (pkg == NULL || pkg->baseType() == UMLObject::ot_Association)
>             pkg = currentObj->umlPackage();
> the RHS of the "||" should never become true (?)

This is not the source of the problem. The probolem happens later in the loop 

To see some details I added the following code:

#define EMBED_BREAKPOINT  asm volatile ("int3;")

        for (; pkg; pkg = currentObj->umlPackage()) {
            if (pkg->umlPackage() && pkg->umlPackage()->baseType() == UMLObject::ot_Association) {
                qDebug() << "1" << pkg << pkg->umlPackage() << pkg->umlPackage()->umlPackage();
                UMLPackage *p = dynamic_cast<UMLPackage*>(pkg->umlPackage());
                qDebug() << "2" << p;
                EMBED_BREAKPOINT;
            }

imported the appended test case and got a break with the following output: 
1 UMLClassifier(0x3ff3ef0, name = "UMLObject") UMLAssociation(0x197a7e0, name = "UMLObject") UMLFolder(0xe405b0, name = "UMLObject") 
2 UMLAssociation(0x197a7e0, name = "UMLObject") 

1. shows that an UMLAssociation instance is a parent of an UML Classifier
    - package class name is 'testing' 
    - the parent is a dependency with 
       - Role A - class "ConstraintPackage"
       - Role B - class "ConstraintPackageField"

2. shows a failed dynamic_cast

(In reply to Ralf Habacker from comment #16)
> (In reply to Oliver Kellogg from comment #13)
> > (In reply to Ralf Habacker from comment #11)
 
> 1. shows that an UMLAssociation instance is a parent of an UML Classifier
>     - package class name is 'testing' 
>     - the parent is a dependency with 
>        - Role A - class "ConstraintPackage"
>        - Role B - class "ConstraintPackageField"
> 
The related code is in reachability/test_util.h

25: class TestNetwork : public ::testing::Test {

in import_utils.cpp::createUMLObject() line 237 there is called: 
            if (components.count() > 1) {
                typeName = components.back();
                components.pop_back();
                while (components.count()) {
                    QString scopeName = components.front();
                    components.pop_front();
                    o = umldoc->findUMLObject(scopeName, UMLObject::ot_UMLObject, parentPkg);

o returns an UMLAssociation instance named 'testing'

                    if (o) {
                        parentPkg = static_cast<UMLPackage*>(o);

which is casted to an UMLPackage and sets parentPkg not to zero. 

It looks wrong to accept an association as class parent, but the root cause is that dynamic_cast fails to cast an UMLAssociation instance to UMLPackage* to zero. 

BTW: I compiled umbrello on opensuse 13.1 x86_64 with gcc 4.8.1

(In reply to Ralf Habacker from comment #17)
> 
> o returns an UMLAssociation instance named 'testing'

This has been fixed with bug 341799