caplin / FlexLayout

Docking Layout Manager for React
MIT License
919 stars 173 forks source link

fix: handle undefined references when page is unmounted #414

Closed paralin closed 10 months ago

paralin commented 10 months ago

When FlexLayout is unmounted from the page, there is a race condition where some FlexLayout internal code may attempt to access selfRef.current when it is undefined (as the DOM has already been removed from the page).

The TypeScript code was using the ! operator to ignore the potential null values in that field, which results in null-access errors when unmounting the page.

Various other locations where undefined reference errors were seen have also been fixed to handle the respective variables being undefined.

https://github.com/caplin/FlexLayout/issues/352#issuecomment-1769322206

Fixes: #352

nealus commented 10 months ago

Hi Christian, have you seen cases where an exception in thrown other than

TypeError: Cannot read properties of null (reading 'getBoundingClientRect') at Layout.getDomRect (Layout.tsx:420:1) at updateRect (TabButton.tsx:112:1) at TabButton.tsx:99:1

I can recreate that one using https://github.com/jugglingcats/flexlayout-352/tree/master

paralin commented 10 months ago

@nealus yes - and those cases are fixed here as well.

nealus commented 10 months ago

In the 'jugglingcats' example the issue is caused by the Layout, tabbutton ... being unmounted then the tabbutton is remounted before the layout, causing the layouts ref to be null in the tabbutton useEffect. So it's to do with one component accessing a ref on another (then the mount order matters).

Basically I don't want to add code to protect every ref access unless it is actually needed, and if it is needed I want to understand why and have a test case.

As far as I can see the cross ref access (in useLayoutEffect) only happens in tabtutton and borderbutton, do you have example code where the null ref happens elsewhere? or are your changes defensive checks just in case?

I cannot see how selfref.current can be null in Layout.onDragStart for example

paralin commented 10 months ago

@nealus they are all locations where I had a null reference exception happen with a particular variable, so I removed all of the non-null assumptions (exclamation marks) for that variable and then fixed the type errors.

Given that a null reference exception crashes the program, and given that the null checks have zero measurable performance impact (null checks are negligible), it makes more sense to be defensive on the null checks and prevent any case where the app could crash due to a null dereference.