Skip to content

Fix: IRC crash when closing the profile#7294

Merged
vadi2 merged 7 commits intodevelopmentfrom
fix-irc-crash
Dec 14, 2024
Merged

Fix: IRC crash when closing the profile#7294
vadi2 merged 7 commits intodevelopmentfrom
fix-irc-crash

Conversation

@vadi2
Copy link
Copy Markdown
Member

@vadi2 vadi2 commented Jun 27, 2024 •
edited
Loading

Brief overview of PR changes/additions

Fixes Mudlet crash with IRC open when closing a profile

Motivation for adding to Mudlet

Mudlet should never crash

Other info (issues closed, discussion etc)

Closes #7293. I'll fix other cases of Host* mpHost; after this PR so we have consistency in the codebase.

Running the provided test case through AddressSanitizer revealed that mpHost was being used after it was deleted:

=================================================================
==17843==ERROR: AddressSanitizer: heap-use-after-free on address 0x623000025e58 at pc 0x5555560abe75 bp 0x7fffffff7460 sp 0x7fffffff7450
READ of size 8 at 0x623000025e58 thread T0
    #0 0x5555560abe74 in QWeakPointer<QObject>::internalData() const /usr/include/x86_64-linux-gnu/qt6/QtCore/qsharedpointer_impl.h:711
    #1 0x5555562c15ad in QPointer<dlgIRC>::data() const /usr/include/x86_64-linux-gnu/qt6/QtCore/qpointer.h:77
    #2 0x5555562bd715 in QPointer<dlgIRC>::operator dlgIRC*() const /usr/include/x86_64-linux-gnu/qt6/QtCore/qpointer.h:85
    #3 0x555556296c13 in dlgIRC::~dlgIRC() /home/vadi/Programs/Mudlet/src/dlgIRC.cpp:109
    #4 0x555556296e1f in dlgIRC::~dlgIRC() /home/vadi/Programs/Mudlet/src/dlgIRC.cpp:112
    #5 0x7ffff6ba04a0 in QObject::event(QEvent*) (/lib/x86_64-linux-gnu/libQt6Core.so.6+0x1a04a0)
...

This helped pinpoint the cause of the crash.

@vadi2 vadi2 requested a review from a team as a code owner June 27, 2024 03:40
@vadi2 vadi2 requested a review from a team June 27, 2024 03:40
@add-deployment-links
Copy link
Copy Markdown

add-deployment-links bot commented Jun 27, 2024 •
edited
Loading

Hey there! Thanks for helping Mudlet improve. 🌟

Test versions

You can directly test the changes here:

No need to install anything - just unzip and run.
Let us know if it works well, and if it doesn't, please give details.

@vadi2 vadi2 mentioned this pull request Jun 27, 2024
Closed
SlySven
SlySven reviewed Jun 27, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@SlySven SlySven left a comment

Choose a reason for hiding this comment

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

I suspect this has come about because I cleaned up the profile closing code to delete and close dialogues like this IRC one - so the one in Host that points to this class now gets cleared by the same actions that cause this destructor to be run!

It might be a good ideal to convert the (Host*) mpHost to a QPointer<Host> mpHost (removing the then redundant nullptr initialisation in the header file) to ensure it is nulled if the Host class has gone away.

Of course since dlgIRC::slot_receiveMessage(...) is a slot it is being called asynchronously as the network code in a separate thread gets a message - and that was happening during profile closing to cause the crashing...

@vadi2
Copy link
Copy Markdown
Member Author

vadi2 commented Jun 27, 2024

This is what this PR does: https://github.com/Mudlet/Mudlet/pull/7294/files#diff-10d784717a374c05586896b20bf54489d3a9acccead56c7cf1414118c574bb8eL132

Are you asking it to do something else?

@hungrybeagle
Copy link
Copy Markdown

hungrybeagle commented Jun 28, 2024

Hey there! Thanks for helping Mudlet improve. 🌟

Test versions

You can directly test the changes here:

No need to install anything - just unzip and run. Let us know if it works well, and if it doesn't, please give details.

Still crashing for me.

I tried the Win64 version, and the behaviour this time was different. Previously, when the "close profile" option was chosen, the mudlet profile console would close, and the IRC window would stay open briefly before the program crashed and the IRC window as well as the mudlet window would both close (because crash, of course).

This time, when I chose "close profile" both the profile console and the irc window stayed open. The window for choosing a new profile to load pops up, and after a couple seconds, the entire program shuts down.

@SlySven
Copy link
Copy Markdown
Member

SlySven commented Jul 7, 2024 •
edited
Loading

I vaguely recall reading somewhere that suggested the order in which the signals associated with QObject (or maybe) QWidget destruction get passed to "parents"(?) compared to when pointers (maybe QPointers<...>) to them has changed between either Qt 4 and 5 or 5 and 6.

Ah, it was Qt4 and 5 and refers to QPointer<T>:

Note that Qt 5 introduces a slight change in behavior when using QPointer.

When using QPointer on a QWidget (or a subclass of QWidget), previously the QPointer would be cleared by the QWidget destructor. Now, the QPointer is cleared by the QObject destructor (since this is when QWeakPointer objects are cleared). Any QPointers tracking a widget will NOT be cleared before the QWidget destructor destroys the children for the widget being tracked.

So it hasn't changed recently but perhaps we are tripping up on it anyway...

@SlySven
Copy link
Copy Markdown
Member

SlySven commented Jul 28, 2024

@hungrybeagle once the updates have been rebuilt into this PR can you test it again and see if it still misbehaves as before?

@hungrybeagle
Copy link
Copy Markdown

hungrybeagle commented Jul 28, 2024

Will try. Most of my playing is on mac right now

@SlySven
Copy link
Copy Markdown
Member

SlySven commented Jul 28, 2024

Ah, other changes to the development branch mean a further update/rebuild is needed...

@hungrybeagle
Copy link
Copy Markdown

hungrybeagle commented Jul 30, 2024

Ah, other changes to the development branch mean a further update/rebuild is needed...

Is there a specific version I should try out?

@hungrybeagle
Copy link
Copy Markdown

hungrybeagle commented Aug 7, 2024

Ah, other changes to the development branch mean a further update/rebuild is needed...

Seems to have been fixed with macOS on v4.18.5

@hungrybeagle
Copy link
Copy Markdown

hungrybeagle commented Aug 7, 2024

Will try to test on PC soon

SlySven
SlySven reviewed Aug 9, 2024
View reviewed changes
src/dlgIRC.cpp Outdated
const QString from = message->nick();
const QString to = getMessageTarget(message, buffer->title());
mpHost->postIrcMessage(from, to, textToLua);
if (mpHost) {
Copy link
Copy Markdown
Member

@SlySven SlySven Aug 9, 2024

Choose a reason for hiding this comment

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

Why not just combine the mpHost check as an AND with the other one within the condition of line 609?

Copy link
Copy Markdown
Member Author

@vadi2 vadi2 Dec 14, 2024

Choose a reason for hiding this comment

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

Added, though it's important to focus on things that actually matter. It helps PRs move quicker.

@SlySven
Copy link
Copy Markdown
Member

SlySven commented Sep 4, 2024

Awaiting your feedback on my comments @vadi2...

SlySven
SlySven approved these changes Dec 14, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@SlySven SlySven left a comment

Choose a reason for hiding this comment

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

LGTM. :shipit:

@vadi2 vadi2 enabled auto-merge (squash) December 14, 2024 18:33
@vadi2 vadi2 merged commit 2bd526d into development Dec 14, 2024
@vadi2 vadi2 deleted the fix-irc-crash branch December 14, 2024 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SlySven SlySven SlySven approved these changes

Assignees

@vadi2 vadi2

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Mudlet crashes when closing a profile connected to IRC

3 participants

@vadi2 @hungrybeagle @SlySven