Fix WinHttpHandler error handling (#28367)

This PR addresses some error handling problems in WinHttpHandler as well as
improves the error messages generated for WinHttpException.

This was root caused to a bug in WinHttp where it can't do a GET request with a
request body using chunked encoding. However, this exception was masked due to
an error handling bug caused by an inconsistency in how WinHttp associates
context values for callbacks during the WinHttpSendRequest API call. This PR now
fixes the error handling around synchronous errors being returned from
WinHttpSendRequest. The root WinHttp bug itself can't be fixed. So, doing a GET request
will still throw an exception, but it will be a catchable HttpRequestException.

Another bug was discovered while investigating #26278. WinHttpCloseHandle should only be
called once and should never race between threads. This is normally protected when
the request handle is closed via SafeHandle Dispose(). But there was a place in the
callback method where we called WinHttpCloseHandle directly against the raw handle.

Finally, the error messages for WinHttpExceptions have been improved to show the
error number and the probable WinHttp API call that generated the error. This will
save diagnostic time.

I also moved the WinHttpException source code back from Common. It was originally shared
between WinHttpHandler and ClientWebSocket. But then ClientWebSocket moved away from
WinHttp implementation. So, it makes more sense for this class to be under WinHttpHandler.

Example:

Original WinHttpException message: "The parameter is incorrect"
New WinHttpException message: "Error 87 calling WinHttpSendRequest, 'The parameter is incorrect'."

Closes #28156
Contributes to #26278

src/System.Net.Http.WinHttpHandler/src/Resources/Strings.resx

@@ -121,6 +121,9 @@
   <data name="net_http_content_stream_already_read" xml:space="preserve">
     <value>The stream was already consumed. It cannot be read again.</value>
   </data>
+  <data name="net_http_winhttp_error" xml:space="preserve">
+    <value>Error {0} calling {1}, '{2}'.</value>
+  </data>
   <data name="PlatformNotSupported_WinHttpHandler" xml:space="preserve">
     <value>WinHttpHandler is only supported on .NET Framework and .NET Core runtimes on Windows. It is not supported for Windows Store Applications (UWP) or Unix platforms.</value>
   </data>

src/System.Net.Http.WinHttpHandler/src/System.Net.Http.WinHttpHandler.msbuild

@@ -23,7 +23,6 @@
     <CompileItem Include="$(CommonPath)\System\Net\UriScheme.cs" />
     <CompileItem Include="$(CommonPath)\System\Net\Http\HttpHandlerDefaults.cs" />
     <CompileItem Include="$(CommonPath)\System\Net\Http\NoWriteNoSeekStreamContent.cs" />
-    <CompileItem Include="$(CommonPath)\System\Net\Http\WinHttpException.cs" />
     <CompileItem Include="$(CommonPath)\System\Net\Security\CertificateHelper.cs" />
     <CompileItem Include="$(CommonPath)\System\Net\Security\CertificateHelper.Windows.cs" />
     <CompileItem Include="$(CommonPath)\System\Runtime\ExceptionServices\ExceptionStackTrace.cs" />
@@ -33,6 +32,7 @@
     <CompileItem Include="$(MSBuildThisFileDirectory)\System\Net\Http\WinHttpCertificateHelper.cs" />
     <CompileItem Include="$(MSBuildThisFileDirectory)\System\Net\Http\WinHttpChannelBinding.cs" />
     <CompileItem Include="$(MSBuildThisFileDirectory)\System\Net\Http\WinHttpCookieContainerAdapter.cs" />
+    <CompileItem Include="$(MSBuildThisFileDirectory)\System\Net\Http\WinHttpException.cs" />
     <CompileItem Include="$(MSBuildThisFileDirectory)\System\Net\Http\WinHttpHandler.cs" />
     <CompileItem Include="$(MSBuildThisFileDirectory)\System\Net\Http\WinHttpRequestCallback.cs" />
     <CompileItem Include="$(MSBuildThisFileDirectory)\System\Net\Http\WinHttpRequestState.cs" />

src/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpAuthHelper.cs

@@ -296,7 +296,7 @@ public void ChangeDefaultCredentialsPolicy(
                 Interop.WinHttp.WINHTTP_OPTION_AUTOLOGON_POLICY,
                 ref optionData))
             {
-                WinHttpException.ThrowExceptionUsingLastError();
+                WinHttpException.ThrowExceptionUsingLastError(nameof(Interop.WinHttp.WinHttpSetOption));
             }
         }
 
