Fix two panes being closed when just one is #17358
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lhecker
added
Issue-Bug
lhecker
commented
Jun 4, 2024
| { | ||
| Close(); |
There was a problem hiding this comment.
For handoff sessions which transition to closed, we would previously call Close() twice. I hope that the new code is just as readable though.
| const auto msg1 = fmt::format(std::wstring_view{ RS_(L"ProcessExited") }, fmt::format(_errorFormat, status)); | ||
| const auto msg2 = RS_(L"CtrlDToClose"); | ||
| const auto msg = fmt::format(FMT_COMPILE(L"\r\n{}\r\n{}\r\n"), msg1, msg2); | ||
| TerminalOutput.raise(msg); |
There was a problem hiding this comment.
This is a lil extra which I did on the side, hoping to piggyback it in. The callback results in locking the terminal to process the input and previously we would do it 5 times. I think formatting the message once and then sending it off is better.
zadjii-msft
approved these changes
Jun 4, 2024
zadjii-msft
added a commit
that referenced
this pull request
Jun 4, 2024
DHowett
approved these changes
Jun 4, 2024
| const auto msg1 = fmt::format(std::wstring_view{ RS_(L"ProcessExited") }, fmt::format(_errorFormat, status)); | ||
| const auto msg2 = RS_(L"CtrlDToClose"); | ||
| const auto msg = fmt::format(FMT_COMPILE(L"\r\n{}\r\n{}\r\n"), msg1, msg2); | ||
| TerminalOutput.raise(msg); |
DHowett
pushed a commit
that referenced
this pull request
Jun 7, 2024
#17333 introduced a regression: While it fixes a recursion *into* `Pane::Close()` that doesn't fix the recursion outside of it. In this case, `Close()` raises the `Closed` event which results in another tab being closed because it's bound to `_RemoveTab`. The recursion is now obvious, because I made the entire process synchronous. Previously, it would (hopefully) just be scheduled after the pane and its content are already gone. The issue can be fixed by moving the recursion check from `Pane::Close()` to `TerminalTab::Shutdown()` but I felt like it would better to fix the issue a bit more thoroughly. `IPaneContent` can raise a `CloseRequested` event to indicate it wants to be closed. However, that also contained recursion, because the content would call its own `Close()` to raise the event, which the tab catches, calls `Close()` on the `Pane` which calls `Close()` on the content which raises the event again and so on. That's what was fixed in #17333 among others. We can do this better by not raising the event from `IPaneContent::Close()`. Instead, that method will now be exclusively called by `Pane`. The `CloseRequested` event will now truly be just a request and nothing more. Furthermore, the ownership of the event handling was moved from the `TerminalTab` to the `Pane`. To make all of this a bit simpler and more robust, two new methods were added to `Pane`: `_takePaneContent` and `_setPaneContent`. These methods ensure that `Close()` is called on the content, that the event handlers are always added and revoked and that the ownership transfers cleanly between panes. ## Validation Steps Performed * Open 3 tabs, close the middle one ✅ * Open 3 vertical panes, close the middle one ✅ * Drag tabs with multiple panes between windows ✅ (cherry picked from commit ce0f8d6) Service-Card-Id: 92678790 Service-Version: 1.21
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jun 20, 2024
#17358 introduced a bug where if you open/close panes very rapidly Terminal will crash. This was because `_content` was being set to `null` and then a `Close` event was being emitted, but several functions attempt to access the pane's `_content` as part of the close routine. For example, `TerminalTab` tries to update the `TaskbarProgress` every time a pane is closed and as part of that update sequence it queries the pane - which has `null` content now - for the taskbar progress, resulting in a crash. This PR fixes that crash. Refs #17358
DHowett
pushed a commit
that referenced
this pull request
Jun 20, 2024
#17358 introduced a bug where if you open/close panes very rapidly Terminal will crash. This was because `_content` was being set to `null` and then a `Close` event was being emitted, but several functions attempt to access the pane's `_content` as part of the close routine. For example, `TerminalTab` tries to update the `TaskbarProgress` every time a pane is closed and as part of that update sequence it queries the pane - which has `null` content now - for the taskbar progress, resulting in a crash. This PR fixes that crash. Refs #17358 (cherry picked from commit 5d46e31) Service-Card-Id: 92776947 Service-Version: 1.21
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#17333 introduced a regression: While it fixes a recursion into
Pane::Close()that doesn't fix the recursion outside of it.In this case,
Close()raises theClosedevent which resultsin another tab being closed because it's bound to
_RemoveTab.The recursion is now obvious, because I made the entire process
synchronous. Previously, it would (hopefully) just be scheduled
after the pane and its content are already gone.
The issue can be fixed by moving the recursion check from
Pane::Close()toTerminalTab::Shutdown()but I felt likeit would better to fix the issue a bit more thoroughly.
IPaneContentcan raise aCloseRequestedevent to indicate it wantsto be closed. However, that also contained recursion, because the
content would call its own
Close()to raise the event, which thetab catches, calls
Close()on thePanewhich callsClose()onthe content which raises the event again and so on. That's what was
fixed in #17333 among others. We can do this better by not raising
the event from
IPaneContent::Close(). Instead, that method will nowbe exclusively called by
Pane. TheCloseRequestedevent will nowtruly be just a request and nothing more. Furthermore, the ownership
of the event handling was moved from the
TerminalTabto thePane.To make all of this a bit simpler and more robust, two new methods
were added to
Pane:_takePaneContentand_setPaneContent.These methods ensure that
Close()is called on the content,that the event handlers are always added and revoked
and that the ownership transfers cleanly between panes.
Validation Steps Performed