fix: PrintDialog not shown

[RussKie]

Resolves #2814

Proposed changes

The bug was caused by the conversion of a struct fields to auto-properties in #2594. This broke the interop binary layout.

Rework the code to

• correctly define x86 and x64 structs
• make the structs blittable
• add tests verifying the structs layouts

Customer Impact

• PrintDialog is now shown as expected

Regression?

• Yes

Test methodology

manual

• x86

dotnet restore --runtime win-x86
dotnet run --runtime win-x86

• x64

dotnet restore --runtime win-x64
dotnet run --runtime win-x64

Microsoft Reviewers: Open in CodeFlow

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@1d0a9f0). Click here to learn what that means.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master       #2851   +/-   ##
============================================
  Coverage          ?   60.24856%           
============================================
  Files             ?        1248           
  Lines             ?      433383           
  Branches          ?       38850           
============================================
  Hits              ?      261107           
  Misses            ?      166913           
  Partials          ?        5363           

Flag	Coverage Δ
#Debug	60.24856% <0.00000%> (?)
#production	32.45288% <0.00000%> (?)
#test	98.97398% <ø> (?)

[weltkante]

By no longer having separate structs for 32bit/64bit you are breaking the layout here, 32 bit will add 2 byte padding here where none is expected.

[hughbe]

Relevant: https://github.com/tpn/winsdk-10/blob/9b69fd26ac0c7d0b83d378dba01080e93349c2ed/Include/10.0.10240.0/um/commdlg.h#L49

[weltkante]

Yes, original source also had this in comments/attributes, 32bit uses Pack=1, 64bit uses default packing. Since the headers explicitely switch packing depending on bitness you probably can't avoid having two layouts, at least I can't think of any single definition compatible with both.

[hughbe]

Could have #if WIN32 or something?

[weltkante]

#if is a compile-time directive, I don't think you want to compile different C# assemblies for 32/64 bit do you? Usually you only have one anycpu assembly for .NET

[RussKie]

Thank you for pointing this one out. Very insightful.
I have built an x86 variant, and sure enough, it didn't work.

[RussKie]

I asked internally and there is no desire to build leaf-node stacks per arch, as it is very involved. The binaries get cross-gened for each setup bundle.
This means we must have two different versions of the struct (due to different packing requirements), and jump through hoops to populate the payload and invoke the underlying Win32 API.

[RussKie]

Thank you for the reviews. This was a quick EOD POC to see it fixed. More cleanups are in order, as you rightly pointed.

[RussKie]

Is there a more optimal way of doing this?

[weltkante]

nothing I'm aware of - you could cache the function pointer in a static variable if the hook were a static method, but then you also need to cache the delegate to prevent it being collected. Overall I don't think its worth it, showing print dialogs isn't exactly a hot loop.

[weltkante]

actually I'm wondering if this needs a GC.KeepAlive on the delegate. There is other interop code running before the dialog is shown so there could be a small risk the GC collects the delegate before entering native code which uses the hook?

[weltkante]

couldn't get it to crash by inserting explicit GC.Collect in the code, so its probably fine, but I'm not sure why as the documentation explicitly says

You must manually keep the delegate from being collected by the garbage collector from managed code. The garbage collector does not track references to unmanaged code.

[RussKie]

Thank you for checking.

@JeremyKuhne any recommenadions?

I'll look into it on Monday

[JeremyKuhne]

Yes, you need to GC.KeepAlive the new object. One gotcha that got me many years ago is that collection behavior is different in debug versus release. When running a debug build your locals are scoped to the end of the frame (the method or it's parent method if it was inlined). I don't know precisely how the heuristics work and if there are any differences in Core. I'll try and make a point of finding out exactly how the frame GC reporting works, but depending on the implementation isn't the way to go. We need to go by the specification, which is as soon as you're no longer directly referencing a managed heap pointer, it's eligible for collection.

[RussKie]

Updated

[weltkante]

As noted here there are differences between blitting structs and classic marshalling. Your test cases are mixing up both styles so you test the size using classic marshalling and offsets using blitting layout. You may want to be consistent in what you test. (Though as far as I'm currently aware it can only differ if bool is used somewhere.)

• Marshal.SizeOf for classic marshalling size
• Marshal.OffsetOf for classic marshalling offsets
• sizeof keyword for blitting size
• pointer arithmetic for blitting offsets

Example where it makes a difference:

public struct Sample
{
    public int a;
    public bool b;
    public short c;
}

var sample = new Sample();
var blittingSize = sizeof(Sample);                              // 8
var blittingOffset = (byte*)&sample.c - (byte*)&sample;         // 6
var marshalSize = Marshal.SizeOf<Sample>();                     // 12
var marshalOffset = Marshal.OffsetOf<Sample>(nameof(Sample.c)); // 8

[RussKie]

Thank you 👍
It started as a non-blittable struct and I think that's when I started writing tests, and forgot to switch. Fixed now.

[RussKie]

@msftbot merge in 24 hours