New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Crash when trying to creat a new folder #98

Closed
theophae opened this Issue Oct 2, 2015 · 26 comments

Comments

Projects
None yet
6 participants
@theophae
Copy link
Contributor

theophae commented Oct 2, 2015

I you try to create a folder in any of the different tabs, after entering the folder's name, it will crash. When I debug it, I find it bug at BtTreeModel.cpp line 483 ( beginInsertRows(parent,row,row); ) but I really don't know why.
This bug happens on windows 7 with the current master version (it is not a new bug, but I wasn't sure of my compilation chain at the time). On linux it works normally.

@rocketman768 rocketman768 added the bug label Oct 3, 2015

@rocketman768

This comment has been minimized.

Copy link
Member

rocketman768 commented Oct 3, 2015

Can you get a backtrace and print parent and row? I am currently without a Windows machine.

@rocketman768 rocketman768 added this to the v2.2.0 milestone Oct 3, 2015

@theophae

This comment has been minimized.

Copy link
Contributor

theophae commented Oct 4, 2015

I tryed to get a backtrace, but it tells me "No stack", so I cant provide what you ask for. I'll check the value for parent and row as you suggest.

@rocketman768

This comment has been minimized.

Copy link
Member

rocketman768 commented Oct 5, 2015

Whoever pushes a fix for this, make the pull request against stable/2.2.0 instead of master.

@theophae

This comment has been minimized.

Copy link
Contributor

theophae commented Oct 5, 2015

I finally succeeded to get a backtrace:

Thread 1 (Thread 16088.0x3fb4):
#0  0x774ce06c in ntdll!RtlAllocateHeap () from C:\Windows\SysWOW64\ntdll.dll
No symbol table info available.
#1  0x7579ade8 in towlower () from C:\Windows\syswow64\msvcrt.dll
No symbol table info available.
#2  0x7579c470 in msvcrt!calloc () from C:\Windows\syswow64\msvcrt.dll
No symbol table info available.
#3  <function called from gdb>
No symbol table info available.
#4  qt_noop () at ../../include/QtCore/../../src/corelib/global/qglobal.h:576
No locals.
#5  0x6b90526d in QAbstractItemModel::beginInsertRows (this=0x3393c970, parent=..., first=6, last=6) at itemmodels\qabstractitemmodel.cpp:2602
        d = 0xcc72ecf0
#6  0x00415587 in BtTreeModel::insertRow (this=0x3393c970, row=6, parent=..., victim=0x353b8c08, victimType=8) at C:\Users\502380514\brewtarget_dev\src\BtTreeModel.cpp:483
        pItem = 0x3393ce88
        type = 0
        success = true
#7  0x0041811b in BtTreeModel::createFolderTree (this=0x3393c970, dirs=..., parent=0x3393ce88, pPath=...) at C:\Users\502380514\brewtarget_dev\src\BtTreeModel.cpp:1164
        fPath = {static null = {<No data fields>}, d = 0x3572ecf0}
        temp = 0x353b8c08
        i = 6
        cur = {static null = {<No data fields>}, d = 0x35726ab0}
        _container_ = {c = {<QList<QString>> = {<QListSpecialMethods<QString>> = {<No data fields>}, {p = {static shared_null = {ref = {atomic = {_q_value = -1}}, alloc = 0, begin = 0, end = 0, array = {0x0}}, d = 0x354738d0}, d = 0x354738d0}}, <No data fields>}, i = {i = 0x354738e0}, e = {i = 0x354738e4}, control = 1}
        pItem = 0x3393ce88
        ndx = {r = 6, c = 0, i = 865324680, m = 0x3393c970}
#8  0x004187ac in BtTreeModel::findFolder (this=0x3393c970, name=..., parent=0x3393ce88, create=true) at C:\Users\502380514\brewtarget_dev\src\BtTreeModel.cpp:1254
        pItem = 0x3393ce88
        dirs = {<QList<QString>> = {<QListSpecialMethods<QString>> = {<No data fields>}, {p = {static shared_null = {ref = {atomic = {_q_value = -1}}, alloc = 0, begin = 0, end = 0, array = {0x0}}, d = 0x354738d0}, d = 0x354738d0}}, <No data fields>}
        current = {static null = {<No data fields>}, d = 0x35726ab0}
        fullPath = {static null = {<No data fields>}, d = 0x355c4f48}
        targetPath = {static null = {<No data fields>}, d = 0x30fac780}
        i = 6
#9  0x00417731 in BtTreeModel::addFolder (this=0x3393c970, name=...) at C:\Users\502380514\brewtarget_dev\src\BtTreeModel.cpp:1030
        ndx = {r = 6150223, c = 896690864, i = 4294967295, m = 0x288ca8}
#10 0x0041a31a in BtTreeView::addFolder (this=0x30fd9240, folder=...) at C:\Users\502380514\brewtarget_dev\src\BtTreeView.cpp:185
No locals.
#11 0x004b348a in MainWindow::newFolder (this=0x33937c98) at C:\Users\502380514\brewtarget_dev\src\MainWindow.cpp:1461
        dPath = {static null = {<No data fields>}, d = 0x3575e380}
        starter = {r = 0, c = 0, i = 865441832, m = 0x3393c900}
        active = 0x30fd9240
        indexes = {<QListSpecialMethods<QModelIndex>> = {<No data fields>}, {p = {static shared_null = {ref = {atomic = {_q_value = -1}}, alloc = 0, begin = 0, end = 0, array = {0x0}}, d = 0x35268e90}, d = 0x35268e90}}
        name = {static null = {<No data fields>}, d = 0x35726ab0}
#12 0x0051c4ab in MainWindow::qt_static_metacall (_o=0x33937c98, _c=QMetaObject::InvokeMetaMethod, _id=38, _a=0x288ef8) at C:/Users/502380514/brewtarget_build/src/moc_MainWindow.cpp:381
        _t = 0x33937c98

and the value of parent is:

parent  "Recettes"  QModelIndex &
[0, 0]  <not accessible>    
[0, 1]  <not accessible>    
[0, 2]  <not accessible>    
[1, 0]  <not accessible>    
[1, 1]  <not accessible>    
[1, 2]  <not accessible>    
[2, 0]  <not accessible>    
[2, 1]  <not accessible>    
[2, 2]  <not accessible>    
[3, 0]  <not accessible>    
[3, 1]  <not accessible>    
[3, 2]  <not accessible>    
[4, 0]  <not accessible>    
[4, 1]  <not accessible>    
[4, 2]  <not accessible>    
[5, 0]  <not accessible>    
[5, 1]  <not accessible>    
[5, 2]  <not accessible>    
c   0   int
i   864706384   quintptr
m   @0x338a5a38 BtTreeModel
    [QAbstractItemModel]    1 x 3   QAbstractItemModel
        [0, 0]  <not accessible>    QModelIndex
        [0, 1]  <not accessible>    QModelIndex
        [0, 2]  <not accessible>    QModelIndex
    [children]  <0 items>   QObjectList
    [methods]   <19 items>  
        elementAdded        
        elementChanged      
        elementRemoved      
        expandFolder        
        folderChanged       
    [parent]    "treeView_recipe"   RecipeTreeView
        [BtTreeView]    "treeView_recipe"   BtTreeView
        [children]  <11 items>  QObjectList
        [methods]   <0 items>   
        [parent]    "tabTree_recipe"    QWidget
        [properties]    <more than 0 items> 
        [signals]   <0 items>   
        staticMetaObject    @0x683200   QMetaObject
    [properties]    <more than 0 items> 
    [signals]   <1 items>   
        [connections]   <more than 0 items> 
        expandFolder        
    _mimeType   "application/x-brewtarget-recipe"   QString
        [0] 97 'a'  QChar
        [1] 112 'p' QChar
        [2] 112 'p' QChar
        [3] 108 'l' QChar
        [4] 105 'i' QChar
        [5] 99 'c'  QChar
        [6] 97 'a'  QChar
        [7] 116 't' QChar
        [8] 105 'i' QChar
        [9] 111 'o' QChar
        [10]    110 'n' QChar
        [11]    47 '/'  QChar
        [12]    120 'x' QChar
        [13]    45 '-'  QChar
        [14]    98 'b'  QChar
        [15]    114 'r' QChar
        [16]    101 'e' QChar
        [17]    119 'w' QChar
        [18]    116 't' QChar
        [19]    97 'a'  QChar
        [20]    114 'r' QChar
        [21]    103 'g' QChar
        [22]    101 'e' QChar
        [23]    116 't' QChar
        [24]    45 '-'  QChar
        [25]    114 'r' QChar
        [26]    101 'e' QChar
        [27]    99 'c'  QChar
        [28]    105 'i' QChar
        [29]    112 'p' QChar
        [30]    101 'e' QChar
    _type   0   int
    parentTree  "treeView_recipe"   RecipeTreeView
    rootItem    @0x338a5860 BtTreeItem
    staticMetaObject    @0x622190   QMetaObject
    treeMask    BtTreeModel::RECIPEMASK (1) BtTreeModel::TypeMasks
r   6   int

And row = 6

I really don't know how it works! When you add a recipe it calls the same function with the same arguments, but when you do the same for a folder it cashes. I need to figure out what is the difference between these two cases.

@theophae

This comment has been minimized.

Copy link
Contributor

theophae commented Oct 5, 2015

Can someone try to reproduce this bug on windows 7 to see if it is my setup that causes the crash ?
I'm using a 64 bits version of windows.
I really stuck on this problem because it crashes on the line I indicate in my first comment, and, as it is a Qt function, I cannot execute it step by step.
It could also be a bug on my version of Qt (5.4)

@f1oki

This comment has been minimized.

Copy link
Contributor

f1oki commented Oct 6, 2015

I only have access to Windows 8. I can give it a try Friday or during the weekend.

@rocketman768

This comment has been minimized.

Copy link
Member

rocketman768 commented Oct 9, 2015

We are smashing the stack with an uninitialized variable somewhere in this chain, as pointed out by valgrind --tool=memcheck --track-origins=yes. The following is on the stable/2.2.0 branch.

==14408== Conditional jump or move depends on uninitialised value(s)
==14408==    at 0x9935FDF: ??? (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.3.2)
==14408==    by 0x993A6D2: ??? (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.3.2)
==14408==    by 0x9999FDC: QMetaObject::activate(QObject*, int, int, void**) (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.3.2)
==14408==    by 0x9A17733: QAbstractItemModel::rowsAboutToBeInserted(QModelIndex const&, int, int, QAbstractItemModel::QPrivateSignal) (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.3.2)
==14408==    by 0x990E402: QAbstractItemModel::beginInsertRows(QModelIndex const&, int, int) (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.3.2)
==14408==    by 0x4DE9F5: BtTreeModel::insertRow(int, QModelIndex const&, QObject*, int) (BtTreeModel.cpp:483)
==14408==    by 0x4E1CA8: BtTreeModel::createFolderTree(QStringList, BtTreeItem*, QString) (BtTreeModel.cpp:1164)
==14408==    by 0x4E248A: BtTreeModel::findFolder(QString, BtTreeItem*, bool) (BtTreeModel.cpp:1254)
==14408==    by 0x4E1130: BtTreeModel::addFolder(QString) (BtTreeModel.cpp:1030)
==14408==    by 0x4E9762: BtTreeView::addFolder(QString) (BtTreeView.cpp:185)
==14408==    by 0x5CE343: MainWindow::newFolder() (MainWindow.cpp:1474)
==14408==    by 0x6A4393: MainWindow::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) (moc_MainWindow.cpp:380)
==14408==  Uninitialised value was created by a heap allocation
==14408==    at 0x4C28C20: malloc (vg_replace_malloc.c:296)
==14408==    by 0x972F23E: QArrayData::allocate(unsigned long, unsigned long, unsigned long, QFlags<QArrayData::AllocationOption>) (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.3.2)
==14408==    by 0x97B74A3: ??? (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.3.2)
==14408==    by 0x97B7674: ??? (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.3.2)
==14408==    by 0x9935A60: ??? (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.3.2)
==14408==    by 0x993BA1D: QSortFilterProxyModel::columnCount(QModelIndex const&) const (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.3.2)
==14408==    by 0x51D4F25: QHeaderView::initializeSections() (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.3.2)
==14408==    by 0x51B9DA0: QAbstractItemView::setModel(QAbstractItemModel*) (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.3.2)
==14408==    by 0x51D6713: QHeaderView::setModel(QAbstractItemModel*) (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.3.2)
==14408==    by 0x520839F: QTreeView::setModel(QAbstractItemModel*) (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.3.2)
==14408==    by 0x4E8D72: BtTreeView::BtTreeView(QWidget*, BtTreeModel::TypeMasks) (BtTreeView.cpp:60)
==14408==    by 0x4ECF83: RecipeTreeView::RecipeTreeView(QWidget*) (BtTreeView.cpp:569)
@rocketman768

This comment has been minimized.

Copy link
Member

rocketman768 commented Oct 12, 2015

@Brewtarget/developers need your help on this one. This bug is blocking 2.2.0.

@theophae

This comment has been minimized.

Copy link
Contributor

theophae commented Oct 12, 2015

As I realized that it doesn't crash if the build is done in release mode, it is may be better to release the 2.2.0 as is and work on the bug as a priority task for the next release.
As I say previously, I found another misbehavior on folder handling when you want to create a new ingredient (it works for recipe) it freeze 3-4s and it create the new ingredient out of the folder. The only way to put an ingredient into a folder is to drag it and drop it on the folder.

@malavv

This comment has been minimized.

Copy link
Contributor

malavv commented Oct 14, 2015

Hi Guys,
I don't know the step of affairs but I can reproduce on Windows 7 x64 without difficulties.
The last line I can access while debugging is :

    // Inside qabstractitemmodel.cpp
    emit rowsAboutToBeInserted(parent, first, last, QPrivateSignal());
    // I cannot go further because I'm hitting an "emit". First and last seems ok.
@theophae

This comment has been minimized.

Copy link
Contributor

theophae commented Oct 14, 2015

It is the same line on which I got a crash. The unanswered question is why. If you have a clue...

@malavv

This comment has been minimized.

