[API Implementation]: Expose `System.Drawing.Graphics.NativeGraphics`. #63058

deeprobin · 2021-12-21T21:26:36Z

Proposal implementation of dotnet/winforms#8833 (closes dotnet/winforms#8833)

Proposal

namespace System.Drawing
{
    public partial class Graphics
    {
        public IntPtr Handle { get; }
        public static Graphics FromHandle(IntPtr handle);
    }
    public partial class Brush
    {
        public IntPtr Handle { get; }
        public static Brush FromHandle(IntPtr handle);
    }
    public partial class Pen
    {
        public IntPtr Handle { get; }
        public static Pen FromHandle(IntPtr handle);
    }
    public partial class Image
    {
        public IntPtr Handle { get; }
        public static Image FromHandle(IntPtr handle);
    }

    public partial class Region
    {
        public IntPtr Handle { get; }
        public static Region FromHandle(IntPtr handle);
    }
}
namespace System.Drawing.Drawing2D
{
    public partial class Matrix
    {
        public IntPtr Handle { get; }
        public static Matrix FromHandle(IntPtr handle);
    }

    public partial class GraphicsPath
    {
        public IntPtr Handle { get; }
        public static GraphicsPath FromHandle(IntPtr handle);
    }
}

Current state of implementation

/cc @danmoseley

dotnet-issue-labeler · 2021-12-21T21:26:41Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2021-12-21T21:26:43Z

Tagging subscribers to this area: @dotnet/area-system-drawing
See info in area-owners.md if you want to be subscribed.

Issue Details

Proposal implementation of dotnet/winforms#8833 (closes dotnet/winforms#8833)

Proposal

namespace System.Drawing
{
    public partial class Graphics
    {
        public IntPtr Handle { get; }
        public static Graphics FromHandle(IntPtr handle);
    }
    public partial class Brush
    {
        public IntPtr Handle { get; }
        public static Brush FromHandle(IntPtr handle);
    }
    public partial class Pen
    {
        public IntPtr Handle { get; }
        public static Pen FromHandle(IntPtr handle);
    }
    public partial class Image
    {
        public IntPtr Handle { get; }
        public static Image FromHandle(IntPtr handle);
    }

    public partial class Region
    {
        public IntPtr Handle { get; }
        public static Region FromHandle(IntPtr handle);
    }
}
namespace System.Drawing.Drawing2D
{
    public partial class Matrix
    {
        public IntPtr Handle { get; }
        public static Matrix FromHandle(IntPtr handle);
    }

    public partial class GraphicsPath
    {
        public IntPtr Handle { get; }
        public static GraphicsPath FromHandle(IntPtr handle);
    }
}

Current state of implementation

Implementation
Ref Assembly
Documentation comments
Fix project files
I think because we forward in most cases the method to the private constructor, that we need no tests

/cc @danmoseley

Author:	deeprobin
Assignees:	-
Labels:	`area-System.Drawing`, `new-api-needs-documentation`
Milestone:	-

danmoseley · 2021-12-21T22:45:29Z

I think because we forward in most cases the method to the private constructor, that we need no tests

We do (or should) test all public API even pass throughs - the code will exist for years, refactoring happens over time and can introduce some issue or another. Plus, it ekes out a little more code coverage when we scan for missing coverage.

deeprobin · 2021-12-24T16:28:38Z

@danmoseley Why is there still a build pipeline for NET Framework (which fails)? I thought that is officially not supported anymore?

teo-tsirpanis · 2021-12-24T20:38:29Z

@deeprobin, System.Drawing.Common tests are executed on both .NET Framework and modern .NET to ensure behavioral compatibility between the two editions of the same library.

danmoseley · 2022-01-08T03:49:46Z

@deeprobin how's this one going? do you need help?

You have removed some entries from the ref, which creates a breaking change for code building against the non-netcoreapp target framework, eg., for .NET Standard. Existing API should stay in there. New API can go in the ref/...netcoreapp.cs file.

If possible, please use https://github.com/dotnet/runtime/blob/4679edc6be3eb91263a7acfe542dfd30b53ed2bf/docs/coding-guidelines/updating-ref-source.md#for-most-assemblies-within-libraries -- it doesn't understand multiple ref files, but you can copy/paste the result to try to get formatting like the tool would create.

deeprobin · 2022-01-09T01:49:18Z

@deeprobin how's this one going? do you need help?

You have removed some entries from the ref, which creates a breaking change for code building against the non-netcoreapp target framework, eg., for .NET Standard. Existing API should stay in there. New API can go in the ref/...netcoreapp.cs file.

If possible, please use https://github.com/dotnet/runtime/blob/4679edc6be3eb91263a7acfe542dfd30b53ed2bf/docs/coding-guidelines/updating-ref-source.md#for-most-assemblies-within-libraries -- it doesn't understand multiple ref files, but you can copy/paste the result to try to get formatting like the tool would create.

@danmoseley

Oh yes, I think I fixed that.

CI is now failing: exit code 139 means SIGSEGV Illegal memory access. Deref invalid pointer, overrunning buffer, stack overflow etc. Core dumped. (because the test-pointer does not exist)

How would you construct a test for this?

Creating 2 Handle-objects and swapping the Handle property? Or do you have a better idea to test this?

danmoseley · 2022-01-09T20:42:38Z

Not sure of your question. If you're asking - how would we construct a test that expected a fatal error like a segfault? - we do not test for fatal errors, except possibly in some exceptional cases (and then by using RemoteExecutor.Invoke).

deeprobin · 2022-01-10T19:02:54Z

@danmoseley I have looked at the tests again. The CI throws gdiplus errors.
Do you think this is a case of #22221.

https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-63058-merge-ffd8398ff9224cd9a3/System.Drawing.Common.Tests/1/console.340b41fa.log?sv=2019-07-07&se=2022-01-29T01%3A02%3A29Z&sr=c&sp=rl&sig=2gsJHhJexH7K8z5Agh30IsXzPwEOXfezeP2vnLL0oNM%3D

      System.TypeInitializationException : The type initializer for 'Gdip' threw an exception.
      ---- System.DllNotFoundException : Unable to load shared library 'libgdiplus' or one of its dependencies. In order to help diagnose loading problems, consider setting the LD_DEBUG environment variable: Error loading shared library liblibgdiplus: No such file or directory
      Stack Trace:
    System.Drawing.Drawing2D.Tests.MatrixTests.FromHandle [FAIL]
        /_/src/libraries/System.Drawing.Common/src/Microsoft.Interop.DllImportGenerator/Microsoft.Interop.DllImportGenerator/GeneratedDllImports.g.cs(518,0): at System.Drawing.SafeNativeMethods.Gdip.GdipCreateMatrix(IntPtr& matrix)
        /_/src/libraries/System.Drawing.Common/src/System/Drawing/Drawing2D/Matrix.cs(18,0): at System.Drawing.Drawing2D.Matrix..ctor()
        /_/src/libraries/System.Drawing.Common/tests/Drawing2D/MatrixTests.Core.cs(47,0): at System.Drawing.Drawing2D.Tests.MatrixTests.FromHandle()
        ----- Inner Stack Trace -----
           at System.Drawing.SafeNativeMethods.Gdip.GdiplusStartup(IntPtr& token, StartupInput& input, StartupOutput& output)
        /_/src/libraries/System.Drawing.Common/src/System/Drawing/Gdiplus.cs(33,0): at System.Drawing.SafeNativeMethods.Gdip..cctor()

This seems to be similar #22221 (comment) to this run https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-63058-merge-14bfe203df1a49fb92/System.Drawing.Common.Tests/1/console.8315be5d.log?sv=2019-07-07&se=2022-01-29T01%3A01%3A45Z&sr=c&sp=rl&sig=4Q5rnTbBhb3SPBRrdJgUCw9sB3Ep9GaNa6i17znSJX8%3D

=================================================================
	Basic Fault Address Reporting
=================================================================
Memory around native instruction pointer (0x7f1e9103ea6d):0x7f1e9103ea5d  c0 0f 85 ac 02 00 00 48 85 ff 0f 84 23 01 00 00  .......H....#...
0x7f1e9103ea6d  48 8b 77 f8 4c 8d 6f f0 40 f6 c6 02 0f 85 39 01  [email protected].
0x7f1e9103ea7d  00 00 48 8b 2d f2 32 35 00 64 48 83 7d 00 00 0f  ..H.-.25.dH.}...
0x7f1e9103ea8d  84 ee 04 00 00 40 f6 c6 04 48 8d 1d a3 41 35 00  [email protected].

=================================================================
	Managed Stacktrace:
=================================================================
	  at <unknown> <0xffffffff>
	  at Gdip:GdipDeletePen <0x000c4>
	  at System.Drawing.Pen:Dispose <0x0014b>
	  at System.Drawing.Pen:Finalize <0x0002f>
	  at System.Object:runtime_invoke_virtual_void__this__ <0x0008c>
=================================================================

deeprobin · 2022-01-17T15:11:14Z

@qmfrederik @eerhardt Can you perhaps give some thoughts on this, since you were involved in #22221?

deeprobin · 2022-01-17T15:23:05Z

I have already implemented the change with the ActiveIssueAttribute. In case of doubt we can also revert this.

https://github.com/deeprobin/runtime/tree/f39d2a3aeef9da389e9de5da7c77ad798e6461f1

danmoseley · 2022-01-18T19:28:03Z

@deeprobin your tests are causing a double-dispose, eg:

        public void FromHandle()
        {
            var region = new Region();

            var expectedHandle = region.Handle;
            var actualRegion = Region.FromHandle(expectedHandle);

            IntPtr actualHandle = actualRegion.Handle;
            Assert.Equal(expectedHandle, actualHandle);

            // Do not dispose `region` because it is the same handle
            actualRegion.Dispose();
        }

Here you're disposing actualRegion which is calling GdipDeleteRegion with this handle. But the Region object is IDisposable - so when it becomes unreachable and is collected, the GC will put it on the finalizer queue. And the finalizer will call dispose, which will call GdipDeleteRegion on the same handle. You see in the log that GDI+ is returning 4, which means ObjectBusy. I am guessing that GdipDeleteRegion may be safe to call twice on the same handle, but not concurrently, which is likely what's happening here.

The fix is likely to change var region = new Region(); to using var region = new Region();. This will cause the Dispose to happen deterministically. The same issue applies to the other tests.

Because you've got 8 PR's active at this time, I'm going to close this one. Please reactivate when you believe all the tests should be passing again. Thanks.

danmoseley · 2022-01-18T19:29:43Z

src/libraries/System.Drawing.Common/ref/System.Drawing.Common.netcoreapp.cs

+        public System.IntPtr Handle { get { throw null; } }
+        public static System.Drawing.Graphics FromHandle(System.IntPtr handle) { throw null; }
+    }
+


Our goal is to either generate these files (as discussed earlier) or make it look like the generator created it. The generator won't insert blank lines like this as we would in normal code.

danmoseley · 2022-01-18T19:31:19Z

src/libraries/System.Drawing.Common/src/System/Drawing/Drawing2D/GraphicsPath.Unix.cs

+        /// <summary>
+        /// The underlying graphics path handle
+        /// </summary>
+        public IntPtr Handle => _nativePath;


We try to order fields then properties then methods, with public before private. I'm surprised the editorconfig isn't flagging this for you.

deeprobin added 8 commits December 21, 2021 19:54

Expose Graphics members Handle and FromHandle

232d840

Expose Brush members Handle and FromHandle

17967bf

Expose Pen members Handle and FromHandle

5bf179c

Expose Image members Handle and FromHandle

38dbfeb

Expose Region members Handle and FromHandle

efa64a6

Expose Matrix members Handle and FromHandle

802ac21

Expose GraphicsPath members Handle and FromHandle

60280e2

Add missing documentation comments

f7ca821

dotnet-issue-labeler bot added area-System.Drawing new-api-needs-documentation labels Dec 21, 2021

ghost added the community-contribution Indicates that the PR has been added by a community member label Dec 21, 2021

deeprobin added 6 commits December 22, 2021 21:10

Usage of Handle property

6b4a01c

Seal RawBrush

51170cd

Add tests

d34edb4

Fix project file

c9511c8

Skip tests in .NET Framework

4e9d081

Improve test cases

1c21ed9

deeprobin added 6 commits December 25, 2021 16:51

Move Matrix-FromHandle test into MatrixTests.Core.cs

35786dc

Move Brush-FromHandle test into BrushTests.Core.cs

b6540c2

Move GraphicsPath-FromHandle test into GraphicsPathTests.Core.cs

efa989e

Remove redundant attribute

f5aac62

Move Region-FromHandle test into RegionTests.Core.cs

8c90979

Move Pen-FromHandle test into `PenTests.Core.cs

bd35a8c

deeprobin added 4 commits December 25, 2021 21:36

Move Image-FromHandle test into `ImageTests.Core.cs

6942c08

Fix PenTests namespace and add partial modifier

b18dbbe

Move Graphics-FromHandle test into GraphicsTests.Core.cs

d075fcd

Move refassembly contents to netcore-only file

b12e2b5

Fix ref

d23a699

Ignore libgdiplus tests on Unix (see dotnet#22221)

f39d2a3

deeprobin added 4 commits January 17, 2022 16:30

Fix TestGetProperties test

1b5b997

Merge branch 'main' into issue-39406

4b437fb

Fix TestGetProperties test

b5d1b89

Try to fix disposal tests on mono runtimes

f230d47

danmoseley closed this Jan 18, 2022

danmoseley reviewed Jan 18, 2022

View reviewed changes

ghost locked as resolved and limited conversation to collaborators Feb 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[API Implementation]: Expose `System.Drawing.Graphics.NativeGraphics`. #63058

[API Implementation]: Expose `System.Drawing.Graphics.NativeGraphics`. #63058

deeprobin commented Dec 21, 2021 •

edited

Loading

dotnet-issue-labeler bot commented Dec 21, 2021

ghost commented Dec 21, 2021

Proposal

Current state of implementation

danmoseley commented Dec 21, 2021

deeprobin commented Dec 24, 2021

teo-tsirpanis commented Dec 24, 2021

danmoseley commented Jan 8, 2022

deeprobin commented Jan 9, 2022 •

edited

Loading

danmoseley commented Jan 9, 2022

deeprobin commented Jan 10, 2022 •

edited

Loading

deeprobin commented Jan 17, 2022

deeprobin commented Jan 17, 2022

danmoseley commented Jan 18, 2022

danmoseley Jan 18, 2022

danmoseley Jan 18, 2022

[API Implementation]: Expose System.Drawing.Graphics.NativeGraphics. #63058

[API Implementation]: Expose System.Drawing.Graphics.NativeGraphics. #63058

Conversation

deeprobin commented Dec 21, 2021 • edited Loading

Proposal

Current state of implementation

dotnet-issue-labeler bot commented Dec 21, 2021

ghost commented Dec 21, 2021

Proposal

Current state of implementation

danmoseley commented Dec 21, 2021

deeprobin commented Dec 24, 2021

teo-tsirpanis commented Dec 24, 2021

danmoseley commented Jan 8, 2022

deeprobin commented Jan 9, 2022 • edited Loading

danmoseley commented Jan 9, 2022

deeprobin commented Jan 10, 2022 • edited Loading

deeprobin commented Jan 17, 2022

deeprobin commented Jan 17, 2022

danmoseley commented Jan 18, 2022

danmoseley Jan 18, 2022

Choose a reason for hiding this comment

danmoseley Jan 18, 2022

Choose a reason for hiding this comment

[API Implementation]: Expose `System.Drawing.Graphics.NativeGraphics`. #63058

[API Implementation]: Expose `System.Drawing.Graphics.NativeGraphics`. #63058

deeprobin commented Dec 21, 2021 •

edited

Loading

deeprobin commented Jan 9, 2022 •

edited

Loading

deeprobin commented Jan 10, 2022 •

edited

Loading