powercord-org / powercord

A lightweight @discord client mod focused on simplicity and performance.
MIT License
1.2k stars 147 forks source link

Trying to store BigInt in settings crashes Discord #616

Closed RazerMoon closed 2 years ago

RazerMoon commented 2 years ago

Describe the bug If you try to store a BigInt or anything containing one using settings.set() discord immediately dies. From what I've seen in my experimentation, there is no way to stop this. try catch doesn't work either.

de

To Reproduce Try to store a BigInt using settings.set() on any plugin, for example:

powercord.pluginManager.plugins.get('pc-emojiUtility').settings.set("crash", 1024n);

Expected behavior Discord doesn't crash, and the BigInt gets stored safely.

Desktop (please complete the following information):

Additional context


Who cares? Just don't try to store BigInt's

You don't want to be like me and spend an hour troubleshooting a plugin, while it is crashing discord without giving any error messages, with try catches not working, only to find out the issue was that you were trying to store a private discord text channel object which just so happens to have a PermissionsOverwrites field which stores permissions as BigInt's (Yes, I am salty).

cyyynthia commented 2 years ago

Settings can only store values that can be serialized to JSON as this is what's used under the hoods to save the settings to disk (hence the delay for the crash, induced by the wait time before settings gets updated to disk), and this will not change (and has already been documented as unsupported for v3). This means no sets, maps, functions, class instances, or bigints can be stored and you have to do the conversion on your end.

randomzigzag commented 2 years ago

technically you could "fix" this in your plugin by defining BigInt.prototype.toJSON() but you should really just convert it in your plugin image

cyyynthia commented 2 years ago

This not only causes a loss of precision for large bigints (they are better off converted to string), doing it at the object level means you still get the bigint during current runtime, but on next client start you'll no longer get a bigint but rather a plain number which is a bad thing to deal with. You really should just transform them before storing, and after reading.

intrnl commented 2 years ago

That approach also requires some way of reviving the value back to bigint, we don't expose the reviver to plugins.

RazerMoon commented 2 years ago

(and has already been documented as unsupported for v3).

Sorry, didn't see those docs.

Is there any possibility that at least some kind of check is implemented in the future so it doesn't just outright crash? When I was debugging I initially assumed my code was the issue cause I didn't think powercord would just intentionally die, without an error at least.

cyyynthia commented 2 years ago

v3 includes type checks during set to avoid unexpected side effects, but here the error is technically thrown (and is even thrown outside of the plugin code flow), but it's what really causes the crash, cross-context errors really do bad things.