Created attachment 374101 [details] Clicking OK crashes GNUCash Just installed 4.6.1 on macOS Registering a fund transfer between two accounts with different currency crashes the program
Would you attach the https://wiki.gnucash.org/wiki/Tracefile ? It may be related to recent changes to the GNCAmountEdit control, related to bug 798219
Created attachment 374106 [details] Crash report from macOS
Created attachment 374107 [details] /var/folders/.../gnucash.trace
I've added the gnucash.trace and crash report
Thank you for the crash report, a couple of further questions, Is this reproducible ? What action were you doing to raise the transfer dialog, creating a new transaction / amending one some thing else ?
It happens every time I created a new transaction. A transfer between two accounts with different currency opens the transfer funds dialog, I just click OK and it crashes
Unfortunately I can not reproduce, have you tried from 'Actions->Transfer' ? I added a Brazil account and was able to transfer to a Euro account after adding rate on transfer dialog, checked price database and rate is the same value there. Strange... What type of accounts involved?
I think I missed one information... I am using trading account The transfer is between two bank type accounts but it involves also trading accounts
I tested on Windows and I get the same behavior. But if I disable the trading accounts it doesn't crash, works as expected.
I still can not reproduce the crash, this is what I did... On my Windows 10 machine I had version 4.5, opened my test file, saved it to a new name, created a Brazil assert account, enabled Trading in properties and then did a check and repair of all transactions and closed. Opened new trading file and made several transfers between Brazil account and euro, gbp and usd accounts using tabs and mouse clicks to no avail. The only thing I can suggest is that you run the 'Actions->Check&Repair->All Transactions' and see if that makes a difference. The other thing to try is use 'File->Export->Export Accounts' to create a new test file or just do 'File->New', open this one and add some dummy transactions to see if this file works. If not, you can upload the test file but make sure there is no personal data.
Sorry forgot to add, after creating some transactions in version 4.5, I downloaded 4.6, installed that and tried adding more transactions and all worked.
Created attachment 374113 [details] Test file to crash Gnucash when transferring funds between different currency accounts
I was able to reproduce the issue. I have two trading accounts for the same commodity, each one with some splits. This worked well until 4.4, and in 4.5 the auto-split created a new hierarchy or selected the oldest trading account among the ones with the same commodity. In 4.6 it Gnucash crashes. Creating any transaction with a commodity that has more than one trading account crashes Gnucash. I have annexed a test file, a.gnucash To reproduce the crash just add a new transfer transaction from Checking EUR to Checking USD accounts.
I guess this is a problem introduced by https://bugs.gnucash.org/show_bug.cgi?id=798093
Do you have two trading accounts for the same currency on purpose or because an extra one got created by bug 798093?
I can reproduce your crash now on my windows 10 machine with version 4.6. I tried to delete the EUR1 trading account and specify moving transactions to EUR account but that would crash also. As a test I tried it manually by changing the guid of the split to the EUR account in the file, deleted the EUR1 account and was then able to create a transfer successfully. If I just created a new EUR1 account in the same location as before, creating a transfer would crash. The other thing I tried was reload 4.5, open the test file and with two trading EUR accounts was still able to create transfers OK. While in this version I deleted the EUR1 account specifying to move transaction to EUR account and this was successful. Reloaded with version 4.6 and all was working OK. Paolo, to get you working you may like to try above unless you have a reason for two same currency trading accounts as John asked.
(In reply to John Ralls from comment #15) > Do you have two trading accounts for the same currency on purpose or because > an extra one got created by bug 798093? On purpose, the intention was to track separately EUR<->BRL exchanges from EUR<->stocks, may be I did wrong :-)
Well, you did an unsupported thing. GnuCash is designed to have only one trading account per commodity and to manage the splits in those accounts itself.
Yes, but... I have a test file that was saved during the time that bug 798093 hadn't been fixed and which hence has two trading accounts for some commodities through no fault of mine. This file also causes a crash if I add a new transaction using one of the commodities with multiple trading accounts. I'm sure I'm not the only one with such a file.
Mike, Don't worry, crashes are never acceptable regardless of whether what induced them is supported.
The reason for the crash when trying to delete the trading account EUR1 and move the transactions to EUR is from commit https://github.com/Gnucash/gnucash/commit/da3c511b6c8f332ae883ab8fccdfec868f42d425 If I comment out the call to xaccTransClearTradingSplits it works, sort of makes sense in that the function is dealing with destroying trading splits and it is those that one is trying to move. Not really sure what is a good fix for this, this works... In Account.cpp move '#include "TransactionP.h"' to within the extern C and then in xaccAccountMoveAllSplits wrap the second g_list_foreach with xaccDisableDataScrubbing() and xaccEnableDataScrubbing()
OK. I guess the alternative is to special-case deleting a trading account to skip moving the splits on the ground that they'll be deleted and recreated in the right account as part of the transaction rebalancing. I think your way is cleaner and justified by the premise that the transactions should be clean and balanced at that point anyway.
Maybe, but it feels like you're hiding the problem instead of fixing it which always bothers me. Scrubbing shouldn't cause a crash.
(In reply to John Ralls from comment #18) > Well, you did an unsupported thing. GnuCash is designed to have only one > trading account per commodity and to manage the splits in those accounts > itself. I've read again the Peter Selinger tutorial and I think I am not doing wrong, it should be possible to have more than one trading account per commodity. I tried to enter the splits manually and it accepts the balancing, but it crashes confirming the transaction. It seems to me the problem is limited to this part as I've worked with multiple trading account per commodity since 2014 with no issues. When you say GnuCash is designed to have only one trading account per currency you say that there may be mores issues elsewhere, even if I stay in 4.4 or 4.5?
(In reply to Paolo Maero from comment #24) > (In reply to John Ralls from comment #18) > > Well, you did an unsupported thing. GnuCash is designed to have only one > > trading account per commodity and to manage the splits in those accounts > > itself. > > I've read again the Peter Selinger tutorial and I think I am not doing > wrong, it should be possible to have more than one trading account per > commodity. > I tried to enter the splits manually and it accepts the balancing, but it > crashes confirming the transaction. > It seems to me the problem is limited to this part as I've worked with > multiple trading account per commodity since 2014 with no issues. > When you say GnuCash is designed to have only one trading account per > currency you say that there may be mores issues elsewhere, even if I stay in > 4.4 or 4.5? I'm saying that if you want more than one trading account per currency you must turn off GnuCash's trading account feature and do it by hand, following Selzinger's procedure. GnuCash is incredibly flexible in what you can do by hand, but the automatic stuff is far more limited.
(In reply to Mike Alexander from comment #23) > Maybe, but it feels like you're hiding the problem instead of fixing it > which always bothers me. Scrubbing shouldn't cause a crash. How so? The problem Bob found--which I doubt is the only one--is due to my fix for bug 798937 deleting the transactions that the account deletion code wants to move. Bob proposes to not delete them and allow the move to proceed. I suggest that the alternative is to not move, allow the delete, and let the balancing code create new splits. A third way is to revert the change and go back to the user is on his own to try and balance a transaction with trading splits after editing it, with low probability of success if it happens to involve two trading accounts for the same commodity. Do you see a fourth?
I think I have found the crash on transfer in find_account_matching_name_in_list in Scrub.c Line 1437... if (g_strcmp0 (accname, xaccAccountGetName(acc))) { g_list_free (acc_list); return acc; } should be... if (g_strcmp0 (accname, xaccAccountGetName(acc)) == 0) return acc; The acc_list is freed at the end of calling function xaccScrubUtilityGetOrMakeAccount Need to do a clean checkout and test again.
Bob, good catch: Both a backwards condition and a double free. Mike, can you test that and see if it resolves your problem?
The fix in comment #27 seems to fix things. There are two crashes reported in this discussion. One happens when adding a new transaction in a commodity that had two trading accounts. The other happens when deleting a trading account and moving its transactions to another trading account in the same commodity. I tried both of these in the test file I mentioned in comment #19 and both worked correctly with no crash.
Note that the change in comment 21 isn't needed to fix either crash. Good work Bob!
John, Hmm, I was going to push my changes then noticed the comment above, not sure what that means.
Bob, it means that you should push only the fixes to find_account_matching_name_in_list in Scrub.c. The changes to Account.cpp don't seem to be needed.
*** Bug 798233 has been marked as a duplicate of this bug. ***
I've tested the change in comment #27 building maint branch and it fixes the crash. But if I build the stable branch (4.6 tag) + the change in comment #27 it still crashes. Something more is fixed from 4.6 tag that resolve the issue.
(In reply to John Ralls from comment #32) > Bob, it means that you should push only the fixes to > find_account_matching_name_in_list in Scrub.c. The changes to Account.cpp > don't seem to be needed. I have pushed the changes to maint for the scrub function find_account_matching_name_in_list and will be in the next nightly or version 4.7 To understand better, I checked out version 4.6 and using the test file was able to crash on both tests. Added only the scrub change and it fixed both, added a print statement and saw scrub function was called for the account delete/split move so the underlying cause was the scrub function.
(In reply to Bob from comment #35) > To understand better, I checked out version 4.6 and using the test file was > able to crash on both tests. Added only the scrub change and it fixed both, > added a print statement and saw scrub function was called for the account > delete/split move so the underlying cause was the scrub function. Hi Bob. During my previous build I had some glitches (libofx, libtasn1, ...) I cleaned up everything and rebuilt 4.6 from scratch with the scrub change and it works. Sorry! I built on Big Sur 11.4 + Xcode 12.5.1
*** Bug 798243 has been marked as a duplicate of this bug. ***
(In reply to John Ralls from comment #25) > (In reply to Paolo Maero from comment #24) > > (In reply to John Ralls from comment #18) > > > Well, you did an unsupported thing. GnuCash is designed to have only one > > > trading account per commodity and to manage the splits in those accounts > > > itself. > > > > I've read again the Peter Selinger tutorial and I think I am not doing > > wrong, it should be possible to have more than one trading account per > > commodity. > > I tried to enter the splits manually and it accepts the balancing, but it > > crashes confirming the transaction. > > It seems to me the problem is limited to this part as I've worked with > > multiple trading account per commodity since 2014 with no issues. > > When you say GnuCash is designed to have only one trading account per > > currency you say that there may be mores issues elsewhere, even if I stay in > > 4.4 or 4.5? > > I'm saying that if you want more than one trading account per currency you > must turn off GnuCash's trading account feature and do it by hand, following > Selzinger's procedure. GnuCash is incredibly flexible in what you can do by > hand, but the automatic stuff is far more limited. I don't know if it is the right place to do it, I apologize in advance :-), but I would like to comment on this point (turn off trading account feature). The scrub function has to select the correct trading account for the commodity and it selects the trading account with the name that matches the commodity name. This is fine if it is a new transaction or a modified one that requires rebalancing. But if it is an existing transaction, already balanced, and the only thing that changes is the trading account gnucash should not force the trading account back to the one whose name matches the commodity name. I've seen that this is done in Scrub.c function xaccTransScrubImbalance() where it unconditionally clear and recreates any trading split using the defaults trading accounts, thus rolling back any changes to any trading account in the transaction. I've rebuilt 4.6 moving xaccTransClearTradingSplits() after the xaccTransIsBalanced() check so if the transaction is already balanced it doesn't clear/rollback any trading account, allowing using more than one trading account per commodity. I've done some tests and it seems it doesn't break anything. Patchfile annexed below
Created attachment 374148 [details] patchfile
Paolo Maero, I added that to repair Mike Alexander's problem where a transaction with splits to more than one trading account can't be balanced. It also serves to protect from a transaction having multiple trading accounts. Removing it absolutely does break things.
But it breaks things also if it is removed _only_ for balanced transactions? (xaccTransIsBalanced() is true)
Not if you have only one trading account per commodity. That's a basic constraint to letting GnuCash manage trading accounts. If you want more then you have to disable Use Trading Accounts in File>Properties. Or have you done that and the scrub is firing anyway?
*** Bug 798265 has been marked as a duplicate of this bug. ***
*** Bug 798282 has been marked as a duplicate of this bug. ***
*** Bug 798314 has been marked as a duplicate of this bug. ***
Is there a particular github branch that now has this fix included?
(In reply to Ivan from comment #46) > Is there a particular github branch that now has this fix included? Yes, maint. It's included in nightly builds for Windows and flatpak, plus we'll be doing a release this weekend.
