Fix assert and leak of WinHttpRequestState object

[davidsh]

Discovered during testing with DEBUG version of CoreCLR running internal ToF tests. The design of the WinHttpRequestState is such that it stays alive due to a GCHandle.Alloc(this) in its constructor. This keeps the object alive during any of the native WinHTTP callbacks. However, the object was being prematurely disposed during the Dispose() of the WinHttpResponseStream. Instead, it should have been explicitly disposed during the final HANDLE_CLOSING callback from WinHTTP when the request handle was disposed. The early disposing of the WinHttpRequestState object was doing a GCHandle.Free() of the state object. This later caused an Assert() in the DEBUG version of the CoreCLR when doing GCHandle.FromIntPtr() indicating that the handle was already deallocated.

[davidsh]

@CIPop @stephentoub PTAL

[stephentoub]

If you pass any args to Trace, the compiler is going to allocate an array to hold them even if !IsTraceEnabled. For now you could add a [Conditional("DEBUG")] to these Trace methods, as they're just calling Debug.WriteLine if enabled and so they're only useful if DEBUG is set. Longer term, I know you're planning on converting the tracing to using an EventSource, at which point you'd want some other solution to ensure that such allocations don't happen if tracing isn't enabled.

[davidsh]

I didn't realize that using params object[] would cause that extra allocation. I think for now I'll just revise the method to have explicit arguments. That is what methods like string.Format do for common overload signatures in addition to the params object[] argument.

[stephentoub]

I think for now I'll just revise the method to have explicit arguments

Sounds good (noting of course that if you make those separate args typed as object, you'll get boxing for any value types you pass).

I didn't realize that using params object[] would cause that extra allocation

Yeah, the compiler needs to allocate the object[] in which to pass the values. Roslyn has an optimization that'll let it use Array.Empty<object>() instead of allocating an array, but that only applies for the case where you don't pass any args.

[stephentoub]

One perf comment, otherwise LGTM.

[stephentoub]

@davidsh, the PR job at:
http://dotnet-ci.cloudapp.net/job/dotnet_corefx_windows_release_prtest/5381/console
failed due to this unhandled exception:

Unhandled Exception: System.InvalidCastException: Unable to cast object of type 'System.Object[]' to type 'System.Net.Http.WinHttpRequestState'.
     at System.Net.Http.WinHttpRequestState.FromIntPtr(IntPtr gcHandle)
     at System.Net.Http.WinHttpRequestCallback.WinHttpCallback(IntPtr handle, IntPtr context, UInt32 internetStatus, IntPtr statusInformation, UInt32 statusInformationLength)

From your description of the problem in this PR, seems like this could be the cause of this exception, right? i.e. we disposed of the GCHandle too early, it got recycled, and the handle that was for a WinHttpRequestState is now for an object[]?

[davidsh]

@stephentoub yes, I do think that the bug I'm fixing is the cause of the unhandled exception from that other PR. I'll try to get my PR merged in soon in the morning.

[stephentoub]

yes, I do think

Great, one less failure to worry about 😄 Thanks.

[stephentoub]

I alluded to this in my previous response before seeing the revamped change, so just commenting on it here again in case the previous comment didn't make sense. Now the caller that's passing in an IntPtr, a Boolean, and a Boolean is going to be allocating three objects when calling Trace, one for each of the boxed value type arguments, even if IsTraceEnabled is false. You might just want to guard the call site with a check for IsTraceEnabled, rather than mucking with the Trace signature, e.g.

if (WinHttpTraceHelper.IsTraceEnabled())
{
    WinHttpTraceHelper.Trace(
        "WinHttpRequestState.Dispose, GCHandle=0x{0:X}, disposed={1}, disposing={2}",
        ToIntPtr(),
        _disposed,
        disposing);
}

[davidsh]

Good points. I'll wrap the call site for now. Although the wrapping code doesn't look as good, it will serve as a reminder to keep perf good when I'm converting this to the final tracing logic

[stephentoub]

@dotnet-bot test this please