Skip to content
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

Significantly improve performance when saving and opening databases #4309

Merged
merged 4 commits into from
Mar 7, 2020

Conversation

droidmonkey
Copy link
Member

@droidmonkey droidmonkey commented Feb 10, 2020

Type of change

  • ✅ Bug fix (non-breaking change which fixes an issue)
  • ✅ Refactor (significant modification to existing code)

Description and Context

  • Fix crash when switching tabs while unlocking

  • Prevent destruction of FileWatcher while calculating checksums

    • Only perform async checksum calculations when triggered by timer (prevents random GUI freezes)
    • Use mutex to block object destruction while an async checksum is being calculated
  • Fix crashes on database save

    • Add saving mutex to database class to prevent re-entrant saving
    • Prevent saving multiple times to the same file if the database is not marked as modified
    • Prevent locking the database while saving. This also prevents closing the application and database tab while saving.
  • Dynamically determine database validity

    • Check that the database composite key exists, has sub-keys associated with it, and the root group exists.
  • Move database open to AsyncTask

    • Wrap key transformation in AsyncTask when reading a database. Significantly reduces user interface lockup.

Fixes #4249
Fixes #4289

Testing strategy

Tested on Windows 10. I used a long encryption time and artificial wait introduced in the FileWatcher checksum calculation to guarantee triggering a crash in the original code (Tools::wait(2000)). Fixes were introduced until the crashes could not be reproduced.

Checklist:

  • ✅ I have read the CONTRIBUTING document. [REQUIRED]
  • ✅ My code follows the code style of this project. [REQUIRED]
  • ✅ All new and existing tests passed. [REQUIRED]
  • ✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]
  • ✅ My change requires a change to the documentation, and I have updated it accordingly.
  • ✅ I have added tests to cover my changes.

@droidmonkey droidmonkey added bug pr: refactoring Pull request that refactors code crash 💥 labels Feb 10, 2020
@droidmonkey droidmonkey added this to the v2.6.0 milestone Feb 10, 2020
@droidmonkey droidmonkey changed the title Hotfix/open crash Prevent crashes while unlocking and saving databases Feb 10, 2020
@droidmonkey droidmonkey changed the title Prevent crashes while unlocking and saving databases Eliminate crash when switching tabs during database unlock and crash when locking database on save Feb 10, 2020
@droidmonkey droidmonkey force-pushed the hotfix/open-crash branch 2 times, most recently from a922709 to 992e3c7 Compare February 10, 2020 04:58
@droidmonkey
Copy link
Member Author

I cleaned up the fixes. Need to introduce setting m_initialized in Database class for more circumstances rather than just opening the database. Perhaps isInitialized() can determine that state dynamically based on a set of criteria.

@droidmonkey
Copy link
Member Author

This is ready for a re-review

@droidmonkey droidmonkey requested a review from phoerious March 4, 2020 16:20
@droidmonkey droidmonkey changed the title Eliminate crash when switching tabs during database unlock and crash when locking database on save Eliminate crashes on database save and open Mar 4, 2020
@droidmonkey droidmonkey changed the title Eliminate crashes on database save and open Significantly improve performance when saving and opening databases Mar 4, 2020
* Add saving mutex to database class to prevent re-entrant saving
* Prevent saving multiple times to the same file if the database is not marked as modified
* Prevent locking the database while saving. This also prevents closing the application and database tab while saving.
* FileWatcher: only perform async checksum calculations when triggered by timer (prevents random GUI freezes)
* Re-attempt database lock when requested during save operation
* Prevent database tabs from closing before all databases are locked on quit
* Check that the database composite key exists, has sub-keys associated with it, and the root group exists.
* Wrap key transformation in AsyncTask when reading a database. Significantly reduces user interface lockup.
* Replace root group with new group instead of deleting the pointer (fulfills member validity promise).
Copy link
Member

@phoerious phoerious left a comment

Choose a reason for hiding this comment

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

One minor comment, but otherwise ready to go.

@Cyrix240
Copy link

Sorry if this is not the place to say this, the issues I would maybe comment on have been closed or merged into this (i think.) I have been experiencing the crash on database save on 2 different PCs and on different versions of keepass.

On my main PC (relatively modern, ssd, etc.) I get the issue maybe once a month on 2.5.4, not hat big a deal for me to kill it and start again.

On my mothers PC (Core2duo, slow HDD) it would happen consistently after just a save or 2, i tried updating this PC to the snapshot version with no change in behavior.

However on both computers the behavior seems to go away completely if browser integration is disabled.

I also saw mention of turning off the minimize on close setting, but that had no effect on either machine or version.

happy to provide and further info if i can.

@droidmonkey
Copy link
Member Author

We have heard that browser integration is to blame for the random freezes/crashes. It's hard to pinpoint the issue without debug logs.

@Cyrix240
Copy link

Is that something i could send you? All i could find relating to debug in the app is the info tab in the about dialog box but that doesn't seem like much info, so i assume there is something else.

here it is either way:
KeePassXC - Version 2.5.4
Revision: c023bf9

Qt 5.14.2
Debugging mode is disabled.

Operating system: Windows 10 Version 1903
CPU architecture: x86_64
Kernel: winnt 10.0.18362

Enabled extensions:

  • Auto-Type
  • Browser Integration
  • SSH Agent
  • KeeShare (signed and unsigned sharing)
  • YubiKey

Cryptographic libraries:
libgcrypt 1.8.5

I'd be happy to send other logs along if you can point me to where/how to get them.

@droidmonkey
Copy link
Member Author

What you can do is see if a snapshot build that includes the fixes from this PR still crashes on your mother's PC. We (the development team) have not been able to reproduce these crashes at all in the past year. You can download a snapshot build at https://snapshot.keepassxc.org. We don't recommend using it permanently, just to test.

@Cyrix240
Copy link

The snapshot as of 4/20 still crashes on moms PC, no noticeable difference.

@droidmonkey
Copy link
Member Author

OK great, we are working some fixes that may work. You will be our Guinea pig if you don't mind

@Cyrix240
Copy link

happy to help.

droidmonkey added a commit that referenced this pull request Jul 7, 2020
Added

- Custom Light and Dark themes [#4110, #4769, #4791, #4796, #4892, #4915]
- Compact mode to use classic Group and Entry line height [#4910]
- View menu to quickly switch themes, compact mode, and toggle UI elements [#4910]
- Search for groups and scope search to matched groups [#4705]
- Save Database Backup feature [#4550]
- Sort entries by "natural order" and move lines up/down [#4357]
- Option to launch KeePassXC on system startup/login [#4675]
- Caps Lock warning on password input fields [#3646]
- Add "Size" column to entry view [#4588]
- Browser-like tab experience using Ctrl+[Num] (Alt+[Num] on Linux) [#4063, #4305]
- Password Generator: Define additional characters to choose from [#3876]
- Reports: Database password health check (offline) [#3993]
- Reports: HIBP online service to check for breached passwords [#4438]
- Auto-Type: DateTime placeholders [#4409]
- Browser: Show group name in results sent to browser extension [#4111]
- Browser: Ability to define a custom browser location (macOS and Linux only) [#4148]
- Browser: Ability to change root group UUID and inline edit connection ID [#4315, #4591]
- CLI: `db-info` command [#4231]
- CLI: Use wl-clipboard if xclip is not available (Linux) [#4323]
- CLI: Incorporate xclip into snap builds [#4697]
- SSH Agent: Key file path env substitution, SSH_AUTH_SOCK override, and connection test [#3769, #3801, #4545]
- SSH Agent: Context menu actions to add/remove keys [#4290]

Changed

- Complete replacement of default database icons [#4699]
- Complete replacement of application icons [#4066, #4161, #4203, #4411]
- Complete rewrite of documentation and manpages using Asciidoctor [#4937]
- Complete refactor of config files; separate between local and roaming [#4665]
- Complete refactor of browser integration and proxy code [#4680]
- Complete refactor of hardware key integration (YubiKey and OnlyKey) [#4584, #4843]
- Significantly improve performance when saving and opening databases [#4309, #4833]
- Remove read-only detection for database files [#4508]
- Overhaul of password fields and password generator [#4367]
- Replace instances of "Master Key" with "Database Credentials" [#4929]
- Change settings checkboxes to positive phrasing for consistency [#4715]
- Improve UX of using entry actions (focus fix) [#3893]
- Set expiration time to Now when enabling entry expiration [#4406]
- Always show "New Entry" in context menu [#4617]
- Issue warning before adding large attachments [#4651]
- Improve importing OPVault [#4630]
- Improve AutoOpen capability [#3901, #4752]
- Check for updates every 7 days even while still running [#4752]
- Improve Windows installer UI/UX [#4675]
- Improve config file handling of portable distribution [#4131, #4752]
- macOS: Hide dock icon when application is hidden to tray [#4782]
- Browser: Use unlock dialog to improve UX of opening a locked database [#3698]
- Browser: Improve database and entry settings experience [#4392, #4591]
- Browser: Improve confirm access dialog [#2143, #4660]
- KeeShare: Improve monitoring file changes of shares [#4720]
- CLI: Rename `create` command to `db-create` [#4231]
- CLI: Cleanup `db-create` options (`--set-key-file` and `--set-password`) [#4313]
- CLI: Use stderr for help text and password prompts [#4086, #4623]
- FdoSecrets: Display existing secret service process [#4128]

Fixed

- Fix changing focus around the main window using tab key [#4641]
- Fix search field clearing while still using the application [#4368]
- Improve search help widget displaying on macOS and Linux [#4236]
- Return keyboard focus after editing an entry [#4287]
- Reset database path after failed "Save As" [#4526]
- Use SHA256 Digest for Windows code signing [#4129]
- Improve handling of ccache when building [#4104, #4335]
- macOS: Properly re-hide application window after browser integration and Auto-Type usage [#4909]
- Auto-Type: Fix crash when performing on new entry [#4132]
- Browser: Send legacy HTTP settings to recycle bin [#4589]
- Browser: Fix merging browser keys [#4685]
- CLI: Fix encoding when exporting database [#3921]
- SSH Agent: Improve reliability and underlying code [#3833, #4256, #4549, #4595]
- FdoSecrets: Fix crash when editing settings before service is enabled [#4332]
@phoerious phoerious added pr: bugfix Pull request that fixes a bug and removed bug labels Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crash 💥 pr: bugfix Pull request that fixes a bug pr: refactoring Pull request that refactors code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crashes when saving App refuses to close after running for long period of time
3 participants