Repository navigation
Use blittable byte for delegate callbacks and harden DelegateProxies - #5479
mattleibow wants to merge 2 commits into
Conversation
|
📖 Documentation Preview Add the Removing the label removes the preview. While the label is present, each new commit updates it. This comment is automatically updated by the documentation staging workflow. |
📦 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 -- 5479PowerShell / Windows: iex "& { $(irm https://raw.githubusercontent.com/mono/SkiaSharp/main/scripts/get-skiasharp-pr.ps1) } 5479"Step 2 — Add the local NuGet source dotnet nuget add source ~/.skiasharp/hives/pr-5479/packages --name skiasharp-pr-5479More options
Or download manually from Azure Pipelines — look for the Remove the source when you're done: dotnet nuget remove source skiasharp-pr-5479 |
📊 SkiaSharp benchmarks — PR #5479⭐ this PR (full source build) vs 🌙 nightly · Linux · Windows · macOS
Highlights⏱️ Time — 🔴 56 slower · 🟢 25 faster
Full per-OS benchmark deltasLinux⏱️ Time (vs 🌙 nightly
Windows⏱️ Time (vs 🌙 nightly
macOS⏱️ Time (vs 🌙 nightly
|
|
@dalexsoto review |
There was a problem hiding this comment.
🟡 Changes recommended
The tests do not cover recovery after callback exceptions, leaving error-state resets on successful Rewind/Seek/Move and preservation on failed repositioning unverified.
1 open issue
What changed in this PR
This PR hardens SkiaSharp and HarfBuzzSharp native callbacks by replacing callback bool values with blittable bytes, containing managed stream exceptions, and simplifying callback state storage.
Changes:
- Updates the generator, generated callback signatures, and proxy implementations to use
byte. - Adds stream exception containment and error-state tracking, with regression tests for stream failures and false-returning HarfBuzz callbacks.
- Replaces closure-based callback state with direct user-data handles and a shared state object.
| File | Description |
|---|---|
| utils/SkiaSharpGenerator/Generate/Generator.cs | Removes callback boolean marshalling attributes. |
| utils/SkiaSharpGenerator/BaseTool.cs | Maps callback booleans to bytes. |
| tests/Tests/SkiaSharp/SKManagedWStreamTest.cs | Tests throwing write-stream callbacks. |
| tests/Tests/SkiaSharp/SKManagedStreamTest.cs | Tests callback failures and disposed-stream termination. |
| tests/Tests/HarfBuzzSharp/HBUnicodeFuncsTest.cs | Tests false-returning Unicode callbacks. |
| tests/Tests/HarfBuzzSharp/HBFontFuncsTest.cs | Tests false-returning font callbacks. |
| binding/SkiaSharp/SKAbstractManagedStream.cs | Adds callback error-state storage. |
| binding/SkiaSharp/Generated/SKManagedWStreamWriteProxyDelegate.generated.cs | Changes write delegate return to byte. |
| binding/SkiaSharp/Generated/SKManagedWStreamDelegates.generated.cs | Updates write function-pointer signature. |
| binding/SkiaSharp/Generated/SKManagedStreamSeekProxyDelegate.generated.cs | Changes seek delegate return to byte. |
| binding/SkiaSharp/Generated/SKManagedStreamRewindProxyDelegate.generated.cs | Changes rewind delegate return to byte. |
| binding/SkiaSharp/Generated/SKManagedStreamMoveProxyDelegate.generated.cs | Changes move delegate return to byte. |
| binding/SkiaSharp/Generated/SKManagedStreamIsAtEndProxyDelegate.generated.cs | Changes EOF delegate return to byte. |
| binding/SkiaSharp/Generated/SKManagedStreamHasPositionProxyDelegate.generated.cs | Changes position-capability return to byte. |
| binding/SkiaSharp/Generated/SKManagedStreamHasLengthProxyDelegate.generated.cs | Changes length-capability return to byte. |
| binding/SkiaSharp/Generated/SKManagedStreamDelegates.generated.cs | Updates stream function-pointer signatures. |
| binding/SkiaSharp/Generated/SKGraphiteImageProviderProxyDelegate.generated.cs | Changes mipmapping parameter to byte. |
| binding/SkiaSharp/Generated/DelegateProxies_SKManagedWStreamWriteProxyDelegate.generated.cs | Updates write proxy declarations. |
| binding/SkiaSharp/Generated/DelegateProxies_SKManagedStreamSeekProxyDelegate.generated.cs | Updates seek proxy declarations. |
| binding/SkiaSharp/Generated/DelegateProxies_SKManagedStreamRewindProxyDelegate.generated.cs | Updates rewind proxy declarations. |
| binding/SkiaSharp/Generated/DelegateProxies_SKManagedStreamMoveProxyDelegate.generated.cs | Updates move proxy declarations. |
| binding/SkiaSharp/Generated/DelegateProxies_SKManagedStreamIsAtEndProxyDelegate.generated.cs | Updates EOF proxy declarations. |
| binding/SkiaSharp/Generated/DelegateProxies_SKManagedStreamHasPositionProxyDelegate.generated.cs | Updates position-capability proxy declarations. |
| binding/SkiaSharp/Generated/DelegateProxies_SKManagedStreamHasLengthProxyDelegate.generated.cs | Updates length-capability proxy declarations. |
| binding/SkiaSharp/Generated/DelegateProxies_SKGraphiteImageProviderProxyDelegate.generated.cs | Updates image-provider proxy declarations. |
| binding/SkiaSharp/DelegateProxies.wstream.cs | Contains write-stream callback exceptions. |
| binding/SkiaSharp/DelegateProxies.stream.cs | Contains exceptions and tracks stream error recovery. |
| binding/SkiaSharp/DelegateProxies.cs | Converts mipmapping byte to managed boolean. |
| binding/HarfBuzzSharp/Generated/UnicodeDecomposeProxyDelegate.generated.cs | Changes decomposition return to byte. |
| binding/HarfBuzzSharp/Generated/UnicodeComposeProxyDelegate.generated.cs | Changes composition return to byte. |
| binding/HarfBuzzSharp/Generated/FontGetVariationGlyphProxyDelegate.generated.cs | Changes variation-glyph return to byte. |
| binding/HarfBuzzSharp/Generated/FontGetNominalGlyphProxyDelegate.generated.cs | Changes nominal-glyph return to byte. |
| binding/HarfBuzzSharp/Generated/FontGetGlyphProxyDelegate.generated.cs | Changes glyph lookup return to byte. |
| binding/HarfBuzzSharp/Generated/FontGetGlyphOriginProxyDelegate.generated.cs | Changes glyph-origin return to byte. |
| binding/HarfBuzzSharp/Generated/FontGetGlyphNameProxyDelegate.generated.cs | Changes glyph-name return to byte. |
| binding/HarfBuzzSharp/Generated/FontGetGlyphFromNameProxyDelegate.generated.cs | Changes name lookup return to byte. |
| binding/HarfBuzzSharp/Generated/FontGetGlyphExtentsProxyDelegate.generated.cs | Changes glyph-extents return to byte. |
| binding/HarfBuzzSharp/Generated/FontGetGlyphContourPointProxyDelegate.generated.cs | Changes contour-point return to byte. |
| binding/HarfBuzzSharp/Generated/FontGetFontExtentsProxyDelegate.generated.cs | Changes font-extents return to byte. |
| binding/HarfBuzzSharp/Generated/DelegateProxies_UnicodeDecomposeProxyDelegate.generated.cs | Updates decomposition proxy declarations. |
| binding/HarfBuzzSharp/Generated/DelegateProxies_UnicodeComposeProxyDelegate.generated.cs | Updates composition proxy declarations. |
| binding/HarfBuzzSharp/Generated/DelegateProxies_FontGetVariationGlyphProxyDelegate.generated.cs | Updates variation-glyph proxy declarations. |
| binding/HarfBuzzSharp/Generated/DelegateProxies_FontGetNominalGlyphProxyDelegate.generated.cs | Updates nominal-glyph proxy declarations. |
| binding/HarfBuzzSharp/Generated/DelegateProxies_FontGetGlyphOriginProxyDelegate.generated.cs | Updates glyph-origin proxy declarations. |
| binding/HarfBuzzSharp/Generated/DelegateProxies_FontGetGlyphNameProxyDelegate.generated.cs | Updates glyph-name proxy declarations. |
| binding/HarfBuzzSharp/Generated/DelegateProxies_FontGetGlyphFromNameProxyDelegate.generated.cs | Updates name lookup proxy declarations. |
| binding/HarfBuzzSharp/Generated/DelegateProxies_FontGetGlyphExtentsProxyDelegate.generated.cs | Updates glyph-extents proxy declarations. |
| binding/HarfBuzzSharp/Generated/DelegateProxies_FontGetGlyphContourPointProxyDelegate.generated.cs | Updates contour-point proxy declarations. |
| binding/HarfBuzzSharp/Generated/DelegateProxies_FontGetFontExtentsProxyDelegate.generated.cs | Updates font-extents proxy declarations. |
| binding/HarfBuzzSharp/Generated/BufferMessageProxyDelegate.generated.cs | Changes buffer-message return to byte. |
| binding/HarfBuzzSharp/DelegateProxies.unicode.cs | Converts Unicode callback results to bytes. |
| binding/HarfBuzzSharp/DelegateProxies.font.cs | Converts font results and short-circuits failed name lookups. |
| binding/Binding.Shared/DelegateProxies.shared.cs | Replaces closures with direct callback state storage. |
Review effort: Balanced
- Update SkiaSharpGenerator to emit blittable Byte instead of bool (and omit [MarshalAs(UnmanagedType.I1)]) for generated delegate types, unmanaged function pointers, and DelegateProxies implementations, avoiding non-blittable bool return ABI issues under [UnmanagedCallersOnly] and Mono's ARM64 interpreter (dotnet/runtime#136578). - Wrap SKManagedStream and SKManagedWStream proxy callbacks in try/catch/finally so exceptions from underlying .NET Streams cannot escape [UnmanagedCallersOnly] and abort the process, and mark the stream as at-end on read/seek failures so native read-until-EOF loops terminate immediately. - Simplify DelegateProxies.CreateUserData/GetUserData and CreateMulti/CreateMultiUserData to store userData or a MultiUserData state object directly in the GCHandle instead of allocating and invoking nested closure delegates on every callback. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…overy Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
baee8fe to
146011d
Compare

Description
Eliminates non-blittable
boolin generated native callback delegates (*ProxyDelegate), unmanaged function pointers (delegate* unmanaged[Cdecl]<...>), andDelegateProxiesimplementations, hardensSKManagedStream/SKManagedWStreamreverse P/Invoke callbacks against uncaught managed exceptions, and simplifiesDelegateProxiesuser-data and multi-delegate state storage:Generator & generated bindings (
utils/SkiaSharpGenerator,binding/*/Generated):boolis non-blittable in .NET,[MarshalAs(UnmanagedType.I1)]is ignored by[UnmanagedCallersOnly], and Mono's ARM64 interpreter (dotnet/runtime#136578) writesMONO_TYPE_BOOLEANreturn values using a 1-bytestrbthat leaves bits8..63ofx0/w0dirty with the first argument pointer — breaking Apple ARM64 C++boolcallers (which emitcbz w0) as well as HarfBuzzhb_bool_t(typedef int hb_bool_t) callbacks.GetManagedFunctionArgumentsandGetFunctionPointerTypenow map Cboolandhb_bool_t(Boolean) to blittableByte(byte) without[MarshalAs(UnmanagedType.I1)]on delegates or delegate proxy implementations, ensuring all 64 bits ofx0/raxare zero-extended on return across RyuJIT and the Mono interpreter.DelegateProxies.stream.cs,DelegateProxies.wstream.cs,DelegateProxies.cs(SKGraphiteImageProviderProxyImplementation),DelegateProxies.font.cs, andDelegateProxies.unicode.cs.Exception containment in
DelegateProxies.stream.cs&DelegateProxies.wstream.cs:SKManagedStream*ProxyImplementationand all 4SKManagedWStream*ProxyImplementationcallbacks intry/catch(withif (gch.IsAllocated) gch.Free()infinallyforDestroy) so exceptions thrown by an underlying.NET Streamcannot escape[UnmanagedCallersOnly]and abort the process (PAL_SEHException).SKAbstractManagedStream(hasError) so subsequentSKManagedStreamIsAtEndProxyImplementationcalls return1(true), ensuring nativedo { read(); } while (!isAtEnd());loops terminate immediately even whenOnIsAtEnd()itself does not throw (such as a disposedMemoryStreamwhereCanSeekisfalse).DelegateProxies.shared.csmodernization:CreateUserData/GetUserDataso theGCHandledirectly holdsuserData(or aWeakReference) rather than allocating aUserDataDelegateclosure and invoking it on every callback.CreateMulti/GetMultiandCreateMultiUserData/GetMultiUserDatato store a lightweightMultiUserDatastate object instead of nestedGetMultiDelegateDelegate+UserDataDelegateclosures.Related issues
Related to #3377
Required skia PR
None.
Areas affected
binding/)externals/skia/src/c,include/c)Changes
Behavior
SKAbstractManagedStream/SKManagedStreamandSKAbstractManagedWStream/SKManagedWStreamno longer allow managed exceptions from an underlying.NET Streamto escape[UnmanagedCallersOnly]and crash the process; failed reads mark the stream as at-end so native read-until-EOF loops terminate cleanly.SkiaSharpandHarfBuzzSharpuse blittablebyteinstead ofbool.Testing
pwsh -NoLogo -NoProfile -File ./utils/generate.ps1— passedSkiaSharpandHarfBuzzSharpacross all target frameworks:dotnet build binding/SkiaSharp/SkiaSharp.csproj— passed (0 errors)dotnet build binding/HarfBuzzSharp/HarfBuzzSharp.csproj— passed (0 errors)net10.0):dotnet test tests/SkiaSharp.Tests.Console.slnx -p:TargetFramework=net10.0 -p:TargetFrameworks=net10.0— passed (SkiaSharp.Tests: 6451 passed, 0 failed, 23 skipped;SkiaSharp.Gallery.Tests.Console: 50 passed, 0 failed;SkiaSharp.Tests.SingletonInit: 1 passed, 0 failed;SkiaSharp.Direct3D.Tests: 2 passed, 0 failed, 3 skipped;SkiaSharp.Vulkan.Tests: 4 passed, 0 failed, 21 skipped)SKManagedStreamTest,SKManagedWStreamTest,HBFontFuncsTest, andHBUnicodeFuncsTest.Checklist
Changesabove lists all public API and behavioral changes (or "None.")///documentation and regenerated bindings when applicablemono/skiaPR linked above and bindings regenerated