empira / PDFsharp

PDFsharp and MigraDoc Foundation for .NET 6 and .NET Framework
https://docs.pdfsharp.net/
Other
531 stars 132 forks source link

XGraphics.FromPdfPage disposes object returned by static method #151

Open moni-dips opened 3 months ago

moni-dips commented 3 months ago

Expected Behavior

Adding content to a document using XGraphics works

Actual Behavior

Document is blank

Steps to Reproduce the Behavior

Install PDFsharp 6.1.1 from nuget.org

Run this code

var document = new PdfDocument()
var page = document.AddPage();
var graphics = XGraphics.FromPdfPage(page, XGraphicsPdfPageOptions.Append, XGraphicsUnit.Millimeter);
var font = new XFont("Arial", 12, XFontStyleEx.Regular);
graphics.DrawString("sample text", font, new XSolidBrush(XColor.FromKnownColor(XKnownColor.Black)), new XPoint(0, 0), XStringFormats.TopLeft);
var docStream = new MemoryStream();
document.Save(docStream, true);
var pdf = Encoding.UTF8.GetString(docStream.ToArray());

Inspect resulting pdf document

Workaround

Stop using that particular override by adding , XPageDirection.Downwards to the argument list

Reason for bug

Looking at disassembly, I found this:

// Decompiled with JetBrains decompiler
// Type: PdfSharp.Drawing.XGraphics
// Assembly: PdfSharp, Version=6.1.1.0, Culture=neutral, PublicKeyToken=f94615aa0424f9eb
...
namespace PdfSharp.Drawing
{
  /// <summary>Represents a drawing surface for a fixed size page.</summary>
  public sealed class XGraphics : IDisposable
  {
...
    /// <summary>
    /// Creates a new instance of the XGraphics class from a PdfSharp.Pdf.PdfPage object.
    /// </summary>
    public static XGraphics FromPdfPage(
      PdfPage page,
      XGraphicsPdfPageOptions options,
      XGraphicsUnit unit)
    {
      using (XGraphics xgraphics = new XGraphics(page, options, unit, XPageDirection.Downwards))
      {
        if (page.Owner._uaManager != null)
          page.Owner.Events.OnPageGraphicsCreated((object) page.Owner, new PageGraphicsEventArgs((PdfObject) page.Owner)
          {
            Page = page,
            Graphics = xgraphics,
            ActionType = PageGraphicsActionType.GraphicsCreated
          });
        PdfSharpLogHost.Logger.XGraphicsCreated("PDF page");
        return xgraphics;
      }
    }
...

The using statement in this static method disposes the object being returned, thus making it not work.

StLange commented 3 months ago

You are right. I have analyzed what I have done, and it seems that I operated ReSharper incorrectly in January this year during a code review. Instead of replacing XGraphics with var using ReSharper, I accidentally selected the second option “Converting declaration into using declaration”. In all other variations of this function, I did it correctly. Very stupid mistake.

moni-dips commented 3 months ago

Easy mistake to do and not immediately obvious :) maybe ReSharper ought to warn about returning objects that are being disposed? ;)

ThomasHoevel commented 3 months ago

There is a warning in VS at the return statement (can't say if it is ReSharper or VS), but easy to miss when you make a change five or six lines higher in the source code. No warning in the Output window during compile.

moni-dips commented 3 months ago

I filed a ticket with JetBrains referring to this ticket just in case :)

ThomasHoevel commented 3 months ago

ReSharper really should not propose a change that makes the method unusable.

At the return statement, they propose to remove the "using".

moni-dips commented 3 months ago

as an aside, should using a disposed XGraphics instance throw an exception rather than silently fail? a consumer can still dispose and use it, regardless of this issue

ThomasHoevel commented 3 months ago

Above is the disassembly, but here is a link to the real source code: Line 732 has the using that is causing trouble: https://github.com/empira/PDFsharp/blob/5fbf6ed14740bc4e16786816882d32e43af3ff5d/src/foundation/src/PDFsharp/src/PdfSharp/Drawing/XGraphics.cs#L732 At line 738, ReSharper now shows a warning and suggests to remove the using. Once the using is removed, ReSharper suggests at line 732 to either add a using block or a using declaration.