Microsoft.Toolkit.Uwp.Notifications.dll needs to be included in .NET Native runtime directives

[mfeingol]

Describe the bug

When a UWP app is linked using .NET Native and shipped to the store, toast notifications built using Microsoft.Toolkit.Uwp.Notifications.dll stop working. They still appear, but they lose all context provided by the user and show up as:

$AppName
New Notification

The workaround is to include the notifications assembly in rd.xml, like so:

<Assembly Name="Microsoft.Toolkit.Uwp.Notifications" Dynamic="Required All" />

• Is this bug a regression in the toolkit?

Yes, because this used to work without unnatural acts. But unclear when that was.

Steps to Reproduce

1. Build a simple app that uses Microsoft.Toolkit.Uwp.Notifications to trigger notifications
2. Confirm it works
3. Compile it with .NET Native (debug builds are fine)
4. Confirm all context is lost in notifications
5. Add the directive mentioned above. Confirm it works again.

Expected behavior

The Toolkit NuGet package should provide an rd.xml that just works, so apps don't have to figure this out themselves.

cf https://devblogs.microsoft.com/dotnet/net-native-deep-dive-making-your-library-great/

Screenshots

Environment

Package Version(s): 6.0.0

Windows 10 Build Number:

• Fall Creators Update (16299)
• April 2018 Update (17134)
• October 2018 Update (17763)
• May 2019 Update (18362)
• Insider Build (build number: )

App min and target version:

• Fall Creators Update (16299)
• April 2018 Update (17134)
• October 2018 Update (17763)
• May 2019 Update (18362)
• Insider Build (xxxxx)

Device form factor:

• Desktop
• Xbox
• Surface Hub
• IoT

Visual Studio

• 2017 (version: )
• 2019 (version: )
• 2019 (version: 16.4.2)

[michael-hawker]

@andrewleader any thoughts on what may have happened here?

[andrewleader]

@michael-hawker no idea, and I'm not aware of or familiar with rd.xml and the new .NET Native runtime directives :/

[mfeingol]

Anything I can do to help with this one?

[michael-hawker]

@mfeingol there are new changes in the PR #3256, but I don't think they'd be related to this, so not sure if it's worth testing this scenario again with those.

But do you happen to know which version you used before where it last worked? Knowing if you have a version of the toolkit and NetCore base NuGet packages that do work would give us a comparison point to see what might have changed since then.

@MattWhilden any advice/thoughts here on what might have changed? I'm assuming we may have upgraded the NetCore package, so is there some config change we need to make on our end?

[mfeingol]

Unfortunately, I don't, sorry. There was a stretch of maybe 2-3 months before the moment I filed this GHI where the notifications feature wasn't really being used by anyone. I tend to stay current on toolkit updates, so that may provide you with a rough timeline.

Best guess is something in the library started using reflection that wasn't before.

If it helps, btw, I could put together a small repro project for you.

[michael-hawker]

@mfeingol a small repo project would be super valuable, especially if we need to get insight from another team about other parts of the platform, makes it easy to hand-off to them without them needing to understand how to build the full toolkit.

[mfeingol]

Here you go:

https://github.com/mfeingol/repros/tree/master/NotificationsRepro-3093

Note the difference between the toasts when .NET Native is enabled vs disabled. You can enable .NET Native in project build properties on the debug build, or just compare debug vs. release.

[michael-hawker]

@azchohfi is this something you could take a look at?

[azchohfi]

I'll look at this now.

[azchohfi]

I could not repro this with v6.0.0 using UniversalWindowsPlatform 6.2.10. A Release build showed the notification exactly the same as a Debug build.

I tried both x86 as well as x64.
@michael-hawker or @Kyaa-dost , can you test this?

[mfeingol]

@azchohfi: I'm still seeing a repro here.

Please make sure you're using .NET Native.

[azchohfi]

I am using .Net Native. That box is marked by default on Release Builds, but I did double check.

[mfeingol]

I'm not sure how to explain the difference then. It's 100% repro here, release or debug, as long as .NET Native is enabled.

[michael-hawker]

@azchohfi I see the repro on my box (x64, 2004/19041 with VS 2019 16.5.5) when switching from Debug to Release, are you on Insiders?

Debug

Release

I downgraded the Toolkit NuGet to 5.1.1 and saw the same issue though (as well as our latest 6.1 preview bits):

I tried changing the NetCore UWP package, even all the way back to 6.0.1, but saw the same behavior.

@MichalStrehovsky & @tommcdon any thoughts on what's going on here?

[andrewleader]

I can't seem to repro this either, @mfeingol do you have a full sample you can share on GitHub or somewhere?

[mfeingol]

@andrewleader: https://github.com/mfeingol/repros/tree/master/NotificationsRepro-3093

[andrewleader]

Thank you!! Trying that repro now

[mfeingol]

No worries. I just double-checked and the repro is still valid with the latest .NET Native and Toolkit nuget packages.

Run it with and without .NET Native and you'll see a difference in how the toast content is displayed.

[andrewleader]

Thanks, your repro indeed did repro, but the same code snippet in another project that was targeting SDK 19041 with min ver 17134 worked fine in release .NET native... So still investigating to find the exact conditions that cause the repro just to fully understand scope before applying fix. Thank you so much!

