RDEV-10097 - Dispose the plugins of a destroyed view

Directory.Build.props

@@ -5,7 +5,7 @@
     <AssemblyVersion>2.0.0.0</AssemblyVersion>
     <FileVersion>2.0.0.0</FileVersion>
     <!-- Please see https://github.com/OutSystems/reactview?tab=readme-ov-file#versioning for versioning rules -->
-    <Version>5.120.5</Version>
+    <Version>5.120.6</Version>
     <Authors>OutSystems</Authors>
     <Product>ReactView</Product>
     <Copyright>Copyright © OutSystems 2023</Copyright>

ReactViewControl/ReactView.cs

@@ -19,7 +19,7 @@ public abstract partial class ReactView : IDisposable {
 
         private static ReactViewRender CreateReactViewInstance(ReactViewFactory factory) {
             ReactViewRender InnerCreateView() {
-                var view = new ReactViewRender(factory.DefaultStyleSheet, () => factory.InitializePlugins(), factory.EnableViewPreload, factory.EnableDebugMode, factory.EnsureInnerViewsAreDisposed, factory.LoadScriptsOncePerDocument);
+                var view = new ReactViewRender(factory.DefaultStyleSheet, () => factory.InitializePlugins(), factory.EnableViewPreload, factory.EnableDebugMode, factory.EnsureInnerViewsAreDisposed, factory.LoadScriptsOncePerDocument, factory.EnsureViewPluginsAreDisposed);
                 if (factory.ShowDeveloperTools) {
                     view.ShowDeveloperTools();
                 }

ReactViewControl/ReactViewFactory.cs

@@ -39,5 +39,13 @@ public class ReactViewFactory {
         /// executed for every other one. Set to false to restore the previous per view behaviour.
         /// </summary>
         public virtual bool LoadScriptsOncePerDocument => true;
+
+        /// <summary>
+        /// The plugins of a view are disposed when that view is destroyed. Plugins are not part of the react
+        /// tree, so unmounting does not reach them, and one that registered itself in document level state
+        /// keeps its view's root, and the whole dom under it, alive. Set to false to restore the previous
+        /// behaviour, where only the host released them.
+        /// </summary>
+        public virtual bool EnsureViewPluginsAreDisposed => true;
     }
 }

ReactViewControl/ReactViewRender.LoaderModule.cs

@@ -22,7 +22,7 @@ public LoaderModule(ReactViewRender viewRender) {
             /// <summary>
             /// Loads the specified react component into the specified frame
             /// </summary>
-            public void LoadComponent(IViewModule component, string frameName, bool hasStyleSheet, bool hasPlugins, bool ensureDisposeInnerViews, bool loadScriptsOncePerDocument) {
+            public void LoadComponent(IViewModule component, string frameName, bool hasStyleSheet, bool hasPlugins, bool ensureDisposeInnerViews, bool loadScriptsOncePerDocument, bool ensureViewPluginsAreDisposed) {
                 var mainSource = ViewRender.ToFullUrl(NormalizeUrl(component.MainJsSource));
                 var dependencySources = component.DependencyJsSources.Select(s => ViewRender.ToFullUrl(NormalizeUrl(s))).ToArray();
                 var cssSources = component.CssSources.Select(s => ViewRender.ToFullUrl(NormalizeUrl(s))).ToArray();
@@ -45,6 +45,7 @@ public void LoadComponent(IViewModule component, string frameName, bool hasStyle
                 // componentHash: string
                 // ensureDisposeInnerViews: boolean
                 // loadScriptsOncePerDocument: boolean
+                // ensureViewPluginsAreDisposed: boolean
 
                 var loadArgs = new[] {
                     JavascriptSerializer.Serialize(component.Name),
@@ -60,6 +61,7 @@ public void LoadComponent(IViewModule component, string frameName, bool hasStyle
                     JavascriptSerializer.Serialize(componentHash),
                     JavascriptSerializer.Serialize(ensureDisposeInnerViews),
                     JavascriptSerializer.Serialize(loadScriptsOncePerDocument),
+                    JavascriptSerializer.Serialize(ensureViewPluginsAreDisposed),
                 };
 
                 ExecuteLoaderFunction("loadComponent", loadArgs);

ReactViewControl/ReactViewRender.cs

@@ -38,10 +38,12 @@ internal partial class ReactViewRender : IChildViewHost, IDisposable {
         private bool isInputDisabled; // used primarly to control the intention to disable input (before the browser is ready)
         private readonly bool ensureDisposeInnerViews;
         private readonly bool loadScriptsOncePerDocument;
+        private readonly bool ensureViewPluginsAreDisposed;
 
-        public ReactViewRender(ResourceUrl defaultStyleSheet, Func<IViewModule[]> initializePlugins, bool preloadWebView, bool enableDebugMode, bool ensureInnerViewsAreDisposed, bool loadScriptsOncePerDocument = true) {
+        public ReactViewRender(ResourceUrl defaultStyleSheet, Func<IViewModule[]> initializePlugins, bool preloadWebView, bool enableDebugMode, bool ensureInnerViewsAreDisposed, bool loadScriptsOncePerDocument = true, bool ensureViewPluginsAreDisposed = true) {
             this.ensureDisposeInnerViews = ensureInnerViewsAreDisposed;
             this.loadScriptsOncePerDocument = loadScriptsOncePerDocument;
+            this.ensureViewPluginsAreDisposed = ensureViewPluginsAreDisposed;
             UserCallingAssembly = GetUserCallingMethod().ReflectedType.Assembly;
 
             // must useSharedDomain for the local storage to be shared
@@ -276,7 +278,7 @@ private void TryLoadComponent(FrameInfo frame) {
 
             RegisterNativeObject(frame.Component, frame);
 
-            Loader.LoadComponent(frame.Component, frame.Name, DefaultStyleSheet != null, frame.Plugins.Length > 0, ensureDisposeInnerViews, loadScriptsOncePerDocument);
+            Loader.LoadComponent(frame.Component, frame.Name, DefaultStyleSheet != null, frame.Plugins.Length > 0, ensureDisposeInnerViews, loadScriptsOncePerDocument, ensureViewPluginsAreDisposed);
             if (isInputDisabled && frame.IsMain) {
                 Loader.DisableMouseInteractions();
             }

ReactViewResources/Loader/Internal/Flags.ts

@@ -1,6 +1,7 @@
 // flags set by the host on the main view load, kept here rather than in ViewMetadataContext because that
 // module depends on react, and bootstrap reads flags before react has been defined
 const LoadScriptsOncePerDocumentFlagKey = "LOAD_SCRIPTS_ONCE_PER_DOCUMENT";
+const EnsureViewPluginsAreDisposedFlagKey = "ENSURE_VIEW_PLUGINS_ARE_DISPOSED";
 
 export function getLoadScriptsOncePerDocumentFlag(): boolean {
     return !!window[LoadScriptsOncePerDocumentFlagKey];
@@ -9,3 +10,11 @@ export function getLoadScriptsOncePerDocumentFlag(): boolean {
 export function setLoadScriptsOncePerDocumentFlag(loadScriptsOncePerDocument: boolean): void {
     window[LoadScriptsOncePerDocumentFlagKey] = loadScriptsOncePerDocument;
 }
+
+export function getEnsureViewPluginsAreDisposedFlag(): boolean {
+    return !!window[EnsureViewPluginsAreDisposedFlagKey];
+}
+
+export function setEnsureViewPluginsAreDisposedFlag(ensureViewPluginsAreDisposed: boolean): void {
+    window[EnsureViewPluginsAreDisposedFlagKey] = ensureViewPluginsAreDisposed;
+}

ReactViewResources/Loader/Internal/ResourcesLoader.ts

@@ -47,6 +47,13 @@ export function loadScript(scriptSrc: string, view: ViewMetadata): Promise<void>
  */
 function loadScriptPerView(scriptSrc: string, view: ViewMetadata): Promise<void> {
     return new Promise(async (resolve) => {
+        if (view.isReleased) {
+            // the view was destroyed and let go of its head, and a script appended to a detached tree never
+            // runs anyway, so whoever is loading is told there is nothing coming rather than left waiting
+            resolve();
+            return;
+        }
+
         const frameScripts = view.scriptsLoadTasks;
 
         // check if script was already added, fallback to main frame

ReactViewResources/Loader/Internal/ViewMetadata.ts

@@ -6,13 +6,15 @@ export type ViewMetadata = {
     name: string;
     generation: number;
     isMain: boolean;
+    isReleased: boolean; // set when the view is destroyed, so that whatever is still loading knows not to attach to it
     placeholder: Element; // element were the view is mounted (where the shadow root is mounted in case of child views)
     root?: Element; // view root element
     head?: Element; // view head element
     scriptsLoadTasks: Map<string, Task<void>>; // maps script source to load task, only used when scripts are tracked per view
     pluginsLoadTask: Task<void>; // plugins load task
     viewLoadTask: Task<void>; // resolved when view is loaded
     modules: Map<string, any>; // maps module name to module instance
+    plugins: any[]; // plugin instances, disposed when the view is destroyed
     nativeObjectNames: string[]; // list of frame native objects
     childViews: ObservableListCollection<ViewMetadata>;
     parentView: ViewMetadata;
@@ -26,10 +28,12 @@ export function newView(id: number, name: string, isMain: boolean, placeholder:
         name: name,
         generation: 0,
         isMain: isMain,
+        isReleased: false,
         placeholder: placeholder,
         head: undefined,
         root: undefined,
         modules: new Map<string, any>(),
+        plugins: [],
         scriptsLoadTasks: new Map<string, Task<void>>(),
         nativeObjectNames: [],
         pluginsLoadTask: new Task(),
@@ -38,4 +42,37 @@ export function newView(id: number, name: string, isMain: boolean, placeholder:
         context: null,
         parentView: null!
     };
-}
+}
+
+/**
+ * Releases what a destroyed view keeps alive. Plugins are not part of the react tree, so unmounting the view
+ * does not reach them: one that registered itself in document level state - a listener on the document, an
+ * entry in a handlers queue - stays registered, holding the view's root, and the whole tree under it, alive.
+ */
+export function releaseView(view: ViewMetadata): void {
+    view.isReleased = true;
+
+    view.plugins.forEach(plugin => disposePlugin(view, plugin));
+    view.plugins = [];
+
+    // holds the plugins and the view component, and each of those reaches the view's dom
+    view.modules.clear();
+
+    view.renderHandler = undefined;
+    view.root = undefined;
+    view.head = undefined;
+}
+
+function disposePlugin(view: ViewMetadata, plugin: any): void {
+    if (!plugin || typeof plugin.dispose !== "function") {
+        return;
+    }
+
+    try {
+        plugin.dispose();
+    } catch (error) {
+        // this runs while react tears the view down, and reporting it the usual way rethrows, which would
+        // abandon the rest of the teardown over a single plugin
+        window.console.error(`Failed to dispose a plugin of view "${view.name}"`, error);
+    }
+}

ReactViewResources/Loader/Internal/ViewPortal.tsx

@@ -1,7 +1,8 @@
 import * as React from "react";
 import { webViewRootId } from "../Internal/Environment";
 import { getStylesheets } from "./Common";
-import { ViewMetadata } from "./ViewMetadata";
+import { releaseView, ViewMetadata } from "./ViewMetadata";
+import { getEnsureViewPluginsAreDisposedFlag } from "./Flags";
 import { ViewSharedContext } from "../Public/ViewSharedContext";
 import { addView, deleteView } from "./ViewsCollection";
 import { notifyViewDestroyed, notifyViewInitialized } from "./NativeAPI";
@@ -26,6 +27,13 @@ export function onChildViewAdded(childView: ViewMetadata) {
 
 export function onChildViewRemoved(childView: ViewMetadata) {
     deleteView(childView.name);
+
+    if (getEnsureViewPluginsAreDisposedFlag()) {
+        // plugins call into their native objects as they release, and the notification below is what
+        // unregisters those, so the release has to come first
+        releaseView(childView);
+    }
+
     notifyViewDestroyed(childView.name);
 }
 

ReactViewResources/Loader/Loader.ts

@@ -12,7 +12,7 @@ import { ViewMetadata } from "./Internal/ViewMetadata";
 import { createPropertiesProxy } from "./Internal/ViewPropertiesProxy";
 import { addView, getView, tryGetView } from "./Internal/ViewsCollection";
 import { setEnsureDisposeInnerViewsFlag } from "./Internal/ViewMetadataContext";
-import { setLoadScriptsOncePerDocumentFlag } from "./Internal/Flags";
+import { setEnsureViewPluginsAreDisposedFlag, setLoadScriptsOncePerDocumentFlag } from "./Internal/Flags";
 
 export { disableMouseInteractions, enableMouseInteractions } from "./Internal/InputManager";
 export { showErrorMessage } from "./Internal/MessagesProvider";
@@ -99,10 +99,20 @@ export function loadPlugins(plugins: any[][], frameName: string): void {
 
                     const pluginNativeObject = await bindNativeObject(nativeObjectFullName);
 
+                    if (view.isReleased) {
+                        // the view was destroyed while this was loading, and plugins reach out to document
+                        // level state as they are built, which nothing would take back: the view is gone and
+                        // it is what disposes them
+                        return;
+                    }
+
                     view.nativeObjectNames.push(nativeObjectFullName); // add to the native objects collection
 
                     const plugin: IPlugin<any> = module.default;
-                    view.modules.set(moduleName, new plugin(pluginNativeObject, view.root as HTMLElement, view.viewLoadTask.promise, getViewInfo(view)));
+                    const pluginInstance = new plugin(pluginNativeObject, view.root as HTMLElement, view.viewLoadTask.promise, getViewInfo(view));
+
+                    view.modules.set(moduleName, pluginInstance);
+                    view.plugins.push(pluginInstance);
                 });
 
                 await Promise.all(pluginsPromises);
@@ -130,7 +140,8 @@ export function loadComponent(
     frameName: string,
     componentHash: string,
     ensureDisposeInnerViews: boolean,
-    loadScriptsOncePerDocument: boolean): void {
+    loadScriptsOncePerDocument: boolean,
+    ensureViewPluginsAreDisposed: boolean): void {
 
     async function innerLoad() {
         let view: ViewMetadata;
@@ -142,8 +153,10 @@ export function loadComponent(
 
             if (frameName === mainFrameName) {
                 setEnsureDisposeInnerViewsFlag(ensureDisposeInnerViews);
-                // the main view always loads first, so the flag is set before any inner view loads a script
+                // the main view always loads first, so the flags are set before any inner view loads a
+                // script or is taken down
                 setLoadScriptsOncePerDocumentFlag(loadScriptsOncePerDocument);
+                setEnsureViewPluginsAreDisposedFlag(ensureViewPluginsAreDisposed);
             }
 
             view = tryGetView(frameName)!;
@@ -174,6 +187,12 @@ export function loadComponent(
             // main component script should be the last to be loaded, otherwise errors might occur
             await loadScript(componentSource, view);
 
+            if (view.isReleased) {
+                // the view was destroyed while its component was loading, and rendering it now would attach
+                // a tree, and the plugins that go with it, to something that is already detached
+                return;
+            }
+
             const renderFinishedTask = cacheEntry ? view.viewLoadTask : null;
             // create proxy for properties obj to delay its methods execution until native object is ready
             const properties = createPropertiesProxy(rootElement, componentNativeObject, componentNativeObjectName, renderFinishedTask);

Tests.ReactView/InnerViewPluginsTests.cs

@@ -0,0 +1,63 @@
+using System.Threading.Tasks;
+using NUnit.Framework;
+using ReactViewControl;
+
+namespace Tests.ReactView {
+
+    public class InnerViewPluginsTests : ReactViewTestBase {
+
+        private class ViewFactoryWithPlugin : TestReactViewFactory {
+
+            public override IViewModule[] InitializePlugins() {
+                return new IViewModule[] { new PluginModule() };
+            }
+        }
+
+        private class ReactViewWithPlugin : TestReactView {
+
+            public ReactViewWithPlugin() { }
+
+            protected override ReactViewFactory Factory => new ViewFactoryWithPlugin();
+        }
+
+        protected override TestReactView CreateView() {
+            return new ReactViewWithPlugin();
+        }
+
+        protected override void InitializeView() {
+            if (TargetView != null) {
+                TargetView.AutoShowInnerView = true;
+            }
+            base.InitializeView();
+        }
+
+        [Test(Description = "Tests inner view plugins are disposed when the view is destroyed")]
+        public async Task PluginIsDisposedWhenInnerViewIsDestroyed() {
+            await Run(async () => {
+                var innerViewLoaded = new TaskCompletionSource<bool>();
+                TargetView.InnerView.Loaded += () => innerViewLoaded.TrySetResult(true);
+#if DEBUG
+                TargetView.Ready += () => TargetView.InnerView.Load();
+#endif
+                TargetView.InnerView.Load();
+                await innerViewLoaded.Task;
+
+                var innerViewHidden = new TaskCompletionSource<bool>();
+                TargetView.Event += name => {
+                    if (name == "InnerViewHidden") {
+                        innerViewHidden.TrySetResult(true);
+                    }
+                };
+                // the notification is sent from the set state callback, which react runs after it has
+                // committed the removal, so the inner view is already torn down by the time it arrives
+                TargetView.ExecuteMethod("hideInnerView");
+                await innerViewHidden.Task;
+
+                // Plugins are not part of the react tree, so unmounting the view is not enough to release them
+                var disposedPlugins = await TargetView.EvaluateMethod<int>("getDisposedPluginModulesCount");
+
+                Assert.AreEqual(1, disposedPlugins, "The plugin of the destroyed view was not disposed!");
+            });
+        }
+    }
+}

Tests.ReactView/TestAppView/PluginModule.ts

@@ -1,4 +1,5 @@
 (window as any).PluginModuleLoaded = true;
+(window as any).DisposedPluginModules = 0;
 
 export default class Plugin {
 
@@ -7,4 +8,8 @@ export default class Plugin {
     constructor(public nativeObject: object, public root: HTMLElement, loadPromise: Promise<void>) {
         loadPromise.then(() => this.viewLoaded = true); 
     }
-}
+
+    public dispose() {
+        (window as any).DisposedPluginModules++;
+    }
+}

Tests.ReactView/TestAppView/TestApp.tsx

@@ -17,7 +17,11 @@ interface IChildViews {
     test: InnerView;
 }
 
-class App extends React.Component<IAppProperties> {
+interface IAppState {
+    isInnerViewHidden: boolean;
+}
+
+class App extends React.Component<IAppProperties, IAppState> {
     firstRenderHtml: string;
     pluginsContext: IPluginsContext;
     innerViewLoadedTask = new Task<boolean>();
@@ -26,10 +30,11 @@ class App extends React.Component<IAppProperties> {
         super(props);
         this.pluginsContext = context;
         this.firstRenderHtml = this.getHtml();
+        this.state = { isInnerViewHidden: false };
     }
 
     renderInnerViewContainer() {
-        if (this.props.autoShowInnerView) {
+        if (this.props.autoShowInnerView && !this.state.isInnerViewHidden) {
             return <ViewFrame<IChildViews> key="test_frame" name="test" className="" loaded={() => this.innerViewLoadedTask.setResult()} />;
         }
 
@@ -106,6 +111,14 @@ class App extends React.Component<IAppProperties> {
         this.innerViewLoadedTask.promise.then(() => this.props.event("InnerViewLoaded"));
     }
 
+    hideInnerView() {
+        this.setState({ isInnerViewHidden: true }, () => this.props.event("InnerViewHidden"));
+    }
+
+    getDisposedPluginModulesCount() {
+        return (window as any).DisposedPluginModules;
+    }
+
     loadCustomResource(url: string) {
         console.log(url);
         var img = document.createElement("img");