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

WebView2 hangs on reading final 0 bytes via NetFxToWinRtStreamAdapter #670

Closed
Scottj1s opened this issue Jan 15, 2021 · 9 comments · Fixed by #1131
Closed

WebView2 hangs on reading final 0 bytes via NetFxToWinRtStreamAdapter #670

Scottj1s opened this issue Jan 15, 2021 · 9 comments · Fixed by #1131
Assignees
Labels
bug Something isn't working fixed Issue has been fixed in an upcoming or existing release
Milestone

Comments

@Scottj1s
Copy link
Member

Scottj1s commented Jan 15, 2021

Originally opened on WebView2:
MicrosoftEdge/WebView2Feedback#806

But may be related to the cswinrt adapter code.

Internal tracking: https://task.ms/31565319

@Scottj1s Scottj1s added the bug Something isn't working label Jan 15, 2021
@Scottj1s Scottj1s added this to the Release 1.1.1 milestone Jan 20, 2021
@Scottj1s Scottj1s added the pri-0 Blocking/issue has no workaround label Jan 20, 2021
@j0shuams
Copy link
Contributor

This is not a C#/WinRT issue exactly -- discussion is still going on offline about how to proceed. Seems the most appropriate fix is on the OS side of things.

@j0shuams
Copy link
Contributor

Removing blocking label as we have a workaround for this:

var stream = new InMemoryRandomAccessStream();
await stream.WriteAsync(Encoding.UTF8.GetBytes($"<html><body><div>Hello, WebView2!</div></body></html>").AsBuffer());

@j0shuams j0shuams removed the pri-0 Blocking/issue has no workaround label Mar 17, 2021
@angelazhangmsft angelazhangmsft added the pri-2 Non-critical bugs/acceptable workaround label Apr 23, 2021
@Eilon
Copy link
Member

Eilon commented Jun 3, 2021

Do we have any update on this? Right now the workaround is to copy all the stream content to a new stream, which is not a great solution due to performance problems.

@manodasanW
Copy link
Member

I hope to spend some time looking at this soon. Need to investigate whether we can remove our synchronization part of the task code if our delegates are already agile.

@angelazhangmsft angelazhangmsft removed the pri-2 Non-critical bugs/acceptable workaround label Jun 3, 2021
@mqudsi
Copy link

mqudsi commented Jul 5, 2021

FYI This only happens with CsWinRt, when the same code is executed in a UAP environment it does not hang. ###

@mqudsi
Copy link

mqudsi commented Jul 14, 2021

@j0shuams can you explain on a technical level why InMemoryRandomAccessStream() works here? I saw your comment a couple of weeks ago and figured "oh, I can just use new MemoryStream(), copy to it, then invoke AsRandomAccessStream() and spent hours trying to get it to work before trying to use a WinRT native new InMemoryRandomAccessStream() instead, which worked immediately (as a workaround).

The documentation on WebView2's threading model states that any streams passed to CreateWebResourceResponse() must be created on a background STA thread, but that doesn't help, e.g. this still hangs:

webView.CoreWebView2.WebResourceRequested += async (s, e) =>
{
    using var deferral = e.GetDeferral();

    var url = e.Request.Uri;
    var id = url.Replace("http://attachments/", "").Replace('/', '\\');
    id = WebUtility.UrlDecode(path);
    var path = TranslateAttachmentIdToPath(id);

    if (attachment is null)
    {
        e.Response = webHistory.CoreWebView2.Environment.CreateWebResourceResponse(null, 404, "Not found", "");
        return;
    }

    TaskCompletionSource<IRandomAccessStream> completion = new ();
    var t = new Thread(() =>
    {
        var stream = File.Open(path).AsRandomAccessStream();
        completion.SetResult(stream);
        // no clue how long this thread should live for, but it doesn't seem to matter
    });
    t.IsBackground = true;
    t.SetApartmentState(ApartmentState.STA);
    t.Start();

    var stream = await completion.Task;

    e.Response = webView.CoreWebView2.Environment.CreateWebResourceResponse(stream, 200, "Found", "");
    deferral.Complete();
}

I'm also not aware that either of new Stream() or stream.AsRandomAccessStream() are thread-specific, as the first can be shared across .NET threads freely and as for the second, MSDN states that RandomAccessStream is agile (but doesn't state anything about IRandomAccessStream one way or the other). There's no documentation that I can find anywhere on the WinRT/CsWinRT-specific marshalling/interop extension AsRandomAccessStream() besides this decaying document on GitHub which doesn't go into actual implementation details.

It seems that the IRandomAccessStream passed to CreateWebResourceResponse must be a native WinRT random access stream and not anything else converted to an IRandomAccessStream via .AsRandomAccessStream() or else the COM threads will lock up.

Can you or @angelazhangmsft confirm whether this is the same issue that is being tracked internally, or if I should open this as a separate bug either in the CsWinRT repo or the WebView2Feedback repo (where a similar but not identical bug was closed after saying it was being tracked here)?

@mqudsi
Copy link

mqudsi commented Nov 21, 2021

@j0shuams @angelazhangmsft

Can you or @angelazhangmsft confirm whether this is the same issue that is being tracked internally, or if I should open this as a separate bug either in the CsWinRT repo or the WebView2Feedback repo (where a similar but not identical bug was closed after saying it was being tracked here)?

@angelazhangmsft
Copy link
Contributor

@mqudsi - yes, this is being tracked internally (added internal link to the issue above). The issue you mentioned above is the same issue being tracked here.

@mqudsi
Copy link

mqudsi commented Nov 22, 2021

Thanks, @angelazhangmsft

@angelazhangmsft angelazhangmsft added the fixed Issue has been fixed in an upcoming or existing release label Mar 15, 2022
pull bot pushed a commit to dandycheung/maui that referenced this issue Aug 8, 2022
Fixes: dotnet#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.
TanayParikh added a commit to dotnet/maui that referenced this issue Aug 27, 2022
* Stop copying content stream in .NET MAUI Blazor Windows

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.

* AutoCloseOnReadCompleteStream

* Ensure old response content is closed out for hot reload

* PR Feedback
TanayParikh added a commit to dotnet/maui that referenced this issue Aug 30, 2022
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixed Issue has been fixed in an upcoming or existing release
Projects
None yet
7 participants