Fix refcount when a composite r2r image is loaded from a bundle

[VSadov]

Re: #60308

Erroneous use of ref counting API introduced in recent refactoring change - SuppressRelease vs. AddRef.

Note: the test in SDK must be re-enabled after this propagates to SDK. I have tested the end-to-end scenario manually.

[ghost]

Tagging subscribers to this area: @agocke, @vitek-karas, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Re: #60308

Erroneous use of ref counting API introduced in recent refactoring change - SuppressRelease vs. AddRef.

Note: the test in SDK must be re-enabled after this propagates to SDK.

Author:	VSadov
Assignees:	-
Labels:	area-Single-File, area-VM-coreclr
Milestone:	-

[VSadov]

cc: @elinor-fung - can you take a look? I think you'd have the best context for the code review.

[elinor-fung]

My reading of the issue is that we were using NewHolder instead of PEImageLayoutHolder/ReleaseHolder, which meant that SuppressRelease wouldn't prevent the delete when the NewHolder went out of scope.

We could have switched to PEImageLayoutHolder and kept what this was doing before (using SuppressRelease). However, with the switch to calling PEImage::OpenImage with MDInternalImport_NoCache, the pImage will not be cached, so it (and therefore the layout created through GetOrCreateLayout) will be released once it goes out of scope - hence we need to do the AddRef instead so that we keep the PEImageLayout around without the PEImage.

Is that correct?

[elinor-fung]

If so, it might be worth having an explicit comment around that. My initial reaction looking at this was that it would make more sense to keep with the SuppressRelease (instead of the explicit AddRef and let the holder release) - before I saw the implications of the switch to MDInternalImport_NoCache.

[VSadov]

We could have switched to PEImageLayoutHolder and kept what this was doing before (using SuppressRelease).

Yes, that would work too. SuppressRelease would effectively "leak" the PE image and that would keep the layout alive. We would not have a way to free the layout, but in this case it would be ok.

On the other hand we only need this PE image to get the layout and we will not be doing that again, since AppDomain caches the native image. I figured I'd rather take the ownership of the layout (by doing AddRef) and let PE image instance be freed.

Using cache is not required here since composite r2r PE is not a part of anyone's identity.
Since we will open the image only once anyways, I just thought we should not involve the cache, regardless if we keep/leak PE image or not.

[VSadov]

I will add a better comment about the whole deal.

[VSadov]

Added comments.

[am11]

Does it fix #54234 as well? i.e. can re-enable:

runtime/src/installer/tests/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/BundledAppWithSubDirs.cs

Lines 231 to 236 in 577a70a

	// ACTIVE ISSUE: https://github.com/dotnet/runtime/issues/54234
	// uncomment extraArgs when fixed.
	outputDirectory: BundleHelper.GetPublishPath(TestSelfContainedFixtureComposite) /*,
	extraArgs: new string[] {
	"/p:PublishReadyToRun=true",
	"/p:PublishReadyToRunComposite=true" } */);

or is it possible to add a separate test in AppHost.Bundle.Tests?

[VSadov]

@am11 - no, #54234 is a test issue. It is actually pretty hard to arrange test environment at runtime repo to test end-to-end building of a singlefile app with composite r2r. We would need to emulate SDK and there are too many parts involved.

There are some ideas on how to get around that, perhaps by replacing msbuild with some tool that tests could use without complete SDK-like environment - like Bundler, but for building. Even a "simple" replacement for msbuild will be quite complex though.

[VSadov]

Thanks!!