Opening Emoji Picker Leaks Memory

[dsanders11]

• I have searched open and closed issues for duplicates

---

Bug Description

Opening the emoji picker in a conversation will leak memory in the main process, due to a bug in Electron.

This is probably the same bug as #4808, but this is a concrete reproduction case. The leak, however, occurs any time images are loaded, which can occur when switching between conversations with reactions, emojis, or any other images (not attached images). See the linked Electron bug for a more technical explanation.

Steps to Reproduce

1. Let app idle for 45 seconds (this is important), or watch the number of threads for the main process and wait for it to hit ~30 threads.
2. Open emoji picker in a conversation, and click through the different categories.
3. Repeat steps 1 and 2 several times.

Actual Result:

~40 MB of memory is leaked by the main process in step 2 each time.

Expected Result:

There should be no significant change in the memory usage of the main process.

Screenshots

Platform Info

Signal Version: v5.2.1

Operating System: macOS Big Sur 11.3.1

Linked Device Version: Android

Link to Debug Log

[indutny-signal]

Wow, this is amazing. Thank you for this report! Since it appears to be a bug in the electron we are limited in what we can do in Signal Desktop app. Will keep this issue open in case of any breakthroughs on the electron side.

[dsanders11]

@indutny-signal, yea, I've already got a PR to fix the issue on Electron, they may want a slightly different approach to the fix, but there's a couple of different fixes possible, so I imagine consensus will form on one of them and it should make it into an Electron release soon.

It is somewhat possible to work around it directly in Signal, but I wouldn't take that approach unless the upstream fix stalls out. Signal already intercepts the file:// protocol, if that code is changed to load the file into a buffer using fs (which Electron patches to support ASAR), and return the buffer as the response, it won't leak. That's because it uses a different code path not affected by the bug. However, that approach loses automatic Content-Type headers for the files loaded, so it would require a bit of extra effort to get it working robustly (you can set them manually). But that's a viable fallback plan B if plan A goes astray.

[indutny-signal]

This makes sense. It sounds like it would be better to wait until the patch goes into electron and just update it for Signal. Thank you for putting so much effort in it! I appreciate it.

[dsanders11]

@indutny-signal, the upstream fix is now released in Electron, it's in v12.0.11 as well as v13.1.2.

[indutny-signal]

This is so awesome. We'll update ASAP!

[dsanders11]

Closing this issue as the upstream fix has been picked up.