[mfeingol]

That's interesting. I just tried my repro project with the following combinations and all seemed to reproduce the problem:

17763 / 18362 => repro
17763 / 19041 => repro
17134 / 19041 => repro
16299 / 19041 => repro

[michael-hawker]

@andrewleader were you testing against your latest changes (which I assume would have an impact here)? #3622

@mfeingol were you testing against the latest package on NuGet or one of our Preview Packages?

[andrewleader]

Nah I was using 6.0.0 to try to repro... still confused with what the difference is but slowly debugging in between other work

[andrewleader]

@mfeingol I figured it out!

Within Solution Explorer Properties -> Default.rd.xml, your project has the following line commented out

<Assembly Name="*Application*" Dynamic="Required All" />

However, when creating a new project today, that line is enabled by default (not commented out). Since Michael also repro'd this, maybe there was a bug in VS where they weren't including this enabled by default? However, new projects seem to be working fine now.

Given that things work now with current versions of VS, I'm not sure we should change anything? What do you propose @mfeingol? If we could add something in the nuget package so it works for everyone even if they have that commented out, I'd be open to that, but at least we now know what the problem is!

[mfeingol]

@michael-hawker: I was testing with the latest release toolkit package, 6.1.1.

@andrewleader: what you observed is accurate. However, most UWP apps need to remove that blanket directive because leaving it in place causes the .NET Native linker to include everything, leading to very large package sizes for non-trivial apps.

The workaround is to let the linker do its job by removing the directive, and then manually specifying directives to include specific units of code (assemblies, namespaces, classes) when the linker breaks the app by excluding that code. A trivial example of this would be a class that appears to be unreferenced, but is called from elsewhere via reflection. At runtime, this would result in a MissingMethodException or some such.

Now, the maintainers of libraries that behave this way (i.e. break without a linker directive) have two choices: they can fail in these scenarios, leaving it up to app owners to detect and diagnose the issue, or they can do enough homework to put together their own manifest with an appropriately scoped directive. E.g. you may not need to force-include the entire toolkit assembly, just the bits that disappear when linked even when in use.

So the ask here for you is to include a manifest in your assembly that makes it unnecessary for people using your library to do all this work. I believe this is documented in https://devblogs.microsoft.com/dotnet/net-native-deep-dive-making-your-library-great/

Does that make sense?

[michael-hawker]

@andrewleader yeah, I don't remember the setup I had in the repo I did way back. Makes sense though, as I believe that's what @mfeingol pointed to initially.

We've typically not done any rd.xml optimizations at the library level for .NET Native in the Toolkit, we've expected app developers to include the default rd.xml settings or optimize themselves for their specific scenarios. We're exploring this a bit more now with #3145, but there'll eventually be a path to .NET 5+ in the future which will work differently, so we're still not trying to micro-optimize.

@andrewleader do you know the serialization types that are missing that are getting optimized out for this scenario? If not, it's going to be a bit of a black box I believe.

@mfeingol would you expect it to be optimized or if we just included a blanket namespace covering one for the package, would that be sufficient for your case where you've removed the default configuration? As I believe it could still be overridden. (Doc Ref on rd.xml config)

@azchohfi @vgromfeld @Sergio0694 any thoughts in this space?

[mfeingol]

I think with AOT on the horizon (hopefully?) there's no need to micro-optimize. I recommend applying a blanket workaround for the affected assembly in your own rd.xml:

        <Assembly Name="Microsoft.Toolkit.Uwp.Notifications" Dynamic="Required All" />

[vgromfeld]

By default, I would favor not doing anything in the toolkit's rd.xml file to not bloat the apps which are not using this specific part of the code.
.Net Native is sometimes requesting a lot of binary code.

On the other side, with the current split being made to isolate each part of the toolkit in dedicated projects, that could make sense to have one default directive for the Microsoft.Toolkit.Uwp.Notifications project as long it does not do anything else than generating toasts.

[andrewleader]

Sounds like consensus is forming around not making any changes, correct? I'm good with that (my apps personally have just used the default rd.xml personally).

And no, I currently don't know the specific serialization types that are getting optimized out.

[mfeingol]

I'm not sure I understand the argument for doing nothing. Microsoft.Toolkit.Uwp.Notifications is its own library, and the directive I mentioned above is scoped to it.

Apps that don't use notifications won't be linking in Microsoft.Toolkit.Uwp.Notifications anyway, so their apps don't get bigger with the directive in place. Apps that do use notifications will either link everything in, in which case things again don't get worse, or they'll need to debug the problem and add that same directive themselves.

So I guess I don't see what the scenario is for the directive making things worse?

[andrewleader]

Thanks for pushing back on this!

@michael-hawker I submitted a PR, seems like super easy fix (I verified it) and doesn't add any bloat or anything that I'm aware of. Other teams have done the same thing to address this issue: https://github.com/OfficeDev/Open-XML-SDK/pull/241/files

[andrewleader]

Thanks @mfeingol again for filing this issue, we've resolved it with the PR! Apps should now be able to use the notifications toolkit with .NET Native regardless of what they've configured in their own Default.rd.xml :)

[mfeingol]

Greatly appreciated, @andrewleader. Thank you.