@@ -365,7 +365,7 @@ private bool SetWinHttpCredential(
                 password,
                 IntPtr.Zero))
             {
-                WinHttpException.ThrowExceptionUsingLastError();
+                WinHttpException.ThrowExceptionUsingLastError(nameof(Interop.WinHttp.WinHttpSetCredentials));
             }
 
             return true;

src/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpCookieContainerAdapter.cs

@@ -61,7 +61,7 @@ public static void ResetCookieRequestHeaders(WinHttpRequestState state, Uri redi
                 int lastError = Marshal.GetLastWin32Error();
                 if (lastError != Interop.WinHttp.ERROR_WINHTTP_HEADER_NOT_FOUND)
                 {
-                    throw WinHttpException.CreateExceptionUsingError(lastError);
+                    throw WinHttpException.CreateExceptionUsingError(lastError, nameof(Interop.WinHttp.WinHttpAddRequestHeaders));
                 }
             }
 
@@ -76,7 +76,7 @@ public static void ResetCookieRequestHeaders(WinHttpRequestState state, Uri redi
                     (uint)cookieHeader.Length,
                     Interop.WinHttp.WINHTTP_ADDREQ_FLAG_ADD))
                 {
-                    WinHttpException.ThrowExceptionUsingLastError();
+                    WinHttpException.ThrowExceptionUsingLastError(nameof(Interop.WinHttp.WinHttpAddRequestHeaders));
                 }
             }
         }

src/Common/src/System/Net/Http/WinHttpException.cs → src/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpException.cs

@@ -3,6 +3,7 @@
 // See the LICENSE file in the project root for more information.
 
 using System.ComponentModel;
+using System.Diagnostics;
 using System.Runtime.ExceptionServices;
 using System.Runtime.InteropServices;
 
@@ -39,37 +40,41 @@ public static int ConvertErrorCodeToHR(int error)
             }
         }
 
-        public static void ThrowExceptionUsingLastError()
+        public static void ThrowExceptionUsingLastError(string nameOfCalledFunction)
         {
-            throw CreateExceptionUsingLastError();
+            throw CreateExceptionUsingLastError(nameOfCalledFunction);
         }
 
-        public static WinHttpException CreateExceptionUsingLastError()
+        public static WinHttpException CreateExceptionUsingLastError(string nameOfCalledFunction)
         {
             int lastError = Marshal.GetLastWin32Error();
-            return CreateExceptionUsingError(lastError);
+            return CreateExceptionUsingError(lastError, nameOfCalledFunction);
         }
 
-        public static WinHttpException CreateExceptionUsingError(int error)
+        public static WinHttpException CreateExceptionUsingError(int error, string nameOfCalledFunction)
         {
-            var e = new WinHttpException(error, GetErrorMessage(error));
+            var e = new WinHttpException(error, GetErrorMessage(error, nameOfCalledFunction));
             ExceptionStackTrace.AddCurrentStack(e);
             return e;
         }
 
-        public static WinHttpException CreateExceptionUsingError(int error, Exception innerException)
+        public static WinHttpException CreateExceptionUsingError(int error, string nameOfCalledFunction, Exception innerException)
         {
-            var e = new WinHttpException(error, GetErrorMessage(error), innerException);
+            var e = new WinHttpException(error, GetErrorMessage(error, nameOfCalledFunction), innerException);
             ExceptionStackTrace.AddCurrentStack(e);
             return e;
         }
 
-        public static string GetErrorMessage(int error)
+        public static string GetErrorMessage(int error, string nameOfCalledFunction)
         {
+            Debug.Assert(!string.IsNullOrEmpty(nameOfCalledFunction));
+
             // Look up specific error message in WINHTTP.DLL since it is not listed in default system resources
             // and thus can't be found by default .Net interop.
             IntPtr moduleHandle = Interop.Kernel32.GetModuleHandle(Interop.Libraries.WinHttp);
-            return Interop.Kernel32.GetMessage(moduleHandle, error);
+            string httpError = Interop.Kernel32.GetMessage(moduleHandle, error);
+
+            return SR.Format(SR.net_http_winhttp_error, error, nameOfCalledFunction, httpError);
         }
     }
 }

src/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs

@@ -661,7 +661,7 @@ private static void AddRequestHeaders(
                 (uint)requestHeadersBuffer.Length,
                 Interop.WinHttp.WINHTTP_ADDREQ_FLAG_ADD))
             {
-                WinHttpException.ThrowExceptionUsingLastError();
+                WinHttpException.ThrowExceptionUsingLastError(nameof(Interop.WinHttp.WinHttpAddRequestHeaders));
             }
         }
 
@@ -728,7 +728,7 @@ private void EnsureSessionHandleExists(WinHttpRequestState state)
                             WinHttpTraceHelper.Trace("WinHttpHandler.EnsureSessionHandleExists: error={0}", lastError);
                             if (lastError != Interop.WinHttp.ERROR_INVALID_PARAMETER)
                             {
-                                ThrowOnInvalidHandle(sessionHandle);
+                                ThrowOnInvalidHandle(sessionHandle, nameof(Interop.WinHttp.WinHttpOpen));
                             }
 
                             // We must be running on a platform earlier than Win8.1/Win2K12R2 which doesn't support
@@ -741,7 +741,7 @@ private void EnsureSessionHandleExists(WinHttpRequestState state)
                                 _proxyHelper.ManualSettingsOnly ? _proxyHelper.Proxy : Interop.WinHttp.WINHTTP_NO_PROXY_NAME,
                                 _proxyHelper.ManualSettingsOnly ? _proxyHelper.ProxyBypass : Interop.WinHttp.WINHTTP_NO_PROXY_BYPASS,
                                 (int)Interop.WinHttp.WINHTTP_FLAG_ASYNC);
-                            ThrowOnInvalidHandle(sessionHandle);
+                            ThrowOnInvalidHandle(sessionHandle, nameof(Interop.WinHttp.WinHttpOpen));
                         }
 
                         uint optionAssuredNonBlockingTrue = 1; // TRUE
@@ -757,7 +757,7 @@ private void EnsureSessionHandleExists(WinHttpRequestState state)
                             int lastError = Marshal.GetLastWin32Error();
                             if (lastError != Interop.WinHttp.ERROR_WINHTTP_INVALID_OPTION)
                             {
-                                throw WinHttpException.CreateExceptionUsingError(lastError);
+                                throw WinHttpException.CreateExceptionUsingError(lastError, nameof(Interop.WinHttp.WinHttpSetOption));
                             }
                         }
 
@@ -788,7 +788,7 @@ private async void StartRequest(WinHttpRequestState state)
                     state.RequestMessage.RequestUri.Host,
                     (ushort)state.RequestMessage.RequestUri.Port,
                     0);
-                ThrowOnInvalidHandle(connectHandle);
+                ThrowOnInvalidHandle(connectHandle, nameof(Interop.WinHttp.WinHttpConnect));
                 connectHandle.SetParentHandle(_sessionHandle);
 
                 // Try to use the requested version if a known/supported version was explicitly requested.
@@ -821,7 +821,7 @@ private async void StartRequest(WinHttpRequestState state)
                     Interop.WinHttp.WINHTTP_NO_REFERER,
                     Interop.WinHttp.WINHTTP_DEFAULT_ACCEPT_TYPES,
                     flags);
-                ThrowOnInvalidHandle(state.RequestHandle);
+                ThrowOnInvalidHandle(state.RequestHandle, nameof(Interop.WinHttp.WinHttpOpenRequest));
                 state.RequestHandle.SetParentHandle(connectHandle);
 
                 // Set callback function.
@@ -957,7 +957,7 @@ private void SetSessionHandleTimeoutOptions(SafeWinHttpHandle sessionHandle)
                 (int)_sendTimeout.TotalMilliseconds,
                 (int)_receiveHeadersTimeout.TotalMilliseconds))
             {
-                WinHttpException.ThrowExceptionUsingLastError();
+                WinHttpException.ThrowExceptionUsingLastError(nameof(Interop.WinHttp.WinHttpSetTimeouts));
             }
         }
 
@@ -1214,7 +1214,7 @@ private void SetWinHttpOption(SafeWinHttpHandle handle, uint option, ref uint op
                 option,
                 ref optionData))
             {
-                WinHttpException.ThrowExceptionUsingLastError();
+                WinHttpException.ThrowExceptionUsingLastError(nameof(Interop.WinHttp.WinHttpSetOption));
             }
         }
 
@@ -1227,7 +1227,7 @@ private void SetWinHttpOption(SafeWinHttpHandle handle, uint option, string opti
                 optionData,
                 (uint)optionData.Length))
             {
-                WinHttpException.ThrowExceptionUsingLastError();
+                WinHttpException.ThrowExceptionUsingLastError(nameof(Interop.WinHttp.WinHttpSetOption));
             }
         }
 
@@ -1244,7 +1244,7 @@ private static void SetWinHttpOption(
                 optionData,
                 optionSize))
             {
-                WinHttpException.ThrowExceptionUsingLastError();
+                WinHttpException.ThrowExceptionUsingLastError(nameof(Interop.WinHttp.WinHttpSetOption));
             }
         }
 
@@ -1314,18 +1314,18 @@ private void SetStatusCallback(
                 int lastError = Marshal.GetLastWin32Error();
                 if (lastError != Interop.WinHttp.ERROR_INVALID_HANDLE) // Ignore error if handle was already closed.
                 {
-                    throw WinHttpException.CreateExceptionUsingError(lastError);
+                    throw WinHttpException.CreateExceptionUsingError(lastError, nameof(Interop.WinHttp.WinHttpSetStatusCallback));
                 }
             }
         }
 
-        private void ThrowOnInvalidHandle(SafeWinHttpHandle handle)
+        private void ThrowOnInvalidHandle(SafeWinHttpHandle handle, string nameOfCalledFunction)
         {
             if (handle.IsInvalid)
             {
                 int lastError = Marshal.GetLastWin32Error();
                 WinHttpTraceHelper.Trace("WinHttpHandler.ThrowOnInvalidHandle: error={0}", lastError);
-                throw WinHttpException.CreateExceptionUsingError(lastError);
+                throw WinHttpException.CreateExceptionUsingError(lastError, nameOfCalledFunction);
             }
         }
 
@@ -1343,11 +1343,13 @@ private RendezvousAwaitable<int> InternalSendRequestAsync(WinHttpRequestState st
                     0,
                     state.ToIntPtr()))
                 {
-                    // Dispose (which will unpin) the state object. Since this failed, WinHTTP won't associate
-                    // our context value (state object) to the request handle. And thus we won't get HANDLE_CLOSING
-                    // notifications which would normally cause the state object to be unpinned and disposed.
+                    // WinHTTP doesn't always associate our context value (state object) to the request handle.
+                    // And thus we might not get a HANDLE_CLOSING notification which would normally cause the
+                    // state object to be unpinned and disposed. So, we manually dispose the request handle and
+                    // state object here.
+                    state.RequestHandle.Dispose();
                     state.Dispose();
-                    WinHttpException.ThrowExceptionUsingLastError();
+                    WinHttpException.ThrowExceptionUsingLastError(nameof(Interop.WinHttp.WinHttpSendRequest));
                 }
             }
 
@@ -1371,7 +1373,7 @@ private RendezvousAwaitable<int> InternalReceiveResponseHeadersAsync(WinHttpRequ
             {
                 if (!Interop.WinHttp.WinHttpReceiveResponse(state.RequestHandle, IntPtr.Zero))
                 {
-                    throw WinHttpException.CreateExceptionUsingLastError();
+                    throw WinHttpException.CreateExceptionUsingLastError(nameof(Interop.WinHttp.WinHttpReceiveResponse));
                 }
             }
 

src/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestCallback.cs

@@ -110,8 +110,19 @@ private static void RequestCallback(
             }
             catch (Exception ex)
             {
-                Interop.WinHttp.WinHttpCloseHandle(handle);
                 state.SavedException = ex;
+                if (state.RequestHandle != null)
+                {
+                    // Since we got a fatal error processing the request callback,
+                    // we need to close the WinHttp request handle in order to
+                    // abort the currently executing WinHttp async operation.
+                    //
+                    // We must always call Dispose() against the SafeWinHttpHandle
+                    // wrapper and never close directly the raw WinHttp handle.
+                    // The SafeWinHttpHandle wrapper is thread-safe and guarantees
+                    // calling the underlying WinHttpCloseHandle() function only once.
+                    state.RequestHandle.Dispose();
+                }
             }
         }
 
@@ -260,7 +271,7 @@ private static void OnRequestSendingRequest(WinHttpRequestState state)
                         return;
                     }
 
-                    throw WinHttpException.CreateExceptionUsingError(lastError);
+                    throw WinHttpException.CreateExceptionUsingError(lastError, "WINHTTP_CALLBACK_STATUS_SENDING_REQUEST/WinHttpQueryOption");
                 }
 
                 // Get any additional certificates sent from the remote server during the TLS/SSL handshake.
@@ -295,7 +306,7 @@ private static void OnRequestSendingRequest(WinHttpRequestState state)
                 catch (Exception ex)
                 {
                     throw WinHttpException.CreateExceptionUsingError(
-                        (int)Interop.WinHttp.ERROR_WINHTTP_SECURE_FAILURE, ex);
+                        (int)Interop.WinHttp.ERROR_WINHTTP_SECURE_FAILURE, "X509Chain.Build", ex);
                 }
                 finally
                 {
@@ -310,7 +321,7 @@ private static void OnRequestSendingRequest(WinHttpRequestState state)
                 if (!result)
                 {
                     throw WinHttpException.CreateExceptionUsingError(
-                        (int)Interop.WinHttp.ERROR_WINHTTP_SECURE_FAILURE);
+                        (int)Interop.WinHttp.ERROR_WINHTTP_SECURE_FAILURE, "ServerCertificateValidationCallback");
                 }
             }
         }
@@ -321,7 +332,7 @@ private static void OnRequestError(WinHttpRequestState state, Interop.WinHttp.WI
 
             Debug.Assert(state != null, "OnRequestError: state is null");
 
-            Exception innerException = WinHttpException.CreateExceptionUsingError(unchecked((int)asyncResult.dwError));
+            Exception innerException = WinHttpException.CreateExceptionUsingError(unchecked((int)asyncResult.dwError), "WINHTTP_CALLBACK_STATUS_REQUEST_ERROR");
 
             switch (unchecked((uint)asyncResult.dwResult.ToInt32()))
             {
@@ -424,7 +435,7 @@ private static void ResetAuthRequestHeaders(WinHttpRequestState state)
                 int lastError = Marshal.GetLastWin32Error();
                 if (lastError != Interop.WinHttp.ERROR_WINHTTP_HEADER_NOT_FOUND)
                 {
-                    throw WinHttpException.CreateExceptionUsingError(lastError);
+                    throw WinHttpException.CreateExceptionUsingError(lastError, "WINHTTP_CALLBACK_STATUS_REDIRECT/WinHttpAddRequestHeaders");
                 }
             }
         }

src/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestStream.cs

@@ -249,7 +249,7 @@ private Task<bool> InternalWriteDataAsync(byte[] buffer, int offset, int count,
                     IntPtr.Zero))
                 {
                     _state.TcsInternalWriteDataToRequestStream.TrySetException(
-                        new IOException(SR.net_http_io_write, WinHttpException.CreateExceptionUsingLastError()));
+                        new IOException(SR.net_http_io_write, WinHttpException.CreateExceptionUsingLastError(nameof(Interop.WinHttp.WinHttpWriteData))));
                 }
             }
 

src/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpResponseParser.cs

@@ -127,7 +127,7 @@ public static uint GetResponseHeaderNumberInfo(SafeWinHttpHandle requestHandle,
                 ref resultSize,
                 IntPtr.Zero))
             {
-                WinHttpException.ThrowExceptionUsingLastError();
+                WinHttpException.ThrowExceptionUsingLastError(nameof(Interop.WinHttp.WinHttpQueryHeaders));
             }
 
             return result;
@@ -189,7 +189,7 @@ public static unsafe bool GetResponseHeader(
                 return GetResponseHeader(requestHandle, infoLevel, ref buffer, ref index, out headerValue);
             }
 
-            throw WinHttpException.CreateExceptionUsingError(lastError);
+            throw WinHttpException.CreateExceptionUsingError(lastError, nameof(Interop.WinHttp.WinHttpQueryHeaders));
         }
 
         /// <summary>
@@ -216,7 +216,7 @@ private static unsafe int GetResponseHeader(SafeWinHttpHandle requestHandle, uin
 
                     Debug.Assert(lastError != Interop.WinHttp.ERROR_INSUFFICIENT_BUFFER, "buffer must be of sufficient size.");
 
-                    throw WinHttpException.CreateExceptionUsingError(lastError);
+                    throw WinHttpException.CreateExceptionUsingError(lastError, nameof(Interop.WinHttp.WinHttpQueryHeaders));
                 }
             }
 
@@ -240,7 +240,7 @@ private static unsafe int GetResponseHeaderCharBufferLength(SafeWinHttpHandle re
 
                 if (lastError != Interop.WinHttp.ERROR_INSUFFICIENT_BUFFER)
                 {
-                    throw WinHttpException.CreateExceptionUsingError(lastError);
+                    throw WinHttpException.CreateExceptionUsingError(lastError, nameof(Interop.WinHttp.WinHttpQueryHeaders));
                 }
             }