Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XGraphics.FromPdfPage disposes object returned by static method #151

Open
moni-dips opened this issue Aug 14, 2024 · 7 comments
Open

XGraphics.FromPdfPage disposes object returned by static method #151

moni-dips opened this issue Aug 14, 2024 · 7 comments
Labels
bug Something isn't working

Comments

@moni-dips
Copy link

moni-dips commented Aug 14, 2024

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
Copy link
Member

StLange commented Aug 15, 2024

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.

@ThomasHoevel ThomasHoevel added the bug Something isn't working label Aug 15, 2024
@moni-dips
Copy link
Author

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

@ThomasHoevel
Copy link
Member

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
Copy link
Author

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

@ThomasHoevel
Copy link
Member

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

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

@moni-dips
Copy link
Author

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
Copy link
Member

Above is the disassembly, but here is a link to the real source code:
Line 732 has the using that is causing trouble:

using XGraphics gfx = new XGraphics(page, options, unit, XPageDirection.Downwards);

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants