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(runtime): resolve memory leak caused by global content ref #5266

Merged
merged 2 commits into from
Jan 17, 2024

Conversation

tanner-reits
Copy link
Member

@tanner-reits tanner-reits commented Jan 16, 2024

What is the current behavior?

There is a memory leak in the Stencil runtime related to a retained reference to a "content ref" comment. This is only an issue when the runtime patches slot behavior for use outside the Shadow DOM.

GitHub Issue Number: N/A

What is the new behavior?

This commit resolves a memory leak in the runtime related to a global reference that existed in the vDOM algorithm. The content ref would get set when entering the render cycle for a component, but was never cleared so the reference would exist after execution. So, now we clear that value at the end of the renderVdom() function execution.

Does this introduce a breaking change?

  • Yes
  • No

Testing

Automated testing
Automated unit & e2e tests continue to pass.

Manual testing

  1. Perform all steps using this branch of the Ionic Framework
  2. Navigate to http://localhost:3333/src/components/footer/test/basic in a Chromium-based browser (Chrome, Brave, Edge, etc).
  3. Open Developer Tools and switch to the “Memory” tab.
  4. Press CMD/CTRL+E to take a heap snapshot. (Or press the record icon in the top left”.
  5. Click “Toggle Footer”. Observe that the text “hello” appears.
  6. Click “Toggle Footer” again. Observe that the text “hello” disappears.
  7. Press CMD/CTRL+E to take a heap snapshot. (Or press the record icon in the top left”.
  8. In Snapshot 2, change from “Summary” to “Comparison” in the dropdown.
  9. In the “Class filter” searchbox, search for “Detached”.
  10. Expand the “Detached HTMLElement” entry that shows up.
  11. Select the detached HTMLElement in the expanded “Detached HTMLElement” menu. You can hover over this entry to verify that this element is ion-footer.
  12. In the “Retainers” section, observe that the element (ion-footer) is being retained by the comment node inside of the component.
  13. Repeat with a build of this branch installed and confirm there are no detached HTML elements this time

Other information

Also confirmed this does not cause issues with subsequent re-renders of the component

This commit resolves a memory leak in the runtime related to a global reference that existed in the vDOM algorithm. The content ref would get set when entering the render cycle for a component, but was never cleared so the reference would exist after execution

STENCIL-1072
Copy link
Contributor

github-actions bot commented Jan 16, 2024

--strictNullChecks error report

Typechecking with --strictNullChecks resulted in 1211 errors on this branch.

That's 1 fewer than on main! 🎉🎉🎉

reports and statistics

Our most error-prone files
Path Error Count
src/dev-server/index.ts 37
src/mock-doc/serialize-node.ts 36
src/dev-server/server-process.ts 32
src/compiler/prerender/prerender-main.ts 22
src/testing/puppeteer/puppeteer-element.ts 22
src/runtime/client-hydrate.ts 20
src/screenshot/connector-base.ts 19
src/runtime/vdom/vdom-render.ts 17
src/compiler/config/test/validate-paths.spec.ts 16
src/dev-server/request-handler.ts 15
src/compiler/prerender/prerender-optimize.ts 14
src/compiler/sys/stencil-sys.ts 14
src/compiler/transpile/transpile-module.ts 14
src/sys/node/node-sys.ts 14
src/compiler/prerender/prerender-queue.ts 13
src/compiler/sys/in-memory-fs.ts 13
src/runtime/connected-callback.ts 13
src/runtime/set-value.ts 13
src/compiler/output-targets/output-www.ts 12
src/compiler/transformers/test/parse-vdom.spec.ts 12
Our most common errors
Typescript Error Code Count
TS2345 367
TS2322 364
TS18048 223
TS18047 99
TS2722 37
TS2532 26
TS2531 23
TS2454 14
TS2352 12
TS2790 11
TS2769 8
TS2538 8
TS2344 6
TS2416 6
TS2493 3
TS18046 2
TS2684 1
TS2430 1

Unused exports report

There are 14 unused exports on this PR. That's the same number of errors on main, so at least we're not creating new ones!

Unused exports
File Line Identifier
src/runtime/bootstrap-lazy.ts 21 setNonce
src/screenshot/screenshot-fs.ts 18 readScreenshotData
src/testing/testing-utils.ts 198 withSilentWarn
src/utils/index.ts 145 CUSTOM
src/utils/index.ts 269 normalize
src/utils/index.ts 7 escapeRegExpSpecialCharacters
src/compiler/app-core/app-data.ts 25 BUILD
src/compiler/app-core/app-data.ts 115 Env
src/compiler/app-core/app-data.ts 117 NAMESPACE
src/compiler/fs-watch/fs-watch-rebuild.ts 123 updateCacheFromRebuild
src/compiler/types/validate-primary-package-output-target.ts 61 satisfies
src/compiler/types/validate-primary-package-output-target.ts 61 Record
src/testing/puppeteer/puppeteer-declarations.ts 485 WaitForEventOptions
src/compiler/sys/fetch/write-fetch-success.ts 7 writeFetchSuccessSync

@tanner-reits tanner-reits marked this pull request as ready for review January 16, 2024 23:45
@tanner-reits tanner-reits requested a review from a team as a code owner January 16, 2024 23:45
@rwaskiewicz
Copy link
Member

Created 4.10.0-dev.1705506829.0d1d51a to test

@tanner-reits tanner-reits added this pull request to the merge queue Jan 17, 2024
Merged via the queue into main with commit fb1b3f5 Jan 17, 2024
123 checks passed
@tanner-reits tanner-reits deleted the treits/fix/content-ref-memory-leak branch January 17, 2024 19:07
@christian-bromann
Copy link
Member

christian-bromann commented Jan 22, 2024

This patch has been released as a part of today's Stencil v4.11.0 release.

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

3 participants