Skip to content

Fix a crash on splash screen when wallet fails to load#3629

Merged
UdjinM6 merged 1 commit into
dashpay:developfrom
UdjinM6:fixsplashcrash
Jul 28, 2020
Merged

Fix a crash on splash screen when wallet fails to load#3629
UdjinM6 merged 1 commit into
dashpay:developfrom
UdjinM6:fixsplashcrash

Conversation

@UdjinM6
Copy link
Copy Markdown

@UdjinM6 UdjinM6 commented Jul 26, 2020

Discovered by @xdustinface:

Happens because this unique_ptr https://github.com/dashpay/dash/blob/develop/src/wallet/wallet.cpp#L5102 gets added to the list of wallets in splashscreen inside https://github.com/dashpay/dash/blob/develop/src/wallet/wallet.cpp#L5104 which calls SplashScreen::ConnectWallet due to the uiInterface if general wallet loading fails. But if the further wallet validation in CreateWalletFromFile fails the unique ptr is deallocated but the splashscreen still hold a reference to its former address. Then it tries to disconnect the signal here https://github.com/dashpay/dash/blob/develop/src/qt/splashscreen.cpp#L182 and crashes.

This is fixed in bitcoin#13063 which depends on a HUGE gui/node/wallet separation PR bitcoin#10244 and there is no way we backport these to 0.16. However the only thing that uses ShowProgress on splash screen is ScanForWalletTransactions(), so we can just call uiInterface.LoadWallet() right next to it (instead of doing it in CWallet:LoadWallet()) as a workaround.

@UdjinM6 UdjinM6 added this to the 16 milestone Jul 26, 2020
@UdjinM6 UdjinM6 changed the title Fix crash on splash screen when wallet fails to load Fix a crash on splash screen when wallet fails to load Jul 26, 2020
Copy link
Copy Markdown

@xdustinface xdustinface left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

Copy link
Copy Markdown
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, what if we only merge this to 0.16 and NOT to 17, since I'm going to be backporting 13063 soon:tm: anyway?

@UdjinM6
Copy link
Copy Markdown
Author

UdjinM6 commented Jul 27, 2020

13063 moved uiInterface.LoadWallet() out of CWallet:LoadWallet() too, so merge conflicts should be minimal/trivial IMO. (or you could simply revert this before applying the backported commit)

Copy link
Copy Markdown
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, utACK

@UdjinM6 UdjinM6 merged commit 469771d into dashpay:develop Jul 28, 2020
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 8, 2020
@UdjinM6 UdjinM6 deleted the fixsplashcrash branch November 26, 2020 13:26
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Mar 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants