diff --git a/Directory.Build.props b/Directory.Build.props index e30e7bfb..ce01d0cb 100644 --- a/Directory.Build.props +++ b/Directory.Build.props @@ -5,7 +5,7 @@ 2.0.0.0 2.0.0.0 - 5.120.6 + 5.120.7 OutSystems ReactView Copyright © OutSystems 2023 diff --git a/ReactViewControl/ReactView.cs b/ReactViewControl/ReactView.cs index 34924c25..24eb45d8 100644 --- a/ReactViewControl/ReactView.cs +++ b/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, factory.EnsureViewPluginsAreDisposed); + var view = new ReactViewRender(factory.DefaultStyleSheet, () => factory.InitializePlugins(), factory.EnableViewPreload, factory.EnableDebugMode, factory.EnsureInnerViewsAreDisposed, factory.LoadScriptsOncePerDocument, factory.EnsureViewPluginsAreDisposed, factory.BailOutOnUnboundNativeObjectCalls); if (factory.ShowDeveloperTools) { view.ShowDeveloperTools(); } diff --git a/ReactViewControl/ReactViewFactory.cs b/ReactViewControl/ReactViewFactory.cs index f92db495..4d7e9d14 100644 --- a/ReactViewControl/ReactViewFactory.cs +++ b/ReactViewControl/ReactViewFactory.cs @@ -47,5 +47,17 @@ public class ReactViewFactory { /// behaviour, where only the host released them. /// public virtual bool EnsureViewPluginsAreDisposed => true; + + /// + /// Calls through the view properties proxy into a view that was already destroyed are dropped, and + /// logged to the console, whatever they return: destroying a view unregisters its native objects, so + /// there is nothing left to call into and nobody left to receive a result. + /// Every other call is left alone and still surfaces as an error, including one whose native object + /// was unregistered while its view is still live: that is a broken channel to the presenter, and + /// dropping it would silently discard a real user interaction. + /// Set to false to restore the previous behaviour, where a call into a destroyed view surfaces as an + /// uncaught error as well. + /// + public virtual bool BailOutOnUnboundNativeObjectCalls => true; } } diff --git a/ReactViewControl/ReactViewRender.LoaderModule.cs b/ReactViewControl/ReactViewRender.LoaderModule.cs index 56742276..d41155c1 100644 --- a/ReactViewControl/ReactViewRender.LoaderModule.cs +++ b/ReactViewControl/ReactViewRender.LoaderModule.cs @@ -22,7 +22,7 @@ public LoaderModule(ReactViewRender viewRender) { /// /// Loads the specified react component into the specified frame /// - public void LoadComponent(IViewModule component, string frameName, bool hasStyleSheet, bool hasPlugins, bool ensureDisposeInnerViews, bool loadScriptsOncePerDocument, bool ensureViewPluginsAreDisposed) { + public void LoadComponent(IViewModule component, string frameName, bool hasStyleSheet, bool hasPlugins, bool ensureDisposeInnerViews, bool loadScriptsOncePerDocument, bool ensureViewPluginsAreDisposed, bool bailOutOnUnboundNativeObjectCalls) { 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(); @@ -46,6 +46,7 @@ public void LoadComponent(IViewModule component, string frameName, bool hasStyle // ensureDisposeInnerViews: boolean // loadScriptsOncePerDocument: boolean // ensureViewPluginsAreDisposed: boolean + // bailOutOnUnboundNativeObjectCalls: boolean var loadArgs = new[] { JavascriptSerializer.Serialize(component.Name), @@ -62,6 +63,7 @@ public void LoadComponent(IViewModule component, string frameName, bool hasStyle JavascriptSerializer.Serialize(ensureDisposeInnerViews), JavascriptSerializer.Serialize(loadScriptsOncePerDocument), JavascriptSerializer.Serialize(ensureViewPluginsAreDisposed), + JavascriptSerializer.Serialize(bailOutOnUnboundNativeObjectCalls), }; ExecuteLoaderFunction("loadComponent", loadArgs); diff --git a/ReactViewControl/ReactViewRender.cs b/ReactViewControl/ReactViewRender.cs index 2d677267..ba1c21f9 100644 --- a/ReactViewControl/ReactViewRender.cs +++ b/ReactViewControl/ReactViewRender.cs @@ -39,11 +39,13 @@ internal partial class ReactViewRender : IChildViewHost, IDisposable { private readonly bool ensureDisposeInnerViews; private readonly bool loadScriptsOncePerDocument; private readonly bool ensureViewPluginsAreDisposed; + private readonly bool bailOutOnUnboundNativeObjectCalls; - public ReactViewRender(ResourceUrl defaultStyleSheet, Func initializePlugins, bool preloadWebView, bool enableDebugMode, bool ensureInnerViewsAreDisposed, bool loadScriptsOncePerDocument = true, bool ensureViewPluginsAreDisposed = true) { + public ReactViewRender(ResourceUrl defaultStyleSheet, Func initializePlugins, bool preloadWebView, bool enableDebugMode, bool ensureInnerViewsAreDisposed, bool loadScriptsOncePerDocument = true, bool ensureViewPluginsAreDisposed = true, bool bailOutOnUnboundNativeObjectCalls = true) { this.ensureDisposeInnerViews = ensureInnerViewsAreDisposed; this.loadScriptsOncePerDocument = loadScriptsOncePerDocument; this.ensureViewPluginsAreDisposed = ensureViewPluginsAreDisposed; + this.bailOutOnUnboundNativeObjectCalls = bailOutOnUnboundNativeObjectCalls; UserCallingAssembly = GetUserCallingMethod().ReflectedType.Assembly; // must useSharedDomain for the local storage to be shared @@ -278,7 +280,7 @@ private void TryLoadComponent(FrameInfo frame) { RegisterNativeObject(frame.Component, frame); - Loader.LoadComponent(frame.Component, frame.Name, DefaultStyleSheet != null, frame.Plugins.Length > 0, ensureDisposeInnerViews, loadScriptsOncePerDocument, ensureViewPluginsAreDisposed); + Loader.LoadComponent(frame.Component, frame.Name, DefaultStyleSheet != null, frame.Plugins.Length > 0, ensureDisposeInnerViews, loadScriptsOncePerDocument, ensureViewPluginsAreDisposed, bailOutOnUnboundNativeObjectCalls); if (isInputDisabled && frame.IsMain) { Loader.DisableMouseInteractions(); } diff --git a/ReactViewResources/Loader/Internal/Flags.ts b/ReactViewResources/Loader/Internal/Flags.ts index 213d9bb4..22d64f5e 100644 --- a/ReactViewResources/Loader/Internal/Flags.ts +++ b/ReactViewResources/Loader/Internal/Flags.ts @@ -2,6 +2,7 @@ // 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"; +const BailOutOnUnboundNativeObjectCallsFlagKey = "BAIL_OUT_ON_UNBOUND_NATIVE_OBJECT_CALLS"; export function getLoadScriptsOncePerDocumentFlag(): boolean { return !!window[LoadScriptsOncePerDocumentFlagKey]; @@ -18,3 +19,11 @@ export function getEnsureViewPluginsAreDisposedFlag(): boolean { export function setEnsureViewPluginsAreDisposedFlag(ensureViewPluginsAreDisposed: boolean): void { window[EnsureViewPluginsAreDisposedFlagKey] = ensureViewPluginsAreDisposed; } + +export function getBailOutOnUnboundNativeObjectCallsFlag(): boolean { + return !!window[BailOutOnUnboundNativeObjectCallsFlagKey]; +} + +export function setBailOutOnUnboundNativeObjectCallsFlag(bailOutOnUnboundNativeObjectCalls: boolean): void { + window[BailOutOnUnboundNativeObjectCallsFlagKey] = bailOutOnUnboundNativeObjectCalls; +} diff --git a/ReactViewResources/Loader/Internal/ViewPropertiesProxy.ts b/ReactViewResources/Loader/Internal/ViewPropertiesProxy.ts index e5a60ae7..32bb4e60 100644 --- a/ReactViewResources/Loader/Internal/ViewPropertiesProxy.ts +++ b/ReactViewResources/Loader/Internal/ViewPropertiesProxy.ts @@ -1,7 +1,9 @@ import { bindNativeObject } from "./NativeAPI"; +import { getBailOutOnUnboundNativeObjectCallsFlag } from "./Flags"; import { Task } from "./Task"; +import { ViewMetadata } from "./ViewMetadata"; -export function createPropertiesProxy(rootElement: Element, objProperties: {}, nativeObjName: string, componentRenderedWaitTask?: Task | null): {} { +export function createPropertiesProxy(rootElement: Element, objProperties: {}, nativeObjName: string, view: ViewMetadata, componentRenderedWaitTask?: Task | null): {} { const proxy = Object.assign({}, objProperties); Object.keys(proxy).forEach(key => { const value = objProperties[key]; @@ -9,6 +11,19 @@ export function createPropertiesProxy(rootElement: Element, objProperties: {}, n proxy[key] = value; } else { proxy[key] = async function () { + // read per call: the proxy outlives the view, and what it should do about a call that + // arrives after the view is gone is decided by the flag in place at that moment + if (getBailOutOnUnboundNativeObjectCallsFlag() && view.isReleased) { + // destroying the view is what unregisters its native objects, so there is nothing left + // to call into, and nobody left to receive what the call would have returned + logUnboundCall(nativeObjName, key, "the view was destroyed"); + return; + } + + // every call that gets this far belongs to a live view, and a live view that cannot reach + // its native object is a broken channel to the presenter, not a teardown race: it has to + // keep failing exactly as it did before this bail out existed. Dropping it would discard a + // real user interaction and leave a view that looks alive but does nothing const nativeObject = window[nativeObjName] || await bindNativeObject(nativeObjName); const result = nativeObject[key].apply(window, arguments); @@ -23,4 +38,12 @@ export function createPropertiesProxy(rootElement: Element, objProperties: {}, n } }); return proxy; -} \ No newline at end of file +} + +/** + * A dropped call is expected while a view is being taken down, but one arriving long after that means + * something is still holding on to a released view, and that is a bug worth finding. + */ +function logUnboundCall(nativeObjName: string, key: string, reason: string): void { + window.console.warn(`Ignored call to "${nativeObjName}.${key}"`, reason); +} diff --git a/ReactViewResources/Loader/Loader.ts b/ReactViewResources/Loader/Loader.ts index 5d8488d7..3ffb4985 100644 --- a/ReactViewResources/Loader/Loader.ts +++ b/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 { setEnsureViewPluginsAreDisposedFlag, setLoadScriptsOncePerDocumentFlag } from "./Internal/Flags"; +import { setBailOutOnUnboundNativeObjectCallsFlag, setEnsureViewPluginsAreDisposedFlag, setLoadScriptsOncePerDocumentFlag } from "./Internal/Flags"; export { disableMouseInteractions, enableMouseInteractions } from "./Internal/InputManager"; export { showErrorMessage } from "./Internal/MessagesProvider"; @@ -139,7 +139,8 @@ export function loadComponent( componentHash: string, ensureDisposeInnerViews: boolean, loadScriptsOncePerDocument: boolean, - ensureViewPluginsAreDisposed: boolean): void { + ensureViewPluginsAreDisposed: boolean, + bailOutOnUnboundNativeObjectCalls: boolean): void { async function innerLoad() { let view: ViewMetadata; @@ -155,6 +156,7 @@ export function loadComponent( // script or is taken down setLoadScriptsOncePerDocumentFlag(loadScriptsOncePerDocument); setEnsureViewPluginsAreDisposedFlag(ensureViewPluginsAreDisposed); + setBailOutOnUnboundNativeObjectCallsFlag(bailOutOnUnboundNativeObjectCalls); } view = tryGetView(frameName)!; @@ -192,7 +194,7 @@ export function loadComponent( 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); + const properties = createPropertiesProxy(rootElement, componentNativeObject, componentNativeObjectName, view, renderFinishedTask); view.nativeObjectNames.push(componentNativeObjectName); // add to the native objects collection const componentClass = (getViewModule(componentName) || {}).default; diff --git a/Tests.ReactView/InnerViewModule.cs b/Tests.ReactView/InnerViewModule.cs index edea4843..5eeb58f1 100644 --- a/Tests.ReactView/InnerViewModule.cs +++ b/Tests.ReactView/InnerViewModule.cs @@ -21,12 +21,19 @@ public void Loaded() { public void MethodCalled(bool contextLoaded) { Owner.MethodCalled?.Invoke(contextLoaded); } + + public string ValueReturningMethodCalled(bool contextLoaded) { + Owner.ValueReturningMethodCalled?.Invoke(contextLoaded); + return nameof(ValueReturningMethodCalled); + } } public event Action Loaded; public event Action MethodCalled; + public event Action ValueReturningMethodCalled; + public void TestMethod() { ExecutionEngine.ExecuteMethod(this, "testMethod"); } @@ -41,7 +48,7 @@ protected override object CreateNativeObject() { return new Properties(this); } - protected override string[] Events => new[] { "loaded", "methodCalled" }; + protected override string[] Events => new[] { "loaded", "methodCalled", "valueReturningMethodCalled" }; } } diff --git a/Tests.ReactView/TestAppView/InnerView.tsx b/Tests.ReactView/TestAppView/InnerView.tsx index 85f5997c..f1728399 100644 --- a/Tests.ReactView/TestAppView/InnerView.tsx +++ b/Tests.ReactView/TestAppView/InnerView.tsx @@ -4,6 +4,7 @@ import { ViewSharedContext } from 'ViewFrame'; interface IInnerViewProperties { loaded: () => void; methodCalled: (contextLoaded: boolean) => void; + valueReturningMethodCalled: (contextLoaded: boolean) => Promise; } interface IInnerViewBehaviors { @@ -15,6 +16,9 @@ export default class InnerView extends React.Component private sharedContextLoaded = false; componentDidMount() { + // kept around on purpose, so that a test can call into this view's native object after the view + // itself is gone + (window as any).InnerViewProperties = this.props; this.props.loaded(); } diff --git a/Tests.ReactView/TestAppView/TestApp.tsx b/Tests.ReactView/TestAppView/TestApp.tsx index 043a08e1..50c2f48e 100644 --- a/Tests.ReactView/TestAppView/TestApp.tsx +++ b/Tests.ReactView/TestAppView/TestApp.tsx @@ -119,6 +119,34 @@ class App extends React.Component { return (window as any).DisposedPluginModules; } + callInnerViewNativeMethod(nativeObjectNameToUnbind?: string) { + this.callInnerViewNativeMethodCore(properties => properties.methodCalled(true), nativeObjectNameToUnbind); + } + + callInnerViewNativeValueMethod(nativeObjectNameToUnbind?: string) { + this.callInnerViewNativeMethodCore(properties => properties.valueReturningMethodCalled(true), nativeObjectNameToUnbind); + } + + private callInnerViewNativeMethodCore(call: (properties: any) => Promise, nativeObjectNameToUnbind?: string) { + const innerViewProperties = (window as any).InnerViewProperties; + + if (nativeObjectNameToUnbind) { + // leaves the js side as unregistering the object does: gone from the window, while its binding + // task stays behind resolved, so binding it again succeeds and hands back nothing + delete (window as any)[nativeObjectNameToUnbind]; + + if ((window as any)[nativeObjectNameToUnbind] !== undefined) { + this.props.event("NativeObjectStillBound"); + return; + } + } + + call(innerViewProperties).then( + () => this.props.event("CallCompleted"), + (error: any) => this.props.event("CallFailed: " + ((error && error.message) || error)) + ); + } + loadCustomResource(url: string) { console.log(url); var img = document.createElement("img"); diff --git a/Tests.ReactView/UnboundNativeObjectCallsTests.cs b/Tests.ReactView/UnboundNativeObjectCallsTests.cs new file mode 100644 index 00000000..cb06d27a --- /dev/null +++ b/Tests.ReactView/UnboundNativeObjectCallsTests.cs @@ -0,0 +1,160 @@ +using System.Threading.Tasks; +using NUnit.Framework; +using ReactViewControl; + +namespace Tests.ReactView { + + public abstract class UnboundNativeObjectCallsTestsBase : ReactViewTestBase { + + /// + /// The name the inner view native object is registered under, as built by GetNativeObjectFullName + /// for the "test" frame. + /// + protected const string InnerViewNativeObjectName = "$test$InnerViewModule"; + + protected const string CallReachedNativeObject = "CallReachedNativeObject"; + + /// + /// The test app methods that call into the inner view native object: one reaching a method that + /// returns nothing, the other one a method that returns a value. + /// + protected const string CallVoidNativeMethod = "callInnerViewNativeMethod"; + + protected const string CallValueReturningNativeMethod = "callInnerViewNativeValueMethod"; + + protected override void InitializeView() { + if (TargetView != null) { + TargetView.AutoShowInnerView = true; + } + base.InitializeView(); + } + + protected async Task LoadInnerView() { + var innerViewLoaded = new TaskCompletionSource(); + TargetView.InnerView.Loaded += () => innerViewLoaded.TrySetResult(true); +#if DEBUG + TargetView.Ready += () => TargetView.InnerView.Load(); +#endif + TargetView.InnerView.Load(); + await innerViewLoaded.Task; + } + + /// + /// Calls a method of the inner view native object after unbinding it, leaving the view itself alive, + /// and returns what came out of the call: the result reported by the view, or + /// CallReachedNativeObject if the call went through. + /// + protected async Task CallUnboundInnerViewNativeMethod(string viewMethod = CallVoidNativeMethod) { + await LoadInnerView(); + + var callResult = new TaskCompletionSource(); + TargetView.Event += result => callResult.TrySetResult(result); + ObserveCallsReachingNativeObject(callResult); + + TargetView.ExecuteMethod(viewMethod, InnerViewNativeObjectName); + + return await callResult.Task; + } + + /// + /// Calls a method of the inner view native object after destroying the view that owns it, and + /// returns what came out of the call, as CallUnboundInnerViewNativeMethod does. + /// + protected async Task CallDestroyedInnerViewNativeMethod(string viewMethod = CallVoidNativeMethod) { + await LoadInnerView(); + + var innerViewHidden = new TaskCompletionSource(); + var callResult = new TaskCompletionSource(); + TargetView.Event += result => { + if (result == "InnerViewHidden") { + innerViewHidden.TrySetResult(true); + } else { + callResult.TrySetResult(result); + } + }; + ObserveCallsReachingNativeObject(callResult); + + // 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; + + TargetView.ExecuteMethod(viewMethod); + + return await callResult.Task; + } + + private void ObserveCallsReachingNativeObject(TaskCompletionSource callResult) { + TargetView.InnerView.MethodCalled += _ => callResult.TrySetResult(CallReachedNativeObject); + TargetView.InnerView.ValueReturningMethodCalled += _ => callResult.TrySetResult(CallReachedNativeObject); + } + } + + public class UnboundNativeObjectCallsTests : UnboundNativeObjectCallsTestsBase { + + [Test(Description = "Tests that a call to the native object of a destroyed view is ignored")] + public async Task CallToDestroyedViewNativeObjectIsIgnored() { + await Run(async () => { + var callResult = await CallDestroyedInnerViewNativeMethod(); + + Assert.AreEqual("CallCompleted", callResult, "The call to the destroyed view native object was not ignored!"); + }); + } + + [Test(Description = "Tests that a value returning call to the native object of a destroyed view is ignored as well: nobody is left to read the result")] + public async Task ValueReturningCallToDestroyedViewNativeObjectIsIgnored() { + await Run(async () => { + var callResult = await CallDestroyedInnerViewNativeMethod(CallValueReturningNativeMethod); + + Assert.AreEqual("CallCompleted", callResult, "The value returning call to the destroyed view native object was not ignored!"); + }); + } + + [Test(Description = "Tests that a call to a native object that is no longer bound, made by a view that is still alive, fails instead of being silently dropped")] + public async Task CallToUnboundNativeObjectOfLiveViewFails() { + await Run(async () => { + var callResult = await CallUnboundInnerViewNativeMethod(); + + Assert.That(callResult, Does.StartWith("CallFailed"), "The call of a live view to its unbound native object did not fail!"); + Assert.That(callResult, Does.Contain("methodCalled"), "The failure does not say which method was called!"); + }); + } + + [Test(Description = "Tests that a value returning call to a native object that is no longer bound, made by a view that is still alive, fails and says which method it was")] + public async Task ValueReturningCallToUnboundNativeObjectOfLiveViewFails() { + await Run(async () => { + var callResult = await CallUnboundInnerViewNativeMethod(CallValueReturningNativeMethod); + + Assert.That(callResult, Does.StartWith("CallFailed"), "The value returning call of a live view to its unbound native object did not fail!"); + Assert.That(callResult, Does.Contain("valueReturningMethodCalled"), "The failure does not say which method was called!"); + }); + } + } + + public class UnboundNativeObjectCallsWithoutBailOutTests : UnboundNativeObjectCallsTestsBase { + + private class ViewFactoryWithoutBailOut : TestReactViewFactory { + + public override bool BailOutOnUnboundNativeObjectCalls => false; + } + + private class ReactViewWithoutBailOut : TestReactView { + + protected override ReactViewFactory Factory => new ViewFactoryWithoutBailOut(); + } + + protected override TestReactView CreateView() { + TestReactView.PreloadedCacheEntriesSize = 0; // disable cache during tests + return new ReactViewWithoutBailOut(); + } + + [Test(Description = "Tests that a call to the native object of a destroyed view fails when bailing out is disabled")] + public async Task CallToDestroyedViewNativeObjectFails() { + await Run(async () => { + var callResult = await CallDestroyedInnerViewNativeMethod(); + + Assert.That(callResult, Does.StartWith("CallFailed"), "The call to the destroyed view native object did not fail!"); + }); + } + } +}