Stop nulling out fields in SafeDeleteSslContext dispose

[rmkerr]

Nulling out these fields in SafeDeleteSslContext.Dispose causes occasional difficult to reproduce exceptions in CI:

Unhandled Exception: System.ArgumentNullException: Value cannot be null.
   at System.Threading.Monitor.ReliableEnter(Object obj, Boolean& lockTaken)
   at System.Net.SafeDeleteSslContext.WriteToConnection(Void* connection, Byte* data, Void** dataLength)
   at Interop.AppleCrypto.SslHandshake(SafeSslHandle sslHandle)
   at System.Net.Security.SslStreamPal.PerformHandshake(SafeSslHandle sslHandle)
   at System.Net.Security.SslStreamPal.HandshakeInternal(SafeFreeCredentials credential, SafeDeleteContext& context, SecurityBuffer inputBuffer, SecurityBuffer outputBuffer, Boolean isServer, Boolean remoteCertRequired, String targetName)

The fix is to stop nulling out these fields in the dispose method, and to allow the garbage collector to take care of them instead.

Fixes: #28759

[davidsh]

This change seems counter intuitive. These assignments to null for those fields are being in the explicit block of the Dispose() method (i.e. not finalization). Usually in that portion of the Dispose() method (if 'disposing == true') the motivation is to free stuff sooner rather than later.

So, why would this change make things better? Is there some other race condition at work here that we're trying to workaround. Overall I don't understand the real root cause of what this "fix" is trying to do.

[stephentoub]

why would this change make things better?

Because in particular it's null'ing out the field that's used as the synchronization object:
https://github.com/dotnet/corefx/pull/28889/files#diff-86b3ae4fe05db79e2326a291c9f1a847R127

[rmkerr]

I could leave two of the four of those lines unchanged since they are not being used for locking, but I decided to go for consistency. If you disagree I can change that.

@dotnet-bot test Outerloop OSX x64 Debug Build please

[davidsh]

Thx for the detailed explanation of the root cause (synchronization object going away).

Based on this:
https://docs.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-dispose

then I would say that its ok and the change is fine. I.e., keep the change that removes the lines of code that null out those fields since that is not part of the best-practice IDispose pattern. If those fields were objects that implemented IDispose themselves, then you would at least call Dispose() on them. But that isn't the case here.

[rmkerr]

I think that the hang in System.Net.Sockets on OSX 10.12 is caused by #21037, but I'm going to run the tests again to be sure.

@dotnet-bot test Outerloop OSX x64 Debug Build please

[rmkerr]

I think the OSX failure was an infrastructure issue, since all the tests appear to have passed. Merging.