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

[20_13] Do not add duplicated values to copy_always to avoid stack overflow #2205

Merged
merged 4 commits into from
Dec 25, 2024

Conversation

wind1900
Copy link
Contributor

@wind1900 wind1900 commented Dec 5, 2024

What

Add check before adding new values to edit_interface_rep::copy_always in function edit_interface_rep::apply_changes().
New rectangle will be added to copy_always if copy_always is empty/nil or the first item of copy_always is not equal to newly added value.

Why

Since apply_changes() will be called when the window receives any event, copy_always will become very large and cause stack overflow when it's cleared.
The copy_always is a linked list and when it's cleared, the destructor is called from head to tail recursively, causing stack overflow.

How to test your changes?

That's the potential risk: I can only test it on Windows with few scenarios, I don't know the result in Linux, Mac.

already run xmake run --yes -vD --group=tests and xmake run --yes -vD --group=integration_tests

The investigations below are all in Windows environment.

Where is the code to clear copy_always?

In edit_interface_rep::scroll_to() and edit_interface_rep::set_extents(), by using copy_always = rectangles ().

When will copy_always be cleared?

scroll_to() is called when the cursor is moved out of current view.
I can't add breakpoint in set_extents(), I don't know when will it will be called.

What's the purpose of copy_always?

The comment says it's used for wiping out cursor.
However I think it doesn't do anything now.
I comment out the code that uses it, get nothing wrong.
But removing it is more dangerous.

Where is copy_always used?

In edit_interface_rep::draw_pre() and edit_interface_rep::draw_with_shadow().

In draw_pre(), it will go through all rectangles in copy_always and call win->put_shadow(), but it doesn't draw anything at all.
Because in qt_renderer_rep::put_shadow(), painter == static_cast<qt_renderer_rep*> (ren)->painter is true and returns directly.

In draw_with_shadow(), it's only used if gui_interrupted() returns true, but gui_interrupted() doesn't return true in my machine.

These shadow functions are related to the double buffering, the behavior may be different in other OS.

In summary, I don't think it's necessary to store all the old rectangles in copy_always.
The latest ones ocr(old cursor) and ncr(new cursor) should be enough for cursor drawing stuff.

@wind1900 wind1900 changed the title Clear copy always to avoid stack overflow Clear copy_always to avoid stack overflow Dec 5, 2024
@da-liii da-liii changed the title Clear copy_always to avoid stack overflow [20_13] Clear copy_always to avoid stack overflow Dec 16, 2024
@da-liii
Copy link
Contributor

da-liii commented Dec 16, 2024

CI已经修复,合并branch-1.2,再push一下,这个pr就可以正常通过CI了

@wind1900 wind1900 changed the title [20_13] Clear copy_always to avoid stack overflow [20_13] Do not add duplicated values to copy_always to avoid stack overflow Dec 21, 2024
Copy link
Contributor

@da-liii da-liii left a comment

Choose a reason for hiding this comment

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

LGTM

@da-liii da-liii merged commit 9ab2645 into XmacsLabs:branch-1.2 Dec 25, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants