sharpdx / SharpDX

SharpDX GitHub Repository
http://sharpdx.org
MIT License
1.7k stars 639 forks source link

TextLayout.Metrics throws ObjectDisposedException #1034

Open MagicMau opened 6 years ago

MagicMau commented 6 years ago

After upgrading to 4.1.0 from 4.0.1, I get that TextLayout.Metrics throws an ObjectDisposedException. My current flow is that I create a new TextLayout and then call TextLayout.Metrics on it.

Since upgrading, this throws an ObjectDisposedException. When calling TextLayout.Metrics in the immediate window, the metrics start showing up. Not sure why this stopped working, or what has changed in SharpDX 4.1.0 that changed the behaviour.

Downgrading to 4.0.1 resolves this issue. Any ideas what might go wrong?

Gillibald commented 6 years ago

I have the same issue and thats my current workaround:

var metrics = new DWrite.TextMetrics();

try
{
    metrics = TextLayout.Metrics;
}
catch (ObjectDisposedException)
{
    metrics = TextLayout.Metrics;
}        

Don't know why this works but it seems that the first get always fails.

jkoritzinsky commented 6 years ago

What's the stack trace of the exception?

Gillibald commented 6 years ago

I try to get the Stacktrace later this day when I have time.

MagicMau commented 6 years ago

The exception is:

'Layout.Metrics' threw an exception of type 'System.ObjectDisposedException'
    Data: {System.Collections.ListDictionaryInternal}
    HResult: -2146232798
    HelpLink: null
    InnerException: null
    Message: "Cannot access a disposed object.\r\nObject name: 'Cannot add a reference to a nonreferenced item'."
    ObjectName: "Cannot add a reference to a nonreferenced item"
    Source: "SharpDX"
    StackTrace: "   at SharpDX.CallbackBase.AddReference()\r\n   at SharpDX.DirectWrite.TextLayout.GetMetrics(TextMetrics& textMetrics)\r\n   at SharpDX.DirectWrite.TextLayout.get_Metrics()"
    TargetSite: {Int32 AddReference()}

calling textLayout.Metrics again (from Visual Studio's immediate window for example) results in a correct result.

jkoritzinsky commented 6 years ago

Any chance you can get the type of the this object in the AddReference call?

MagicMau commented 6 years ago

I've tried, but haven't been able to pinpoint what "this" is in this case. Unfortunately I was unable to build a debug build of the SharpDX code to see if I could dive deeper into the exception. Perhaps @Gillibald has had more success?

Gillibald commented 6 years ago

I had no luck too. That part is unmanaged and couldn't figure out to make it possible to debug. Probably have to build everything from source.

MagicMau commented 5 years ago

Update: I've seem to have narrowed it down to calls to Release() on the custom font loaders that I use. These loaders are copied from the CustomFont example: https://github.com/sharpdx/SharpDX-Samples/tree/f4fb0fe0f12e3edc4eda3f6307d135a66fa172cd/Desktop/DirectWrite/CustomFont

Now to find out why Release() is called on these fontloaders in 4.1/4.2 where they apparently weren't called in 4.0.1.

MagicMau commented 5 years ago

OK, I seem to have fixed it (for my case) by rewriting the example ResourceFontFileStream to not extend CallbackBase, but provide a (dummy) implementation.

This resulted in the following code, which seems to work, although I am a bit worried that I am setting comObject in QueryInterface to zero.

/// <summary>
/// This FontFileStream implem is reading data from a <see cref="DataStream"/>.
/// </summary>
public class ResourceFontFileStream : FontFileStream
{
    private readonly DataStream _stream;
    private bool disposedValue = false; // To detect redundant calls to Dispose()

    public IDisposable Shadow { get; set; }

    /// <summary>
    /// Initializes a new instance of the <see cref="ResourceFontFileStream"/> class.
    /// </summary>
    /// <param name="stream">The stream.</param>
    public ResourceFontFileStream(DataStream stream) => _stream = stream;

    /// <summary>
    /// Reads a fragment from a font file.
    /// </summary>
    /// <param name="fragmentStart">When this method returns, contains an address of a  reference to the start of the font file fragment.  This parameter is passed uninitialized.</param>
    /// <param name="fileOffset">The offset of the fragment, in bytes, from the beginning of the font file.</param>
    /// <param name="fragmentSize">The size of the file fragment, in bytes.</param>
    /// <param name="fragmentContext">When this method returns, contains the address of</param>
    /// <remarks>
    /// Note that ReadFileFragment implementations must check whether the requested font file fragment is within the file bounds. Otherwise, an error should be returned from ReadFileFragment.   {{DirectWrite}} may invoke <see cref="SharpDX.DirectWrite.FontFileStream"/> methods on the same object from multiple threads simultaneously. Therefore, ReadFileFragment implementations that rely on internal mutable state must serialize access to such state across multiple threads. For example, an implementation that uses separate Seek and Read operations to read a file fragment must place the code block containing Seek and Read calls under a lock or a critical section.
    /// </remarks>
    /// <unmanaged>HRESULT IDWriteFontFileStream::ReadFileFragment([Out, Buffer] const void** fragmentStart,[None] __int64 fileOffset,[None] __int64 fragmentSize,[Out] void** fragmentContext)</unmanaged>
    public void ReadFileFragment(out IntPtr fragmentStart, long fileOffset, long fragmentSize, out IntPtr fragmentContext)
    {
        lock (this)
        {
            fragmentContext = IntPtr.Zero;
            _stream.Position = fileOffset;
            fragmentStart = _stream.PositionPointer;
        }
    }

    /// <summary>
    /// Releases a fragment from a file.
    /// </summary>
    /// <param name="fragmentContext">A reference to the client-defined context of a font fragment returned from {{ReadFileFragment}}.</param>
    /// <unmanaged>void IDWriteFontFileStream::ReleaseFileFragment([None] void* fragmentContext)</unmanaged>
    public void ReleaseFileFragment(IntPtr fragmentContext)
    {
        // Nothing to release. No context are used
    }

    /// <summary>
    /// Obtains the total size of a file.
    /// </summary>
    /// <returns>the total size of the file.</returns>
    /// <remarks>
    /// Implementing GetFileSize() for asynchronously loaded font files may require downloading the complete file contents. Therefore, this method should be used only for operations that either require a complete font file to be loaded (for example, copying a font file) or that need to make decisions based on the value of the file size (for example, validation against a persisted file size).
    /// </remarks>
    /// <unmanaged>HRESULT IDWriteFontFileStream::GetFileSize([Out] __int64* fileSize)</unmanaged>
    public long GetFileSize() => _stream.Length;

    /// <summary>
    /// Obtains the last modified time of the file.
    /// </summary>
    /// <returns>
    /// the last modified time of the file in the format that represents the number of 100-nanosecond intervals since January 1, 1601 (UTC).
    /// </returns>
    /// <remarks>
    /// The "last modified time" is used by DirectWrite font selection algorithms to determine whether one font resource is more up to date than another one.
    /// </remarks>
    /// <unmanaged>HRESULT IDWriteFontFileStream::GetLastWriteTime([Out] __int64* lastWriteTime)</unmanaged>
    public long GetLastWriteTime() => 0;

    public Result QueryInterface(ref Guid guid, out IntPtr comObject)
    {
        comObject = IntPtr.Zero;
        return Result.Ok;
    }        

    public int AddReference() => 0;

    public int Release() => 0;

    protected virtual void Dispose(bool disposing)
    {
        if (!disposedValue)
        {
            if (disposing)
            {
                _stream.Dispose();
            }

            disposedValue = true;
        }
    }

    // This code added to correctly implement the disposable pattern.
    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }
}
jgimness commented 5 years ago

Burned a couple hours today looking at this issue.

@MagicMau Your code "works", but it will leak memory because the underlying COM shadow objects will never get disposed. You can see how the comObject is set in https://github.com/sharpdx/SharpDX/blob/1f89381bc08f2d8cf8d294c6c5fce8dc85a849a1/Source/SharpDX/CallbackBase.cs#L87 , but unfortunately ShadowContainer is private so we can't get access to it in our classes.

One of the problems is that the SharpDX Sample for custom fonts isn't done correctly, at least in the way the SharpDX library works. CreateStreamFromKey needs to actually create a create a new ResourceFontFileStream(stream) instead of returning the one that was created in the constructor. So modify this https://github.com/sharpdx/SharpDX-Samples/blob/f4fb0fe0f12e3edc4eda3f6307d135a66fa172cd/Desktop/DirectWrite/CustomFont/ResourceFontLoader.cs#L111 to do so. The reason is that when the font stream is first read, it gets AddReference and Release called multiple times and will actually dispose the underlying Shadow object (see here: https://github.com/sharpdx/SharpDX/blob/1f89381bc08f2d8cf8d294c6c5fce8dc85a849a1/Source/SharpDX/CallbackBase.cs#L75 )

Then, when we finally call textLayout.Metrics it calls AddReference and tries to get the metrics, but it fails because the underlying FontFileStreamShadow COM object is disposed.

What I have working is to override the Dispose method from CallbackBase with this to always release the shadow object

protected override void Dispose(bool disposing) { // Dispose native resources var callback = ((ICallbackable)this); if (callback.Shadow != null) { callback.Shadow.Dispose(); callback.Shadow = null; } }

To summarize

  1. Keep CallbackBase in ResourceFontFileStream
  2. Add the Dispose override to ResourceFontFileStream and ResourceFontLoader
  3. Have the CreateStreamFromKey method actually construct a new ResourceFontFileStream

@Gillibald This fix would be better than the try/catch that is in Avalonia.

Gillibald commented 5 years ago

@jgimness Thanks for the fix will try it out. Would have never been able to fix that myself 👍