Copy link
Contributor

malavv commented Oct 14, 2015

In order to debug this issue, I tried the recommended approach to plug-in the model checker on run-time, and the model checker really wasn't happy. It kept asserting for other problems in the model before I even get to the core of this issue.

On a related note, I removed the (begin|end)InsertRows in BtTreeModel::insertRow and it seemed to work. I haven't seen any problems with my Windows instance when I did that. I must say that I do not know why it works, but this might lead to a qucikFix for 2.2.0? If we add enough comment saying we need to fix this?

Just a thought

@rocketman768

This comment has been minimized.

Copy link
Member

rocketman768 commented Oct 15, 2015

So far, everything suggest a memory corruption somewhere. I don't doubt that adding or removing a line will seemingly make the problem go away or go somewhere else, but the real problem we have is to find where the corruption is. I'm not comfortable pushing a 'fix' that we don't understand.

@theophae

This comment has been minimized.

Copy link
Contributor

theophae commented Oct 15, 2015

I agree with @rocketman768 that it is not a good idea to add a fix without understanding it, and moreover it to be completely not logical regarding documentation.
I as said previously, this bug does not appear with binary compiled in release mode, so it is not a big problem to release 2.2.0 version without this fix because this version is late already and the 2.3.0 version will come very soon.

I did not know about model checker before @malavv evoke it. I think it would be a good idea to add some test for all test model into brewtarget tests so that we ensure all models are good and won't be broken.

