Sellorio / JazSharp

Jasmine inspired mocking and unit testing framework for C#.
Apache License 2.0
66 stars 4 forks source link

Test host process crashed on some Linq #17

Open Sellorio opened 5 years ago

Sellorio commented 5 years ago

Not sure what the cause of this is. Debugging results in ExecutionEngineException. The cause may be a Where function call that takes in KeyValuePair<,>. The exception appears to trigger when calling the getter on the Key property.

lukesdm commented 4 years ago

Is this whilst debugging just the JazSharp source, or an independent test project? if its the latter, could you post a project which can reproduce it please?

Sellorio commented 4 years ago

Hi @lukesdm,

I was able to reproduce it with the following code:

public static void UseAcceptLanguage(this IApplicationBuilder applicationBuilder, Action<LocalizationConfiguration> configure)
{
    var configuration = new LocalizationConfiguration();
    configure(configuration);

    applicationBuilder.Use(async (context, next) =>
    {
        var acceptLangauge = (string)context.Request.Headers["Accept-Language"];

        if (!string.IsNullOrWhiteSpace(acceptLangauge))
        {
            var supportedLanguages = configuration.GetSupportedLanguages();
            var languages = ParseLanguagesAndOrderByPriority(acceptLangauge);

            var _checkSupportedLanguages = (Func<string, bool>)(x => supportedLanguages.Any(y => y.Key == x));
            var language = languages.FirstOrDefault(_checkSupportedLanguages);
            language = language ?? supportedLanguages.First().Key;

            var mappedLanguage = supportedLanguages.First(x => x.Key == language).Value;

            var culture = CultureInfo.GetCultureInfo(mappedLanguage);
            CultureInfo.CurrentCulture = culture;
            CultureInfo.CurrentUICulture = culture;
        }

        // Call the next delegate/middleware in the pipeline
        await next();
    });
}

The error triggers on _checkSupportedLanguages apparently in the Any call. The problem with ExecutionEngineExceptions is that they don't give much info (i.e. none).

This is the other code surrounding this function:

private static List<string> ParseLanguagesAndOrderByPriority(string acceptLanguage)
{
    var languages = acceptLanguage.Replace(" ", "").ToLowerInvariant().Split(',');
    Dictionary<string, float> result = new Dictionary<string, float>();

    foreach (var language in languages)
    {
        var parts = language.Split(';');
        var languageCode = parts[0];
        var priority = 1.0f;

        foreach (var part in parts.Skip(1))
        {
            if (part.StartsWith("q="))
            {
                priority = float.Parse(part.Substring(2));
            }
        }

        result.Add(languageCode, priority);
    }

    return result.OrderByDescending(x => x.Value).Select(x => x.Key).ToList();
}
public class LocalizationConfiguration
{
    private static Dictionary<string, string> _supportedLanguages = new Dictionary<string, string>();

    public void AddLanguage(string code, string mappedTo = null)
    {
        _supportedLanguages[code.ToLower()] = mappedTo ?? code;
    }

    internal Dictionary<string, string> GetSupportedLanguages()
    {
        return new Dictionary<string, string>(_supportedLanguages);
    }
}

The issue can be reproduced with the following test code:

var factory = new DefaultServiceProviderFactory();
var applicationBuilder = new ApplicationBuilder(factory.CreateServiceProvider(new ServiceCollection()));

var useSpy = Jaz.SpyOn(typeof(UseExtensions), nameof(UseExtensions.Use));

LocalizationMiddleware.UseAcceptLanguage(applicationBuilder, Jaz.CreateSpy<LocalizationConfiguration>(out var configureSpy));

var localizationConfiguration = (LocalizationConfiguration)configureSpy.Calls[0].Arguments[0];
var middleware = (Func<HttpContext, Func<Task>, Task>)useSpy.Calls[0].Arguments[1];

var context = new DefaultHttpContext();

Jaz.SpyOn(typeof(string), nameof(string.IsNullOrWhiteSpace)).And.ReturnValue(false);
Jaz.SpyOn(localizationConfiguration, nameof(localizationConfiguration.GetSupportedLanguages)).And.ReturnValue(new Dictionary<string, string> { { "en-US", "en-US" }, { "en-GB", "en-GB" } });
Jaz.SpyOn(typeof(LocalizationMiddleware), "ParseLanguagesAndOrderByPriority").And.ReturnValue(new List<string> { "en-US", "en-GB" });

var currentCultureSpy = Jaz.SpyOnProperty(typeof(CultureInfo), nameof(CultureInfo.CurrentCulture));
var currentUiCultureSpy = Jaz.SpyOnProperty(typeof(CultureInfo), nameof(CultureInfo.CurrentUICulture));
var next = Jaz.CreateSpyFunc<Task>(out var nextSpy);
nextSpy.And.ReturnValue(Task.CompletedTask);

middleware.Invoke(context, next);

The test is pulling out the parameter to the Use method and then calling it.

lukesdm commented 4 years ago

Flipping heck, I see what you mean about this being a tricky one... this code has all the things - nested lambdas, async lambdas, interception of static extension methods... I'm not very familiar with ExecutionEngineException myself, but from what I gather it's often caused by invalid IL generation, or a related memory (or threading?) issue. My gut feeling is that it's related to the async in the static method, which is being spied on:

applicationBuilder.Use(async (context, next) =>...

I've not had time to run any code yet though. Quick question. Looking at this line...

Jaz.SpyOn(localizationConfiguration, nameof(localizationConfiguration.GetSupportedLanguages)).And.ReturnValue(new Dictionary<string, string> { { "en-US", "en-US" }, { "en-GB", "en-GB" } });

Does the issue still occur if you take off the And.ReturnValue(...)?:

Jaz.SpyOn(localizationConfiguration, nameof(localizationConfiguration.GetSupportedLanguages));
Sellorio commented 4 years ago

Yeah its a bit of a mess hehe. Code could probably use some clean up :)

That caused a NullReferenceException on this line:

var _checkSupportedLanguages = (Func<string, bool>)(x => supportedLanguages.Any(y => y.Key == x));
var language = languages.FirstOrDefault(_checkSupportedLanguages); // here
language = language ?? supportedLanguages.First().Key;

(which was then wrapped in a faulted Task object automatically)

lukesdm commented 4 years ago

Hi, I finally had some more time to spend on this. I managed to narrow the issue down a bit. I managed to repro the ExecutionEngineException from the code you provided, then took bits out until a more informative exception fell out. Eventually ended up with a couple of tests which should help diagnose the issue - just calling .Key shows the problem!

Describe("I", () => 
{
    It("can get string key from dict", () =>
    {
        var dict = new Dictionary<string, string> { { "A", "1" } };

        var result = dict.FirstOrDefault();
        var key = result.Key;

        Expect(key).ToBe("A");
    });

    It("can get int key from dict", () =>
    {
        var dict = new Dictionary<int, string> { { 1, "A" } };

        var result = dict.FirstOrDefault();
        var key = result.Key;

        Expect(key).ToBe(1);
    });
});

Result of the first test: image

Result of the second test (the number is different on each run): image

So, it's looking like a branch in the rewriter is misbehaving - property getter on a generic struct?

Sellorio commented 4 years ago

Sounds like it's worth a look! I'll check it out and see if anything comes up. The weird thing is that property calls are just method calls so there is no special logic for them and methods in generic classes are working.

Thanks for the help :)

Sellorio commented 4 years ago

So the problem here is that the this parameter is passed by reference. This is to ensure that if method changes the state of the object, that affects the variable instance instead of a copy of it.

The fix might (extra emphasis on might) be to construct an IntPtr instance from the on the next IL line after the ldloca which loads the this parameter as a pointer. It works in unsafe code blocks but is likely to cause an exception on other code blocks. Then instead of the first parameter being the struct instance, it would be an IntPtr that is converted into a boxed object (thus working as expected).

Unfortunately without writing a full complex IL parser, there is no reliable way for me to back-track through the IL to figure out what the this parameter actually is.

I can't even try globally converting pointers to IntPtr (without extra parsing) since that would break generic methods.

Might look into working on the IL super-parser.

Sellorio commented 4 years ago

IL super-parser work is underway in https://github.com/Sellorio/ILusion. Always open to another hand ;)

lukesdm commented 4 years ago

Glad to hear you've found the problem. Shame it's not a simple solution though. I've never done anything serious with IL before so i'm afraid i can't be of more help. Best of luck with the project!