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

Conversation

@ojhad
Copy link

@ojhad ojhad commented Jun 10, 2019

In order to break the System.Security.Permissions dependency WPF has, we need to move the public types defined in WPF into the System.Security.Permissions.dll. All the types are hollowed out with the exception of XamlAccessLevel since there is data being stored and retrieved there that isn't purely about permissions, so this is done to avoid potential breaks.

@vatsan-madhavan
Copy link
Member

Related: dotnet/wpf#241

@vatsan-madhavan
Copy link
Member

Can you help us understand your comment about XamlAccessLevel ? In particular, is there a specific reason why we can't be more aggressive about removing the functionality exposed by this type?

What happens if AssemblyAccessTo always returned null, for e.g.?

Copy link
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

Fix up the netfx / package build

@ojhad
Copy link
Author

ojhad commented Jun 10, 2019

Can you help us understand your comment about XamlAccessLevel ? In particular, is there a specific reason why we can't be more aggressive about removing the functionality exposed by this type?

What happens if AssemblyAccessTo always returned null, for e.g.?

When attempting to hollow out this type in WPF, I discovered that XamlAccessLevel was actually not being exclusively used with XamlLoadPermission. As one specific example, XamlAccessLevel was actually being used to pass the name of an assembly which in turn was used to load an assembly by the XamlRuntime. Here's what I am referring to:
https://github.com/dotnet/wpf/blob/ed9f2dabfde96104e13b91108ae186ffa336acae/src/Microsoft.DotNet.Wpf/src/System.Xaml/System/Xaml/Runtime/DynamicMethodRuntime.cs#L159

While this will need to be fixed in WPF regardless, I decided to keep the functionality intact since this could be a potential breaking change in other places where this type was used to pass contained data.

@stevenbrix
Copy link
Contributor

Can you explain more on what the end user experience here is? When we spoke offline it seemed that hollowing out these types broke their core functionality, and so any app that was using them would be broken moving to .NET Core. Is that the experience we will still have now? Or is this what you are referring to with XamlAccessLevel?

@ericstj
Copy link
Member

ericstj commented Jun 12, 2019

You don’t need to spend a time of effort on these. We don’t want to give folks any illusion that these types are functional. For details you can see https://github.com/dotnet/corefx/issues/9482 and related commits.

@ojhad
Copy link
Author

ojhad commented Jun 12, 2019

Can you explain more on what the end user experience here is? When we spoke offline it seemed that hollowing out these types broke their core functionality, and so any app that was using them would be broken moving to .NET Core. Is that the experience we will still have now? Or is this what you are referring to with XamlAccessLevel?

When we spoke offline, I was specifically referring to what I mention above with XamlAccessLevel. Hollowing out the other permission types should not break any of the core functionality as they now are essentially no-ops.

@ojhad ojhad marked this pull request as ready for review June 12, 2019 23:19
Copy link
Member

@vatsan-madhavan vatsan-madhavan left a comment

Choose a reason for hiding this comment

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

See comments re PublicKeyToken

@ericstj ericstj requested a review from bartonjs June 13, 2019 19:24
Copy link
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

Please add basic tests along the same line as what already exists for the other dummy types.

Copy link
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

Thanks!

@ericstj ericstj added the auto-merge Automatically merge PR once CI passes. label Jun 14, 2019
@ghost
Copy link

ghost commented Jun 14, 2019

Hello @ericstj!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost removed the auto-merge Automatically merge PR once CI passes. label Jun 14, 2019
@bartonjs bartonjs merged commit df5bb8e into dotnet:master Jun 14, 2019
@stevenbrix
Copy link
Contributor

stevenbrix commented Jun 17, 2019

You don’t need to spend a time of effort on these. We don’t want to give folks any illusion that these types are functional. For details you can see #9482 and related commits.

@ericstj yeah I understand that. I was trying to understand what the developer experience is for porting an app to .NET Core when their app uses these types. Is it expected that the app just breaks when the developer presses F5? Does the developer get an exception with an error message, or does some functionality in their app that was depending on these just no longer work?

@danmoseley
Copy link
Member

I was trying to understand what the developer experience is for porting an app to .NET Core when their app uses these types. Is it expected that the app just breaks when the developer presses F5? Does the developer get an exception with an error message, or does some functionality in their app that was depending on these just no longer work?

I assume the expectation is that everything works essentially as if the app was running in full trust, ie., all demands always succeed. Is that not what will happen?

@vatsan-madhavan
Copy link
Member

I assume the expectation is that everything works essentially as if the app was running in full trust, ie., all demands always succeed. Is that not what will happen?

That's the expectation, modulo the fact that CAS-the-functionality in itself will no longer work - which is old news for .NET Core (and really, old news even for .NET Framework), but this will be the first time many WPF developers will encounter this shift. All first-order breaks would be graceful-degradations (like demands silently succeeding as a no-op behind the scenes)

There can be second order breaks that only affect niche/advanced scenarios. For e.g., say someone has implemented a type that is similar to XamlLoadPermission (this one is sealed, so they couldn't have derived from it) that uses/depends on XamlAccessType. They'd have to adjust their implementation a bit to successfully port (preferably, throw away their *Permission type entirely). These sorts of "breaks" are edge cases.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants