test: add memory profiling CLI for extension flows#42733
Closed
pedronfigueiredo wants to merge 3 commits into
Closed
test: add memory profiling CLI for extension flows#42733pedronfigueiredo wants to merge 3 commits into
pedronfigueiredo wants to merge 3 commits into
Conversation
Contributor
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
7 tasks
Contributor
Builds ready [80879bd]
⚡ Performance Benchmarks (Total: 🟢 14 pass · 🟡 10 warn · 🔴 0 fail)
Bundle size diffs [🚀 Bundle size reduced!]
|
This was referenced May 20, 2026
pull Bot
pushed a commit
to Reality2byte/metamask-extension
that referenced
this pull request
May 21, 2026
## **Description** The `settings-route` memory profile still retained one detached Settings page per route cycle after the shared image-listener fix. Heap snapshots showed the full settings subtree retained through React 17's per-node non-delegated listener on the native select: `V8EventListener -> native_bind -> select[data-testid="default-address-scope-dropdown"] -> parent settings DOM subtree` React 17 attaches per-node non-delegated listeners to form controls such as `input`, `select`, and `textarea`. On Settings unmount, this change detaches those retained listener targets and clears React's private host references so a retained listener target cannot keep the entire Settings tree alive. The cleanup is scoped to Settings and covered by a unit test that verifies the retained select is detached and scrubbed on unmount. ## **Changelog** CHANGELOG entry: null ## **Related issues** Fixes: ## **Manual testing steps** 1. `source ~/.nvm/nvm.sh && nvm use && yarn build:test` 2. In the memory investigation worktree, with profiler tooling intentionally excluded from this PR and the earlier memory fixes applied: `source ~/.nvm/nvm.sh && nvm use && yarn llm:memory -- --iterations 20 --flow settings-route --sample final --probe cdp --snapshot both --output test-artifacts/memory/settings-route-detach-form-controls-probe-cdp-heap-20.json` PR: MetaMask#42733 Profiler evidence: - `settings-route` before this change, after the earlier Safe Chains and image-listener fixes: runtime `+10.49 MiB`, JS `+6.89 MiB`, DOM nodes `+5581`, JS listeners `+534` over 20 iterations. - `settings-route` after this change: runtime `+5.60 MiB`, JS `+3.49 MiB`, DOM nodes `+183`, JS listeners `+55` over 20 iterations. - Heap delta after this change no longer shows retained copies of the settings root or `default-address-scope-dropdown`; remaining retained `maskicon` image nodes are detached from the settings tree. <!-- ## **Screenshots/Recordings** --> <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> <!-- ### **Before** --> <!-- [screenshots/recordings] --> <!-- ### **After** --> <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Adds manual DOM cleanup during Settings unmount, which could inadvertently remove or interfere with elements if the selector scope or ref wiring is wrong. Changes are localized to the Settings page and covered by a new unit test. > > **Overview** > Prevents Settings-route memory retention by adding an unmount cleanup in `SettingsLayout` that finds `input/select/textarea/img` under the settings root, deletes React 17 internal `__reactFiber$`/`__reactProps$` references, and removes those nodes from the DOM. > > Adds a regression test that mounts Settings with the default-address dropdown enabled, injects fake React internals onto the `<select>`, unmounts, and asserts the element is detached and the internal keys are cleared. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 6e95b11. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
sgextcsi
pushed a commit
to sgextcsi/metamask-extension-ne-regression
that referenced
this pull request
May 21, 2026
## **Description** The `settings-route` memory profile still retained one detached Settings page per route cycle after the shared image-listener fix. Heap snapshots showed the full settings subtree retained through React 17's per-node non-delegated listener on the native select: `V8EventListener -> native_bind -> select[data-testid="default-address-scope-dropdown"] -> parent settings DOM subtree` React 17 attaches per-node non-delegated listeners to form controls such as `input`, `select`, and `textarea`. On Settings unmount, this change detaches those retained listener targets and clears React's private host references so a retained listener target cannot keep the entire Settings tree alive. The cleanup is scoped to Settings and covered by a unit test that verifies the retained select is detached and scrubbed on unmount. ## **Changelog** CHANGELOG entry: null ## **Related issues** Fixes: ## **Manual testing steps** 1. `source ~/.nvm/nvm.sh && nvm use && yarn build:test` 2. In the memory investigation worktree, with profiler tooling intentionally excluded from this PR and the earlier memory fixes applied: `source ~/.nvm/nvm.sh && nvm use && yarn llm:memory -- --iterations 20 --flow settings-route --sample final --probe cdp --snapshot both --output test-artifacts/memory/settings-route-detach-form-controls-probe-cdp-heap-20.json` PR: MetaMask#42733 Profiler evidence: - `settings-route` before this change, after the earlier Safe Chains and image-listener fixes: runtime `+10.49 MiB`, JS `+6.89 MiB`, DOM nodes `+5581`, JS listeners `+534` over 20 iterations. - `settings-route` after this change: runtime `+5.60 MiB`, JS `+3.49 MiB`, DOM nodes `+183`, JS listeners `+55` over 20 iterations. - Heap delta after this change no longer shows retained copies of the settings root or `default-address-scope-dropdown`; remaining retained `maskicon` image nodes are detached from the settings tree. <!-- ## **Screenshots/Recordings** --> <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> <!-- ### **Before** --> <!-- [screenshots/recordings] --> <!-- ### **After** --> <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Adds manual DOM cleanup during Settings unmount, which could inadvertently remove or interfere with elements if the selector scope or ref wiring is wrong. Changes are localized to the Settings page and covered by a new unit test. > > **Overview** > Prevents Settings-route memory retention by adding an unmount cleanup in `SettingsLayout` that finds `input/select/textarea/img` under the settings root, deletes React 17 internal `__reactFiber$`/`__reactProps$` references, and removes those nodes from the DOM. > > Adds a regression test that mounts Settings with the default-address dropdown enabled, injects fake React internals onto the `<select>`, unmounts, and asserts the element is detached and the internal keys are cleared. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 6e95b11. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
sgextcsi
pushed a commit
to sgextcsi/metamask-extension-ne-regression
that referenced
this pull request
May 21, 2026
## **Description** Fixes a shared Popover listener leak that retained old route trees during send/settings route churn. Popover registered document `keydown` and `click` listeners with capture enabled, but removed them without matching capture options. Since `removeEventListener` matches on event type, callback, and capture flag, cleanup failed and leaked document listeners retained their React closure scope. This PR keeps the change scoped to `ui/components/component-library/popover/popover.tsx` by: - defining one shared capture listener options object - adding document listeners only while the Popover is open and the matching callback exists - removing those listeners with the same callback and capture options - adding a regression test that verifies unmount cleanup uses the same listener and options captured from `addEventListener` ## **Changelog** CHANGELOG entry: null <!-- ## **Related issues** Fixes: --> ## **Manual testing steps** 1. Build the test extension: ```bash source ~/.nvm/nvm.sh && nvm use yarn build:test:webpack ``` 2. The `yarn llm:memory` profiler is available on the separate [profiler PR](MetaMask#42733). With that tooling available and this branch's `dist/chrome` build, run: ```bash yarn llm:memory -- --iterations 50 --flow send-open-back --sample final --probe cdp --wait-after-flow 500 --extension-path dist/chrome yarn llm:memory -- --iterations 100 --flow settings-route --sample final --probe cdp --wait-after-flow 500 --extension-path dist/chrome ``` 3. Expected post-fix profile from the prior investigation with the [Snow WeakMap patch](LavaMoat/snow#170) still applied: | Flow | Before | After | |---|---:|---:| | settings-route 50x | +118.91 MiB | +2.22 MiB rerun, +37 listeners | | settings-route 100x | n/a | -0.34 MiB, +34 listeners | | settings-route 50x paired heap snapshots | n/a | -5.84 MiB | | send-open-back 50x | +111.66 MiB | +0.05 MiB, +307 listeners | <!-- ## **Screenshots/Recordings** If applicable, add screenshots and/or recordings to visualize the before and after of your change. ### **Before** [screenshots/recordings] ### **After** [screenshots/recordings] --> ## **Validation** - `source ~/.nvm/nvm.sh && nvm use && yarn test:unit ui/components/component-library/popover/popover.test.tsx` - `source ~/.nvm/nvm.sh && nvm use && yarn lint:changed:fix` - `source ~/.nvm/nvm.sh && nvm use && yarn webpack:tsc` - `source ~/.nvm/nvm.sh && nvm use && yarn build:test:webpack` ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I've included tests if applicable - [x] I've documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I've applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Low risk: scoped to Popover’s document event listener lifecycle and adds a regression test; main risk is behavior change in when listeners are attached (only when open and handlers provided). > > **Overview** > Fixes a Popover document-listener leak by **reusing a shared capture options object** and ensuring `removeEventListener` is called with the *same* callback and capture options used for `addEventListener`. > > Listeners for `keydown` (Esc) and `click` (outside) are now only attached when the popover is open and the corresponding handler prop is provided, and a new unit test asserts the add/remove signatures match on unmount to prevent regressions. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 0ccfb58. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
c867ec1 to
e19b750
Compare
Contributor
Builds ready [e19b750]
⚡ Performance Benchmarks (Total: 🟢 22 pass · 🟡 2 warn · 🔴 0 fail)
Bundle size diffs [🚀 Bundle size reduced!]
|
e19b750 to
6c3d863
Compare
|
Contributor
Builds ready [099c069] [reused from e19b750]
⚡ Performance Benchmarks (Total: 🟢 22 pass · 🟡 2 warn · 🔴 0 fail)
Bundle size diffs [🚀 Bundle size reduced!]
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds a repeatable Chrome DevTools Protocol memory profiler for MetaMask extension flows. The profiler launches the existing LLM/E2E extension environment, optionally unlocks the default wallet, runs configurable route/send flows repeatedly, collects forced-GC heap and DOM counter samples, can write heap snapshots, and can fail on configurable heap/listener/node growth thresholds.
This is tooling-only. It does not include the Popover or Snow memory-leak fixes found during investigation.
Changelog
CHANGELOG entry: null
Manual testing steps
dist/chromeis not current:yarn build:test:webpackyarn llm:memory -- --helpyarn llm:memory -- --iterations 5 --flow send-open-back --sample final --probe cdp --extension-path dist/chrometest-artifacts/memoryand prints heap/listener/node deltasPre-merge author checklist
Pre-merge reviewer checklist
Validation
yarn test:unit test/e2e/playwright/llm-workflow/memory-profiler.test.tsyarn lint:changed:fixyarn llm:memory -- --helpyarn build:test:webpack