fix(router): prevent memory leak in RouterScroller scroll position store#68177
fix(router): prevent memory leak in RouterScroller scroll position store#68177arturovt wants to merge 1 commit intoangular:mainfrom
Conversation
|
Woah, looks like you've opened a lot of issues/PRs recently. While we appreciate contributions from the community, triaging and reviewing a large influx of content in a short time period takes time away from other ongoing projects. As a result, we're closing these issues/PRs to maintain the team's focus. Note that this is not necessarily a rejection of the goals or direction of any of these contributions in particular, so much as a reflection of the team's current capacity and priorities. You are welcome to open a smaller subset of issues/PRs in accordance with our policy focused on the most important and impactful contributions and we will do our best to prioritize a response as soon as possible. |
The `store` map in `RouterScroller` accumulates scroll positions keyed by navigation ID, which increments indefinitely. Entries were never evicted, causing unbounded memory growth over the lifetime of the application. Scroll positions are only ever read on popstate events, meaning only entries corresponding to current browser history stack entries are useful. Chromium/Blink enforces a hard cap of 50 history entries via `kMaxSessionHistoryEntries`, pruning via `NavigationControllerImpl::PruneOldestSkippableEntryIfFull()`. Entries beyond that limit can never be restored via popstate. The fix evicts the oldest entry (smallest navigation ID) whenever the store exceeds 50 entries, bounding memory usage to a constant size. Closes angular#51120
dda159e to
830c999
Compare
|
@arturovt do you have concrete numbers on how much memory this actually consumes? Even in an unreasonably long lived session, I cannot imagine storing a couple numbers adds up to an amount that’s worth addressing. Do you have real concerns here or are you trying to tackle low hanging fruit? fwiw, I do not think the approach works. Navigation ids to not follow page order exactly. Safari also has a larger allowance for history storage than 50 as far as I can tell |
|
From the analysis sent over, a single item takes about ~32 bytes of storage. To hit even 1MB of memory (which is still tiny), a user would have to navigate to roughly 33,000 unique pages in a single session without refreshing. A single tab's baseline memory usage is usually 50MB to 150MB just to render a basic page. I think this generally lands in the bucket of really over-optimizing and not worth a potentially incorrect fix. FWIW, the Navigation API does not need to track scroll positions in memory because it supports scroll restoration for SPAs natively. |
The
storemap inRouterScrolleraccumulates scroll positions keyed by navigation ID, which increments indefinitely. Entries were never evicted, causing unbounded memory growth over the lifetime of the application.Scroll positions are only ever read on popstate events, meaning only entries corresponding to current browser history stack entries are useful. Chromium/Blink enforces a hard cap of 50 history entries via
kMaxSessionHistoryEntries, pruning viaNavigationControllerImpl::PruneOldestSkippableEntryIfFull(). Entries beyond that limit can never be restored via popstate.The fix evicts the oldest entry (smallest navigation ID) whenever the store exceeds 50 entries, bounding memory usage to a constant size.
Closes #51120