Blazor Hybrid CSS Hot Reload Fixes #9645
Conversation
Fixes: #9206 microsoft/CsWinRT#670 and https://task.ms/31565319 have been marked as resolved. This PR removes the existing logic which copies the content into a memory stream, and replaces it with a call to `AsRandomAccessStream()` (`IRandomAccessStream` is still required per the `CoreWebView2` API). I tried out CSS hot reload with this change and things are working as expected. Not sure if there's a particular behavior/technique we can use to verify the validity of this change. Spoke with @Eilon regarding this, and per his recollection things were immediately hanging, and that's why this change was necessitated.
| // 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); |
There was a problem hiding this comment.
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.
src/BlazorWebView/src/SharedSource/AutoCloseOnReadCompleteStream.cs
Outdated
Show resolved
Hide resolved
src/BlazorWebView/src/SharedSource/AutoCloseOnReadCompleteStream.cs
Outdated
Show resolved
Hide resolved
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
| if (bytesRead == 0) | ||
| { | ||
| _baseStream.Close(); | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
TanayParikh
deleted the
taparik/mauiWindowsStopCopyingContentBeforeSendingToWebView
branch
TanayParikh
restored the
taparik/mauiWindowsStopCopyingContentBeforeSendingToWebView
branch
TanayParikh
deleted the
taparik/mauiWindowsStopCopyingContentBeforeSendingToWebView
branch
Adds AutoCloseOnReadCompleteStream to close out the content stream upon read completion.
Fixes #7479
If the stream content is supplemented by the static hot reload manager, ensures the initial stream is closed out.
Fixes #9197