mwittrien / BetterDiscordAddons

A series of plugins and themes for BetterDiscord.
GNU General Public License v2.0
2.05k stars 3.88k forks source link

[BUG] - RemoveNicknames: Discord can hang through a stack overflow condition #2672

Closed aldelaro5 closed 2 months ago

aldelaro5 commented 3 months ago

Describe the bug With the plugin enabled, it is possible to get into a situation where Discord completely hangs. There seems to exists a broad variety of ways to trigger it, but the most consistent I found is to click on a server or to type @ in the chat message box.

To Reproduce

  1. Ensure the plugin is enabled
  2. Select a server (I selected one with a couple thousand members)
  3. If no hang occurs, type @ in the chat message box
  4. Observe the hang, there may be messages in the console about the size of the stack exceeding

Expected behavior Discord should not hang.

Discord Build (stable, ptb or canary): Latest PTB as of this post

Additional context So I investigated this one and while I don't fully understand what's happening (the patching libs seems complex for me to get), I think I have an idea of what's going on as well as figuring out a workaround for now.

Basically, the problem starts to happen on this line during getNewName: https://github.com/mwittrien/BetterDiscordAddons/blob/13a82fa675a60c30dfb92a1a3cc9c676ae126ac6/Plugins/RemoveNicknames/RemoveNicknames.plugin.js#L253 which tries to fetch the GuildMemberStore and it goes through the lib plugin to attempt to get it here: https://github.com/mwittrien/BetterDiscordAddons/blob/13a82fa675a60c30dfb92a1a3cc9c676ae126ac6/Library/0BDFDB.plugin.js#L1458 Most importantly, it filters by calling getName on what I think is a module (unsure here) to determine if it's what it's looking for.

The problem is for some strange reasons, calling that causes to trigger a patch this plugin hooked on startup: the "after" patch of "getName". The patch is hooked here: https://github.com/mwittrien/BetterDiscordAddons/blob/13a82fa675a60c30dfb92a1a3cc9c676ae126ac6/Plugins/RemoveNicknames/RemoveNicknames.plugin.js#L102-L105 and interestingly, it calls getNewName, the same function we were earlier that tried to find the store.

The end result is it just seems to infinetely call itself resulting in a stack overflow.

But interestingly enough, when I could reproduce, the first time getNewName was called wasn't from this hook, it was the other one (the "getAuthor" / "getMessageAuthor" one) so as a workaround, I tried to comment out the "getName" hook like this:

onStart () {
    let init = false;
    BDFDB.TimeUtils.timeout(_ => init = true, 3000);
    /*BDFDB.PatchUtils.patch(this, BDFDB.LibraryModules.UserNameUtils, "getName", {after: e => {
        if (!init) return;
        return this.getNewName(e.methodArguments[2], e.methodArguments[0]);
    }});*/

    BDFDB.PatchUtils.patch(this, BDFDB.LibraryModules.MessageAuthorUtils, ["getAuthor", "getMessageAuthor"], {after: e => {
        if (this.settings.places.chat && e.methodArguments[0] && e.methodArguments[0].author) {
            let newName = this.getNewName(e.methodArguments[0].author, (BDFDB.LibraryStores.ChannelStore.getChannel(e.methodArguments[0].channel_id) || {}).guild_id);
            if (newName) e.returnValue.nick = newName;
        }
    }});

    this.forceUpdateAll();
}

and it allowed Discord to not hang while I still get what I cared for: not see nicknames in the chat. I imagine other features stops working, but it's a workaround for now.

Still, hopefully what I found is enough to find a proper fix because it's very strange: it seems to patch itself into stack overflow...

mwittrien commented 2 months ago

Thanks this was actually really helpful and the issue should be fixed now