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 crash resulting from a mutex deadlock when switching users #4675

Merged
merged 3 commits into from
Jun 10, 2023

Conversation

Nerixyz
Copy link
Contributor

@Nerixyz Nerixyz commented Jun 9, 2023

Description

Reproduction

  • Open a reply thread popup in a Twitch channel
  • Close it
  • Switch the current user (make sure no message was sent in that channel after you closed the popup)
  • 💥

Why?

  1. The reply thread popup creates a virtual channel that displays the messages in that thread:
    ChannelPtr virtualChannel;
    if (sourceChannel->isTwitchChannel())
    {
    virtualChannel =
    std::make_shared<TwitchChannel>(sourceChannel->getName());
    }
  2. This channel is then part of the messageAppended callback:
    sourceChannel->messageAppended.connect(
    [this, virtualChannel](MessagePtr &message, auto) {
  3. When you close the popup, the connection is destroyed (~ScopedConnectionCallback::disconnectCallbackBodyBase::disconnect).
  4. When you switch the current user, the application reconnects:
    getApp()->accounts->twitch.currentUserChanged.connect([this]() {
    postToThread([this] {
    this->connect();
    void AbstractIrcServer::connect()
    {
    assert(this->initialized_);
    this->disconnect();
  5. Eventually you'll get disconnected, causing AbstractIrcServer::onDisconnected to be called. This will send a message on all channels:
    void AbstractIrcServer::onDisconnected()
    {
    std::lock_guard<std::mutex> lock(this->channelMutex);
    MessageBuilder b(systemMessage, "disconnected");
    b->flags.set(MessageFlag::DisconnectedMessage);
    auto disconnectedMsg = b.release();
    for (std::weak_ptr<Channel> &weak : this->channels.values())
    {
    std::shared_ptr<Channel> chan = weak.lock();
    if (!chan)
    {
    continue;
    }
    chan->addMessage(disconnectedMsg);
    }
    }
  6. Specifically, it will send a message in the channel you opened the reply popup from.
  7. This will invoke the messageAppended signal:
    this->messageAppended.invoke(message, overridingFlags);
  8. This will destroy all disconnected callback bodies.
  9. Remember the disconnected callback body with our virtual channel from step 3? It will get destroyed here.
  10. Since the virtual channel is part of that callback, it will be destroyed. Thus, dropSeventvChannel will be called:
    TwitchChannel::~TwitchChannel()
    {
    getApp()->twitch->dropSeventvChannel(this->seventvUserID_,
    this->seventvEmoteSetID_);
  11. This will lock the channelMutex:
    void TwitchIrcServer::dropSeventvChannel(const QString &userID,
    const QString &emoteSetID)
    {
    if (!this->seventvEventAPI)
    {
    return;
    }
    std::lock_guard<std::mutex> lock(this->channelMutex);
    But AbstractIrcServer locked this mutex in step 5.

This PR moves the virtual channel to the popup, thus it will be destroyed when the popup is destroyed.

@pajlada pajlada changed the title Prevent Recursive Mutex Lock When Switching Users Fix crash resulting from a mutex deadlock when switching users Jun 10, 2023
@pajlada pajlada enabled auto-merge (squash) June 10, 2023 11:00
@pajlada pajlada merged commit 65a14fb into Chatterino:master Jun 10, 2023
@Nerixyz Nerixyz deleted the fix/recursive-mutex branch June 10, 2023 11:43
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.

2 participants