Repository navigation
Conversation
In some of our applications, memory kept growing with event replay enabled. The page used more memory after every route change. Event replay keeps every element that has an event listener, so it can replay early events on it later. After hydration, Angular clears this list once. But then it keeps adding new elements to it, and nothing reads them again. For example: - A list with 100 rows, where each row has a `(click)` listener, adds 100 elements every time the list is shown again. - After a route change, the elements of the old page stay in memory, together with their listeners and components. Only elements that come from the server have a `jsaction` attribute, and only these elements can be replayed. With this fix, Angular does not keep other elements anymore. This works with and without incremental hydration. Fixes angular#66506
d5e6da8 to
77d3b0f
Compare
JeanMeche
left a comment
JeanMeche
left a comment
There was a problem hiding this comment.
AGENT: Thanks for tackling this long-standing issue (#66506)! Preventing client-created elements from leaking into jsActionMap is critical.
A few questions and observations regarding the implementation and lifecycle:
-
Post-hydration overhead on hot paths:
enableStashEventListenerImplhooks intolistenToDomEvent. With this change, every event listener created throughout the entire lifetime of the SPA (route navigations, table scrolling, dynamic@for/@ifcomponents, dialogs) will perform:
lView[INJECTOR].get(APP_ID) -> stashEventListeners.get() -> (rEl as Element).hasAttribute('jsaction').
Since(rEl as Element).hasAttributeis a DOM read, running this on every event listener binding post-hydration introduces unnecessary overhead on hot rendering paths. -
Lifecycle of
clearStashFn():
When incremental hydration is disabled (withNoIncrementalHydration()), event replay is 100% complete onceappRef.whenStable()resolves (eventContract.cleanUp()is called and early events are already dispatched).
Could we callclearStashFn()directly insidewhenStable()rather than waiting forappRef.onDestroy()? OnceclearStashFn()is called,stashEventListeners.get(appId)isundefinedandlistenToDomEventincurs no DOM checks. -
Invariance for eager content (
blockName === ''):
Relying solely onhasAttribute('jsaction')means that if any client-created element happens to have ajsactionattribute (e.g. apps integrating with Wiz / hybrid setups, custom elements, or templates with[attr.jsaction]), it will still be pushed tojsActionMap.get('')and leak. Since eager content listeners are only replayed at initial stability, should we simply refuse to stash any element withblockName === ''once initialwhenStable()has settled? -
Test assertions:
Could we also assert thatdoc.getElementById('late-btn')?.__jsaction_fnsis undefined, to ensure no handler closures are retained on the element?
Please note that this fixes a real production issue; it wasn't found speculatively through AI code analysis.
In some of our applications, memory kept growing with event replay enabled. The page used more memory after every route change.
Event replay keeps every element that has an event listener, so it can replay early events on it later. After hydration, Angular clears this list once. But then it keeps adding new elements to it, and nothing reads them again. For example:
(click)listener, adds 100 elements every time the list is shown again.Only elements that come from the server have a
jsactionattribute, and only these elements can be replayed. With this fix, Angular does not keep other elements anymore. This works with and without incremental hydration.Fixes #66506