Type Safe Marshaling of Objects Between .NET and Javascript #3138
Conversation
- Adds a type safe binder that can reliably turn Javascript objects back into .NET types without forcing the .NET and Javascript code to break from their standard conventions. It also has better support for Enums, .NET only types, and catches undefined behavior - Adds a type safe interceptor that will handle serializing the return values of .NET methods (both synchronous and asynchronous) to Javascript compatible values. That includes .NET only types like Guid and Tuple. - Adds a new TypeBindingException that will allow developers to catch errors in their code such as nulls or data type mismatches. - Adds async execution support for IMethodInterceptor
|
❌ Build CefSharp 81.3.100-CI3539 failed (commit d28d50f189 by @Codeusa) |
|
❌ Build CefSharp 81.3.100-CI3540 failed (commit 418d4633e1 by @Codeusa) |
|
❌ Build CefSharp 81.3.100-CI3541 failed (commit 5eb57a481b by @Codeusa) |
|
@amaitland if I can make one suggestion -- .NET 4.5.2 is no longer supported by Microsoft and was released in 2013. Windows 7 is no longer supported by Microsoft. CefSharp should bump it's target to 4.7.2 because perpetual backwards compatibility means not being able to take advantage of what the language has to offer today. |
|
@Codeusa Thanks, looks interesting 👍 I'll have a look and respond in detail hopefully tomorrow. |
There was a problem hiding this comment.
Fixes:
#3129
From what I can tell your IBinder implementation would throw an exception as detailed in #3129 You can run it through the unit test at ef59044#diff-d5a2c335de34e13e7d04a3681643f9ccR125 if you are interested.
Screenshots (if appropriate):
Looks like you have some test code or unit tests, are you able to provide the code to use as a basis for some unit tests?
- Nulls for non-nullable types and other undefine behavior raise detailed exceptions to developers so they can fix their code.
Null for value types is a tricky one, it's kind of an opinionated topic, I'm actually in favour of assigning the default value for a type. e.g. null for an integer maps to 0. This is the current behaviour for the 81.3.100 release. Your TypeSafeBinder implementation is largely copied from the DefaultBinder which previously only converted null to the default value when part of a collection reference https://github.com/cefsharp/CefSharp/pull/3138/files#diff-68f2409c5b26e3887e39a37dcf038496R131
- Security has been introduced to prevent immutable types from being marshaled.
Seems reasonable, thanks 👍 We should update the DefaultBinder, probably use a common base class (possibly your implementation can replace the default implementation, I need to compare and determine impact on existing users).
For extra "safety" I'll open a separate pull request for a TypeScript code generator.
What dependencies does it have exactly? Probably needs to be in it's own class library and published as a separate Nuget package if it's going to have additional dependencies.
Because our build of CefSharp targets .NET 4.7.2 we have the luxury of some modern .NET features
If the code would benefit greatly from a newer .Net version then we should look at a separate class library and publishing a different Nuget package, can probably go with the TypeScript generator.
Other than that, we applied only the code that could be introduced without making breaking changes
Unfortunately there are breaking changes, you've added new methods to the public API, the javascript naming is now potentially different. The current camelcase conversion is basic and yours looks like an excellent improvement, there is however scope for the generation of a different name after upgrading to a version that includes this code.
Which probably isn't entirely a bad thing because using this code in production, we've eliminated almost all issues you would expect to get from sending complex objects from a typed language, to an untyped language, and back again.
Your binder implementation looks like it was based off the DefaultBinder, I'm hopeful we can at a minimum use a common base class to reduce code duplication.
- The formatting is consistent with the project (project supports .editorconfig)
To be consistent the current code uses braces for even single line if/case blocks. I'm happy to fix this up after merge.
.NET 4.5.2 is no longer supported by Microsoft
Do you have an official Microsoft reference that states it's no longer supported?
Windows 7 is no longer supported by Microsoft.
It's my understanding that .Net 4.5.2 is tied to the lifespan of Windows 8.1. The .Net Product Lifecycle has .Net 4.5.2. listed as a OS component, Windows 8.1 shipped with .Net 4.5.1 as per https://docs.microsoft.com/en-us/dotnet/framework/get-started/system-requirements#supported-client-operating-systems which was replaced with .Net 4.5.2 as the supported OS Component. A quick check and others have a similar opinion see https://stackoverflow.com/questions/58732338/when-is-end-of-life-for-dotnet-framework-4-5-2
If you've got something from MS that states otherwise then I'd be very interested in a link.
CefSharp should bump it's target to 4.7.2 because perpetual backwards compatibility means not being able to take advantage of what the language has to offer today.
If it was just up to my personal preference then I'd be all in favour of updating. Have to consider the existing user base, a large percentage of developers have set version requirements and cannot upgrade to a newer version. I have previously stated that I'd stick with the Minimum supported version of .Net. I still get the occasional request for support for .Net 4.0 and .Net 4.5.
There are actually people backporting CefSharp to older versions of .Net e.g. https://github.com/Secbyte/CefSharp.Net40
| /// This setting has no backwards compatibility with the default binding behavior, so if you built code around it enabling this will probably break app. <br/> <br/> | ||
| /// All bound objects and CefSharp post will use a internal TypeSafe binder and interceptor to passively handle objects. | ||
| /// </summary> | ||
| public static bool TypeSafeMarshaling { get; set; } |
There was a problem hiding this comment.
Using this sort of global static property for things related to javascript binding was a mistake my part, it's difficult to test and not very flexible. I think the cleanest and simplest way forward is to add a static property or method to BindingOptions that creates the binder and interceptor.
There was a problem hiding this comment.
I agree this is the best approach, I've never been a fan of massive configuration files since it increases the surface area of your conditional checking.
| @@ -190,15 +191,17 @@ public void Register(string name, object value, bool isAsync, BindingOptions opt | |||
| throw new ArgumentException("Registering of .Net framework built in types is not supported, " + | |||
| "create your own Object and proxy the calls if you need to access a Window/Form/Control.", "value"); | |||
| } | |||
|
|
|||
| var camelCaseJavascriptNames = options == null ? true : options.CamelCaseJavascriptNames; | |||
| // if TypeSafeMarshaling is enabled then we always set names to camelCase | |||
There was a problem hiding this comment.
I'd prefer not to make any changes in here other than improving the property naming.
- Testing becomes more difficult
- Easily get users who've enabled the setting and registered the object as
syncwondering why the code isn't behaving. I understand it's documented, there will be more than a few who won't read it.
| @@ -314,6 +317,14 @@ internal bool TryCallMethod(long objectId, string name, object[] parameters, out | |||
| } | |||
| else | |||
| { | |||
| // anyone extending the Method Interceptor should be able to use Task | |||
| if (obj.MethodInterceptor.IsAsynchronous) | |||
There was a problem hiding this comment.
I'm not really keen on this either, in this case specifically the code would just as easily be in the TypeSafeInterceptor implementation without having to make any breaking changes to the API.
We could potentially split the async handling out into
- A decorator that holds all the
asyncspecific functionality. - A base class that's that has some protected virtual methods to override
We can then test that functionality in isolation and allow users to leverage that code in their custom implementation.
Using Castle.DynamicProxy as a model, I think we keep the interceptor as is and rely on the implementation to deal with the async nature. Reference https://github.com/castleproject/Core/blob/master/docs/dynamicproxy-async-interception.md
There was a problem hiding this comment.
@Codeusa I'll make my proposed code changes and push them to a branch for further discussion. Hopefully in the next day or so I'll have something to discuss 👍
Thanks for the review! I will look over your comments in the next few hours and reply. I also apologize if any of my comments inline came off as opinionated (bumping version, etc.)
There was a problem hiding this comment.
. I also apologize if any of my comments inline came off as opinionated (bumping version, etc.)
Totally fine 😄 Hopefully my response came across as diplomatic, personally I wish bumping version was a simple process that everyone would be onboard with. When I upgraded from .Net 4.0 to .Net 4.5.2 i copped quite a bit of flack from people. I'm hoping to avoid comments like #1785 (comment) if can be helped 😦
When I can find a few minutes I'll try install the Microsoft.Net.Compilers packages again, at least we'll be able to use the newer syntax if I can get it building.
| /// <summary> | ||
| /// Returns an enumerable sequence of bindable properties for the specified type. | ||
| /// Returns an enumerable sequence of bindable properties for the specified type. |
There was a problem hiding this comment.
Looks like some unnecessary whitespace has been added.
There was a problem hiding this comment.
Sorry, my code styling was applied for our main project and we use indented XML comments.
There was a problem hiding this comment.
If you use Edit -> Advanced -> Format Document it should apply the editorconfig style.
Personally I use https://marketplace.visualstudio.com/items?itemName=mynkow.FormatdocumentonSave
| return nativeType.CreateEnumMember(javaScriptObject); | ||
| } | ||
|
|
||
| // if the source object is null and is not an enum, then there isn't anything left to do. |
There was a problem hiding this comment.
The current implementation as of 83.1.100 will map null to the default value for a Value Type. I've had requests for this behaviour previously. See ef59044#diff-e2e8176d85173d54b9b3dec2acadac86R32
There was a problem hiding this comment.
We should probably make this a configurable option then. For example in our production use we ran into a bug and didn't realize the Javascript object was null because it was using the default of the Enum. There is some value in strict null checks from a development perspective.
There was a problem hiding this comment.
Sorry, somehow I totally missed this set of comments.
Configurable is fine with me. I can see people wanting to use either option.
| /// <param name="javaScriptObject">An instance of the Javascript object.</param> | ||
| /// <param name="nativeType">The type this method will try to create an instance of using the Javascript object.</param> | ||
| /// <returns>An instance of the native type.</returns> | ||
| public object Bind(object javaScriptObject, Type nativeType) |
There was a problem hiding this comment.
I think nativeType is a little ambiguous, what's the type native to? The interface param is targetParamType, I'd be happy with something else, modelType maybe?
There was a problem hiding this comment.
Well model is usually referring to a class of some sort, whereas this method can be used to bind primitives as well, so maybe destinationType is best?
There was a problem hiding this comment.
destinationType is fine with me 👍 Happy to update the interface and default implementation to match.
| var javaScriptType = javaScriptObject.GetType(); | ||
|
|
||
| // if the object can be directly assigned to the destination type, then return and let the runtime handle the rest. | ||
| if (nativeType.IsAssignableFrom(javaScriptType)) |
There was a problem hiding this comment.
From here on this method is pretty much identical to the DefaultBinder, to reduce code duplicate we should probably have a common base class if possible.
There was a problem hiding this comment.
Definitely, I based it around DefaultBinder as not to introduce a few dozen files into this pull. I also left out generic type resoling (e.g. Model) because it would have been a lot for one pull.
| if (javaScriptItem == null) | ||
| { | ||
| // for value types like int we'll create the default value and assign that as we cannot assign null | ||
| model.Add(genericType.IsValueType ? Activator.CreateInstance(genericType) : null); |
There was a problem hiding this comment.
The DefaultBinder was updated to remove this block of code, we just need to add Value Type checking for null to the Bind method and none of this is required. See ef59044#diff-e2e8176d85173d54b9b3dec2acadac86L109
| /// <returns> | ||
| /// An instance of the .NET type which should have all the same values as the <paramref name="javaScriptObject"/> | ||
| /// </returns> | ||
| protected virtual object BindObject(Type nativeType, Type javaScriptType, object javaScriptObject) |
There was a problem hiding this comment.
Common base class seems reasonable, on the surface I'm happy to adopt these changes into the DefaultBinder.
|
@Codeusa I'll make my proposed code changes and push them to a branch for further discussion. Hopefully in the next day or so I'll have something to discuss 👍 |
|
Work in progress of my proposed changes is at https://github.com/amaitland/CefSharp/commits/enhancement/binder The code compiles but probably doesn't run yet, I've got a bit more to do, this is more of a status update. I'll post a detailed follow up tomorrow after I have a chance to finish a few more things. |
|
I've added @Codeusa Before I go manually importing the new binder/interceptor what do you think about having them in a separate library? Say something like |
|
Hey @amaitland I apologize for the delay, I spent most of this week "offline" trying to support my local community during a unprecedented time. I will look over this today and provide feedback / suggestions where needed, an I appreciate your patience. |
@Codeusa Absolutely no need to apologise. Supporting your community is far more important than writing/reviewing code. Please take whatever time you need. |
The Typescript code generator is just a single class (not counting reflection extensions) with no dependencies. I am going to clean it up before submitting. That being said, having the interceptor and binder in a library targeting a higher framework would be great! We could more easily support newer .NET types. |
@Codeusa Let's go with that plan then. Now the fun part, naming the library! 😄 Do we go with something generic with the possibility of having other unrelated functionality added later or keep it specific to binding and focus on say
Open to suggestions. |
Again sorry to open this pull only to become the most inactive person in the world. CefSharp.Extensions is probably the most accurate name available. It will allow for the adding of additional extension features without distorting the purpose of the library. It can also be used as a place to experiment with new features without bumping the main CefSharp assembly. |
|
I've noticed a few issues in the logic around tuples so I will commit a patch for that and address some other changes you requested. |
Totally fine. I myself have been sick and my 2yr old was sick before that (just a cold, nothing major), so haven't spent any time in front of a computer.
Do you know what the minimum |
Just as a follow up to this I've created #3141 which moves the
Still a little bit to do before it's ready, default binder needs updating so it's naming matches. Bit of code cleanup and some additional test cases. |
- Now only supports ValueTuples - Improved the check for the type - Added a binder that can successfully create a generic ValueTuple object when not running under a newer version of .NET
So as you can see in 3ad514a, because CefSharp currently doesn't use a framework target that supports ValueTuples there is a lot of type punning. Targeting 4.7.2 in the new library will allow for the use of C# 8, and make a lot of the reflection code cleaner overall. |
To be included in the main repository we still need to be able to build using We have a couple of options
I'm leaning towards option two, option suggestions though. |
| /// to. | ||
| /// </param> | ||
| /// <param name="javaScriptObject">A collection that contains all the components of the tuple.</param> | ||
| /// <returns>A tuple I'd fucking hope</returns> |
There was a problem hiding this comment.
Can you remove the profanity please 😄
| /// The custom parser below supports converting strings into a <see cref="Enum"/> that has the <see cref="FlagsAttribute"/> defined. | ||
| /// These are commonly used separators to assist that process. | ||
| /// </summary> | ||
| private static readonly char[] EnumSeparators = { '|', ',', ';', '+', ' ' }; |
There was a problem hiding this comment.
Do you have a reference source for this list? Are you using different separators in your code to represent a set of flags?
Do we really need to support all these possibilities?
Let's create a new project under the org and use |
We can use the new SDK project format and build a specific .Net Core version as well for minimal cost. I'll create a new repository after I've finished the official
Keep it simple to start with, a new interceptor and binder implementation that can manually be assigned. I was wondering if having a different binder/interceptor for each object was actually necessary, switching to a single instance per JavascriptObjectRepository (per ChromiumWebBrowser) would make things much simpler. Could then have an extension method that assigned the newer versions. Obviously a breaking change, I'm not sure if anyone actually uses a different binder/interceptor per object. Open to suggestions |
|
I've created https://github.com/cefsharp/CefSharp.Extensions and setup a basic test project so we can get some |
|
This has been merged into https://github.com/cefsharp/CefSharp.Extensions/tree/master/CefSharp.Extensions/ModelBinding with some changes
The project is targeting Closing this |
|
@amaitland thank you for moving the needle on this. is there a preferred email I could reach out to you at? |
This comment has been minimized.
This comment has been minimized.
|
I've pushed CefSharp.Extensions.83.4.20-pre.nupkg to Nuget.org, it should hopefully appear in the next hour or so. I've created some very basic release notes with a summary based on the comments above, see https://github.com/cefsharp/CefSharp.Extensions/releases/tag/v83.4.20-pre |
Fixes:
#3137 #3129 #2231 and probably a whole host of people who have been surprised to find out that
BindingOptions.CamelCaseJavascriptNamesis a one way operation.Summary:
When
CefSharpSettings.TypeSafeMarshalingis set to true CefSharp will use strict type-safe analysis for sending and receiving data.For extra "safety" I'll open a separate pull request for a TypeScript code generator.
Changes:
Because our build of CefSharp targets .NET 4.7.2 we have the luxury of some modern .NET features. For this change I had to implement things like a crude Tuple and ValueTuple (which isn't even supported in 4.5.2, and is no longer supported by Microsoft) detection, but it seems to work fine.
Other than that, we applied only the code that could be introduced without making breaking changes. This pull request has backwards combability for people who built their code around the default binder, but when they enable the new setting their code will probably break.
Which probably isn't entirely a bad thing because using this code in production, we've eliminated almost all issues you would expect to get from sending complex objects from a typed language, to an untyped language, and back again.
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: