Page MenuHomePhabricator

Disconnect observer for sticky header feature.
Closed, InvalidPublic3 Estimated Story Points

Description

Coming out of T295103, Intersection Observers should be explicitly disconnected to prevent memory leaks.

Acceptance criteria

  • Observers are manually disconnected when elements no longer need to be observed.

Developer notes

  • In the Vector repo, main.js, stickyHeader.js, scrollObserver.js are the files that invoke/use IntersectionObserver.
  • We have some options to disconnect the observer using different events (i.e. beforeunload, visibilitychange, pagehide, etc).
  • The MDN recommendation states:
The best event to use to signal the end of a user's session is the visibilitychange event. In browsers that don't support visibilitychange the pagehide event is the next-best alternative.
  • Note though that the beforeunload and pagehide events are not reliably fired by browsers, especially on mobile (see Usage notes at pagehide and beforeunload docs). This might not be a problem now since sticky header is only a desktop feature. However, since these events have full cross-browser support, we might consider them for expediency.
  • Per beforeunload docs:
There is a legitimate use case for the beforeunload event: the scenario where the user has entered unsaved data that will be lost if the page is unloaded. It is recommended that developers listen for beforeunload only in this scenario, and only when they actually have unsaved changes, so as to minimize the effect on performance.
window.addEventListener(  'pagehide', ( event ) => {
	if ( !event.persisted && observer ) {
		observer.disconnect();
	}
} );

Event Timeline

Jdlrobson added subscribers: ovasileva, Jdlrobson.

We shouldn't introduce new memory leaks @ovasileva so suggesting this as a deployment blocker.

@cjming could you flesh this task with a rough idea of the work needed and how tricky this will be?

@cjming could you flesh this task with a rough idea of the work needed and how tricky this will be?

sure thing - I'll prioritize this for today

Change 743509 had a related patch set uploaded (by Clare Ming; author: Clare Ming):

[mediawiki/skins/Vector@master] Disconnect observer for sticky header before unload.

https://gerrit.wikimedia.org/r/743509

Change 743509 abandoned by Clare Ming:

[mediawiki/skins/Vector@master] Disconnect observer for sticky header before unload.

Reason:

need to do more research

https://gerrit.wikimedia.org/r/743509

Change 744039 had a related patch set uploaded (by Clare Ming; author: Clare Ming):

[mediawiki/skins/Vector@master] Disconnect observer for sticky header.

https://gerrit.wikimedia.org/r/744039

Change 744039 abandoned by Clare Ming:

[mediawiki/skins/Vector@master] Disconnect observer for sticky header.

Reason:

not needed

https://gerrit.wikimedia.org/r/744039

Turns out it's only removing the element from the DOM altogether without disconnecting the observer that causes this memory leak issue (I got muddled in my mind about how sticky header is using IntersectionObserver) so closing this issue for now with a note saying that if we ever remove an intersection element while using IntersectionObserver, then we should explicitly disconnect the observer while this issue is still outstanding in Firefox.