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

[macOS] Fix crash when setting window properties #21357

Conversation

MartyIX
Copy link
Contributor

@MartyIX MartyIX commented Mar 21, 2024

Description of Change

I'm not really sure if the approach of the patch is correct. It's the only one I could make as I don't understand details here.

Feedback is welcome.

See #21306 (comment) for an explanation of the bug.

Issues Fixed

Fixes #21306

@MartyIX MartyIX requested a review from a team as a code owner March 21, 2024 14:22
@MartyIX MartyIX requested review from rmarinho and tj-devel709 March 21, 2024 14:22
@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Mar 21, 2024
Copy link
Contributor

Hey there @MartyIX! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@MartyIX
Copy link
Contributor Author

MartyIX commented Mar 21, 2024

cc @mattleibow because of his work on #16637.

@rmarinho
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@MartyIX MartyIX force-pushed the feature/2024-03-21-macOS-window-setting-width-heigh-crash branch from 3ad3310 to 1ebaaa0 Compare March 28, 2024 09:22
@rmarinho
Copy link
Member

rmarinho commented Apr 1, 2024

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@rmarinho rmarinho requested a review from mattleibow April 1, 2024 10:37
@MartyIX
Copy link
Contributor Author

MartyIX commented Apr 13, 2024

@mattleibow Could you please take a look?

@MartyIX
Copy link
Contributor Author

MartyIX commented Apr 23, 2024

@rmarinho @mattleibow Feedback would be greatly appreaciated on this one.

@@ -198,6 +198,10 @@ public double MinimumHeight
return ValidatePositive(coord);
}

#if MACCATALYST
bool _frameUpdateInProgress = false;
Copy link
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.

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

Comment on lines +216 to +223
if (_frameUpdateInProgress) {
X = frame.X;
Y = frame.Y;
Width = frame.Width;
Height = frame.Height;

return;
}
Copy link
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?

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

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.

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

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

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

@PureWeen
Copy link
Member

Closing this PR in favor of
#22413

Which pretty has the same end result.

Once we merge 22413, if that doesn't appear to fix @MartyIX let me know!

@PureWeen PureWeen closed this May 21, 2024
@MartyIX MartyIX deleted the feature/2024-03-21-macOS-window-setting-width-heigh-crash branch May 21, 2024 20:26
@github-actions github-actions bot locked and limited conversation to collaborators Jun 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-window Window community ✨ Community Contribution platform/macOS 🍏 macOS / Mac Catalyst
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[macOS] Crash in multi-window mode in the Controls.sample project
5 participants