@rocketman768 : I have seen you added a check tool for continuous integration. It could be nice to execute all the tests in this tool.

@malavv

This comment has been minimized.

Copy link
Contributor

malavv commented Oct 15, 2015

@theophae, @rocketman768, I agree on you both about not releasing untested code. It was a potential quick fix for @rocketman768 to evaluate if time was really of the essence.

On launching in Release because the bug does not appear, I am personally against. For the same reason you mentioned about the code. This would be us saying we do not want the assert to warn against indexing and retrieving out of array. It's fine in debug, as long as the uninitialized memory is filled with 0, but not in any other scenario.

@JulianVolodia

This comment has been minimized.

Copy link
Contributor

JulianVolodia commented Nov 1, 2015

Some classes are deprecated, maybe in Debug they removed it at all? http://doc.qt.io/qt-5/qdirmodel.html

@JulianVolodia

This comment has been minimized.

Copy link
Contributor

JulianVolodia commented Nov 2, 2015

I reproduced it. Only renaming folder with the same name is safe (do not crash).

Creating or renaming folder cause crash... I think it's the same.
Win 7 pro, using Qt 5.5 MinGW 4.9.2

@JulianVolodia

This comment has been minimized.

Copy link
Contributor

JulianVolodia commented Nov 8, 2015

I actually found sth - I will send screenshots soon.

Everything crashes at:
a) BtTreeModel.cpp:1254:
return createFolderTree( dirs, pItem, fullPath);

First big memory lack at:
b) qlist.h:610
*n = copy;

Steps:
Try to create folder named 'a' in Reciepes.
When at (a) line change fullpath from "/" to "/a".
And watch...

Effect:
When I try to run in Debug again it crashes like on 1,2,3 pictures. (every run, before cleanup and build).
1
2
3


