Fix ReadBufferState dispose cases

[MaxPatri]

I moved the initialization of ReadBufferState after other related operations to prevent skipping Dispose in case of an exception, and consequently, to avoid memory leaks.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

[stephentoub]

to avoid memory leaks

What leaks?

[huoyaoyuan]

If ReadStack.Initialize throws, the array rented in ReadBufferState won't be returned. new JsonReaderState won't throw because it's pure POCO initialization.
There won't be leak, because failing to return pooled array isn't a fatal leak.

[eiriktsarpalis]

This is the code for ReadStack.Initialize:

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStack.cs

Lines 96 to 110 in db9a4c6

	internal void Initialize(JsonTypeInfo jsonTypeInfo, bool supportContinuation = false)
	{
	JsonSerializerOptions options = jsonTypeInfo.Options;
	if (options.ReferenceHandlingStrategy == JsonKnownReferenceHandler.Preserve)
	{
	ReferenceResolver = options.ReferenceHandler!.CreateResolver(writing: false);
	PreserveReferences = true;
	}
	Current.JsonTypeInfo = jsonTypeInfo;
	Current.JsonPropertyInfo = jsonTypeInfo.PropertyInfoForTypeInfo;
	Current.NumberHandling = Current.JsonPropertyInfo.EffectiveNumberHandling;
	Current.CanContainMetadata = PreserveReferences || jsonTypeInfo.PolymorphicTypeResolver?.UsesTypeDiscriminators == true;
	SupportContinuation = supportContinuation;
	}

And this is the code for the JsonReaderState:

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Reader/JsonReaderState.cs

Lines 39 to 54 in db9a4c6

	public JsonReaderState(JsonReaderOptions options = default)
	{
	_lineNumber = default;
	_bytePositionInLine = default;
	_inObject = default;
	_isNotPrimitive = default;
	_valueIsEscaped = default;
	_trailingCommaBeforeComment = default;
	_tokenType = default;
	_previousTokenType = default;
	_readerOptions = options;
	// Only allocate if the user reads a JSON payload beyond the depth that the _allocationFreeContainer can handle.
	// This way we avoid allocations in the common, default cases, and allocate lazily.
	_bitStack = default;
	}

Excluding potential bugs in the underlying implementation(s), neither of these calls are expected to throw. Is there a real-world scenario that could result in unreturned buffers?

FWIW I don't think we couldn't consider this change, but to make things more explicit I would just move the initialization methods inside the try clause instead (or at least leave a comment explaining why the state object should be created last).

[MaxPatri]

Moved ReadBufferState initialization under try clause.

[eiriktsarpalis]

Moved ReadBufferState initialization under try clause.

That's not quite that I meant. My suggestion was to either keep the current order (but nest the initialization methods inside the try clause) or add a comment to your original change explaining why the ReadBufferState is being initialized last.

[MaxPatri]

Initial change was returned with comment

[MaxPatri]

@dotnet-policy-service agree

[eiriktsarpalis]

Thanks

[MaxPatri]

Is this normal for check "runtime (Build browser-wasm linux Release LibraryTests)" to be running so long? Or may be it is stuck?
Also I would like to know what should I do with failed checks? I don't see restart button or something else and I assume that my changes could not affect the build or tests.

[eiriktsarpalis]

It happens from time to time, I've merged main to retrigger a run.