Skip to content

[macOS] Fix crash when setting window properties #21357

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.

Sign up for GitHub

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

Closed
MartyIX wants to merge 3 commits into from
Closed

[macOS] Fix crash when setting window properties #21357

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
95555ef
fix?
MartyIX Mar 21, 2024
1362fb6
Another approach
MartyIX Mar 21, 2024
1ebaaa0
Fix warnings in CI
MartyIX Mar 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions src/Controls/src/Core/Window/Window.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,10 @@ double GetSizeCoordinate(BindableProperty property)
return ValidatePositive(coord);
}

#if MACCATALYST
bool _frameUpdateInProgress = false;

@mattleibow mattleibow Apr 23, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to use the _batchFrameUpdate field? That is already being set when the frame is updating.

@MartyIX MartyIX Apr 23, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try if it works.

#endif

int _batchFrameUpdate = 0;

void IWindow.FrameChanged(Rect frame)
Expand All @@ -206,9 +210,26 @@ void IWindow.FrameChanged(Rect frame)
var y = Y;
var width = Width;
var height = Height;

#if MACCATALYST
// MacCatalyst does not support setting window properties: X, Y, width, and height.
if (_frameUpdateInProgress) {
X = frame.X;
Y = frame.Y;
Width = frame.Width;
Height = frame.Height;

return;
}
Comment on lines +216 to +223

@mattleibow mattleibow Apr 23, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the difference between setting this and using SetValueCore?

@MartyIX MartyIX Apr 23, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't recall specifically but I think it was that SetValueCore lead to a crash precisely because it leads to an infinite loop:

maui/src/Core/src/Platform/iOS/WindowExtensions.cs

Line 22 in 5fa7b3d

platformWindow.UpdateUnsupportedCoordinate(window);

Does it make sense to you? I can try to dig into it again if that doesn't sound right but I don't have a solid grasp of SetValueCore so a little explanation of what it is supposed to would be very welcome.

@mattleibow mattleibow Apr 23, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SetValueCore updates the property value, but you are also setting Height which should also update the property. If there is a loop, maybe there is a bug in the "unused" logic - or maybe something should not be updating...

@MartyIX MartyIX Apr 23, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, my VS Code on macOS got totally broken, I can't debug MAUI projects anymore.

Anyway, I think it's good to put a breakpoint on L217 to see the stacktrace there and if this PR makes sense or not.

I'll try to work on this PR later. I spent 2 hours and I couldn't get that stacktrace. I'm out of time for now.

@MartyIX MartyIX Apr 24, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I got bitten by #22033.

#endif

if (new Rect(x, y, width, height) == frame)
return;

#if MACCATALYST
_frameUpdateInProgress = true;
#endif

_batchFrameUpdate++;

SetPropertyChanging(XProperty, nameof(X), x, frame.X);
Expand All @@ -235,6 +256,10 @@ void IWindow.FrameChanged(Rect frame)
SizeChanged?.Invoke(this, EventArgs.Empty);
}

#if MACCATALYST
_frameUpdateInProgress = false;
#endif

[MethodImpl(MethodImplOptions.AggressiveInlining)]
void SetPropertyChanging(BindableProperty property, string name, double oldValue, double newValue)
{
Expand Down