Skip to content

Fix swallowed NRE when receiving some types of events in an uncached DM channel

Mateusz Brawański requested to merge github/fork/uwx/patch-123 into master

Created by: uwx

Summary

This PR fixes the following exception when an external client adds/removes/clears a reaction to any message, or deletes a message, or bulk deletes a message (OAuth only), in a DM channel that isn't cached, among others:

[2019-07-02 19:19:52 -05:00] [Websocket] [Error] Socket swallowed an exception:
System.NullReferenceException: Object reference not set to an instance of an object.
   at DSharpPlus.DiscordClient.<OnMessageReactionAddAsync>d__110.MoveNext() in D:\Code\DSharpPlus\DSharpPlus\DiscordClient.cs:line 1977
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at DSharpPlus.DiscordClient.<HandleDispatchAsync>d__76.MoveNext() in D:\Code\DSharpPlus\DSharpPlus\DiscordClient.cs:line 817
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at DSharpPlus.DiscordClient.<HandleSocketMessageAsync>d__75.MoveNext() in D:\Code\DSharpPlus\DSharpPlus\DiscordClient.cs:line 612
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at DSharpPlus.DiscordClient.<<InternalConnectAsync>g__SocketOnMessage|60_1>d.MoveNext() in D:\Code\DSharpPlus\DSharpPlus\DiscordClient.cs:line 380

Details

Discord only sends Channel Create events for DMs alongside Message Receive events. This means that you can receive events for messages when the client doesn't even know about the channel! The client, of course, safely handles missing message cache entries, but not missing channel cache entries, since they don't happen outside of DMs. This causes the NRE to be thrown.

I also had to alter the event args for the reaction events as the channel not being cached would also mean no way to get its ID to request it manually. I don't know if a skeleton entity would be preferable in this scenario.

Changes proposed

  • Change OnMessageReactionAddAsync, OnMessageReactionRemoveAsync, OnMessageReactionRemoveAllAsync, OnMessageDeleteEventAsync and OnMessageBulkDeleteEventAsync to support channels not in cache.
  • Change the Channel properties in MessageReactionAddEventArgs, MessageReactionRemoveEventArgs and MessageReactionsClearEventArgs to use Message.Channel like the other event args do.

Notes

Message Delete and Message Bulk Delete are untested. The rest of this PR was tested, but was forward-ported from an old version of the library. I tried to be surgical, but if there are any sneaky regressions as a result of the patch, feel free to blame me.

Thanks to Becca on Discord for finding and reporting this issue. Just the line number was enough for me to deduce what the problem was, which made debugging it really easy!

Merge request reports