iTwin
Add extra cleanup functionality in PrimitiveTool
A continuation of the memory leak we see on refreshes and navigation away from a viewer.
In ToolAdmin, we see PrimitiveTool objects still carrying references to targetView, a Viewport object, preventing Viewport objects to be garbage collected, even after the ViewManager calls onShutdown().
This PR removes that reference to the viewport object when the PrimitiveTool's cleanup function is called, and also adds this cleanup functionality to the ToolAdmin's onShutdown().
Also, refactored the onCleanup() of subclasses extending from PrimitiveTool, to call super.cleanup() after custom cleanup functionality - Some custom cleanup functionality might rely on the iModel the tool is associated with, which is accessed through the renderTarget's reference. We don't want to remove that reference until after custom cleanup functionality from the subclasses have ran.
This feels like whack-a-mole. Isn't the root problem that you've got IModelApp inside some React state that's repeatedly getting recreated?
This feels like whack-a-mole. Isn't the root problem that you've got IModelApp inside some React state that's repeatedly getting recreated?
You're right that it is whack-a-mole, but we can't dismiss the presence of moles, and the need to whack when we've found them.
The root problem isn't the IModelApp being recreated - that's not the problem we're seeing anymore now that we are explictly calling onShutdown() in our viewer components.
The problem is the instances where a viewport is unmounted and we want to dispose of the resources that the allocated ScreenViewport uses. The unmounting event comes from dropViewport(), which can can be called outside of IModelApp being shutdown. This particular case being ViewportComponent's handleDisconnectFromViewManager() .
After your comment, I went back to look at how ViewManager calls ToolAdmin's forgetViewport, and added a line where we call the current tool's onCleanup(), since the tools themselves also carry references to the viewport in use. I didn't want to set the tool's themselves to be undefined, in case the tools are to be reused in between viewports.
This feels like whack-a-mole. Isn't the root problem that you've got IModelApp inside some React state that's repeatedly getting recreated?
Agree. You can't now require that sub-class call super when it was previously a no-op.
