NrbfReader tests not working in WASM/mobile

[adamsitnik]

In #104104 I've tried to re-enable all BF tests and got following error for WASM/Mobile builds:

[00:49:29] info: [FAIL] System.Formats.Nrbf.Tests.ArraySinglePrimitiveRecordTests.CanReadArrayOfAnySize_Bool(size: 127, canSeek: False)
[00:49:29] info: System.TypeInitializationException : The type initializer for 'System.Runtime.Serialization.Formatters.Binary.Converter' threw an exception.
[00:49:29] info: ---- System.IO.FileNotFoundException :
[00:49:29] info:    at System.Runtime.Serialization.Formatters.Binary.ObjectWriter.GetAssemblyId(WriteObjectInfo )
[00:49:29] info:    at System.Runtime.Serialization.Formatters.Binary.ObjectWriter.Serialize(Object , BinaryFormatterWriter )
[00:49:29] info:    at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Serialize(Stream , Object )
[00:49:29] info:    at System.Formats.Nrbf.Tests.ReadTests.Serialize[Boolean[]](Boolean[] instance)
[00:49:29] info:    at System.Formats.Nrbf.Tests.ArraySinglePrimitiveRecordTests.Test[Boolean](Int32 size, Boolean canSeek)
[00:49:29] info:    at System.Object.InvokeStub_ArraySinglePrimitiveRecordTests.CanReadArrayOfAnySize_Bool(Object , Span`1 )

Most likely it's trying to load "mscorlib" which is not available for these platforms:

runtime/src/libraries/System.Runtime.Serialization.Formatters/src/System/Runtime/Serialization/Formatters/Binary/Converter.cs

Line 38 in d6f4bee

internal static readonly Assembly s_urtAssembly = Assembly.Load("mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089");

Prior to #103255 BF would simply throw PNSE for them:

runtime/src/libraries/System.Runtime.Serialization.Formatters/src/System.Runtime.Serialization.Formatters.csproj

Line 3 in a82225e

<TargetFrameworks>$(NetCoreAppCurrent);$(NetCoreAppCurrent)-browser;$(NetCoreAppCurrent)-ios;$(NetCoreAppCurrent)-tvos;$(NetCoreAppCurrent)-android</TargetFrameworks>

runtime/src/libraries/System.Runtime.Serialization.Formatters/src/System.Runtime.Serialization.Formatters.csproj

Lines 77 to 79 in a82225e

	<ItemGroup Condition="'$(TargetPlatformIdentifier)' != ''">
	<Compile Include="System\Runtime\Serialization\Formatters\Binary\BinaryFormatter.PlatformNotSupported.cs" />
	</ItemGroup>

@bartonjs should we change the OOB package to keep throwing PNSE for WASM/Mobile?

[dotnet-policy-service]

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

[jkotas]

Most likely it's trying to load "mscorlib" which is not available for these platforms

mscorlib is available for these platforms.

I think that the problem that you are seeing is that these tests have trimming enabled when running on mobile/WASM, but binary serialization is incompatible with trimming. The trimming strips mscorlib since it does not see it used anywhere. I expect that you would see similar problems if you enabled these tests e.g. on native AOT on any platforms.

[eiriktsarpalis]

@adamsitnik @bartonjs If I'm understanding the failing stack trace correctly, it is not product code from NrbfReader that is failing in these platforms but BinaryFormatter which is being used to generate test inputs. I see that the test class is already conditioned on BinaryFormatter being supported, so presumably some platforms are being missed out in the relevant platform detection logic?

Assuming NrbfReader is supported in trimmed/AOT applications perhaps a better solution longer term is to precompute the blobs and store them as files in the test project.

[adamsitnik]

If I'm understanding the failing stack trace correctly, it is not product code from NrbfReader that is failing in these platforms but BinaryFormatter which is being used to generate test inputs. I see that the test class is already conditioned on BinaryFormatter being supported, so presumably some platforms are being missed out in the relevant platform detection logic?

That is correct. IMO we should make sure that these tests are disabled on WASM/Mobile but also change the new OOB package to throw PNS rather than FileNotFound.

[eiriktsarpalis]

Unless I'm missing something the detection logic already excludes these platforms:

runtime/src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs

Lines 734 to 737 in 0f2ce10

	else if (IsNativeAot || IsBrowser || IsMobile)
	{
	return false;
	}

cc @dotnet/dotnet-webassembly-admin

[adamsitnik]

Unless I'm missing something the detection logic already excludes these platforms:

It does. But I am not sure if the line that follows this if block won't try to load the referenced type anyway (and run the static ctor and produce the FileNotFound exception wrapped by TypeInitializationException).

runtime/src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs

Line 739 in 0f2ce10

Assembly assembly = typeof(System.Runtime.Serialization.Formatters.Binary.BinaryFormatter).Assembly;

@eiriktsarpalis I may be paranoid here and it may be working as expected, just sharing my thoughts here.

[adamsitnik]

System.Formats.Nrbf.Tests is not listed here, so the condition most likely works as expected:

runtime/src/libraries/tests.proj

Lines 437 to 440 in c4624d0

	<ItemGroup Condition="'$(TestNativeAot)' == 'true' or '$(TargetsMobile)' == 'true'">
	<ProjectExclusions Include="$(MSBuildThisFileDirectory)System.Resources.Extensions\tests\BinaryFormatTests\System.Resources.Extensions.BinaryFormat.Tests.csproj" />
	<ProjectExclusions Include="$(MSBuildThisFileDirectory)System.Resources.Extensions\tests\CompatTests\System.Resources.Extensions.Compat.Tests.csproj" />
	</ItemGroup>

[eiriktsarpalis]

I think I misunderstood the issue. It's not that it's failing in CI but rather it was failing when you were working on the tests and you then got disabled in ec45dad. I don't think there is much that we can do for .NET 9. In the future can try to enable the tests in those platforms by precomputing the BF blobs ahead of time.

[adamsitnik]

I don't think there is much that we can do for .NET 9.

BinaryFormatter used to throw PNSE on WASM before our most recent changes, now it's throwing FileNotFound exception (when the new OOB package is installed). Should we unify that to always throw PNSE (no matter if the package is installed or not?)?

[eiriktsarpalis]

Ok, so you're saying that issue is with the BinaryFormatter product and not with the impacted test or the product that the test is testing.

[dotnet-policy-service]

Tagging subscribers to 'binaryformatter-migration': @adamsitnik, @bartonjs, @jeffhandley, @terrajobst

[jkotas]

now it's throwing FileNotFound exception

You will get this behavior anywhere trimming is enabled. It is not specific to WASM. You will see it even with e.g. regular CoreCLR when you enable trimming.

BinaryFormatter used to throw PNSE on WASM before our most recent changes

These is nothing fundamental preventing the binary formatter OOB package from working on WASM (if you disable trimming). The app will be big, but it should work.

BinaryFormatter used to throw PNSE on WASM before our most recent changes

This behavior was introduced when we were chipping on the binary formatter without understanding the desired state. I do not think it makes sense to keep it.

[eiriktsarpalis]

I see the BF APIs are already annotated RUC/RDC as well so I think this type of error is par for the course. We could try improving the error message for that particular failure but then other runtime errors related to trimming would pop up elsewhere. STJ has similar behaviour in this context.

cc @bartonjs for a third opinion

[bartonjs]

should we change the OOB package to keep throwing PNSE for WASM/Mobile?

Not by going back to dual-compile. That was an easy pill to swallow for inbox, but it's a pretty big cost to increase the package size to make one "doesn't work" turn into another.

That said, changing PNSE into TypeInitializationException is a breaking change... so we should make a best-effort at fixing it (and if we can't, then doc it as a breaking change)

[jkotas]

PNSE into TypeInitializationException is a breaking change

It is not a breaking change in this case. We make no guarantees about behavior of trim-incompatible APIs in trimmed apps. The trim-incompatible APIs can do anything. The API behavior can even change due to unrelated changes in the app. It is by design.

[terrajobst]

From what I understand this situation is as follows:

• If trimming is not enabled, BinaryFormatter will throw PlatformNotSupportedException
• If trimming is enabled, using BinaryFormatter can result in FileNotFoundException
• WASM happens to have trimming enabled by default

If that's correct then I agree with @jkotas and @eiriktsarpalis: since BinaryFormatter is already attributed as [RequiresDynamicCode, RequiresUnreferencedCode] it's essentially marked as not trimming safe, so it feels like the observed behavior is by-design.

[jkotas]

Nit - I think that the situation is as follows:

• If the BF OOB package is not referenced, BinaryFormatter will throw PlatformNotSupportedException
• If the BF OOB package is referenced and trimming is not enabled, BinaryFormatter should work fine.
• If the BF OOB package is referenced and trimming is enabled, using BinaryFormatter can result in FileNotFoundException
• WASM happens to have trimming enabled by default

[terrajobst]

Fair enough. I think it doesn't change my conclusion -- still feels fine to me.

[eiriktsarpalis]

Sounds good. I'll leave this issue open to track potentially making the NrbfReader tests work in these platforms using precomputed blobs.