Don't dispose SKStreamAsset in ToHarfBuzzBlob

[jeremy-visionaid]

Description of Change

HarfBuzz Blob should not have dispose ownership of SKStreamAsset as it might still be in use by the caller.

SKShaper should also deterministically dispose the stream.

Bugs Fixed

Discussion #3373

API Changes

None.

Behavioral Changes

ToHarfBuzzBlob no longer takes dispose ownership of the given SKStreamAsset

Required skia PR

None.

PR Checklist

• Has tests (if omitted, state reason in description)
• Rebased on top of main at time of PR
• Merged related skia PRs
• Changes adhere to coding standard
• Updated documentation

[mattleibow]

This looks much better. I asked copilot to write some tests (#3467) that will cover this. Hopefully it works. But please see if you can use these (when done) as areference or write specific tests for this change so we don't do this again.

[Copilot]

Pull request overview

This PR adjusts ownership and lifetime handling between SKStreamAsset and HarfBuzz Blob when constructing SKShaper, so that the HarfBuzz blob no longer disposes the Skia stream asset and SKShaper deterministically disposes its own stream.

Changes:

• Updated SKShaper to explicitly open a typeface stream into a using variable and convert it to a HarfBuzz Blob before creating the Face/Font, ensuring deterministic disposal of the SKStreamAsset.
• Updated BlobExtensions.ToHarfBuzzBlob so that, for memory-backed streams, the created HarfBuzz Blob no longer takes a dispose callback that disposes the SKStreamAsset, leaving ownership and disposal of the stream to the caller.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
source/SkiaSharp.HarfBuzz/SkiaSharp.HarfBuzz/SKShaper.cs	Changes SKShaper’s constructor to store the result of Typeface.OpenStream(out index) in a using variable before calling ToHarfBuzzBlob, so the SKStreamAsset is deterministically disposed by SKShaper instead of indirectly via the HarfBuzz blob.
source/SkiaSharp.HarfBuzz/SkiaSharp.HarfBuzz/BlobExtensions.cs	Removes the dispose callback from the Blob constructed from asset.GetMemoryBase(), so HarfBuzz no longer disposes the SKStreamAsset; the asset’s lifetime is now entirely managed by its caller.

[jeremy-visionaid]

Yeah, I was curious about your thoughts for testing this. There are 2 changes here:

1. The stream shouldn't be being disposed by the blob because it doesn't own it
2. The stream in SKShaper should be being disposed deterministically

Part one is trivial, I don't mind doing it to prevent regressions (Copilot's attempt seems a bit clumsy and off topic IMHO). For the second part, the stream is internal so we don't have a reference to it - there are missing dispose calls in other places too (e.g. #3319). Are you happy just accepting that existing tests still work for those kinds of cases?

[jeremy-visionaid]

Heya. Sorry that took a while - took a while to update workloads etc. for the latest SDK. I've added a couple of simpler and more targeted tests. One to test for regression of the unwanted disposal (that would currently fail on main) and another to ensure that a managed reference to the stream was not required by the blob (since it previously had one from the release delegate). Probably some more HarfBuzz related tests would be good, but feels to me like the Copilot ones either test the wrong things or are beyond the scope of this PR.

[mattleibow]

PR Review: Don't dispose SKStreamAsset in ToHarfBuzzBlob (#3466)

Hey @jeremy-visionaid! Thanks for fixing this ownership issue. The change to not dispose the stream is correct — ToHarfBuzzBlob() shouldn't dispose a stream it doesn't own. 👍

⚠️ Note: Lifetime Requirement

The blob may reference the stream's memory directly (zero-copy for performance). The caller must keep the stream alive for the lifetime of the blob. This was always the case, but the previous code obscured it by having the blob dispose the stream.

@mattleibow — The docs for ToHarfBuzzBlob should mention that sometimes the stream data is not copied, and the stream must stay alive for the lifetime of the blob.

✅ Good changes

1. SKShaper.cs — Explicitly disposing the stream with using is proper resource management
2. ToHarfBuzzBlobDoesNotDisposeStream test — Good regression test

🟡 Add code comment about lifetime requirements

Please add a comment in the code explaining the lifetime contract:

var memoryBase = asset.GetMemoryBase();
if (memoryBase != IntPtr.Zero)
{
    // The blob references the stream's memory directly (zero-copy).
    // The caller must keep the stream alive for the lifetime of the blob.
    blob = new Blob(memoryBase, size, MemoryMode.ReadOnly);
}

🔴 Exception safety issue in copy path

The else branch has a memory leak if asset.Read() or new Blob() throws:

else
{
    var ptr = Marshal.AllocCoTaskMem(size);
    asset.Read(ptr, size);  // If this throws, ptr leaks
    blob = new Blob(ptr, size, MemoryMode.ReadOnly, () => Marshal.FreeCoTaskMem(ptr));
}

Suggested fix:

else
{
    var ptr = Marshal.AllocCoTaskMem(size);
    try
    {
        asset.Read(ptr, size);
        blob = new Blob(ptr, size, MemoryMode.ReadOnly, () => Marshal.FreeCoTaskMem(ptr));
    }
    catch
    {
        Marshal.FreeCoTaskMem(ptr);
        throw;
    }
}

🔴 Test issue: ToHarfBuzzBlobBlobCanBeUsedOutsideStreamLifetime

This test is unsafe if the stream has a memoryBase (which SKTypeface.OpenStream() likely does). The blob would have a dangling pointer after the stream is disposed.

Suggested fix: Add an assert to ensure the test is actually exercising the copy path:

[SkippableFact]
public void ToHarfBuzzBlobBlobCanBeUsedOutsideStreamLifetime()
{
    static Blob GetBlob(SKTypeface typeface)
    {
        using var stream = typeface.OpenStream(out var index);
        
        // Ensure this test is exercising the copy path, not the zero-copy path
        Assert.Equal(IntPtr.Zero, stream.GetMemoryBase());
        
        return stream.ToHarfBuzzBlob();
    }

    var fontFile = Path.Combine(PathToFonts, "content-font.ttf");
    using var tf = SKTypeface.FromFile(fontFile);
    using var blob = GetBlob(tf);
    Assert.Equal(1, blob.FaceCount);
}

If the assert fails, the test should be removed as it would be testing undefined behavior.

💡 Future enhancement: bool copyData parameter

Following existing SkiaSharp patterns (e.g., SKMemoryStream(data, length, bool copyData)), consider adding an overload:

public static Blob ToHarfBuzzBlob(this SKStreamAsset asset, bool copyData = false)

When copyData = true, always copy to CoTaskMem regardless of memoryBase. This gives users a safe option when they don't want to manage stream lifetime. The test could then become:

[SkippableFact]
public void ToHarfBuzzBlobWithCopyCanBeUsedOutsideStreamLifetime()
{
    static Blob GetBlob(SKTypeface typeface)
    {
        using var stream = typeface.OpenStream(out var index);
        return stream.ToHarfBuzzBlob(copyData: true);  // Force copy, safe to outlive stream
    }

    var fontFile = Path.Combine(PathToFonts, "content-font.ttf");
    using var tf = SKTypeface.FromFile(fontFile);
    using var blob = GetBlob(tf);
    Assert.Equal(1, blob.FaceCount);
}

(Not required for this PR)

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

source/SkiaSharp.HarfBuzz/SkiaSharp.HarfBuzz/BlobExtensions.cs:30

• The memoryBase != IntPtr.Zero branch still constructs the HarfBuzz Blob directly over the SKStreamAsset's unmanaged buffer without copying and with no lifetime callback, so if the caller disposes the SKStreamAsset before disposing the Blob (as in the new ToHarfBuzzBlobBlobCanBeUsedOutsideStreamLifetime test pattern), the Blob will hold a pointer to freed memory. To make the extension safe and consistent with the new test's expectations, this branch should also ensure the blob's storage outlives the stream (for example by copying into owned memory as in the else branch, or otherwise guaranteeing that the asset's buffer remains valid for the blob's lifetime).

			var memoryBase = asset.GetMemoryBase();
			if (memoryBase != IntPtr.Zero)
			{
				blob = new Blob(memoryBase, size, MemoryMode.ReadOnly);
			}
			else
			{
				var ptr = Marshal.AllocCoTaskMem(size);
				asset.Read(ptr, size);
				blob = new Blob(ptr, size, MemoryMode.ReadOnly, () => Marshal.FreeCoTaskMem(ptr));