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

[wasm] System.Net.Http.BrowserHttpHandler.SendAsync() throws NRE #53872

Closed
pavelsavara opened this issue Jun 8, 2021 · 10 comments · Fixed by #54453
Closed

[wasm] System.Net.Http.BrowserHttpHandler.SendAsync() throws NRE #53872

pavelsavara opened this issue Jun 8, 2021 · 10 comments · Fixed by #54453
Assignees
Labels
arch-wasm WebAssembly architecture
Milestone

Comments

@pavelsavara
Copy link
Member

pavelsavara commented Jun 8, 2021

In unit test System.Net.Http.Functional.Tests.HttpClientHandler_RemoteServerTest.GetAsync_RequestHeadersAddCustomHeaders_HeaderAndEmptyValueSent(remoteServer: (BaseUri: https://127.0.0.1:61813/, HttpVersion: 1.1) on WASM and browser.

System.NullReferenceException : Object reference not set to an instance of an object.
   at System.Net.Http.BrowserHttpHandler.SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
   at System.Net.Http.HttpClient.<SendAsync>g__Core|83_0(HttpRequestMessage request, HttpCompletionOption completionOption, CancellationTokenSource cts, Boolean disposeCts, CancellationTokenSource pendingRequestsCts, CancellationToken originalCancellationToken)
   at System.Net.Http.Functional.Tests.HttpClientHandler_RemoteServerTest.GetAsync_RequestHeadersAddCustomHeaders_HeaderAndEmptyValueSent(RemoteServer remoteServer, Uri uri)

This is not network related issue, it could be reproduced without BrowserHttpHandler see https://github.com/dotnet/runtime/pull/54453/files

@pavelsavara pavelsavara added arch-wasm WebAssembly architecture area-System.Net.Http labels Jun 8, 2021
@ghost
Copy link

ghost commented Jun 8, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

In unit test System.Net.Http.Functional.Tests.HttpClientHandler_RemoteServerTest.GetAsync_RequestHeadersAddCustomHeaders_HeaderAndEmptyValueSent(remoteServer: (BaseUri: https://127.0.0.1:61813/, HttpVersion: 1.1) on WASM and browser.

System.NullReferenceException : Object reference not set to an instance of an object.
   at System.Net.Http.BrowserHttpHandler.SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
   at System.Net.Http.HttpClient.<SendAsync>g__Core|83_0(HttpRequestMessage request, HttpCompletionOption completionOption, CancellationTokenSource cts, Boolean disposeCts, CancellationTokenSource pendingRequestsCts, CancellationToken originalCancellationToken)
   at System.Net.Http.Functional.Tests.HttpClientHandler_RemoteServerTest.GetAsync_RequestHeadersAddCustomHeaders_HeaderAndEmptyValueSent(RemoteServer remoteServer, Uri uri)
Author: pavelsavara
Assignees: -
Labels:

arch-wasm, area-System.Net.Http

Milestone: -

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Jun 8, 2021
pavelsavara added a commit to pavelsavara/runtime that referenced this issue Jun 8, 2021
- move tests to inner loop
- more granular ActiveIssue dotnet#53592 for lack of TRACE
- more granular ActiveIssue dotnet#53591 for content on GET/HEAD
- more granular ActiveIssue dotnet#53874 for HttpRequestMessage.Headers.Host
- more granular ActiveIssue dotnet#53872 for NPE on System.Net.Http.BrowserHttpHandler
- fix HTTP vs HTTPS test configuration `Http2SecureRemoteEchoServer`
- include echo middleware in xharness server
- include middleware in Helix correlation payload
@lewing lewing added this to the 6.0.0 milestone Jun 8, 2021
@lewing lewing removed the untriaged New issue has not been triaged by the area owner label Jun 8, 2021
@lewing
Copy link
Member

lewing commented Jun 8, 2021

Looking at the test I don't see an obvious reason it should fail like this. Is this failing on all browser lanes? There was some work done allow trimming more of the header logic out and if these tests weren't running they may have regressed.

pavelsavara added a commit that referenced this issue Jun 9, 2021
- move tests to inner loop
- include echo middleware in xharness server
- improve doc
- more granular ActiveIssue #53592 for lack of TRACE
- more granular ActiveIssue #53591 for content on GET/HEAD
- more granular ActiveIssue #53874 for HttpRequestMessage.Headers.Host
- more granular ActiveIssue #53872 for NPE on System.Net.Http.BrowserHttpHandler
- more granular ActiveIssue #53876
- include middleware in Helix correlation payload
@pavelsavara
Copy link
Member Author

Here is example of the failure on CI
https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-53925-merge-160a064b73f24741b2/System.Net.Http.Functional.Tests/console.3a6c0e9f.log?sv=2019-07-07&se=2021-06-30T09%3A14%3A59Z&sr=c&sp=rl&sig=qBO1wWtqkq9AGYRX0b82P6An%2BBE0LKFnuckJY0ed6BU%3D

This time different test and on ubuntu, not windows.
System.Net.Http.Functional.Tests.HttpClientHandler_RemoteServerTest.GetAsync_ServerNeedsAuthAndNoCredential_StatusCodeUnauthorized

Maybe race condition ?

@pavelsavara
Copy link
Member Author

Yes second CI run on the same code as above is green.

@pavelsavara pavelsavara changed the title [wasm] System.Net.Http.BrowserHttpHandler.SendAsync() throws NPE [wasm] System.Net.Http.BrowserHttpHandler.SendAsync() throws NRE Jun 10, 2021
@pavelsavara
Copy link
Member Author

Another locally

System.Net.Http.Functional.Tests.HttpClientHandler_RemoteServerTest.GetAsync_RequestHeadersAddCustomHeaders_HeaderAndValueSent(remoteServer: (BaseUri: https://127.0.0.1:57664/, HttpVersion: 1.1), name: "X-CustomHeader", value: "x-value", uri: https://127.0.0.1:57664/Echo.ashx)
  info: System.NullReferenceException : Object reference not set to an instance of an object.
  info:    at System.Net.Http.BrowserHttpHandler.SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
  info:    at System.Net.Http.HttpClient.<SendAsync>g__Core|83_0(HttpRequestMessage request, HttpCompletionOption completionOption, CancellationTokenSource cts, Boolean disposeCts, CancellationTokenSource pendingRequestsCts, CancellationToken originalCancellationToken)
  info:    at System.Net.Http.Functional.Tests.HttpClientHandler_RemoteServerTest.GetAsync_RequestHeadersAddCustomHeaders_HeaderAndValueSent(RemoteServer remoteServer, String name, String value, Uri uri)

@pavelsavara
Copy link
Member Author

System.ArgumentNullException : Value cannot be null. (Parameter 'fetchResponse')
  info:    at System.Net.Http.BrowserHttpHandler.SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)

@pavelsavara
Copy link
Member Author

Repro at pavelsavara@a2b0a7b#diff-66ce677ded58c3a6a0639d321f4460bb0b2c4bcaeb3216ff3db4381937365bd8

It fails randomly with s, q, x3 and x10 as last state

I think JSObject is returned as null from some of the marshaling calls.

@lewing @kg thoughts ?

@kg
Copy link
Contributor

kg commented Jun 10, 2021

Most likely explanation is a GC issue. I think if it were memory corruption in the runtime (which has been known to happen, but hasn't - afaik - been seen recently) it wouldn't just be consistently producing NREs, other bad stuff would be happening.

@pavelsavara
Copy link
Member Author

Very simple steps to reproduce here https://github.com/dotnet/runtime/pull/54453/files

dotnet build /p:TargetOS=Browser /p:TargetArchitecture=wasm /p:Configuration=Release /t:Test src\libraries\System.Private.Runtime.InteropServices.JavaScript\tests /p:WithCategories="Pavel"

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jun 21, 2021
@lewing
Copy link
Member

lewing commented Jun 28, 2021

looks similar to

[20:02:53] info: Initializing.....
[20:02:55] info: {"method":"console.error","payload":{},"arguments":[{}]}
[20:02:55] fail: Error: System.NullReferenceException: Object reference not set to an instance of an object.
                    at System.Runtime.InteropServices.JavaScript.Runtime.<>c__DisplayClass27_0.<SetupJSContinuation>g__Complete|0()
                    at System.Runtime.InteropServices.JavaScript.Runtime.SetupJSContinuation(Task task, JSObject continuationObj)
                     at Object._convert_exception_for_method_call (http://127.0.0.1:35977/dotnet.js:1:171696)
                     at Object._handle_exception_for_call (http://127.0.0.1:35977/dotnet.js:1:173665)
                     at Object._handle_exception_and_produce_result_for_call (http://127.0.0.1:35977/dotnet.js:1:173986)
                     at Object._call_method_with_converted_args (http://127.0.0.1:35977/dotnet.js:1:175060)
                     at Object.call_method (http://127.0.0.1:35977/dotnet.js:1:173455)
                     at Object._unbox_task_rooted (http://127.0.0.1:35977/dotnet.js:1:152563)
                     at Object._unbox_mono_obj_rooted_with_known_nonprimitive_type (http://127.0.0.1:35977/dotnet.js:1:153449)
                     at Object._unbox_mono_obj_root (http://127.0.0.1:35977/dotnet.js:1:154685)
                     at Object._handle_exception_and_produce_result_for_call (http://127.0.0.1:35977/dotnet.js:1:174106)
                     at Object._call_method_with_converted_args (http://127.0.0.1:35977/dotnet.js:1:175060)
[20:02:55] info: Discovering: System.Xml.Linq.xNodeBuilder.Tests.dll (method display = ClassAndMethod, method display options = None)
[20:02:55] info: Discovered:  System.Xml.Linq.xNodeBuilder.Tests.dll (found 7 of 8 test cases)
[20:02:55] info: Starting:    System.Xml.Linq.xNodeBuilder.Tests.dll
[20:02:55] info: Finished:    System.Xml.Linq.xNodeBuilder.Tests.dll
[20:02:55] info: 
[20:02:55] info: === TEST EXECUTION SUMMARY ===

from https://dev.azure.com/dnceng/public/_build/results?buildId=1206770&view=logs&j=108d2c4a-8a62-5a58-8dad-8e1042acc93c&t=568f884b-cc12-5fd3-e7fe-790b5ac403f4&l=125

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 15, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants