NullReferenceException in ExceptionHelper.BuildMessage on Mono

[asbjornu]

When executing the tests in this project with NUnit.ConsoleRunner version 3.7 (and NUnit version 3.8.1), it currently fails in Mono with a NullReferenceException:

System.NullReferenceException: Object reference not set to an instance of an object
  at NUnit.Framework.Internal.ExceptionHelper.BuildMessage (System.Exception exception, System.Boolean excludeExceptionNames) [0x00009] in <d0c69b98d4da4342a70f9b139b717b6d>:0
  at NUnit.Framework.Internal.TestResult.RecordException (System.Exception ex) [0x000aa] in <d0c69b98d4da4342a70f9b139b717b6d>:0
  at NUnit.Framework.Internal.Execution.SimpleWorkItem.PerformWork () [0x00032] in <d0c69b98d4da4342a70f9b139b717b6d>:0
  at NUnit.Framework.Internal.Execution.WorkItem.RunOnCurrentThread () [0x0007b] in <d0c69b98d4da4342a70f9b139b717b6d>:0
  at NUnit.Framework.Internal.Execution.WorkItem.Execute () [0x000c4] in <d0c69b98d4da4342a70f9b139b717b6d>:0
  at NUnit.Framework.Internal.Execution.ParallelWorkItemDispatcher.Dispatch (NUnit.Framework.Internal.Execution.WorkItem work, NUnit.Framework.Internal.Execution.ParallelExecutionStrategy strategy) [0x00039] in <d0c69b98d4da4342a70f9b139b717b6d>:0
  at NUnit.Framework.Internal.Execution.ParallelWorkItemDispatcher.Dispatch (NUnit.Framework.Internal.Execution.WorkItem work) [0x00008] in <d0c69b98d4da4342a70f9b139b717b6d>:0
  at NUnit.Framework.Internal.Execution.CompositeWorkItem.RunChildren () [0x000a7] in <d0c69b98d4da4342a70f9b139b717b6d>:0
  at NUnit.Framework.Internal.Execution.CompositeWorkItem.PerformWork () [0x000cb] in <d0c69b98d4da4342a70f9b139b717b6d>:0
  at NUnit.Framework.Internal.Execution.WorkItem.RunOnCurrentThread () [0x0007b] in <d0c69b98d4da4342a70f9b139b717b6d>:0
  at NUnit.Framework.Internal.Execution.WorkItem.Execute () [0x000c4] in <d0c69b98d4da4342a70f9b139b717b6d>:0
  at NUnit.Framework.Internal.Execution.TestWorker.TestWorkerThreadProc () [0x00079] in <d0c69b98d4da4342a70f9b139b717b6d>:0
  at System.Threading.ThreadHelper.ThreadStart_Context (System.Object state) [0x00014] in <ab2f5ad0f8c84dfe97dfd6a06a64cf44>:0
  at System.Threading.ExecutionContext.RunInternal (System.Threading.ExecutionContext executionContext, System.Threading.ContextCallback callback, System.Object state, System.Boolean preserveSyncCtx) [0x00071] in <ab2f5ad0f8c84dfe97dfd6a06a64cf44>:0
  at System.Threading.ExecutionContext.Run (System.Threading.ExecutionContext executionContext, System.Threading.ContextCallback callback, System.Object state, System.Boolean preserveSyncCtx) [0x00000] in <ab2f5ad0f8c84dfe97dfd6a06a64cf44>:0
  at System.Threading.ExecutionContext.Run (System.Threading.ExecutionContext executionContext, System.Threading.ContextCallback callback, System.Object state) [0x0002b] in <ab2f5ad0f8c84dfe97dfd6a06a64cf44>:0
  at System.Threading.ThreadHelper.ThreadStart () [0x00008] in <ab2f5ad0f8c84dfe97dfd6a06a64cf44>:0

I've tried to do a custom build of NUnit locally and drop in the .pdb, but I don't get line numbers out so I'm probably not twisting the right knobs. Looking at the method, it's not very complicated:

public static string BuildMessage(Exception exception, bool excludeExceptionNames=false)
{
    StringBuilder sb = new StringBuilder();
    if (!excludeExceptionNames)
        sb.AppendFormat("{0} : ", exception.GetType());
    sb.Append(GetExceptionMessage(exception));

    foreach (Exception inner in FlattenExceptionHierarchy(exception))
    {
        sb.Append(Environment.NewLine);
        sb.Append("  ----> ");
        if (!excludeExceptionNames)
            sb.AppendFormat("{0} : ", inner.GetType());
        sb.Append(GetExceptionMessage(inner));
    }

    return sb.ToString();
}

