From 767a2373c8bbcb72b266314639fc6e89c1931db6 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Mon, 9 Jul 2018 13:36:03 +0100 Subject: [PATCH] Eliminate ElementRef's static incrementing ID for remote rendering cases ... because it's important not to disclose cross-user state, such as the number of IDs that have been assigned. Plus we don't want to run out of unique IDs, which we could if it's limited by the range of an 'int'. --- .../src/Rendering/BrowserRenderer.ts | 2 +- .../src/Rendering/ElementReferenceCapture.ts | 8 ++-- .../src/Rendering/RenderBatch/RenderBatch.ts | 2 +- .../RenderBatch/SharedMemoryRenderBatch.ts | 2 +- .../Circuits/RenderBatchWriter.cs | 2 +- src/Microsoft.AspNetCore.Blazor/ElementRef.cs | 39 +++++++++++++------ .../PlatformInfo.cs | 17 ++++++++ .../RenderTree/RenderTreeFrame.cs | 10 ++--- .../RenderTreeDiffBuilderTest.cs | 10 ++--- .../Circuits/RenderBatchWriterTest.cs | 4 +- 10 files changed, 64 insertions(+), 32 deletions(-) create mode 100644 src/Microsoft.AspNetCore.Blazor/PlatformInfo.cs diff --git a/src/Microsoft.AspNetCore.Blazor.Browser.JS/src/Rendering/BrowserRenderer.ts b/src/Microsoft.AspNetCore.Blazor.Browser.JS/src/Rendering/BrowserRenderer.ts index 09d13db7b9..164c63c4c7 100644 --- a/src/Microsoft.AspNetCore.Blazor.Browser.JS/src/Rendering/BrowserRenderer.ts +++ b/src/Microsoft.AspNetCore.Blazor.Browser.JS/src/Rendering/BrowserRenderer.ts @@ -153,7 +153,7 @@ export class BrowserRenderer { return this.insertFrameRange(batch, componentId, parent, childIndex, frames, frameIndex + 1, frameIndex + frameReader.subtreeLength(frame)); case FrameType.elementReferenceCapture: if (parent instanceof Element) { - applyCaptureIdToElement(parent, frameReader.elementReferenceCaptureId(frame)); + applyCaptureIdToElement(parent, frameReader.elementReferenceCaptureId(frame)!); return 0; // A "capture" is a child in the diff, but has no node in the DOM } else { throw new Error('Reference capture frames can only be children of element frames.'); diff --git a/src/Microsoft.AspNetCore.Blazor.Browser.JS/src/Rendering/ElementReferenceCapture.ts b/src/Microsoft.AspNetCore.Blazor.Browser.JS/src/Rendering/ElementReferenceCapture.ts index 27ea7b0ba2..5d898eaa52 100644 --- a/src/Microsoft.AspNetCore.Blazor.Browser.JS/src/Rendering/ElementReferenceCapture.ts +++ b/src/Microsoft.AspNetCore.Blazor.Browser.JS/src/Rendering/ElementReferenceCapture.ts @@ -1,20 +1,20 @@ -export function applyCaptureIdToElement(element: Element, referenceCaptureId: number) { +export function applyCaptureIdToElement(element: Element, referenceCaptureId: string) { element.setAttribute(getCaptureIdAttributeName(referenceCaptureId), ''); } -function getElementByCaptureId(referenceCaptureId: number) { +function getElementByCaptureId(referenceCaptureId: string) { const selector = `[${getCaptureIdAttributeName(referenceCaptureId)}]`; return document.querySelector(selector); } -function getCaptureIdAttributeName(referenceCaptureId: number) { +function getCaptureIdAttributeName(referenceCaptureId: string) { return `_bl_${referenceCaptureId}`; } // Support receiving ElementRef instances as args in interop calls const elementRefKey = '_blazorElementRef'; // Keep in sync with ElementRef.cs DotNet.attachReviver((key, value) => { - if (value && typeof value === 'object' && value.hasOwnProperty(elementRefKey) && typeof value[elementRefKey] === 'number') { + if (value && typeof value === 'object' && value.hasOwnProperty(elementRefKey) && typeof value[elementRefKey] === 'string') { return getElementByCaptureId(value[elementRefKey]); } else { return value; diff --git a/src/Microsoft.AspNetCore.Blazor.Browser.JS/src/Rendering/RenderBatch/RenderBatch.ts b/src/Microsoft.AspNetCore.Blazor.Browser.JS/src/Rendering/RenderBatch/RenderBatch.ts index e18b54af12..3f95e9f5f7 100644 --- a/src/Microsoft.AspNetCore.Blazor.Browser.JS/src/Rendering/RenderBatch/RenderBatch.ts +++ b/src/Microsoft.AspNetCore.Blazor.Browser.JS/src/Rendering/RenderBatch/RenderBatch.ts @@ -43,7 +43,7 @@ export interface RenderTreeEditReader { export interface RenderTreeFrameReader { frameType(frame: RenderTreeFrame): FrameType; subtreeLength(frame: RenderTreeFrame): number; - elementReferenceCaptureId(frame: RenderTreeFrame): number; + elementReferenceCaptureId(frame: RenderTreeFrame): string | null; componentId(frame: RenderTreeFrame): number; elementName(frame: RenderTreeFrame): string | null; textContent(frame: RenderTreeFrame): string | null; diff --git a/src/Microsoft.AspNetCore.Blazor.Browser.JS/src/Rendering/RenderBatch/SharedMemoryRenderBatch.ts b/src/Microsoft.AspNetCore.Blazor.Browser.JS/src/Rendering/RenderBatch/SharedMemoryRenderBatch.ts index 7ad2186e0a..d6dad6cfbc 100644 --- a/src/Microsoft.AspNetCore.Blazor.Browser.JS/src/Rendering/RenderBatch/SharedMemoryRenderBatch.ts +++ b/src/Microsoft.AspNetCore.Blazor.Browser.JS/src/Rendering/RenderBatch/SharedMemoryRenderBatch.ts @@ -77,7 +77,7 @@ const frameReader = { structLength: 28, frameType: (frame: RenderTreeFrame) => platform.readInt32Field(frame as any, 4) as FrameType, subtreeLength: (frame: RenderTreeFrame) => platform.readInt32Field(frame as any, 8), - elementReferenceCaptureId: (frame: RenderTreeFrame) => platform.readInt32Field(frame as any, 8), + elementReferenceCaptureId: (frame: RenderTreeFrame) => platform.readStringField(frame as any, 16), componentId: (frame: RenderTreeFrame) => platform.readInt32Field(frame as any, 12), elementName: (frame: RenderTreeFrame) => platform.readStringField(frame as any, 16), textContent: (frame: RenderTreeFrame) => platform.readStringField(frame as any, 16), diff --git a/src/Microsoft.AspNetCore.Blazor.Server/Circuits/RenderBatchWriter.cs b/src/Microsoft.AspNetCore.Blazor.Server/Circuits/RenderBatchWriter.cs index 4554018f65..1e892d7b1d 100644 --- a/src/Microsoft.AspNetCore.Blazor.Server/Circuits/RenderBatchWriter.cs +++ b/src/Microsoft.AspNetCore.Blazor.Server/Circuits/RenderBatchWriter.cs @@ -158,7 +158,7 @@ namespace Microsoft.AspNetCore.Blazor.Server.Circuits WritePadding(_binaryWriter, 4); break; case RenderTreeFrameType.ElementReferenceCapture: - _binaryWriter.Write(frame.ElementReferenceCaptureId); + WriteString(frame.ElementReferenceCaptureId); WritePadding(_binaryWriter, 8); break; case RenderTreeFrameType.Region: diff --git a/src/Microsoft.AspNetCore.Blazor/ElementRef.cs b/src/Microsoft.AspNetCore.Blazor/ElementRef.cs index 135853a8ae..011d71e523 100644 --- a/src/Microsoft.AspNetCore.Blazor/ElementRef.cs +++ b/src/Microsoft.AspNetCore.Blazor/ElementRef.cs @@ -2,6 +2,7 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using Microsoft.JSInterop.Internal; +using System; using System.Collections.Generic; using System.Threading; @@ -12,25 +13,18 @@ namespace Microsoft.AspNetCore.Blazor /// public readonly struct ElementRef : ICustomJsonSerializer { - // Static to ensure uniqueness even if there are multiple Renderer instances - // This would not be necessary if the JS-side code maintained a lookup from capureId to Element instances, - // but we're not doing that presently as it causes more work during disposal to remove those entries - // WARNING: Once we support server-side rendering, we should check if running on the server and avoid - // populating element reference capture IDs at all, because doing so could (a) eventually - // overflow the static int, and (b) disclose information to clients about how many other - // requests the server is handling, etc. In general, as part of implementing SSR, we need to - // audit the code it calls for any use of statics. - private static int _nextId = 0; + static long _nextIdForWebAssemblyOnly = 1; - internal int Id { get; } + // The Id is unique at least within the scope of a given user/circuit + internal string Id { get; } - private ElementRef(int id) + private ElementRef(string id) { Id = id; } internal static ElementRef CreateWithUniqueId() - => new ElementRef(Interlocked.Increment(ref _nextId)); + => new ElementRef(CreateUniqueId()); object ICustomJsonSerializer.ToJsonPrimitive() { @@ -39,5 +33,26 @@ namespace Microsoft.AspNetCore.Blazor { "_blazorElementRef", Id } }; } + + static string CreateUniqueId() + { + if (PlatformInfo.IsWebAssembly) + { + // On WebAssembly there's only one user, so it's fine to expose the number + // of IDs that have been assigned, and this is cheaper than creating a GUID. + // It's unfortunate that this still involves a heap allocation. If that becomes + // a problem we could extend RenderTreeFrame to have both "string" and "long" + // fields for ElementRefCaptureId, of which only one would be in use depending + // on the platform. + var id = Interlocked.Increment(ref _nextIdForWebAssemblyOnly); + return id.ToString(); + } + else + { + // For remote rendering, it's important not to disclose any cross-user state, + // such as the number of IDs that have been assigned. + return Guid.NewGuid().ToString("D"); + } + } } } diff --git a/src/Microsoft.AspNetCore.Blazor/PlatformInfo.cs b/src/Microsoft.AspNetCore.Blazor/PlatformInfo.cs new file mode 100644 index 0000000000..1297811490 --- /dev/null +++ b/src/Microsoft.AspNetCore.Blazor/PlatformInfo.cs @@ -0,0 +1,17 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System.Runtime.InteropServices; + +namespace Microsoft.AspNetCore.Blazor +{ + internal static class PlatformInfo + { + public static bool IsWebAssembly { get; } + + static PlatformInfo() + { + IsWebAssembly = RuntimeInformation.IsOSPlatform(OSPlatform.Create("WEBASSEMBLY")); + } + } +} diff --git a/src/Microsoft.AspNetCore.Blazor/RenderTree/RenderTreeFrame.cs b/src/Microsoft.AspNetCore.Blazor/RenderTree/RenderTreeFrame.cs index 8c679a543a..9fc1ea870e 100644 --- a/src/Microsoft.AspNetCore.Blazor/RenderTree/RenderTreeFrame.cs +++ b/src/Microsoft.AspNetCore.Blazor/RenderTree/RenderTreeFrame.cs @@ -137,13 +137,13 @@ namespace Microsoft.AspNetCore.Blazor.RenderTree /// If the property equals , /// gets the ID of the reference capture. Otherwise, the value is undefined. /// - [FieldOffset(8)] public readonly int ElementReferenceCaptureId; + [FieldOffset(16)] public readonly string ElementReferenceCaptureId; /// /// If the property equals , /// gets the action that writes the reference to its target. Otherwise, the value is undefined. /// - [FieldOffset(16)] public readonly Action ElementReferenceCaptureAction; + [FieldOffset(24)] public readonly Action ElementReferenceCaptureAction; // -------------------------------------------------------------------------------- // RenderTreeFrameType.ComponentReferenceCapture @@ -227,7 +227,7 @@ namespace Microsoft.AspNetCore.Blazor.RenderTree RegionSubtreeLength = regionSubtreeLength; } - private RenderTreeFrame(int sequence, Action elementReferenceCaptureAction, int elementReferenceCaptureId) + private RenderTreeFrame(int sequence, Action elementReferenceCaptureAction, string elementReferenceCaptureId) : this() { FrameType = RenderTreeFrameType.ElementReferenceCapture; @@ -264,7 +264,7 @@ namespace Microsoft.AspNetCore.Blazor.RenderTree => new RenderTreeFrame(sequence, regionSubtreeLength: 0); internal static RenderTreeFrame ElementReferenceCapture(int sequence, Action elementReferenceCaptureAction) - => new RenderTreeFrame(sequence, elementReferenceCaptureAction: elementReferenceCaptureAction, elementReferenceCaptureId: 0); + => new RenderTreeFrame(sequence, elementReferenceCaptureAction: elementReferenceCaptureAction, elementReferenceCaptureId: null); internal static RenderTreeFrame ComponentReferenceCapture(int sequence, Action componentReferenceCaptureAction, int parentFrameIndex) => new RenderTreeFrame(sequence, componentReferenceCaptureAction: componentReferenceCaptureAction, parentFrameIndex: parentFrameIndex); @@ -287,7 +287,7 @@ namespace Microsoft.AspNetCore.Blazor.RenderTree internal RenderTreeFrame WithRegionSubtreeLength(int regionSubtreeLength) => new RenderTreeFrame(Sequence, regionSubtreeLength: regionSubtreeLength); - internal RenderTreeFrame WithElementReferenceCaptureId(int elementReferenceCaptureId) + internal RenderTreeFrame WithElementReferenceCaptureId(string elementReferenceCaptureId) => new RenderTreeFrame(Sequence, ElementReferenceCaptureAction, elementReferenceCaptureId); /// diff --git a/test/Microsoft.AspNetCore.Blazor.Test/RenderTreeDiffBuilderTest.cs b/test/Microsoft.AspNetCore.Blazor.Test/RenderTreeDiffBuilderTest.cs index fa9a432348..169299b55f 100644 --- a/test/Microsoft.AspNetCore.Blazor.Test/RenderTreeDiffBuilderTest.cs +++ b/test/Microsoft.AspNetCore.Blazor.Test/RenderTreeDiffBuilderTest.cs @@ -1,4 +1,4 @@ -// Copyright (c) .NET Foundation. All rights reserved. +// Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using Microsoft.AspNetCore.Blazor.Components; @@ -1342,9 +1342,9 @@ namespace Microsoft.AspNetCore.Blazor.Test // Act var (diff, referenceFrames) = GetSingleUpdatedComponent(); - // Assert: Distinct nonzero IDs - Assert.NotEqual(0, ref1.Id); - Assert.NotEqual(0, ref2.Id); + // Assert: Distinct nonnull IDs + Assert.NotNull(ref1.Id); + Assert.NotNull(ref2.Id); Assert.NotEqual(ref1.Id, ref2.Id); // Assert: Also specified in diff @@ -1388,7 +1388,7 @@ namespace Microsoft.AspNetCore.Blazor.Test // Note: We're not preserving the ReferenceCaptureId on the actual RenderTreeFrames in the same // way we do for event handler IDs, simply because there's no need to do so. We only do // anything with ReferenceCaptureId when frames are first inserted into the document. - Assert.NotEqual(0, ref1.Id); + Assert.NotNull(ref1.Id); Assert.Equal(1, refWriteCount); Assert.Empty(diff.Edits); Assert.Empty(referenceFrames); diff --git a/test/Microsoft.AspnetCore.Blazor.Server.Test/Circuits/RenderBatchWriterTest.cs b/test/Microsoft.AspnetCore.Blazor.Server.Test/Circuits/RenderBatchWriterTest.cs index 370c6542b0..277e48a3b2 100644 --- a/test/Microsoft.AspnetCore.Blazor.Server.Test/Circuits/RenderBatchWriterTest.cs +++ b/test/Microsoft.AspnetCore.Blazor.Server.Test/Circuits/RenderBatchWriterTest.cs @@ -194,7 +194,7 @@ namespace Microsoft.AspNetCore.Blazor.Server RenderTreeFrame.Element(128, "Some element") .WithElementSubtreeLength(1234), RenderTreeFrame.ElementReferenceCapture(129, value => { }) - .WithElementReferenceCaptureId(12121), + .WithElementReferenceCaptureId("my unique ID"), RenderTreeFrame.Region(130) .WithRegionSubtreeLength(1234), RenderTreeFrame.Text(131, "Some text"), @@ -212,7 +212,7 @@ namespace Microsoft.AspNetCore.Blazor.Server RenderTreeFrameType.Component, 5678, 2000, 0, RenderTreeFrameType.ComponentReferenceCapture, 0, 0, 0, RenderTreeFrameType.Element, 1234, "Some element", 0, - RenderTreeFrameType.ElementReferenceCapture, 12121, 0, 0, + RenderTreeFrameType.ElementReferenceCapture, "my unique ID", 0, 0, RenderTreeFrameType.Region, 1234, 0, 0, RenderTreeFrameType.Text, "Some text", 0, 0 );