Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Fix AssemblyName cache hash and key#24138

Merged
sdmaclea merged 6 commits into
dotnet:masterfrom
sdmaclea:Fix24135
Apr 21, 2019
Merged

Fix AssemblyName cache hash and key#24138
sdmaclea merged 6 commits into
dotnet:masterfrom
sdmaclea:Fix24135

Conversation

@sdmaclea
Copy link
Copy Markdown

@sdmaclea sdmaclea commented Apr 20, 2019

Fixes #24135

  • AssemblyName::Hash(DWORD dwIncludeFlags) was including should be ignored attributes in its hash.
  • AssemblyNative::Load was using stackMark in a case where it wasn't needed.
  • Compiler started erroring because RuntimeAssembly.m_assembly was unsused in C#. It is used in native code. Disable the warning.
  • Add ContextualReflection LoadWithPartialName test case.
  • Remove unnecessary NoInlining attribute
  • Disable broken CoreFX test case Fix DefaultLoadContextTest corefx#37071

@sdmaclea sdmaclea added this to the 3.0 milestone Apr 20, 2019
@sdmaclea sdmaclea requested a review from jkotas April 20, 2019 06:12
@sdmaclea sdmaclea self-assigned this Apr 20, 2019
Comment thread src/System.Private.CoreLib/src/System/Reflection/RuntimeAssembly.cs
@jkotas
Copy link
Copy Markdown
Member

jkotas commented Apr 20, 2019

src/System/Reflection/Assembly.CoreCLR.cs(40,20): error CS0246: The type or namespace name 'FileNotFoundException' could not be found (are you missing a using directive or an assembly reference?)

@jkotas
Copy link
Copy Markdown
Member

jkotas commented Apr 20, 2019

Do you plan to add any tests for this?

@sdmaclea
Copy link
Copy Markdown
Author

Do you plan to add any tests for this?

The Assembly.LoadWithPartialName test is completely missing from the ContextualReflection test. I will add it.

I expected the ContextualReflections test to catch the Assembly.Load(string) failure, but it didn't. The corefx rewrite of the test did however. I need to look closer at it, to figure out why this use case was missed. If/when I can repro, I'll edit/add the test.

I am adding tests in corefx. These found the issue, but since they are not yet passing they are disabled until these bits make it.

@sdmaclea sdmaclea changed the title Remove RuntimeAssembly.CreateAssemblyName WIP Remove RuntimeAssembly.CreateAssemblyName Apr 20, 2019
@sdmaclea
Copy link
Copy Markdown
Author

Need to diagnose the root cause further. This is not fixing the observed issue.

@sdmaclea
Copy link
Copy Markdown
Author

Spent a lot of time in debugger root causing this:

  • The binder cache was not finding assemblies already loaded because the key lookup was failing.
  • The AssemblyName Hash function was using the public_key and public_token when it wasn't supposed to.
  • Also we were populating the parent assembly using the stack mark when we didn't need to. This led to cases where the wrong binder_id was used as the key.

I have added the LoadWithPartialName test. The corefx test will catch the binder cache issue. I used that to debug the failure. I should be able to create a lighter test, but I'll put it in a separate PR.

I would like this fix to make preview5.

@sdmaclea
Copy link
Copy Markdown
Author

There are two "regressions" in the test runs.
https://github.com/dotnet/corefx/blob/13cd96122c25328a1b180a6289923657180a7b69/src/System.Runtime/tests/System/ActivatorTests.netcoreapp.cs#L263-L265, Is testing a resolve event is fired when the assemblyName is illegal/malformed. That is the removed CreateAssemblyName function. I'll remove that change.

The other regression is.
https://github.com/dotnet/corefx/blob/13cd96122c25328a1b180a6289923657180a7b69/src/System.Runtime.Loader/tests/DefaultContext/DefaultLoadContextTest.cs#L260-L270

The test is bad. It is regressing because the assembly cache is now finding the System.Runtime assembly which was loaded before clearing the LoadedFromContext flag. So this assert is failing. Moving the clear earlier in the function seems to resolve the failure.

@sdmaclea
Copy link
Copy Markdown
Author

This looks like it will miss preview5.

@sdmaclea sdmaclea changed the title WIP Remove RuntimeAssembly.CreateAssemblyName Fix AssemblyName cache hash and key Apr 21, 2019
@jkotas
Copy link
Copy Markdown
Member

jkotas commented Apr 21, 2019

resolve event is fired when the assemblyName is illegal/malformed

FWIW, I would not mind losing this "feature". Illegal/malformed assembly names will fail everywhere else, except this one place that tries to recover from them. It can be done separately / later.

Copy link
Copy Markdown
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Test is failing due to a logic error.
Fix is pending in dotnet/corefx#37071
@sdmaclea sdmaclea merged commit a36bc61 into dotnet:master Apr 21, 2019
@sdmaclea sdmaclea deleted the Fix24135 branch June 10, 2021 00:29
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
* Add ContextualReflection LoadWithPartialName case

* Remove unnecessary MethodImplOptions.NoInlining

* Remove m_assembly warning

* Fix AssemblyName hash function

* AssemblyNative::Load fix stackMark usage

Do not use the stackMark if (ptrLoadContextBinder != NULL)

* Temporarily disable DefaultContextOverrideTPA

Test is failing due to a logic error.
Fix is pending in dotnet/corefx#37071


Commit migrated from dotnet/coreclr@a36bc61
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Assembly.Load(string assemblyName) not using StackMark in all cases

2 participants