Runtime: Assert failure in sgen-mono.c while running System.Drawing tests

[stephentoub]

https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-44136-merge-c86262a09b5b4ea99f/System.Drawing.Common.Tests/console.5aca01fc.log?sv=2019-07-07&se=2020-11-22T19%3A39%3A42Z&sr=c&sp=rl&sig=D2w6NnhCPSVf5cfFCGA%2BU%2By6nJ5llAgjBphBA5DUxMk%3D

    System.Drawing.Drawing2D.Tests.ColorBlendTests.Ctor_LargeCount_ThrowsOutOfMemoryException [SKIP]
      Condition(s) not met: "IsNotIntMaxValueArrayIndexSupported"
* Assertion at F:\workspace\_work\1\s\src\mono\mono\metadata\sgen-mono.c:2375, condition `stack == NULL || mono_handle_stack_is_empty (stack)' not met

[msftbot[bot]]

Tagging subscribers to this area: @CoffeeFlux
See info in area-owners.md if you want to be subscribed.

[msftbot[bot]]

Tagging subscribers to this area: @brzvlad
See info in area-owners.md if you want to be subscribed.

[CoffeeFlux]

@BrzVlad can you take a look? If it doesn't seem like a GC issue feel free to bounce it back.

[BrzVlad]

1. This doesn't look like a GC issue, rather an icall not clearing its handle stack
2. I ran this suite 3000 times on windows without getting any crashes
3. This is the list of icalls called by the suite, with their call count https://gist.github.com/BrzVlad/d2f121bfffd3aa0cda6bf8443acf893b
4. I remember seeing this assertion in the past. Maybe @lambdageek has some more ideas

[msftbot[bot]]

Tagging subscribers to this area: @CoffeeFlux
See info in area-owners.md if you want to be subscribed.

[lambdageek]

bah we should change

                g_assert (stack == NULL || mono_handle_stack_is_empty (stack));

to

                g_assertf (stack == NULL || mono_handle_stack_is_empty (stack), "skipped thread %p (reason %d) has non-empty handle stack", info, skip_reason);

so that we get a better assert message.

---

So there are two things happening here: we decided to skip the thread from a stop-the-world suspend for some reason, and the thread has a non-empty handle stack.

On Windows one reason we might skip a thread is if SuspendThread failed - which can happen transiently, I think - in that case we abort the attempt to suspend and skip the thread during STW (https://github.com/mono/mono/pull/15486).

If that's what's happening, it's not surprising that the thread had a non-empty handle stack. it could have been doing any arbitrary work when we tried to preempt it.

---

I think this is another piece of evidence that we need to be able to back out of the whole STW and start over. (Similar to the pthread_kill transient failures - https://github.com/dotnet/runtime/issues/32377#issuecomment-629265314). Skipping the thread just invites the GC to miss roots. /cc @lateralusX