Fix #3817: avoid re-entrant singleton cctor NullReferenceException#4107
Closed
mattleibow wants to merge 1 commit into
Closed
Fix #3817: avoid re-entrant singleton cctor NullReferenceException#4107mattleibow wants to merge 1 commit into
mattleibow wants to merge 1 commit into
Conversation
Contributor
📦 Try the packages from this PRWarning Do not run these scripts without first reviewing the code in this PR. Step 1 — Download the packages bash / macOS / Linux: curl -fsSL https://raw.githubusercontent.com/mono/SkiaSharp/main/scripts/get-skiasharp-pr.sh | bash -s -- 4107PowerShell / Windows: iex "& { $(irm https://raw.githubusercontent.com/mono/SkiaSharp/main/scripts/get-skiasharp-pr.ps1) } 4107"Step 2 — Add the local NuGet source dotnet nuget add source ~/.skiasharp/hives/pr-4107/packages --name skiasharp-pr-4107More options
Or download manually from Azure Pipelines — look for the Remove the source when you're done: dotnet nuget remove source skiasharp-pr-4107 |
mattleibow
changed the base branch from
dev/issue-3817-fix-singleton-init
to
main
May 30, 2026 19:24
The SKObject static constructor eagerly initializes an ordered cascade of singletons (SKColorSpace, SKData, SKFontManager, SKTypeface, ...). The SKTypeface static constructor in that cascade read the managed singletons SKFontManager.Default and SKFontStyle.Normal. When SKFontManager (or SKFontStyle) was the first SkiaSharp type touched in a process, the CLR permits same-thread cctor re-entry and observed a not-yet-assigned null singleton, producing a NullReferenceException surfaced as a TypeInitializationException. Fix: build the default typeface in the SKTypeface static constructor from raw native handles (sk_fontmgr_create_default / sk_fontstyle_new / sk_fontmgr_legacy_create_typeface) in a try/finally, severing both managed singleton reads. This is additive and ABI-safe. Native temporaries are released in finally. Add a dedicated regression test assembly (SkiaSharp.Tests.SingletonInit.Console) with a single test that touches SKFontManager.Default as the first SkiaSharp type in the process. Because static constructors run once per process and 'dotnet test' launches a separate test host per assembly, this reliably reproduces the first-touch ordering with no child processes or copy targets. Wired into the netcore and netfx cake test runners and the solution. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
mattleibow
force-pushed
the
mattleibow/fix-3817-singleton-init
branch
from
May 30, 2026 19:47
cad9149 to
5ee76e9
Compare
Contributor
|
📖 Documentation Preview The documentation for this PR has been deployed and is available at: 🔗 View Staging Site This preview will be updated automatically when you push new commits to this PR. This comment is automatically updated by the documentation staging workflow. |
5 tasks
mattleibow
added a commit
that referenced
this pull request
Jun 2, 2026
…#4107) Adds the dedicated SkiaSharp.Tests.SingletonInit.Console assembly with a single [Fact] that touches SKFontManager.Default as the FIRST SkiaSharp type in a fresh test-host process, guarding the #3817 re-entrant static-cctor regression. Wires it into tests-netcore.cake, tests-netfx.cake and the console solution. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
mattleibow
added a commit
that referenced
this pull request
Jun 4, 2026
…ixture StaticTypes list PR #4116 is a diagnostic experiment pairing PR #4107's alternate fix for #3817 with PR #4080's test suite. 4107 resists public dispose structurally (SKxxxStatic subclasses override Dispose(bool) as no-op) rather than via 4080's IgnorePublicDispose flag model, so the dispose-protection scaffolding ported into the binding was unused by production code and only referenced by test glue. - HandleDictionary.cs, SKObject.cs: reverted to main (removed the additive disposeProtected overloads that nothing in production used). - GarbageCleanupFixture.cs: restored main's StaticTypes name-list. 4080's version keyed assembly-teardown cleanup off IgnorePublicDispose, which the static wrappers under 4107 never set, so Assert.Empty(aliveObjects) threw at assembly cleanup and xUnit smeared a single teardown failure across ~1900 already-passing tests (console showed Failed: 1921). Reverting removes the smear so CI reflects only the genuine experiment signal. - Dropped SKHandleDictionaryDisposeProtectedTest.cs and the two SKObjectTest cases that depended on the removed overloads. After this change the suite reports 13 failures (was ~1921): the genuine behavioral deltas where 4107 does not implement 4080's IgnorePublicDispose model, plus one pre-existing GC-timing flake (SKDocumentTest.StreamIsNot CollectedPrematurely, unchanged from main). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
mattleibow
added a commit
that referenced
this pull request
Jun 4, 2026
Each SKxxxStatic subclass protects its singleton structurally (no-op Dispose(bool) + OwnsHandle=false) but never set the IgnorePublicDispose flag. The singleton tests from #4080 assert that flag, so they failed under #4107's protection model. Call PreventPublicDisposal() in each static subclass constructor to unify both mechanisms. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
mattleibow
added a commit
that referenced
this pull request
Jun 4, 2026
…uard PreventPublicDisposalOnDisposedWrapperThrows (gated on THROW_OBJECT_EXCEPTIONS, active only in Debug/device builds) asserted that PreventPublicDisposal() throws InvalidOperationException on a disposed wrapper. That throwing guard only existed in #4080's production rewrite, which this PR deliberately omits in favor of #4107's minimal fix. On main's PreventPublicDisposal() (sets IgnorePublicDispose, no throw) the assertion cannot hold, so the test failed on Mac Catalyst/iOS while passing everywhere else. Adding the guard back would reintroduce #4080 production behavior with a real debug-only race in SKFontManager/SKFontStyleSet callers, so the test is removed instead. Also corrected the now-stale comment on the sibling live-wrapper test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3817
Root cause
SKTypeface's static constructor built its default typeface by reading the managed singletonsSKFontManager.Default.HandleandSKFontStyle.Normal.Handle. Those areSKObjecttypes whose own static constructors participate in the same eager-initialization cascade driven bySKObject.EnsureStaticInstanceAreInitialized().When
SKFontManagerorSKFontStylewas the first SkiaSharp type touched in a process, reading them fromSKTypeface's cctor re-entered a static constructor that was still running on the current thread. The CLR allows a thread to re-enter a type initializer it is already running, so the read observed a not-yet-assigned (null) singleton field and threwNullReferenceException→ surfaced asTypeInitializationException(e.g. onSKFontManager.Default). This is the chronicmainCI crash (Windows/Linux .NET Core test jobs).Fix
Build the default typeface directly from raw native handles (
sk_fontmgr_create_default/sk_fontstyle_new/sk_fontmgr_legacy_create_typeface) insideSKTypeface's cctor, in atry/finallythat releases them. This severs both reads of the vulnerable managed singletons, so the cctor no longer depends on any other singleton being fully constructed.SKFontStyle.Normal==sk_fontstyle_new(Normal=400, Normal=5, Upright).sk_fontmgr_create_defaultreturns +1;legacy_create_typefacedoes not retain the font manager; both temporaries are released infinally.Regression test
Static constructors run once per process, so an in-process xUnit test in an existing assembly can't reproduce "type X touched first" — something always touches
SKObjectbefore the test body runs.Added a small dedicated test assembly,
SkiaSharp.Tests.SingletonInit.Console, modeled onSkiaSharp.Vulkan.Tests.Console.dotnet testlaunches a separate test-host process per assembly, so this assembly's single[Fact]is the first SkiaSharp code touched in its process. The test accessesSKFontManager.Defaultfirst and asserts it — plusSKFontStyle.Normal.HandleandSKTypeface.Default.Handle— are non-zero, reproducing the exact #3817 first-touch ordering with no child processes or copy targets.The assembly is wired into the cake test runners (
tests-netcore.cakefor net10.0,tests-netfx.cakefor net48) andtests/SkiaSharp.Tests.Console.sln, so it builds and runs on every CI leg including .NET Framework.Verification
main: with the fix reverted, the new test fails with exit code 1 and the exact chainSKFontManager..cctor → SKObject..cctor → SKTypeface.EnsureStaticInstanceAreInitialized() → SKTypeface..cctor. With the fix it passes, exit code 0.