search

polywock / globalSpeed

Web extension to set a default speed for video and audio
https://chrome.google.com/webstore/detail/global-speed-youtube-netf/jpbjcnkcffbooppibceonlgknpkniiff?hl=en
1.58k stars 179 forks source link

Possible memory leaks #307

Closed jody-frankowski closed 1 week ago

jody-frankowski commented 1 year ago

It looks like there are multiple (at least two, I would say) memory leaks in the extension. I stumbled upon them when I did a about:memory report. I found that the extension held in memory lots of copies of strings, and the main culprits being base64 encoded favicons of visited sites belonging to tabs.Tab objects (I think). The session was long-running, with lots of tabs opened and closed since its beginning. I have only been able to pinpoint the PortCapture class as one of the sources. But there seems to be another one, because I have tried to remove the class entirely, and I still saw the same allocations.

How to reproduce:

  1. Remove and reinstall the extension
  2. Do a first about:memory report
  3. Open a lot of youtube video tabs
  4. Close all the video tabs
  5. Do a second about:memory report
  6. Filter with data:image
  7. See that the extension has a multiple copies of a string in a leaf like explicit/window-objects/top(moz-...)/js-zones(0x...)/string(length=...)
  8. Inspect the extension, and see that window.portCapture.ports contains some information of a closed tab
jody-frankowski commented 1 year ago

See #308 for a simple solution for PortCapture. I simply removed it as I have not found it to be currently used.

polywock commented 1 year ago

Interesting find. Removing PortCapture doesn't seem to solve the leak for me. As for why I even have PortCapture in first place. I'm not 100% sure 😕, but I remember it had something to do with port disconnect event not triggering properly without keeping a reference. I will sidestep this issue by working on the Manifest V3 update. Background pages are no longer supported in Manifest V3, so the issue should resolve indirectly.

jody-frankowski commented 1 year ago

As there seems to be at least another one (based on my research), if you ping this post when the manifest V3 lands on git, I'll give it a try if I have the time :)

pasabanov commented 5 months ago

+1 This extension definitely has memory leaks as it can take up to several gigabytes of memory until I restart it or the browser (Firefox). On Chromium however I didn't encounter any memory leaks.

lilydjwg commented 1 month ago

I just did a diff in about:memory for before and after disabling this extension, and the result is -1061 MiB memory usage for the extension process.

polywock commented 1 month ago

Apologies for the long delay. Just uploaded the Manifest V3 version of Global Speed. Hopefully the issue is resolved with that. Still not sure what caused the memory leak. I'm surprised not a lot of people complained. Maybe it was only affecting certain people.

The update should get approved soon, maybe tomorrow.

lilydjwg commented 2 weeks ago

I don't have the memory issue with the new version.