-
Notifications
You must be signed in to change notification settings - Fork 775
Fix unhandled ArgumentOutOfRangeException when exporting FlameGraph with unrendered canvas #2339
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
Changes from 3 commits
f89aa05
be6c536
1320239
0699b4a
f684ba5
9f9e872
7af7d62
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -859,5 +859,39 @@ public override string GetFrameName(StackSourceFrameIndex frameIndex, bool verbo | |||||
| return frameIndex.ToString(); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| [WpfFact] | ||||||
| [WorkItem(2308, "https://github.com/Microsoft/perfview/issues/2308")] | ||||||
| public void TestExportFlameGraphWithInvalidCanvasSize() | ||||||
| { | ||||||
| // Create a canvas with zero size (simulating an unrendered or collapsed canvas) | ||||||
| var canvas = new Canvas(); | ||||||
| canvas.Width = 0; | ||||||
| canvas.Height = 0; | ||||||
| canvas.Measure(new Size(0, 0)); | ||||||
| canvas.Arrange(new Rect(0, 0, 0, 0)); | ||||||
|
|
||||||
| var tempFile = Path.GetTempFileName(); | ||||||
| try | ||||||
| { | ||||||
| // Attempt to export should throw InvalidOperationException with a meaningful message | ||||||
| var exception = Assert.Throws<InvalidOperationException>(() => | ||||||
| { | ||||||
| FlameGraph.Export(canvas, tempFile); | ||||||
| }); | ||||||
|
|
||||||
| // Verify the exception message is helpful | ||||||
| Assert.Contains("canvas has an invalid size", exception.Message); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 9f9e872 |
||||||
| Assert.Contains("width=0", exception.Message); | ||||||
| Assert.Contains("height=0", exception.Message); | ||||||
| } | ||||||
| finally | ||||||
| { | ||||||
| if (File.Exists(tempFile)) | ||||||
| { | ||||||
| File.Delete(tempFile); | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -83,7 +83,18 @@ public static IEnumerable<FlameBox> Calculate(CallTree callTree, double maxWidth | |||||
| public static void Export(Canvas flameGraphCanvas, string filePath) | ||||||
| { | ||||||
| var rectangle = new Rect(flameGraphCanvas.RenderSize); | ||||||
| var renderTargetBitmap = new RenderTargetBitmap((int)rectangle.Right, (int)rectangle.Bottom, 96d, 96d, PixelFormats.Default); | ||||||
| int width = (int)rectangle.Right; | ||||||
| int height = (int)rectangle.Bottom; | ||||||
|
|
||||||
| // Validate that the canvas has a valid size before attempting to export | ||||||
| if (width <= 0 || height <= 0) | ||||||
| { | ||||||
| throw new InvalidOperationException( | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 0699b4a |
||||||
| $"Cannot export flame graph: the canvas has an invalid size (width={width}, height={height}). " + | ||||||
| "Please ensure the flame graph is visible and has been rendered before attempting to export."); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The text here is too long to appear in the status bar. Please use this text: "Please ensure the flame graph is visible and has been rendered before attempting to export."
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 0699b4a - shortened the error message to just the essential guidance |
||||||
| } | ||||||
|
|
||||||
| var renderTargetBitmap = new RenderTargetBitmap(width, height, 96d, 96d, PixelFormats.Default); | ||||||
| renderTargetBitmap.Render(flameGraphCanvas); | ||||||
|
|
||||||
| var pngEncoder = new PngBitmapEncoder(); | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -2764,10 +2764,18 @@ private void DoSaveFlameGraph(object sender, RoutedEventArgs e) | |||||
| var result = saveDialog.ShowDialog(); | ||||||
| if (result == true) | ||||||
| { | ||||||
| if (FlameGraphCanvas.IsEmpty || m_RedrawFlameGraphWhenItBecomesVisible) | ||||||
| RedrawFlameGraph(); | ||||||
| try | ||||||
| { | ||||||
| if (FlameGraphCanvas.IsEmpty || m_RedrawFlameGraphWhenItBecomesVisible) | ||||||
| RedrawFlameGraph(); | ||||||
|
|
||||||
| FlameGraph.Export(FlameGraphCanvas, saveDialog.FileName); | ||||||
| FlameGraph.Export(FlameGraphCanvas, saveDialog.FileName); | ||||||
| StatusBar.Log($"Saved flame graph to {saveDialog.FileName}"); | ||||||
| } | ||||||
| catch (InvalidOperationException ex) | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 0699b4a |
||||||
| { | ||||||
| StatusBar.LogError($"Failed to save flame graph: {ex.Message}"); | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 0699b4a