Skip to content

Commit

Permalink
Merge pull request #3975 from unoplatform/dev/jela/xbind-leak
Browse files Browse the repository at this point in the history 
fix(reg): Fix invalid lifetime for generated delegates on x:Bind
  • Loading branch information
@dr1rrb
dr1rrb authored Sep 6, 2020
2 parents 1016fee + 4e45e00 commit 414e106
Show file tree
Hide file tree
Showing 5 changed files with 65 additions and 34 deletions.
26 changes: 14 additions & 12 deletions src/SourceGenerators/Uno.UI.SourceGenerators.Tests/Given_XBindRewriter.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,16 @@ public class Given_XBindRewriter
// DataTemplates (with context)
[DataRow("ctx", "MyProperty.A", "ctx.MyProperty.A")]
[DataRow("ctx", "MyProperty", "ctx.MyProperty")]
[DataRow("ctx", "MyStaticProperty", "MyStaticProperty")]
[DataRow("ctx", "MyStaticMethod()", "MyStaticMethod()")]
[DataRow("ctx", "MyProperty.A.ToLower()", "ctx.MyProperty.A.ToLower()")]
[DataRow("ctx", "System.String.Format('{0:X8}', a.Value)", "System.String.Format('{0:X8}', ctx.a.Value)")]
[DataRow("ctx", "Static.MyFunction(42.0)", "Static.MyFunction(42.0)")]
[DataRow("ctx", "Static.MyFunction(true)", "Static.MyFunction(true)")]
[DataRow("ctx", "Static.MyFunction(MyProperty)", "Static.MyFunction(ctx.MyProperty)")]
[DataRow("ctx", "MyNameSpace.Static2.MyProperty", "MyNameSpace.Static2.MyProperty")]
[DataRow("ctx", "MyNameSpace.Static2.MyEnum.EnumMember", "MyNameSpace.Static2.MyEnum.EnumMember")]
[DataRow("ctx", "MyNameSpace.Static2.MyProperty.ToArray()", "MyNameSpace.Static2.MyProperty.ToArray()")]
[DataRow("ctx", "MyNameSpace.Static2.MyFunction(MyProperty)", "MyNameSpace.Static2.MyFunction(ctx.MyProperty)")]
[DataRow("ctx", "MyFunction(MyProperty)", "ctx.MyFunction(ctx.MyProperty)")]
[DataRow("ctx", "", "ctx")]
Expand All @@ -38,19 +42,17 @@ public void When_PathRewrite(string contextName, string inputExpression, string
{
bool IsStaticMethod(string name)
{
switch (name)
return name switch
{
case "Static.MyFunction":
return true;
case "System.String.Format":
return true;
case "MyNameSpace.Static2.MyFunction":
return true;
case "MyNameSpace.Static2.MyProperty":
return true;
}

return false;
"MyStaticProperty" => true,
"MyStaticMethod" => true,
"Static.MyFunction" => true,
"System.String.Format" => true,
"MyNameSpace.Static2.MyFunction" => true,
"MyNameSpace.Static2.MyProperty" => true,
"MyNameSpace.Static2.MyEnum" => true,
_ => false,
};
}

var output = XBindExpressionParser.Rewrite(contextName, inputExpression, IsStaticMethod);
Expand Down
18 changes: 14 additions & 4 deletions src/SourceGenerators/Uno.UI.SourceGenerators/XamlGenerator/Utils/XBindExpressionParser.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,18 @@ public override SyntaxNode VisitInvocationExpression(InvocationExpressionSyntax
{
var e = base.VisitInvocationExpression(node);

var isValidParent = !Helpers.IsInsideMethod(node).result && !Helpers.IsInsideMemberAccessExpression(node).result;
var isParentMemberStatic = node.Expression switch
{
MemberAccessExpressionSyntax ma => _isStaticMember(ma.Expression.ToFullString()),
IdentifierNameSyntax ins => _isStaticMember(ins.ToFullString()),
_ => false
};

var isValidParent = !Helpers.IsInsideMethod(node).result
&& !Helpers.IsInsideMemberAccessExpression(node).result
&& !Helpers.IsInsideMemberAccessExpression(node.Expression).result;

if (isValidParent && !_isStaticMember(node.Expression.ToFullString()))
if (isValidParent && !_isStaticMember(node.Expression.ToFullString()) && !isParentMemberStatic)
{
if (e is InvocationExpressionSyntax newSyntax)
{
Expand Down Expand Up @@ -92,8 +101,9 @@ public override SyntaxNode VisitMemberAccessExpression(MemberAccessExpressionSyn
{
var e = base.VisitMemberAccessExpression(node);
var isValidParent = !Helpers.IsInsideMethod(node).result && !Helpers.IsInsideMemberAccessExpression(node).result;
var isParentMemberStatic = node.Expression is MemberAccessExpressionSyntax m && _isStaticMember(m.ToFullString());

if (isValidParent)
if (isValidParent && !_isStaticMember(node.Expression.ToFullString()) && !isParentMemberStatic)
{
var expression = e.ToFullString();
var contextBuilder = _isStaticMember(expression) ? "" : ContextBuilder;
Expand All @@ -112,7 +122,7 @@ public override SyntaxNode VisitIdentifierName(IdentifierNameSyntax node)
{
var isValidParent = !Helpers.IsInsideMethod(node).result && !Helpers.IsInsideMemberAccessExpression(node).result;

if (isValidParent)
if (isValidParent && !_isStaticMember(node.ToFullString()))
{
var newIdentifier = node.ToFullString();

Expand Down
44 changes: 36 additions & 8 deletions src/SourceGenerators/Uno.UI.SourceGenerators/XamlGenerator/XamlFileGenerator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3202,7 +3202,7 @@ string buildBindBack()
}
else
{
rawFunction = string.IsNullOrEmpty(rawFunction) ? "___ctx" : rawFunction;
rawFunction = string.IsNullOrEmpty(rawFunction) ? "___ctx" : XBindExpressionParser.Rewrite("___tctx", rawFunction, IsStaticMember);

string buildBindBack()
{
Expand All @@ -3224,7 +3224,10 @@ string buildBindBack()
if (propertyPaths.properties.Length == 1)
{
var targetPropertyType = GetXBindPropertyPathType(propertyPaths.properties[0]);
return $"(___tctx, __value) => {rawFunction} = ({targetPropertyType})global::Windows.UI.Xaml.Markup.XamlBindingHelper.ConvertValue(typeof({targetPropertyType}), __value)";
return $"(___ctx, __value) => {{ " +
$"if(___ctx is global::{_className.ns + "." + _className.className} ___tctx) " +
$"{rawFunction} = ({targetPropertyType})global::Windows.UI.Xaml.Markup.XamlBindingHelper.ConvertValue(typeof({targetPropertyType}), __value);" +
$" }}";
}
else
{
Expand All @@ -3238,7 +3241,8 @@ string buildBindBack()
}
}

return $".Apply(___b => /*defaultBindMode{GetDefaultBindMode()}*/ global::Uno.UI.Xaml.BindingHelper.SetBindingXBindProvider(___b, this, ___ctx => {rawFunction}, {buildBindBack()} {pathsArray}))";
var bindFunction = $"___ctx is global::{_className.ns + "." + _className.className} ___tctx ? (object)({rawFunction}) : null";
return $".Apply(___b => /*defaultBindMode{GetDefaultBindMode()}*/ global::Uno.UI.Xaml.BindingHelper.SetBindingXBindProvider(___b, this, ___ctx => {bindFunction}, {buildBindBack()} {pathsArray}))";
}
}

Expand Down Expand Up @@ -3282,17 +3286,41 @@ private ITypeSymbol GetXBindPropertyPathType(string propertyPath, INamedTypeSymb
return currentType;
}

bool IsStaticMember(string fullMemberName)
private bool IsStaticMember(string fullMemberName)
{
fullMemberName = fullMemberName.TrimStart("global::");

var lastDotIndex = fullMemberName.LastIndexOf(".");

var className = lastDotIndex != -1 ? fullMemberName.Substring(0, lastDotIndex) : fullMemberName;
var memberName = lastDotIndex != -1 ? fullMemberName.Substring(lastDotIndex + 1) : fullMemberName;
if (lastDotIndex != -1)
{
var className = lastDotIndex != -1 ? fullMemberName.Substring(0, lastDotIndex) : fullMemberName;
var memberName = lastDotIndex != -1 ? fullMemberName.Substring(lastDotIndex + 1) : fullMemberName;

if (_metadataHelper.FindTypeByFullName(className) is INamedTypeSymbol typeSymbol)
{
var hasStaticMethod = typeSymbol.GetMethods().Any(m => m.IsStatic && m.Name == memberName);
var hasStaticProperty = typeSymbol.GetProperties().Any(m => m.Name == memberName && m.IsStatic);
var isEnum = typeSymbol.TypeKind == TypeKind.Enum;

return hasStaticMethod || hasStaticProperty || isEnum;
}

return _metadataHelper.FindTypeByFullName(className) is INamedTypeSymbol typeSymbol
&& (typeSymbol.GetMethods().Any(m => m.IsStatic && m.Name == memberName) || typeSymbol.GetProperties().Any(m => m.IsStatic && m.Name == memberName));
return false;
}
else
{
if (_metadataHelper.FindTypeByFullName(_className.ns + "." + _className.className) is INamedTypeSymbol typeSymbol)
{
var hasStaticMethod = typeSymbol.GetMethods().Any(m => m.IsStatic && m.Name == fullMemberName);
var isStaticProperty = typeSymbol.GetProperties().Any(m => m.Name == fullMemberName && m.IsStatic);
var isStaticField = typeSymbol.GetFields().Any(m => m.Name == fullMemberName && m.IsStatic);

return isStaticProperty || isStaticField || hasStaticMethod;
}

return false;
}
}

private string RewriteNamespaces(string xamlString)
Expand Down
1 change: 1 addition & 0 deletions src/SourceGenerators/Uno.UI.SourceGenerators/XamlGenerator/XamlFileParser.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
using System.Text.RegularExpressions;
using Windows.Foundation.Metadata;
using Uno.UI.SourceGenerators.XamlGenerator.Utils;
using System.Diagnostics;

namespace Uno.UI.SourceGenerators.XamlGenerator
{
Expand Down
10 changes: 0 additions & 10 deletions src/Uno.UI/UI/Xaml/Data/Binding.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,6 @@ public partial class Binding : BindingBase
/// the binding is explicitly tied to the object containing the weak references. This means
/// that there's weak references will be kept alive properly.
/// </summary>

private WeakReference _xBindSelector, _xBindBack;
private ManagedWeakReference _compiledSource;
#endif

Expand Down Expand Up @@ -184,21 +182,13 @@ public object CompiledSource
/// get the resulting value.
/// </summary>
internal Func<object, object> XBindSelector
#if UNO_HAS_UIELEMENT_IMPLICIT_PINNING
{ get => (Func<object, object>)_xBindSelector?.Target; private set => _xBindSelector = new WeakReference(value); }
#else
{ get; private set; }
#endif

/// <summary>
/// Provides the method used to set the value back to the source.
/// </summary>
internal Action<object, object> XBindBack
#if UNO_HAS_UIELEMENT_IMPLICIT_PINNING
{ get => (Action<object, object>)_xBindBack?.Target; private set => _xBindBack = new WeakReference(value); }
#else
{ get; private set; }
#endif

/// <summary>
/// List of paths to observe in the context x:Bind expressions
Expand Down

0 comments on commit 414e106

Please sign in to comment.