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.

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

CoreWebView2 does not close stream content #2513

Closed
TanayParikh opened this issue Jun 10, 2022 · 22 comments
Closed

CoreWebView2 does not close stream content #2513

TanayParikh opened this issue Jun 10, 2022 · 22 comments
Assignees
Labels
bug Something isn't working tracked We are tracking this work internally.

Comments

@TanayParikh
Copy link

TanayParikh commented Jun 10, 2022

Description

In Blazor WebView (Blazor Hybrid on WinForms & WPF), a content stream given to CoreWebView2 isn't closed by CoreWebView2 after serving the content.

The open stream that was sent to CoreWebView2 leads to Visual Studio being unable to save updates to that file.

_coreWebView2Environment!.CreateWebResourceResponse:

https://github.com/dotnet/maui/blob/d6bc1ca6ff47d49d0ce2d254df7416f0aee52e68/src/BlazorWebView/src/SharedSource/WebView2WebViewManager.cs#L307

_webview.CoreWebView2.WebResourceRequested:

https://github.com/dotnet/maui/blob/d6bc1ca6ff47d49d0ce2d254df7416f0aee52e68/src/BlazorWebView/src/SharedSource/WebView2WebViewManager.cs#L256

Repro Steps

  1. Provide stream content to: https://docs.microsoft.com/en-us/dotnet/api/microsoft.web.webview2.core.corewebview2environment.createwebresourceresponse?view=webview2-dotnet-1.0.1210.39
  2. After reading/consuming the stream to show the content CoreWebView2 does not close the content stream. Expectation is CoreWebView2 closes the stream after serving the content.

Note: if this is expected behavior for CoreWebView2 to not close the content stream, when is it appropriate for the callee to close out the stream? Is there a lifecycle event we can hook into to know when it's acceptable to close the stream?

Additional Context:

Version
SDK: Latest
Runtime: Latest
Framework: WinForms and WPF
OS: Windows 11

AB#40512420

@TanayParikh TanayParikh added the bug Something isn't working label Jun 10, 2022
@TanayParikh
Copy link
Author

TanayParikh commented Jun 21, 2022

Hey @oggy22, just checking if there were any updates or guidance available for this. Please let me know if you need any additional information to assist with the investigation. This is currently blocking hot-reload support for these types of assets for Blazor Hybrid WinForms & WPF applications.

@TanayParikh
Copy link
Author

TanayParikh commented Jul 13, 2022

Hey @oggy22 @victorthoang, just wanted to kindly follow-up on this issue, are there any updates by any chance? This is blocking our hot reload scenarios for Blazor Hybrid WinForms & WPF applications.

@champnic champnic added the tracked We are tracking this work internally. label Jul 18, 2022
@champnic
Copy link
Member

Thanks for reaching out @TanayParikh - I've added this as a bug on our backlog, and also included @yildirimcagri who may have more insight here. Thanks!

@yildirimcagri-msft
Copy link
Member

Hi @TanayParikh , I believe we made a fix in Edge 104 runtime about this, which should be in Beta now. Can you try the WebView2 runtimes with Edge Beta or Canary to see if this fixes your issue?

@TanayParikh
Copy link
Author

@yildirimcagri do you happen to know if there's a 104 WebView2 runtime I can download? I'm only seeing 103 on the download page:

image

Or would downloading the Edge Beta/Canary by itself (without a specific WebView2 standalone runtime) be sufficient?

@yildirimcagri-msft
Copy link
Member

You can download the Edge Beta which is 104 and then specify the environment variable WEBVIEW2_BROWSER_EXECUTABLE_FOLDER to the Edge Beta installation directory and WebView2 will use that runtime.

@TanayParikh
Copy link
Author

image

Tried using just C:\Program Files (x86)\Microsoft\Edge Beta\Application as well. Am I using the right directory? Should I be changing something else?

@champnic
Copy link
Member

Overview: https://docs.microsoft.com/en-us/microsoft-edge/webview2/how-to/set-preview-channel

You'll need the version info as well, as that's the folder that contains the msedgewebview2.exe. You can verify which version is getting picked up by checking the command line of the msedgewebview2.exe child processes in your app, or check the the CoreWebView2Environment.BrowserVersionString property: https://docs.microsoft.com/en-us/dotnet/api/microsoft.web.webview2.core.corewebview2environment.browserversionstring?view=webview2-dotnet-1.0.1264.42

You can also try setting the environment variable within your app.

@TanayParikh
Copy link
Author

Thanks, I set the WEBVIEW2_RELEASE_CHANNEL_PREFERENCE=1 env var in the WinForms app. I verified the correct version is being used via navigator.userAgent (is that sufficient?).

image

However, the issue is still reproducing.

image

@champnic
Copy link
Member

Yes, checking the user agent is sufficient. Sounds like the issue wasn't resolved in your case, so we'll take another look (Cagri is out of the office for a bit). Thanks!

@TanayParikh
Copy link
Author

TanayParikh commented Aug 3, 2022

Hey folks, just wanted to follow-up on this as it's blocking several user scenarios.

Edit: cc/ @yildirimcagri

@plantree
Copy link
Contributor

Hi @TanayParikh
We are paying high priority to this issue. There is one simple way to avoid this, perhaps you can try.
The demo like this: copy your stream to a MemoryStream, and you can dispose or close the origin one.

MemoryStream mStream = new MemoryStream();
fileStream.CopyTo(mStream);
fileStream.Close();

We are trying better solutions. If you have any problems, please let me konw.

@TanayParikh
Copy link
Author

Thanks @plantree, appreciate the investigation & looking forward to hearing more! We'd considered the MemoryStream copy workaround in the past, however decided against it due to performance concerns with having a copy of the content in memory.

@yildirimcagri-msft
Copy link
Member

yildirimcagri-msft commented Aug 18, 2022

Sorry for the confusion, the bug we had fixed was related to the WebView2 in fact leaking the stream object until navigating away. Could you rely on the NavigationCompleted event to know when to close the stream? At that time the stream data should have been definitely read. The issue on our side is that we read the stream on a worker thread asynchronously and we don't want to close the stream that can be reused by the app.

@TanayParikh
Copy link
Author

Could you rely on the NavigationCompleted event to know when to close the stream? At that time the stream data should have been definitely read.

Closing the stream on navigation completion could potentially be problematic in the event resources are loaded at a later point dynamically, and the stream is already closed. This also requires the client to maintain a reference to (every) stream the client sends to the CoreWebView2, till it's possible to close out. Given this, we'd prefer to not go down this particular path.

The issue on our side is that we read the stream on a worker thread asynchronously and we don't want to close the stream that can be reused by the app.

Can you please provide some more details about this. When is that possible / what considerations come into play here? Is the app referring to the client that provides resources to the CoreWebView2 in this case? Just trying to better understand this scenario 😄


As a side note; we're also running into another file locking issue here: dotnet/maui#9197

This is using the WinRT API which uses a IRandomAccessStream​: CoreWebView2WebResourceResponse | Microsoft Docs

this is different from this issue (#2513) which is focused on this api using IStream.

Initially, we didn't see the same locking behavior in .NET MAUI (WinRT / IRandomAccessStream​), as we saw on WinForms/WPF (IStream). In WinForms/WPF, you get a file lock error on the first save. On .NET MAUI you get an exception on the 2-3 save. Given it's still a file locking issue however, I wonder if the underlying cause of these issues could be related?

@yildirimcagri-msft
Copy link
Member

Could you rely on the NavigationCompleted event to know when to close the stream? At that time the stream data should have been definitely read.

Closing the stream on navigation completion could potentially be problematic in the event resources are loaded at a later point dynamically, and the stream is already closed. This also requires the client to maintain a reference to (every) stream the client sends to the CoreWebView2, till it's possible to close out. Given this, we'd prefer to not go down this particular path.

The issue on our side is that we read the stream on a worker thread asynchronously and we don't want to close the stream that can be reused by the app.

Can you please provide some more details about this. When is that possible / what considerations come into play here? Is the app referring to the client that provides resources to the CoreWebView2 in this case? Just trying to better understand this scenario 😄

As a side note; we're also running into another file locking issue here: dotnet/maui#9197

This is using the WinRT API which uses a IRandomAccessStream​: CoreWebView2WebResourceResponse | Microsoft Docs

this is different from this issue (#2513) which is focused on this api using IStream.

Initially, we didn't see the same locking behavior in .NET MAUI (WinRT / IRandomAccessStream​), as we saw on WinForms/WPF (IStream). In WinForms/WPF, you get a file lock error on the first save. On .NET MAUI you get an exception on the 2-3 save. Given it's still a file locking issue however, I wonder if the underlying cause of these issues could be related?

The main consideration is performance. We read the stream in a background thread in native code so the UI thread is not blocked on a network request. However, if the .NET stream is closed before the background thread could read it, we cannot read it from native side. Ideally, we would want to use the using pattern in .NET for the stream, unfortunately there isn't a way for us to retain the stream from the native side when it goes out of scope in .NET. We'd have to copy the stream, which we would likewise want to refrain for perf reasons.
WebResourceResponseReceived should also fire for the requests that were completed via the app via WebResourceRequested event and by that time the stream should be closable. This event could be better as it will give you URL level granularity to know when you can close the specific streams. If this also won't work for you, we can consider providing another callback to report that it is safe to close, however that won't be available until another API release.

In the other issue you mentioned, it doesn't seem like the WebView2 is in play as you are copying the stream to a MemoryStream and passing that instead, if I understood right?

@TanayParikh
Copy link
Author

should be closable

Could you kindly clarify what should means in this context. What scenarios could we inadvertently close the stream before reading?

We read the stream in a background thread in native code so the UI thread is not blocked on a network request. However, if the .NET stream is closed before the background thread could read it, we cannot read it from native side. Ideally, we would want to use the using pattern in .NET for the stream, unfortunately there isn't a way for us to retain the stream from the native side when it goes out of scope in .NET.

To confirm, it's not possible to just close the stream on the native side because the Close API isn't available on IStream?

If this also won't work for you, we can consider providing another callback to report that it is safe to close, however that won't be available until another API release.

Instead of providing this as a public API for consumption by frameworks/clients, could this instead be done internally within the CoreWebView2, so that we don't have to create a new callback / "concept" to manage the streams on the framework/client end?

In the other issue you mentioned, it doesn't seem like the WebView2 is in play as you are copying the stream to a MemoryStream and passing that instead, if I understood right?

We (very) recently stopped copying the stream, but this issue still persists: dotnet/maui#9254

Given the stream is now sent directly to the CoreWebView2 could this be related?

@yildirimcagri-msft
Copy link
Member

should be closable

Could you kindly clarify what should means in this context. What scenarios could we inadvertently close the stream before reading?

Since WebView2 reads the stream asynchronously after the app handles the WebResourceRequested event and sets the response stream, if the app prematurely closes the stream WebView2 will not be able to read the response. By the time WebResourceResponseReceived event fires, the response was already read and processed, so if the app also doesn't need the stream further, it can close it.

We read the stream in a background thread in native code so the UI thread is not blocked on a network request. However, if the .NET stream is closed before the background thread could read it, we cannot read it from native side. Ideally, we would want to use the using pattern in .NET for the stream, unfortunately there isn't a way for us to retain the stream from the native side when it goes out of scope in .NET.

To confirm, it's not possible to just close the stream on the native side because the Close API isn't available on IStream?

Close is available in IStream. However, when the app scopes the stream's lifetime via the using clause such as with

delegate(object sender, CoreWebView2WebResourceRequestedEventArgs args) {
  using (MemoryStream ms = new MemoryStream(file)) {
      response = webViewEnvironment.CreateWebResourceResponse(200, "OK", "", ms);
      args.response = response;
   }
}

unfortunately, this doesn't work as the WebView2 will attempt to read ms after it goes out of scope and is closed. AFAIK, there is no way to prevent this from happening from native side, therefore we guide the apps to rely on either NavigationCompleted or WebResourceResponseReceived to close the streams when they also don't need them.

If this also won't work for you, we can consider providing another callback to report that it is safe to close, however that won't be available until another API release.

Instead of providing this as a public API for consumption by frameworks/clients, could this instead be done internally within the CoreWebView2, so that we don't have to create a new callback / "concept" to manage the streams on the framework/client end?

Automatically closing the stream from WebView2's side would be a breaking change of behavior. Some apps may have been coded in a way to rely on reusing the streams they provide for multiple WebResourceRequested events and would be broken if we close it for them. We'll look into making this an optional setting or under a feature flag.

In the other issue you mentioned, it doesn't seem like the WebView2 is in play as you are copying the stream to a MemoryStream and passing that instead, if I understood right?

We (very) recently stopped copying the stream, but this issue still persists: dotnet/maui#9254

Given the stream is now sent directly to the CoreWebView2 could this be related?

In that case, I agree these issues would be related.

@yildirimcagri-msft
Copy link
Member

Hi, circling back on this with a fresh mind, I can think of a simpler workaround to this problem. When WebView2 reads the WebResourceResponse data stream, it reads it until the end or the Read fails, and does not have any further use to it afterwards.

class ManagedStream : Stream {
    public ManagedStream(Stream s)
    {
        s_ = s;
    }

    public override bool CanRead => s_.CanRead;

    public override bool CanSeek => s_.CanSeek;

    public override bool CanWrite => s_.CanWrite;

    public override long Length => s_.Length;

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

    public override void Flush()
    {
        throw new NotImplementedException();
    }

    public override long Seek(long offset, SeekOrigin origin)
    {
        return s_.Seek(offset, origin);
    }

    public override void SetLength(long value)
    {
        throw new NotImplementedException();
    }

    public override int Read(byte[] buffer, int offset, int count)
    {
        int read = 0;
        try
        {
            read = s_.Read(buffer, offset, count);
            if (read == 0)
            {
                s_.Dispose();
            }
        } catch (Exception e)
        {
            s_.Dispose();
            throw e;
        }
        return read;
    }

    public override void Write(byte[] buffer, int offset, int count)
    {
        throw new NotImplementedException();
    }

   private Stream s_;
}

If you wrap the stream you are passing to WebView with this ManagedStream class, this should ensure that the stream is disposed when WebView is done with it. This should hopefully be a simpler workaround.

@TanayParikh
Copy link
Author

Hey @yildirimcagri, thanks for the comment. That's exactly the workaround we're using presently 😄

dotnet/maui#9645

@yildirimcagri-msft
Copy link
Member

Ohh great, I must have missed that reference. :) To prevent any breaking changes, I think we will retain the behavior here, so we will add this workaround to our guidance with regarding the streams passed to WebView2 that need to be disposed. Closing this issue for now, please let me know if you need a different resolution here.

@atehrani-statrad
Copy link

If this workaround is the long term "fix", why not create an official class in the SDK, say WebResourceStream that has that implementation. Deprecate the method that takes in a Stream and create a new one that only takes in WebResourceStream. I highly doubt most people are fully aware of this issue and have inadvertent memory leaks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working tracked We are tracking this work internally.
Projects
None yet
Development

No branches or pull requests

6 participants