Fix #5927: BodyCell memory leak when not editable (#6011)

* Fix #5927: BodyCell memory leak when not editable * Fix #5927: BodyCell memory leak when not editable
primefaces · Feb 20, 2024 · 5d60914 · 5d60914
1 parent 6cd5730
commit 5d60914
Show file tree

Hide file tree

Showing 5 changed files with 74 additions and 31 deletions.
diff --git a/components/lib/datatable/BodyCell.js b/components/lib/datatable/BodyCell.js
@@ -54,6 +54,10 @@ export const BodyCell = React.memo((props) => {
     const field = getColumnProp('field') || `field_${props.index}`;
     const editingKey = props.dataKey ? (props.rowData && props.rowData[props.dataKey]) || props.rowIndex : props.rowIndex;
 
+    const isEditable = () => {
+        return getColumnProp('editor');
+    };
+
     const [bindDocumentClickListener, unbindDocumentClickListener] = useEventListener({
         type: 'click',
         listener: (e) => {
@@ -66,13 +70,10 @@ export const BodyCell = React.memo((props) => {
 
             selfClick.current = false;
         },
-        options: true
+        options: true,
+        when: isEditable()
     });
 
-    const isEditable = () => {
-        return getColumnProp('editor');
-    };
-
     const isSelected = () => {
         return props.selection ? (props.selection instanceof Array ? findIndex(props.selection) > -1 : equals(props.selection)) : false;
     };

diff --git a/components/lib/datatable/ColumnFilter.js b/components/lib/datatable/ColumnFilter.js
@@ -1,9 +1,11 @@
 import * as React from 'react';
 import PrimeReact, { FilterMatchMode, FilterOperator, PrimeReactContext, localeOption } from '../api/Api';
+import { ariaLabel } from '../api/Locale';
 import { Button } from '../button/Button';
 import { ColumnBase } from '../column/ColumnBase';
 import { CSSTransition } from '../csstransition/CSSTransition';
 import { Dropdown } from '../dropdown/Dropdown';
+import FocusTrap from '../focustrap/FocusTrap';
 import { useMergeProps, useOverlayListener, useUnmountEffect, useUpdateEffect } from '../hooks/Hooks';
 import { FilterIcon } from '../icons/filter';
 import { FilterSlashIcon } from '../icons/filterslash';
@@ -14,13 +16,11 @@ import { OverlayService } from '../overlayservice/OverlayService';
 import { Portal } from '../portal/Portal';
 import { Ripple } from '../ripple/Ripple';
 import { DomHandler, IconUtils, ObjectUtils, UniqueComponentId, ZIndexUtils } from '../utils/Utils';
-import { ariaLabel } from '../api/Locale';
-import FocusTrap from '../focustrap/FocusTrap';
 
 export const ColumnFilter = React.memo((props) => {
     const [overlayVisibleState, setOverlayVisibleState] = React.useState(false);
     const overlayRef = React.useRef(null);
-    const overlayId = React.useRef(UniqueComponentId());
+    const overlayId = React.useRef(() => UniqueComponentId());
     const iconRef = React.useRef(null);
     const selfClick = React.useRef(false);
     const overlayEventListener = React.useRef(null);

diff --git a/components/lib/hooks/useEventListener.js b/components/lib/hooks/useEventListener.js
@@ -1,4 +1,3 @@
-/* eslint-disable */
 import * as React from 'react';
 import { DomHandler, ObjectUtils } from '../utils/Utils';
 import { usePrevious } from './usePrevious';
@@ -7,13 +6,15 @@ import { useUnmountEffect } from './useUnmountEffect';
 export const useEventListener = ({ target = 'document', type, listener, options, when = true }) => {
     const targetRef = React.useRef(null);
     const listenerRef = React.useRef(null);
-    const prevListener = usePrevious(listener);
-    const prevOptions = usePrevious(options);
+    let prevListener = usePrevious(listener);
+    let prevOptions = usePrevious(options);
 
     const bind = (bindOptions = {}) => {
-        if (ObjectUtils.isNotEmpty(bindOptions.target)) {
+        const { target: bindTarget } = bindOptions;
+
+        if (ObjectUtils.isNotEmpty(bindTarget)) {
             unbind();
-            (bindOptions.when || when) && (targetRef.current = DomHandler.getTargetElement(bindOptions.target));
+            (bindOptions.when || when) && (targetRef.current = DomHandler.getTargetElement(bindTarget));
         }
 
         if (!listenerRef.current && targetRef.current) {
@@ -29,27 +30,44 @@ export const useEventListener = ({ target = 'document', type, listener, options,
         }
     };
 
-    React.useEffect(() => {
+    const dispose = () => {
+        unbind();
+        // Prevent memory leak by releasing
+        prevListener = null;
+        prevOptions = null;
+    };
+
+    const updateTarget = React.useCallback(() => {
         if (when) {
             targetRef.current = DomHandler.getTargetElement(target);
         } else {
             unbind();
             targetRef.current = null;
         }
+        // eslint-disable-next-line react-hooks/exhaustive-deps
     }, [target, when]);
 
     React.useEffect(() => {
-        // to properly compare functions we can implicitly converting the function to it's body's text as a String
-        if (listenerRef.current && ('' + prevListener !== '' + listener || prevOptions !== options)) {
+        updateTarget();
+    }, [updateTarget]);
+
+    React.useEffect(() => {
+        const listenerChanged = `${prevListener}` !== `${listener}`;
+        const optionsChanged = prevOptions !== options;
+        const listenerExists = listenerRef.current;
+
+        if (listenerExists && (listenerChanged || optionsChanged)) {
             unbind();
             when && bind();
+        } else if (!listenerExists) {
+            dispose();
         }
+        // eslint-disable-next-line react-hooks/exhaustive-deps
     }, [listener, options, when]);
 
     useUnmountEffect(() => {
-        unbind();
+        dispose();
     });
 
     return [bind, unbind];
 };
-/* eslint-enable */
diff --git a/components/lib/hooks/useOverlayScrollListener.js b/components/lib/hooks/useOverlayScrollListener.js
@@ -1,16 +1,16 @@
-/* eslint-disable */
 import * as React from 'react';
 import PrimeReact, { PrimeReactContext } from '../api/Api';
 import { DomHandler, ObjectUtils } from '../utils/Utils';
 import { usePrevious } from './usePrevious';
 import { useUnmountEffect } from './useUnmountEffect';
 
 export const useOverlayScrollListener = ({ target, listener, options, when = true }) => {
+    const context = React.useContext(PrimeReactContext);
     const targetRef = React.useRef(null);
     const listenerRef = React.useRef(null);
-    const scrollableParents = React.useRef([]);
-    const prevOptions = usePrevious(options);
-    const context = React.useContext(PrimeReactContext);
+    const scrollableParentsRef = React.useRef([]);
+    let prevListener = usePrevious(listener);
+    let prevOptions = usePrevious(options);
 
     const bind = (bindOptions = {}) => {
         if (ObjectUtils.isNotEmpty(bindOptions.target)) {
@@ -20,7 +20,7 @@ export const useOverlayScrollListener = ({ target, listener, options, when = tru
 
         if (!listenerRef.current && targetRef.current) {
             const hideOnScroll = context ? context.hideOverlaysOnDocumentScrolling : PrimeReact.hideOverlaysOnDocumentScrolling;
-            const nodes = (scrollableParents.current = DomHandler.getScrollableParents(targetRef.current, hideOnScroll));
+            const nodes = (scrollableParentsRef.current = DomHandler.getScrollableParents(targetRef.current, hideOnScroll));
 
             listenerRef.current = (event) => listener && listener(event);
             nodes.forEach((node) => node.addEventListener('scroll', listenerRef.current, options));
@@ -29,33 +29,53 @@ export const useOverlayScrollListener = ({ target, listener, options, when = tru
 
     const unbind = () => {
         if (listenerRef.current) {
-            const nodes = scrollableParents.current;
+            const nodes = scrollableParentsRef.current;
+
             nodes.forEach((node) => node.removeEventListener('scroll', listenerRef.current, options));
 
             listenerRef.current = null;
         }
     };
 
-    React.useEffect(() => {
+    const dispose = () => {
+        unbind();
+        // #5927 prevent memory leak by releasing
+        scrollableParentsRef.current = null;
+        prevListener = null;
+        prevOptions = null;
+    };
+
+    const updateTarget = React.useCallback(() => {
         if (when) {
             targetRef.current = DomHandler.getTargetElement(target);
         } else {
             unbind();
             targetRef.current = null;
         }
+        // eslint-disable-next-line react-hooks/exhaustive-deps
     }, [target, when]);
 
     React.useEffect(() => {
-        if (listenerRef.current && (listenerRef.current !== listener || prevOptions !== options)) {
+        updateTarget();
+    }, [updateTarget]);
+
+    React.useEffect(() => {
+        const listenerChanged = `${prevListener}` !== `${listener}`;
+        const optionsChanged = prevOptions !== options;
+        const listenerExists = listenerRef.current;
+
+        if (listenerExists && (listenerChanged || optionsChanged)) {
             unbind();
             when && bind();
+        } else if (!listenerExists) {
+            dispose();
         }
-    }, [listener, options]);
+        // eslint-disable-next-line react-hooks/exhaustive-deps
+    }, [listener, options, when]);
 
     useUnmountEffect(() => {
-        unbind();
+        dispose();
     });
 
     return [bind, unbind];
 };
-/* eslint-enable */
diff --git a/components/lib/hooks/usePrevious.js b/components/lib/hooks/usePrevious.js
@@ -1,11 +1,15 @@
 import * as React from 'react';
 
 export const usePrevious = (newValue) => {
-    const ref = React.useRef(undefined);
+    const ref = React.useRef(null);
 
     React.useEffect(() => {
         ref.current = newValue;
-    });
+
+        return () => {
+            ref.current = null;
+        };
+    }, [newValue]);
 
     return ref.current;
 };