Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Blazor Hybrid CSS Hot Reload Fixes #9645

Merged
merged 4 commits into from
Aug 27, 2022
Merged

Blazor Hybrid CSS Hot Reload Fixes #9645

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f0114eb
Stop copying content stream in .NET MAUI Blazor Windows
TanayParikh Aug 8, 2022
8dd1316
AutoCloseOnReadCompleteStream
TanayParikh Aug 25, 2022
ebdfdf4
Ensure old response content is closed out for hot reload
TanayParikh Aug 25, 2022
122426c
PR Feedback
TanayParikh Aug 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Microsoft.Maui.sln
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "MauiBlazorWebView.DeviceTes
EndProject
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "SharedSource", "SharedSource", "{4F2926C8-43AB-4328-A735-D9EAD699F81D}"
ProjectSection(SolutionItems) = preProject
src\BlazorWebView\src\SharedSource\AutoCloseOnReadCompleteStream.cs = src\BlazorWebView\src\SharedSource\AutoCloseOnReadCompleteStream.cs
src\BlazorWebView\src\SharedSource\QueryStringHelper.cs = src\BlazorWebView\src\SharedSource\QueryStringHelper.cs
src\BlazorWebView\src\SharedSource\UrlLoadingEventArgs.cs = src\BlazorWebView\src\SharedSource\UrlLoadingEventArgs.cs
src\BlazorWebView\src\SharedSource\UrlLoadingStrategy.cs = src\BlazorWebView\src\SharedSource\UrlLoadingStrategy.cs
Expand Down
9 changes: 1 addition & 8 deletions src/BlazorWebView/src/Maui/Windows/WinUIWebViewManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,15 +83,8 @@ protected override async Task HandleWebResourceRequest(CoreWebView2WebResourceRe
if (TryGetResponseContent(requestUri, allowFallbackOnHostPage, out var statusCode, out var statusMessage, out var content, out var headers)
&& statusCode != 404)
{
// NOTE: This is stream copying is to work around a hanging bug in WinRT with managed streams.
// See issue https://github.com/microsoft/CsWinRT/issues/670
var memStream = new MemoryStream();
content.CopyTo(memStream);
var ms = new InMemoryRandomAccessStream();
await ms.WriteAsync(memStream.GetWindowsRuntimeBuffer());

var headerString = GetHeaderString(headers);
eventArgs.Response = _coreWebView2Environment!.CreateWebResourceResponse(ms, statusCode, statusMessage, headerString);
eventArgs.Response = _coreWebView2Environment!.CreateWebResourceResponse(content.AsRandomAccessStream(), statusCode, statusMessage, headerString);
Comment on lines -86 to +87
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is already in main: https://github.com/dotnet/maui/blob/main/src/BlazorWebView/src/Maui/Windows/WinUIWebViewManager.cs#L83-L88

Not sure why GitHub is explicitly displaying it here given the effective 0 diff.

}
else if (new Uri(requestUri) is Uri uri && AppOriginUri.IsBaseOf(uri))
{
Expand Down
51 changes: 51 additions & 0 deletions src/BlazorWebView/src/SharedSource/AutoCloseOnReadCompleteStream.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
#if WEBVIEW2_WINFORMS || WEBVIEW2_WPF

using System.IO;

namespace Microsoft.AspNetCore.Components.WebView.WebView2
{
internal class AutoCloseOnReadCompleteStream : Stream
{
private readonly Stream _baseStream;

public AutoCloseOnReadCompleteStream(Stream baseStream)
{
_baseStream = baseStream;
}

public override bool CanRead => _baseStream.CanRead;

public override bool CanSeek => _baseStream.CanSeek;

public override bool CanWrite => _baseStream.CanWrite;

public override long Length => _baseStream.Length;

public override long Position { get => _baseStream.Position; set => _baseStream.Position = value; }

public override void Flush() => _baseStream.Flush();

public override int Read(byte[] buffer, int offset, int count)
{
var bytesRead = _baseStream.Read(buffer, offset, count);

// Stream.Read only returns 0 when it has reached the end of stream
// and no further bytes are expected. Otherwise it blocks until
// one or more (and at most count) bytes can be read.
if (bytesRead == 0)
{
_baseStream.Close();
}
Comment on lines +35 to +38
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think this check is correct. You might receive a 0 count or an empty buffer and those will result in _baseStream.Read returning 0.

Would it work if instead we compare the current position against the length of the stream? We should be able to leverage here the fact that we are dealing with files on disk.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm that's what I initially had, but it necessitated a bit more complexity to avoid accessing the properties of a closed out stream. I did this based on:

The total number of bytes read into the buffer. This can be less than the number of bytes requested if that many bytes are not currently available, or zero (0) if the end of the stream has been reached.

https://docs.microsoft.com/en-us/dotnet/api/system.io.stream.read?view=net-6.0#system-io-stream-read(system-byte()-system-int32-system-int32)

Implementations return the number of bytes read. The implementation will block until at least one byte of data can be read, in the event that no data is available. Read returns 0 only when there is no more data in the stream and no more is expected (such as a closed socket or end of file). An implementation is free to return fewer bytes than requested even if the end of the stream has not been reached.

Based on this, I believe this should be fine as-is?

Copy link
Contributor Author

@TanayParikh TanayParikh Aug 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the empty buffer is still potentially problematic here, but that almost seems like an issue in the caller. Maybe we just throw an exception in this case?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you not keep track of whether you closed the underlying stream already? Would it also not throw before you get to this check in the call to Read if the internal stream was already closed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am somewhat fine with the additional info provided; those are the types of things that are great comments to add in the code to give context on why that might be Ok to do.

Are there other APIs on the stream that a caller can call instead of just Read that might bypass the check?

Copy link
Contributor Author

@TanayParikh TanayParikh Aug 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there other APIs on the stream that a caller can call instead of just Read that might bypass the check?

As far as I can tell this should be sufficient as it'll serve as the marker for when the end-of-stream is reached on read. I've done some basic testing locally and things are working as expected.

cc/ @yildirimcagri this is a workaround we're implementing for MicrosoftEdge/WebView2Feedback#2513. Essentially wrapping the stream and closing it out when the read is completed. Wanted to get your thoughts on this approach and see if you had any concerns with how the CoreWebView2 interacts with this content stream.


return bytesRead;
}

public override long Seek(long offset, SeekOrigin origin) => _baseStream.Seek(offset, origin);

public override void SetLength(long value) => _baseStream.SetLength(value);

public override void Write(byte[] buffer, int offset, int count) => _baseStream.Write(buffer, offset, count);
}
}

#endif
1 change: 1 addition & 0 deletions src/BlazorWebView/src/SharedSource/StaticContentHotReloadManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ public static bool TryReplaceResponseContent(string contentRootRelativePath, str
if (_updatedContent.TryGetValue((assemblyName, relativePath), out var values))
{
responseStatusCode = 200;
responseContent.Close();
responseContent = new MemoryStream(values.Content);
if (!string.IsNullOrEmpty(values.ContentType))
{
Expand Down
5 changes: 4 additions & 1 deletion src/BlazorWebView/src/SharedSource/WebView2WebViewManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,10 @@ protected virtual Task HandleWebResourceRequest(CoreWebView2WebResourceRequested
StaticContentHotReloadManager.TryReplaceResponseContent(_contentRootRelativeToAppRoot, requestUri, ref statusCode, ref content, headers);

var headerString = GetHeaderString(headers);
eventArgs.Response = _coreWebView2Environment!.CreateWebResourceResponse(content, statusCode, statusMessage, headerString);

var autoCloseStream = new AutoCloseOnReadCompleteStream(content);

eventArgs.Response = _coreWebView2Environment!.CreateWebResourceResponse(autoCloseStream, statusCode, statusMessage, headerString);
}
#elif WEBVIEW2_MAUI
// No-op here because all the work is done in the derived WinUIWebViewManager
Expand Down