I will try to achieve sth better and more concrete, but it could take a while ;( If there is sb out there, try find out sth parallerly with me.

I wish I'm right and I help project to be released...
Best...

malavv pushed a commit to malavv/brewtarget that referenced this issue Nov 9, 2015

Maxime Lavigne Maxime Lavigne
Fixes Brewtarget#98 : Crash when trying to create a new folder.
  - Modified model changing for item children insertion.
  - Also added lots of const modifier to function in the model.
@malavv

This comment has been minimized.

Copy link
Contributor

malavv commented Nov 9, 2015

Hi guys,
As you might have seen from the last comment. Can somebody take a look at pull request #114 ? I think that it should do the trick. When testing there were a few odd things, like not being able to save an empty folder, but I think it was already like that.

Cheers,

@JulianVolodia

This comment has been minimized.

Copy link
Contributor

JulianVolodia commented Nov 9, 2015

My opinion is not very important here, but that change looks ugly, cause:

  • touches places where you do not fix anything (formatting)
  • touches many things, so you should chop that commit and describe better (in another issue) what is the way you go,
  • here, put (as smallest as could be) fix for that folder creation issue without fixing anything else parallerly...

So...

not being able to save an empty folder

actually, your fix works or you are not sure? :)

@malavv

This comment has been minimized.

Copy link
Contributor

malavv commented Nov 9, 2015

@JulianVolodia, please comment on pull request for pull related question and comments.

As mentioned, this fixes issue #98. My comment was about not knowing if an unrelated issue was a regression or an open bug. This is one of the reason why I asking for help from dev. here. On my Windows 7 x64 2.2, I cannot save an empty folder and this would not be a regression.

So again, if anyone is interested in finding regression or side effect of this pull, it would be appreciated. But as far as I know this fixes #98 and does not cause regression.

@JulianVolodia

This comment has been minimized.

Copy link
Contributor

JulianVolodia commented Nov 9, 2015

@malavv, I haven't mentioned that your commit is regression. I just said in my opinion it should be chooped, because I really don't know which change fix this (#98) issue.

You asked to look at it - I just answered your question. I'm not the best programmer as you, I need concrete answer to concrete problem.

About saving empty folder, OK - I know. Before your change nobody could even create that empty folder, so... no one tested saving that you mean ;D

rocketman768 added a commit that referenced this issue Nov 10, 2015

Merge pull request #114 from malavv/bug98
Fixes #98 : Crash when trying to create a new folder.

rocketman768 added a commit that referenced this issue Nov 10, 2015

Fixes #98 : Crash when trying to create a new folder.
  - Modified model changing for item children insertion.
  - Also added lots of const modifier to function in the model.
  - Reverted some reformating and fixed a type pointed out in the pull req.

@rocketman768 rocketman768 assigned malavv and unassigned theophae Nov 10, 2015

@theophae

This comment has been minimized.

Copy link
Contributor

theophae commented Nov 10, 2015

Great ! I'm so happy that this tricky bug has been fixed :)

@mikfire

This comment has been minimized.

Copy link
Contributor

mikfire commented Nov 17, 2015

Not being able to save an empty folder is documented behavior. The folder
information is saved with each element. When there are no elements
referencing the folder, there is no folder.

I pulled some interesting tricks to shove a hierarchical data structure
into an relational database. This is one of the consequences.

Mk
On Nov 9, 2015 02:04, "Maxime Lavigne (malavv)" notifications@github.com
wrote:

Hi guys,
As you might have seen from the last comment. Can somebody take a look at
pull request #114 #114 ? I
think that it should do the trick. When testing there were a few odd
things, like not being able to save an empty folder, but I think it was
already like that.

Cheers,


Reply to this email directly or view it on GitHub
#98 (comment)
.

@JulianVolodia

This comment has been minimized.

Copy link
Contributor

JulianVolodia commented Nov 29, 2015

@mikfire, maybe I misunderstand you, but should we try to avoid this consequence?

Always we could hack it... ? inserting '(no entry)' element, with special type 'emptySet' (without any permission to edit it, and in view - shown only when it is only one element in folder), but I think it is ugly as ... and it is not what we want.

@mikfire

This comment has been minimized.

Copy link
Contributor

mikfire commented Nov 30, 2015

I was simply pointing out that not saving empty folders is documented and
expected.

I don't want to discourage possible solutions, but it is a very minor
annoyance to me and so I really haven't invested much effort in it. So far,
the trees and folders have been mostly my work, so the issue has gone
unaddressed :-)

Mik
On Nov 29, 2015 10:19, "Volodia" notifications@github.com wrote:

@mikfire https://github.com/mikfire, maybe I missunderstand you, but
should we try to avoid this consequence?

Always we could hack it... ? inserting '(no entry)' element, with special
type 'emptySet' (without any permission to edit it, and in view - shown
only when it is only one element in folder), but I think it is ugly as ...
and it is not what we want.


Reply to this email directly or view it on GitHub
#98 (comment)
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment