Cache JS->.NET string decoding results within a single renderbatch (#23773)

Author: SteveSandersonMS

src/Components/Web.JS/dist/Release/blazor.server.js

@@ -89,7 +89,7 @@ async function initializeConnection(options: CircuitStartOptions, logger: Logger
   const connection = connectionBuilder.build();
 
   setEventDispatcher((descriptor, args) => {
-    return connection.send('DispatchBrowserEvent', JSON.stringify(descriptor), JSON.stringify(args));
+    connection.send('DispatchBrowserEvent', JSON.stringify(descriptor), JSON.stringify(args));
   });
 
   // Configure navigation via SignalR

src/Components/Web.JS/dist/Release/blazor.webassembly.js

@@ -22,14 +22,32 @@ async function boot(options?: Partial<WebAssemblyStartOptions>): Promise<void> {
   }
   started = true;
 
-  setEventDispatcher((eventDescriptor, eventArgs) => DotNet.invokeMethodAsync('Microsoft.AspNetCore.Components.WebAssembly', 'DispatchEvent', eventDescriptor, JSON.stringify(eventArgs)));
+  setEventDispatcher((eventDescriptor, eventArgs) => {
+    // It's extremely unusual, but an event can be raised while we're in the middle of synchronously applying a
+    // renderbatch. For example, a renderbatch might mutate the DOM in such a way as to cause an <input> to lose
+    // focus, in turn triggering a 'change' event. It may also be possible to listen to other DOM mutation events
+    // that are themselves triggered by the application of a renderbatch.
+    monoPlatform.invokeWhenHeapUnlocked(() => DotNet.invokeMethodAsync('Microsoft.AspNetCore.Components.WebAssembly', 'DispatchEvent', eventDescriptor, JSON.stringify(eventArgs)));
+  });
 
   // Configure environment for execution under Mono WebAssembly with shared-memory rendering
   const platform = Environment.setPlatform(monoPlatform);
   window['Blazor'].platform = platform;
   window['Blazor']._internal.renderBatch = (browserRendererId: number, batchAddress: Pointer) => {
     profileStart('renderBatch');
-    renderBatch(browserRendererId, new SharedMemoryRenderBatch(batchAddress));
+
+    // We're going to read directly from the .NET memory heap, so indicate to the platform
+    // that we don't want anything to modify the memory contents during this time. Currently this
+    // is only guaranteed by the fact that .NET code doesn't run during this time, but in the
+    // future (when multithreading is implemented) we might need the .NET runtime to understand
+    // that GC compaction isn't allowed during this critical section.
+    const heapLock = monoPlatform.beginHeapLock();
+    try {
+      renderBatch(browserRendererId, new SharedMemoryRenderBatch(batchAddress));
+    } finally {
+      heapLock.release();
+    }
+
     profileEnd('renderBatch');
   };
 

src/Components/Web.JS/src/Boot.Server.ts

@@ -2,17 +2,18 @@ import { DotNet } from '@microsoft/dotnet-js-interop';
 import { attachDebuggerHotkey, hasDebuggingEnabled } from './MonoDebugger';
 import { showErrorNotification } from '../../BootErrors';
 import { WebAssemblyResourceLoader, LoadingResource } from '../WebAssemblyResourceLoader';
-import { Platform, System_Array, Pointer, System_Object, System_String } from '../Platform';
+import { Platform, System_Array, Pointer, System_Object, System_String, HeapLock } from '../Platform';
 import { loadTimezoneData } from './TimezoneDataFile';
 import { WebAssemblyBootResourceType } from '../WebAssemblyStartOptions';
 import { initializeProfiling } from '../Profiling';
 
-let mono_string_get_utf8: (managedString: System_String) => Pointer;
 let mono_wasm_add_assembly: (name: string, heapAddress: number, length: number) => void;
 const appBinDirName = 'appBinDir';
 const uint64HighOrderShift = Math.pow(2, 32);
 const maxSafeNumberHighPart = Math.pow(2, 21) - 1; // The high-order int32 from Number.MAX_SAFE_INTEGER
 
+let currentHeapLock: MonoHeapLock | null = null;
+
 // Memory access helpers
 // The implementations are exactly equivalent to what the global getValue(addr, type) function does,
 // except without having to parse the 'type' parameter, and with less risk of mistakes at the call site
@@ -124,12 +125,38 @@ export const monoPlatform: Platform = {
       return unboxedValue;
     }
 
-    return BINDING.conv_string(fieldValue as any as System_String);
+    let decodedString: string | null | undefined;
+    if (currentHeapLock) {
+      decodedString = currentHeapLock.stringCache.get(fieldValue);
+      if (decodedString === undefined) {
+        decodedString = BINDING.conv_string(fieldValue as any as System_String);
+        currentHeapLock.stringCache.set(fieldValue, decodedString);
+      }
+    } else {
+      decodedString = BINDING.conv_string(fieldValue as any as System_String);
+    }
+
+    return decodedString;
   },
 
   readStructField: function readStructField<T extends Pointer>(baseAddress: Pointer, fieldOffset?: number): T {
     return ((baseAddress as any as number) + (fieldOffset || 0)) as any as T;
   },
+
+  beginHeapLock: function() {
+    assertHeapIsNotLocked();
+    currentHeapLock = new MonoHeapLock();
+    return currentHeapLock;
+  },
+
+  invokeWhenHeapUnlocked: function(callback) {
+    // This is somewhat like a sync context. If we're not locked, just pass through the call directly.
+    if (!currentHeapLock) {
+      callback();
+    } else {
+      currentHeapLock.enqueuePostReleaseAction(callback);
+    }
+  }
 };
 
 function addScriptTagsToDocument(resourceLoader: WebAssemblyResourceLoader) {
@@ -246,7 +273,6 @@ function createEmscriptenModuleInstance(resourceLoader: WebAssemblyResourceLoade
   module.preRun.push(() => {
     // By now, emscripten should be initialised enough that we can capture these methods for later use
     mono_wasm_add_assembly = cwrap('mono_wasm_add_assembly', null, ['string', 'number', 'number']);
-    mono_string_get_utf8 = cwrap('mono_wasm_string_get_utf8', 'number', ['number']);
     MONO.loaded_files = [];
 
     if (timeZoneResource) {
@@ -387,6 +413,7 @@ function attachInteropInvoker(): void {
 
   DotNet.attachDispatcher({
     beginInvokeDotNetFromJS: (callId: number, assemblyName: string | null, methodIdentifier: string, dotNetObjectId: any | null, argsJson: string): void => {
+      assertHeapIsNotLocked();
       if (!dotNetObjectId && !assemblyName) {
         throw new Error('Either assemblyName or dotNetObjectId must have a non null value.');
       }
@@ -409,6 +436,7 @@ function attachInteropInvoker(): void {
       );
     },
     invokeDotNetFromJS: (assemblyName, methodIdentifier, dotNetObjectId, argsJson) => {
+      assertHeapIsNotLocked();
       return dotNetDispatcherInvokeMethodHandle(
         assemblyName ? assemblyName : null,
         methodIdentifier,
@@ -460,3 +488,42 @@ function changeExtension(filename: string, newExtensionWithLeadingDot: string) {
 
   return filename.substr(0, lastDotIndex) + newExtensionWithLeadingDot;
 }
+
+function assertHeapIsNotLocked() {
+  if (currentHeapLock) {
+    throw new Error('Assertion failed - heap is currently locked');
+  }
+}
+
+class MonoHeapLock implements HeapLock {
+  // Within a given heap lock, it's safe to cache decoded strings since the memory can't change
+  stringCache = new Map<number, string | null>();
+
+  private postReleaseActions?: Function[];
+
+  enqueuePostReleaseAction(callback: Function) {
+    if (!this.postReleaseActions) {
+      this.postReleaseActions = [];
+    }
+
+    this.postReleaseActions.push(callback);
+  }
+
+  release() {
+    if (currentHeapLock !== this) {
+      throw new Error('Trying to release a lock which isn\'t current');
+    }
+
+    currentHeapLock = null;
+
+    while (this.postReleaseActions?.length) {
+      const nextQueuedAction = this.postReleaseActions.shift()!;
+
+      // It's possible that the action we invoke here might itself take a succession of heap locks,
+      // but since heap locks must be released synchronously, by the time we get back to this stack
+      // frame, we know the heap should no longer be locked.
+      nextQueuedAction();
+      assertHeapIsNotLocked();
+    }
+  }
+}

src/Components/Web.JS/src/Boot.WebAssembly.ts

@@ -18,6 +18,13 @@ export interface Platform {
   readObjectField<T extends System_Object>(baseAddress: Pointer, fieldOffset?: number): T;
   readStringField(baseAddress: Pointer, fieldOffset?: number, readBoolValueAsString?: boolean): string | null;
   readStructField<T extends Pointer>(baseAddress: Pointer, fieldOffset?: number): T;
+
+  beginHeapLock(): HeapLock;
+  invokeWhenHeapUnlocked(callback: Function): void;
+}
+
+export interface HeapLock {
+  release();
 }
 
 // We don't actually instantiate any of these at runtime. For perf it's preferable to

src/Components/Web.JS/src/Platform/Mono/MonoPlatform.ts

@@ -10,9 +10,9 @@ export function dispatchEvent(eventDescriptor: EventDescriptor, eventArgs: UIEve
     throw new Error('eventDispatcher not initialized. Call \'setEventDispatcher\' to configure it.');
   }
 
-  return eventDispatcherInstance(eventDescriptor, eventArgs);
+  eventDispatcherInstance(eventDescriptor, eventArgs);
 }
 
-export function setEventDispatcher(newDispatcher: (eventDescriptor: EventDescriptor, eventArgs: UIEventArgs) => Promise<void>): void {
+export function setEventDispatcher(newDispatcher: (eventDescriptor: EventDescriptor, eventArgs: UIEventArgs) => void): void {
   eventDispatcherInstance = newDispatcher;
 }

src/Components/Web.JS/src/Platform/Platform.ts

@@ -239,6 +239,37 @@ public void InteractiveElementWithDisabledAttributeDoesNotRespondToMouseEvents(s
             Browser.Equal("Got event on enabled button", () => eventLog.GetAttribute("value"));
         }
 
+        [Fact]
+        public void EventDuringBatchRendering_CanTriggerDOMEvents()
+        {
+            Browser.MountTestComponent<EventDuringBatchRendering>();
+
+            var input = Browser.FindElements(By.CssSelector("#reversible-list input"))[0];
+            var eventLog = Browser.FindElement(By.Id("event-log"));
+
+            SendKeysSequentially(input, "abc");
+            Browser.Equal("abc", () => input.GetAttribute("value"));
+            Browser.Equal(
+                "Change event on item First with value a\n" +
+                "Change event on item First with value ab\n" +
+                "Change event on item First with value abc",
+                () => eventLog.Text.Trim().Replace("\r\n", "\n"));
+        }
+
+        [Fact]
+        public void EventDuringBatchRendering_CannotTriggerJSInterop()
+        {
+            Browser.MountTestComponent<EventDuringBatchRendering>();
+            var errorLog = Browser.FindElement(By.Id("web-component-error-log"));
+
+            Browser.FindElement(By.Id("add-web-component")).Click();
+            var expectedMessage = _serverFixture.ExecutionMode == ExecutionMode.Client
+                ? "Assertion failed - heap is currently locked"
+                : "There was an exception invoking 'SomeMethodThatDoesntNeedToExistForThisTest' on assembly 'SomeAssembly'";
+
+            Browser.Contains(expectedMessage, () => errorLog.Text);
+        }
+
         void SendKeysSequentially(IWebElement target, string text)
         {
             // Calling it for each character works around some chars being skipped

src/Components/Web.JS/src/Rendering/RendererEventDispatcher.ts

@@ -0,0 +1,70 @@
+<h1>Event during batch rendering</h1>
+
+<p>
+    While Blazor WebAssembly is rendering a batch, the JavaScript code reads data from the .NET heap directly.
+    So, it's essential that .NET code doesn't run during this time (either to modify the state of the
+    render tree or to perform garbage collection which may relocate objects in the heap).
+</p>
+<p>
+    To ensure this is safe, batch rendering is a fully synchronous process during which the JS code doesn't
+    yield control to user code. However, there are possible cases where user code may be triggered unavoidably
+    including (1) JavaScript DOM mutation observers, (2) Web Component lifecycle events, and (3) edge cases
+    where Blazor performing a DOM mutation can itself trigger a .NET-bound event such as "change".
+</p>
+<p>
+    Cases (1) and (2) result in developer-supplied JS code executing, which may try to perform .NET interop.
+    The intended behavior is that .NET interop calls should be blocked while a batch is being rendered.
+    Developers need to wrap such calls in <code>requestAnimationFrame</code> or <code>setTimeout(..., 0)</code>
+    or similar, so that it runs after the current batch has finished rendering.
+</p>
+<p>
+    Case (3) more directly results in developer-supplied .NET code executing. The intended behavior in this case
+    is that Blazor takes care of deferring the event dispatch until the current batch finishes rendering. This
+    shouldn't be regarded as problematic, because Blazor has never guaranteed synchronous dispatch of DOM event
+    handlers (in the Blazor Server case, all DOM event handlers run asynchronously).
+</p>
+
+<h2>WebComponent attempting JS interop during batch rendering (cases 1 & 2 above)</h2>
+
+@for (var i = 0; i < numWebComponents; i++)
+{
+    <custom-web-component-performing-js-interop>Instance @i</custom-web-component-performing-js-interop>
+}
+
+<button id="add-web-component" @onclick="@(() => numWebComponents++)">Add a web component</button>
+
+<pre id="web-component-error-log"></pre>
+
+<h2>DOM mutation triggering a .NET event handler (case 3 above)</h2>
+
+<p>
+    Type into either text box. Each keystroke will swap the list order, causing a change event during batch rendering.
+</p>
+
+<div id="reversible-list">
+    @foreach (var item in itemsList)
+    {
+        <div @key="item">
+            <input @oninput="@(() => itemsList.Reverse())"
+                   @onchange="@(evt => eventLog += $"Change event on item {item.Name} with value {evt.Value}\n")" />
+        </div>
+    }
+</div>
+
+<pre id="event-log">@eventLog</pre>
+
+@code {
+    string eventLog = "";
+    int numWebComponents = 0;
+
+    class ListItem
+    {
+        public string Name { get; set; }
+    }
+
+    List<ListItem> itemsList = new List<ListItem>
+    {
+        new ListItem { Name = "First" },
+        new ListItem { Name = "Second" },
+    };
+}

src/Components/test/E2ETest/Tests/EventTest.cs

@@ -27,6 +27,7 @@
         <option value="BasicTestApp.EventCallbackTest.EventCallbackCases">EventCallback</option>
         <option value="BasicTestApp.EventCasesComponent">Event cases</option>
         <option value="BasicTestApp.EventDisablingComponent">Event disabling</option>
+        <option value="BasicTestApp.EventDuringBatchRendering">Event during batch rendering</option>
         <option value="BasicTestApp.EventPreventDefaultComponent">Event preventDefault</option>
         <option value="BasicTestApp.ExternalContentPackage">External content package</option>
         <option value="BasicTestApp.FocusEventComponent">Focus events</option>

src/Components/test/testassets/BasicTestApp/EventDuringBatchRendering.razor

@@ -21,8 +21,9 @@
         <a class='dismiss' style="cursor: pointer;">🗙</a>
     </div>
 
-    <!-- Used for testing interop scenarios between JS and .NET -->
+    <!-- Used for specific test cases -->
     <script src="js/jsinteroptests.js"></script>
+    <script src="js/webComponentPerformingJsInterop.js"></script>
 
     <script>
         // Used by ElementRefComponent

src/Components/test/testassets/BasicTestApp/Index.razor

@@ -0,0 +1,24 @@
+// This web component is used from the EventDuringBatchRendering test case
+
+window.customElements.define('custom-web-component-performing-js-interop', class extends HTMLElement {
+    connectedCallback() {
+        this.attachShadow({ mode: 'open' });
+        this.shadowRoot.innerHTML = `
+            <div style='border: 2px dashed red; margin: 10px 0; padding: 5px; background: #dddddd;'>
+                <slot></slot>
+            </div>
+        `;
+
+        // Since this happens during batch rendering, it will be blocked.
+        // In the future we could allow async calls, but this is enough of an edge case
+        // that it doesn't need to be implemented currently. Developers who need to do this
+        // can wrap their interop call in requestAnimationFrame or setTimeout(..., 0).
+        (async function () {
+            try {
+                await DotNet.invokeMethodAsync('SomeAssembly', 'SomeMethodThatDoesntNeedToExistForThisTest');
+            } catch (ex) {
+                document.getElementById('web-component-error-log').innerText += ex.toString() + '\n';
+            }
+        })();
+    }
+});

src/Components/test/testassets/BasicTestApp/wwwroot/index.html

@@ -34,15 +34,17 @@
 
   <Target Name="CopyClientAssetsForTest" BeforeTargets="Build"
           Inputs="..\BasicTestApp\wwwroot\js\jsinteroptests.js;
+                  ..\BasicTestApp\wwwroot\js\webComponentPerformingJsInterop.js;
                   ..\BasicTestApp\wwwroot\NotAComponent.html;
                   ..\BasicTestApp\wwwroot\style.css"
           Outputs="wwwroot\js\jsinteroptests.js;
+                   wwwroot\js\webComponentPerformingJsInterop.js;
                    wwwroot\NotAComponent.html;
                    wwwroot\style.css">
 
     <MakeDir Directories="wwwroot" />
 
-    <Copy SourceFiles="..\BasicTestApp\wwwroot\js\jsinteroptests.js;..\BasicTestApp\wwwroot\NotAComponent.html;..\BasicTestApp\wwwroot\style.css"
-          DestinationFiles="wwwroot\js\jsinteroptests.js;wwwroot\NotAComponent.html;wwwroot\style.css" />
+    <Copy SourceFiles="..\BasicTestApp\wwwroot\js\jsinteroptests.js;..\BasicTestApp\wwwroot\js\webComponentPerformingJsInterop.js;..\BasicTestApp\wwwroot\NotAComponent.html;..\BasicTestApp\wwwroot\style.css"
+          DestinationFiles="wwwroot\js\jsinteroptests.js;wwwroot\js\webComponentPerformingJsInterop.js;wwwroot\NotAComponent.html;wwwroot\style.css" />
   </Target>
 </Project>

src/Components/test/testassets/BasicTestApp/wwwroot/js/webComponentPerformingJsInterop.js

@@ -14,8 +14,9 @@
 <body>
     <root><component type="typeof(BasicTestApp.Index)" render-mode="Server" /></root>
 
-    <!-- Used for testing interop scenarios between JS and .NET -->
+    <!-- Used for specific test cases -->
     <script src="js/jsinteroptests.js"></script>
+    <script src="js/webComponentPerformingJsInterop.js"></script>
 
     <div id="blazor-error-ui">
         An unhandled error has occurred.