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

Remove unused variables, Refactor in MessageLayout #4520

Merged
merged 4 commits into from
Apr 8, 2023

Conversation

dnsge
Copy link
Contributor

@dnsge dnsge commented Apr 8, 2023

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

No changelog entry needed in my opinion

Description

  • Moves layoutCount_ and bufferUpdatedCount_ debug counters to an ifdef block in MessageLayout
  • Removes unused variable MessageLayout::collapsedHeight_
  • Replaces std::shared_ptr<MessageLayoutContainer> container_ with MessageLayoutContainer container_ in MessageLayout
    • The container never escapes nor can be null so it made no sense to be in a shared_ptr
  • Replaces std::shared_ptr<QPixmap> buffer_ with std::unique_ptr<QPixmap> buffer_ in MessageLayout
    • The buffer never escapes so I replaced it with a unique_ptr instead
  • Extracts buffer creation logic to MessageLayout::ensureBuffer

Why? While scrolling on a debug build, my Chatterino crashed with the following stack trace:

Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
Exception Codes:       KERN_INVALID_ADDRESS at 0xff76443500e54f18 -> 0x0000443500e54f18 (possible pointer authentication failure)
Exception Codes:       0x0000000000000001, 0xff76443500e54f18
Exception Note:        EXC_CORPSE_NOTIFY

Termination Reason:    Namespace SIGNAL, Code 11 Segmentation fault: 11
Terminating Process:   exc handler [4395]

VM Region Info: 0x443500e54f18 is not in any region.  Bytes after previous region: 74513402646297  Bytes before following region: 30558677283048
      REGION TYPE                    START - END         [ VSIZE] PRT/MAX SHRMOD  REGION DETAIL
      commpage (reserved)        1000000000-7000000000   [384.0G] ---/--- SM=NUL  ...(unallocated)
--->  GAP OF 0x5f9000000000 BYTES
      MALLOC_NANO              600000000000-600008000000 [128.0M] rw-/rwx SM=PRV  

Thread 0 Crashed::  Dispatch queue: com.apple.main-thread
0   chatterino                    	       0x1012b0ba4 std::__1::shared_ptr<chatterino::MessageLayoutContainer>::operator->() const + 12 (memory:2872)
1   chatterino                    	       0x1012b0b88 chatterino::MessageLayout::getHeight() const + 28 (MessageLayout.cpp:70)
2   chatterino                    	       0x101b8f87c chatterino::ChannelView::wheelEvent(QWheelEvent*) + 376 (ChannelView.cpp:1376)
3   QtWidgets                     	       0x1096c4404 QWidget::event(QEvent*) + 128
4   QtWidgets                     	       0x10968e740 QApplicationPrivate::notify_helper(QObject*, QEvent*) + 292
5   QtWidgets                     	       0x109690de4 QApplication::notify(QObject*, QEvent*) + 5424
6   QtCore                        	       0x10aead6e4 QCoreApplication::notifyInternal2(QObject*, QEvent*) + 208

Ostensibly caused by the MessageLayoutContainer in the MessageLayout being null. I suppose this could be caused by a race condition in which the destructor was in progress and the layout container had been deleted, but not the layout? Not sure. Regardless, while investigating, I noticed that the MessageLayoutContainer didn't need to be a pointer at all. I also noticed the other few things that I included in this PR.

Feel free to say that this doesn't matter/shouldn't be changed. Though, I think extracting the debug counters to an ifdef and removing the unused variable should be a change made anyway.

std::shared_ptr<MessageLayoutContainer> container_;
std::shared_ptr<QPixmap> buffer_{};
MessageLayoutContainer container_;
std::unique_ptr<QPixmap> buffer_{};
Copy link
Contributor

Choose a reason for hiding this comment

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

You might be able to do QPixmap buffer_; here and use QPixmap::isNull. This would remove another indirection, because a QPixmap is essentially another shared-pointer and constructing a null-pixmap is cheap. You then need to avoid using the QPixmap's copy-constructor, since this potentially copies the pixmap-data.

Copy link
Member

Choose a reason for hiding this comment

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

follow-up pr, but can be explored if someone wants

Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

Good refactor, easy to follow 👍

@pajlada pajlada enabled auto-merge (squash) April 8, 2023 09:53
@pajlada pajlada merged commit 73435b4 into Chatterino:master Apr 8, 2023
@dnsge dnsge deleted the dev/message-layout-cleanup branch April 8, 2023 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants