Skip to content

Fix JSRefreshRateMonitor memory leak - #633

Merged
maxep merged 1 commit into
DataDog:developfrom
voloshink:develop
Apr 4, 2024
Merged

Fix JSRefreshRateMonitor memory leak#633
maxep merged 1 commit into
DataDog:developfrom
voloshink:develop

Conversation

@voloshink

Copy link
Copy Markdown
Contributor

What does this PR do?

We call stop on an existing refresRateListener when creating a new one to ensure that the existing CADisplayLink is properly invalidated. As mentioned in the documentation for CADisplayLink "The run loop retains the display link. To remove the display link from all run loops, call invalidate()." Without this, JSRefreshRateListeners are not properly deallocated and stay active.

Motivation

Fix issue #632

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests
  • Make sure you discussed the feature or bugfix with the maintaining team in an Issue
  • Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)
  • If this PR is auto-generated, please make sure also to manually update the code related to the change

@voloshink
voloshink requested a review from a team as a code ownerMarch 21, 2024 10:10
maxep
maxep previously approved these changes Apr 4, 2024

@maxepmaxep left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @voloshink 👋 Thank you very much for this contribution!
Indeed, we should stop the listener before allocating a new one.

@maxep
maxep changed the base branch from develop to mainApril 4, 2024 14:36
@maxep
maxep dismissed their stale reviewApril 4, 2024 14:36

The base branch was changed.

@maxep
maxep requested a review from a team as a code ownerApril 4, 2024 14:36
@maxep
maxep changed the base branch from main to developApril 4, 2024 14:36
@maxep
maxep merged commit eb947ac into DataDog:developApr 4, 2024
@marco-saia-datadogmarco-saia-datadog mentioned this pull request Apr 11, 2024
4 tasks
Sign up for freeto 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.

2 participants

@voloshink@maxep