Since BuildMessage is at the top of the stack, the problem can't be within GetExceptionMessage() or FlattenExceptionHierarchy(), sot he only obvious problem here is if exception is null and excludeExceptionNames is false, which will cause exception.GetType() to blow up.

I can submit a PR that preforms some null checks on the code, but would like some advice from you guys first.

[asbjornu]

Digging a bit more, TestResult.RecordException() has the following code that might cause the problem if ex.InnerException is null:

if (ex is NUnitException || ex is TargetInvocationException)
    ex = ex.InnerException;

I have no idea if that's actually possible, but it might be worth changing the if statement to:

if ((ex is NUnitException || ex is TargetInvocationException) && ex.InnerException != null)

[ChrisMaddock]

Could you use the --labels=Before option to pinpoint the test which is crashing the runner, and then recreate the situation as a unit test in the NUnit tests? I think we should definitely have a unit test for this, if there's a specific mono oddity.

[jnm2]

What I usually do is open the NUnit assembly in dotPeek with the 'Show IL code in comments' option and find the line with the offset from the stack trace (in this case, 0x00009). I think you're right about the problem being a null exception.

I like your suggested fix, but we should also throw ArgumentNullException for null exception no matter what the other argument is. And we should add tests on both things.

[asbjornu]

@ChrisMaddock, @jnm2: Thanks for the tips, I'll try to get my hands dirty with this tomorrow.

[rprouse]

We should do a null check in BuildMessage just to be safe, but your guess about the inner exception looks promising and the fix looks good.

[asbjornu]

@ChrisMaddock: This is the failing test. Given the amount of code required to reproduce that, I'm not sure how to do it, to be honest. If you can spot anything weird going on there, please let me know and I'll try to extract just the weird bits into a unit test for NUnit.

@jnm2: With IL code comments on, dotPeek did not give me the 0x00009 instruction, but it gives me 0x00008 and 0x0000d:

// IL_0008: call         class [mscorlib]System.Globalization.CultureInfo [mscorlib]System.Globalization.CultureInfo::get_CurrentCulture()
// IL_000d: ldstr        "{0} : {1}"

Given that the decompiled code looks like this:

stringBuilder.AppendFormat((IFormatProvider) CultureInfo.CurrentCulture, "{0} : {1}", new object[2]
{
    (object) exception.GetType().ToString(),
    (object) exception.Message
});

It seems plausible to me that instruction 0x0009 is (object) exception.GetType().ToString(), since that needs to be performed before its value can be interpolated into the string.

Would you accept a PR just performing some more rigorous null checks in this path of the code or do you need a unit test provoking the problem as well? I don't know how to compose such a unit test without brining over the entirety of Pomona's source code.

[jnm2]

@asbjornu Weird. I pulled the net45 DLL from the 3.8.1 NuGet package, which it looked like your project was referencing when I read your csproj, and I do get this for the first three lines:

// IL_0000: newobj       instance void [mscorlib]System.Text.StringBuilder::.ctor()
// IL_0005: stloc.0      // 'stringBuilder [Range(Instruction(IL_0005 stloc.0)-Instruction(IL_0091 ldloc.0))]'
// IL_0006: ldarg.1      // excludeExceptionNames
// IL_0007: brtrue.s     IL_001b
// IL_0009: ldloc.0      // 'stringBuilder [Range(Instruction(IL_0005 stloc.0)-Instruction(IL_0091 ldloc.0))]'
// IL_000a: ldstr        "{0} : "
// IL_000f: ldarg.0      // exception
// IL_0010: callvirt     instance class [mscorlib]System.Type [mscorlib]System.Exception::GetType()
// IL_0015: callvirt     instance class [mscorlib]System.Text.StringBuilder [mscorlib]System.Text.StringBuilder::AppendFormat(string, object)
// IL_001a: pop          

But we come to the same conclusion, so no matter. =)

Provoking the problem would be preferred unless it is too involved.

[CharliePoole]

I think placing a guard clause on entry to a method that doesn't handle null args is common sense. Adding an if clause where an arg is used, so that null is silently ignored could hide a coding error in the caller. It makes sense to look at all the callers in that case.

I think complex cases where an arg or other value is sometimes null and sometimes not calls for a test. Simple cases (null not alowed) don't seem to need a test.

[hoihoi8]

Is there a workaround for this issue? I'm getting this exception when using nunit-console 3.8

[jnm2]

Implementation finished.

@hoihoi8 Do you still have a repro? I apologize that so much time has passed, but if I could make sure I actually covered your scenario, that would be great!