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

Prevent crash when processing line caches in RichTextLabel #78975

Merged
merged 1 commit into from Jul 3, 2023

Conversation

YuriSizov
Copy link
Contributor

@YuriSizov YuriSizov commented Jul 3, 2023

Fixes #78912.
Fixes #78849.
Fixes #78932.
Supersedes #78916 as a solution (though sizing changes may be welcome at some point).

I investigated #78241 which introduced this regression and noticed a small logical difference in what appeared to be a simple act of refactoring. We used the old value of scroll_w, valid only at the time of the function call, while in the process the value of scroll_w was updated. This broke pre-existing logic from before the refactoring resulting in the crash.

I admit, I don't really understand the entirety of what's going on, but this doesn't seem intentional and does seem to be the source of the issue.


PS. There is also another difference in that refactoring which I haven't touched because it has nothing to do with the crash. But when we do _process_line_caches we used to call main->first_invalid_line.store and main->first_invalid_font_line.store while iterating. In the factored out code of _update_scroll_exceeds this no longer happens, and only main->first_resized_line.store is called. Might be something to look into.

@YuriSizov YuriSizov added this to the 4.1 milestone Jul 3, 2023
@YuriSizov YuriSizov requested a review from bruvzg July 3, 2023 11:40
@YuriSizov YuriSizov requested a review from a team as a code owner July 3, 2023 11:40
Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

There is also another difference in that refactoring which I haven't touched because it has nothing to do with the crash. But when we do _process_line_caches we used to call main->first_invalid_line.store and main->first_invalid_font_line.store while iterating. In the factored out code of _update_scroll_exceeds this no longer happens, and only main->first_resized_line.store is called. Might be something to look into.

Setting this should not be necessary since thread will never exit while inside _update_scroll_exceeds.

@bruvzg
Copy link
Member

bruvzg commented Jul 3, 2023

Supersedes #78916 as a solution (though sizing changes may be welcome at some point).

Note: Sizing changes are still relevant, but should it be tested for the possible Label sizing regressions first (there were a lost of issues).

@YuriSizov
Copy link
Contributor Author

Note: Sizing changes are still relevant, but should it be tested for the possible Label sizing regressions first (there were a lost of issues).

Yes, though I wouldn't risk it right now, for 4.1. We need to create a gauntlet of unit tests and make sure these changes are safe first. Then maybe pick them for a patch release.

@akien-mga akien-mga merged commit 1b38e92 into godotengine:master Jul 3, 2023
13 checks passed
@YuriSizov YuriSizov deleted the rtl-fix-refactoring-typo branch July 3, 2023 13:28
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants