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

Fix memory leak, prevent corrupt database, fix typos in qml, various other fixes #731

Merged
merged 9 commits into from
Feb 2, 2025

Conversation

zjeffer
Copy link
Collaborator

@zjeffer zjeffer commented Jan 16, 2025

I added an option to CMakeLists.txt to enable address sanitizer (first commit). I ran it and noticed the following memory leak when closing the main window:

=================================================================
==143229==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 24 byte(s) in 1 object(s) allocated from:
    #0 0x56a6832241b2 in operator new(unsigned long) (/home/zjeffer/git/notes/build/notes+0x2521b2) (BuildId: f3febbdce0342b087d8f64ae39d11f58ce6cd95c)
    #1 0x56a6833aadfb in MainWindow::MainWindow(QWidget*) /home/zjeffer/git/notes/src/mainwindow.cpp:132:20
    #2 0x56a6833a8184 in main /home/zjeffer/git/notes/src/main.cpp:96:16
    #3 0x7a8a82a34e07 in __libc_start_call_main /usr/src/debug/glibc/glibc/csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #4 0x7a8a82a34ecb in __libc_start_main /usr/src/debug/glibc/glibc/csu/../csu/libc-start.c:360:3
    #5 0x56a6830e5584 in _start (/home/zjeffer/git/notes/build/notes+0x113584) (BuildId: f3febbdce0342b087d8f64ae39d11f58ce6cd95c)

SUMMARY: AddressSanitizer: 24 byte(s) leaked in 1 allocation(s).

It's not much but it should be fixed either way, so I did that in the 3rd commit. We have lots more (and much bigger) memory leaks btw, will take a look at the others too.

  • In the 2nd commit, I fixed a deprecation warning (setActiveWindow)
  • In the 4th commit, I just fixed some typos in qml files.
  • In the 5th commit, I fixed an issue where the folder structure can get corrupted in the database when moving a note into the same folder it's already in, by simply returning early. I followed @guihkx steps with a non-empty note and could not reproduce the issue after this commit. It is still reproducable when moving an empty note into a folder.
  • In the 6th commit, I fixed clang-formatting of the emit keyword
  • In the 7th commit, I fixed some possible injection with .arg() chains
  • In the 8th commit, I fixed a cmake warning
  • In the 9th commit, I fixed a crash on exit (Crash when exiting #730)

Closes #234
Closes #730
Closes #727

@zjeffer zjeffer requested review from guihkx and nuttyartist January 16, 2025 22:52
@zjeffer zjeffer self-assigned this Jan 16, 2025
@zjeffer zjeffer added the Bug label Jan 16, 2025
@zjeffer
Copy link
Collaborator Author

zjeffer commented Jan 16, 2025

There's an issue with clang-format, apparently it wants to format

emit itemDelegate()->sizeHintChanged(index);

into

emit itemDelegate() -> sizeHintChanged(index);

Investigating 🤔

EDIT: fixed locally by updating clang-format based on these instructions: https://bugreports.qt.io/browse/QTCREATORBUG-31858, but the action still fails, likely because the Github Action uses clang-format v18, where this bug occurs. I changed the ci/cd to always run all jobs, even if linting fails. @guihkx can you take a look at those changes?

@guihkx
Copy link
Collaborator

guihkx commented Jan 17, 2025

I changed the ci/cd to always run all jobs, even if linting fails

Not a big fan of that tbh... Can you try downgrading the runner used in the "Code Linting" job to ubuntu-22.04 and see if the issue goes away?

As for the other changes, I'm not on my Linux machine right now to test this properly, but I will do that as soon as I can.

@zjeffer
Copy link
Collaborator Author

zjeffer commented Jan 17, 2025

@guihkx Done, it worked!

There's a problem with the snap build, seems like an authentication issue. @nuttyartist can you take a look?

@guihkx
Copy link
Collaborator

guihkx commented Jan 17, 2025

Done, it worked!

Nice.

There's a problem with the snap build, seems like an authentication issue

Hmm, I actually think that error comes from sg, not snap...

Do you wanna try to also use ubuntu-22.04 for the snap job?

It looks like the recent upgrade of ubuntu-latest to Ubuntu 24.04 will require a few changes on our side, but we don't have to do it right now, because the ubuntu-22.04 runner will not be deprecated anytime soon.

Note to self: actions/runner-images#9932

@nuttyartist
Copy link
Owner

Thanks, @zjeffer! I want to take some time to review this, hopefully today.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The changes to this file are fine, I guess, but I can still reproduce the database corruption of #504 by following these reproduction steps.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can't reproduce it anymore with those steps. Step 4 logs onMoveNodeRequested Can't move a node into the same parent in the console, indicating it won't try to move it into itself (which caused the bug).

Did you test it on a clean database?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you test it on a clean database?

Yup. I'm not sure why you can't reproduce it, but here's a video if it helps:

broken.mp4

I also didn't do all steps because my terminal was already being spammed with qt.sql.qsqlquery: QSqlQuery::value: not positioned on a valid record at only step 2, and by then the db is already broken.

Copy link
Collaborator Author

@zjeffer zjeffer Jan 18, 2025

Choose a reason for hiding this comment

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

Ah, we're seeing 2 different bugs. The bug I fixed in this PR occured when moving a non-empty node into the folder it's already in. The bug you're triggering in your video happens when moving an empty note into a folder. It does indeed also trigger the same problem where folders then can't be deleted.
I'll look into that bug in another PR.

@zjeffer zjeffer force-pushed the fix/zjeffer/fixes branch 2 times, most recently from 028bd4d to a466805 Compare January 19, 2025 16:10
@zjeffer zjeffer mentioned this pull request Jan 20, 2025
@nuttyartist
Copy link
Owner

@zjeffer Thanks a lot for this! We should probably drop the mention of fixing #504 since this bug is still reproducible.

@nuttyartist nuttyartist self-requested a review February 2, 2025 07:13
@nuttyartist
Copy link
Owner

@zjeffer, in order to fix #730, can you please add this as a parent to all instances of QWidget::createWindowContainer in the code?

@zjeffer zjeffer changed the title Fix memory leak, prevent corrupt database, fix typos in qml Fix memory leak, prevent corrupt database, fix typos in qml, various other fixes Feb 2, 2025
@zjeffer zjeffer merged commit d7d781e into master Feb 2, 2025
11 checks passed
@guihkx guihkx deleted the fix/zjeffer/fixes branch February 2, 2025 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash when exiting Qt 6.8 build warning from CMake Note don't get saved
3 participants