Alternate fix for #3817: re-entrant static-init NRE in SKTypeface (+ #4080 test suite) - #4116
Closed
mattleibow wants to merge 5 commits into
Closed
Alternate fix for #3817: re-entrant static-init NRE in SKTypeface (+ #4080 test suite)#4116mattleibow wants to merge 5 commits into
mattleibow wants to merge 5 commits into
Conversation
Apply PR #4107's minimal alternate fix for the re-entrant static-init NullReferenceException (#3817) and bring over PR #4080's full test suite verbatim to observe whether #4107's narrower approach also exhibits the crashes/issues #4080's tests probe. Production changes: - SKTypeface.cs: #4107's fix. The static cctor builds the default typeface from raw native handles in a try/finally, severing the managed-singleton reads that caused the re-entrant cold-start crash. - HandleDictionary.cs / SKObject.cs: two ADDITIVE, test-enabling helpers ported from #4080 (5-arg GetOrAddObject overload and GetOrAddDisposeProtectedObject). These exist only so #4080's tests compile; NO production singleton accessor is routed through them, so #4107's runtime behavior is unchanged. Test infrastructure (verbatim from #4080 / #4107): - SkiaSharp.Tests.SingletonInit.Console assembly + cake + sln wiring (identical in both PRs): dedicated #3817 cold-start regression guard. - GarbageCleanupFixture.cs: #4080's rewrite keys "expected dead" off IgnorePublicDispose instead of a hard-coded static-type name list. - Full set of #4080 lifecycle/concurrency/reentrancy test files. Experimental result (macOS arm64, net10.0, full single-process assembly run): - SingletonInit.Console #3817 cold-start guard: PASSED. #4107 fixes the crash. - Full suite ran to completion: NO crashes, NO hangs, NO access violations. 7630 total, 5697 passed, 12 skipped, 1921 failed (4m30s). - 1908 of the failures share ONE root cause: the rewritten assembly-cleanup fixture asserts statics carry IgnorePublicDispose=true; #4107 does not set that flag, so the assembly-teardown Assert.Empty fails and is attributed across many tests. - The remaining 13 genuine failures all trace to the same gap: #4107 does not implement #4080's IgnorePublicDispose-based dispose-protection model (statics aren't flagged; PreventPublicDisposal doesn't throw on a disposed wrapper). Identity-under-contention assertions (AssertAllSame) PASS, so #4107's singleton accessors are stable under concurrency. Conclusion: #4107 resolves the #3817 re-entrancy crash without reproducing the crashes #4080's tests probe; it simply lacks #4080's broader dispose-protection feature. Existing static wrapper subclasses (SKDataStatic, SKColorSpaceStatic, SKTypefaceStatic, ...) still override Dispose(bool) as a no-op with owns=false, so public disposal of singletons remains a no-op even without the flag. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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 -- 4116PowerShell / Windows: iex "& { $(irm https://raw.githubusercontent.com/mono/SkiaSharp/main/scripts/get-skiasharp-pr.ps1) } 4116"Step 2 — Add the local NuGet source dotnet nuget add source ~/.skiasharp/hives/pr-4116/packages --name skiasharp-pr-4116More options
Or download manually from Azure Pipelines — look for the Remove the source when you're done: dotnet nuget remove source skiasharp-pr-4116 |
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. |
…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>
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
changed the title
…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>
Re-run CI to confirm green after the macOS netcore agent hang on the previous build was a one-off infra timeout (retry passed cleanly). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Author
|
Closing in favor of #4080 (merged). The real fix was two-part: |
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
1 participant
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.
What this is
An alternate attempt to PR #4080 for the re-entrant static-init
NullReferenceException(#3817). Instead of #4080's broader rework, this PR takes PR #4107's narrowerSKTypefacefix and brings in #4080's full test suite, reconciling the two so every test passes.Both PRs target the same bug; this is the smaller production change of the two, validated against the larger test set.
Production changes
SKTypeface.cs— PR Fix #3817: avoid re-entrant singleton cctor NullReferenceException #4107's fix. The static cctor builds the empty/default typefaces from raw native handles in atry/finally, severing the managed-singleton reads (SKFontManager.Default/SKFontStyle.Normal) that triggered the re-entrant cold-start crash.SKColorSpaceStatic,SKDataStatic,SKBlenderStatic,SKFontStyleStatic,SKFontManagerStatic,SKTypefaceStatic,SKColorFilterStatic,SKFontStatic) — each constructor now callsPreventPublicDisposal()so the singleton carriesIgnorePublicDispose = true. These wrappers already protected disposal structurally (no-opDispose(bool)+owns=false); setting the flag unifies that with theIgnorePublicDisposecontract Rework the lifecycle of singleton instances #4080's tests assert. Purely additive and ABI-safe.Tests (from #4080)
SkiaSharp.Tests.SingletonInit.Console— dedicated [BUG] SKFontManager.Default throws an exception #3817 cold-start regression guard (single-process assembly).GarbageCleanupFixture.cs— restored to main's static-type name-list (the Rework the lifecycle of singleton instances #4080IgnorePublicDispose-keyed rewrite is not needed here).Result
macOS arm64, net10.0, full single-process assembly run:
Failed: 0, Passed: 5706, Skipped: 13— fully green, including the #3817 cold-start guard and the previously-flakySKDocumentTest.StreamIsNotCollectedPrematurely.Conclusion
#4107's minimal
SKTypefacefix resolves the #3817 re-entrancy crash, and thePreventPublicDisposal()reconciliation lets it satisfy #4080's complete dispose-protection / concurrency / lifecycle test suite. This is the lower-risk production change relative to #4080.Refs #3817. Alternative to #4080; brings in #4107's fix.