Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: remove dead PSI forwarding listener memory leak #69

Merged
merged 1 commit into from
Jun 26, 2020

Conversation

patrickhulce
Copy link
Collaborator

@patrickhulce patrickhulce commented Jun 20, 2020

AFAICT this code is not used. The metric values are pulled from storage in the popup and nothing I could find listened for the event that was being issued.

On top of that, this chunk of code adds a listener every time a metric update event is issued :)

Repro Steps

  1. Open a clean browser profile.
  2. Inspect background page and open Memory + Performance Monitor panels.
  3. Reset the extension at chrome://extensions and then open 5 tabs of https://melodic-class.glitch.me/cls-forever.html
  4. Hold down Ctrl+Tab for 60 seconds.

Before
image

After
image

@patrickhulce patrickhulce changed the title fix: remove dead PSI forwarding listener fix: remove dead PSI forwarding listener memory leak Jun 20, 2020
@addyosmani
Copy link
Member

addyosmani commented Jun 24, 2020

Thank you for digging in here, @patrickhulce! 🤛

On top of that, this chunk of code adds a listener every time a metric update event is issued :)

Removing this forwarding listener certainly makes sense until we land #53.

I may be missing a step, but after following the steps outlined above, I was able to reproduce the memory issues but was not able to see the drop in memory comparing this PR to master.

Could you check if there's a step missing from the above to get that nice "after" view? :) I'm also happy to retest this using a particular Chrome channel you're testing against (using ToT atm)

@patrickhulce
Copy link
Collaborator Author

but was not able to see the drop in memory comparing this PR to master

Were you observing memory usage using the method described in #71 or this PR?

There's a separate issue that causes needless cycling of memory (#73) but not really leak on the same level as this.

@addyosmani
Copy link
Member

There's a separate issue that causes needless cycling of memory (#73) but not really leak on the same level as this.

Ack! Thanks for clarifying. I circled back and used the approach you outlined with the Memory panel again to verify this patch reduced memory and there is a verifiable reduction.

Thanks once again for discovering this leak, @patrickhulce!

Screen Shot 2020-06-25 at 6 08 54 PM

Screen Shot 2020-06-25 at 6 06 58 PM

@addyosmani addyosmani merged commit d2a8ea1 into master Jun 26, 2020
@addyosmani addyosmani deleted the onupdated_leak branch June 26, 2020 01:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants