-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
Theoretical fix for some crashes #16047
Merged
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
Found this while looking through dumps for failure `f544cf8e-1879-c59b-3f0b-1a364b92b974`. That's MSFT:45210947. From the dump I looked at, Looks like, * we're on Windows 10 * We're refrigerating a window * We are pumping the remaining XAML messages as we refrigerate (`_pumpRemainingXamlMessages`) * In there, we're finally getting the `TerminalPage::_CompleteInitialization` * that calls up to the `_root->Initialized` lambda set up in `TerminalWindow::Initialize` * There it tries to get the launch mode from the settings, and explodes. Presumably _settings is null, but can't see in this dump. so the window is closing before it's initialized. When we `_warmWindow = std::move(_host->Refrigerate())`, we call `AppHost::Refrigerate`, which will null out the TerminalWindow. So when we're getting to `TerminalWindow::Initialize`, we're calling that on a nullptr. That's the trick. We need to revoke the internal Initialized callback. Which makes sense. It's a lambda that binds _this_ 🤦 --- After more looking, it really doesn't _seem_ like the stacks that are tracked in `f544cf8e-1879-c59b-3f0b-1a364b92b974` look like the same stack that I was debugging, but this _is_ a realy issue regardless.
note: I'm not sure I could repro this locally. This is more of a "well, look at the code, that's definitely a bug" fix. |
lhecker
approved these changes
Sep 27, 2023
carlos-zamora
approved these changes
Sep 27, 2023
DHowett
approved these changes
Sep 27, 2023
DHowett
approved these changes
Sep 28, 2023
DHowett
pushed a commit
that referenced
this pull request
Sep 29, 2023
Found this while looking through dumps for failure `f544cf8e-1879-c59b-3f0b-1a364b92b974`. That's MSFT:45210947. (1% of our 1.19 crashes) From the dump I looked at, Looks like, * we're on Windows 10 * We're refrigerating a window * We are pumping the remaining XAML messages as we refrigerate (`_pumpRemainingXamlMessages`) * In there, we're finally getting the `TerminalPage::_CompleteInitialization` * that calls up to the `_root->Initialized` lambda set up in `TerminalWindow::Initialize` * There it tries to get the launch mode from the settings, and explodes. Presumably _settings is null, but can't see in this dump. so the window is closing before it's initialized. When we `_warmWindow = std::move(_host->Refrigerate())`, we call `AppHost::Refrigerate`, which will null out the TerminalWindow. So when we're getting to `TerminalWindow::Initialize`, we're calling that on a nullptr. That's the trick. We need to revoke the internal Initialized callback. Which makes sense. It's a lambda that binds _this_ 🤦 --- After more looking, it really doesn't _seem_ like the stacks that are tracked in `f544cf8e-1879-c59b-3f0b-1a364b92b974` look like the same stack that I was debugging, but this _is_ a realy issue regardless. (cherry picked from commit 7073ec0) Service-Card-Id: 90672654 Service-Version: 1.19
DHowett
pushed a commit
that referenced
this pull request
Nov 7, 2023
Found this while looking through dumps for failure `f544cf8e-1879-c59b-3f0b-1a364b92b974`. That's MSFT:45210947. (1% of our 1.19 crashes) From the dump I looked at, Looks like, * we're on Windows 10 * We're refrigerating a window * We are pumping the remaining XAML messages as we refrigerate (`_pumpRemainingXamlMessages`) * In there, we're finally getting the `TerminalPage::_CompleteInitialization` * that calls up to the `_root->Initialized` lambda set up in `TerminalWindow::Initialize` * There it tries to get the launch mode from the settings, and explodes. Presumably _settings is null, but can't see in this dump. so the window is closing before it's initialized. When we `_warmWindow = std::move(_host->Refrigerate())`, we call `AppHost::Refrigerate`, which will null out the TerminalWindow. So when we're getting to `TerminalWindow::Initialize`, we're calling that on a nullptr. That's the trick. We need to revoke the internal Initialized callback. Which makes sense. It's a lambda that binds _this_ 🤦 Service-Card-Id: 90878965 Service-Version: 1.18 --- After more looking, it really doesn't _seem_ like the stacks that are tracked in `f544cf8e-1879-c59b-3f0b-1a364b92b974` look like the same stack that I was debugging, but this _is_ a realy issue regardless. (cherry picked from commit 7073ec0)
DHowett
pushed a commit
that referenced
this pull request
Nov 7, 2023
Found this while looking through dumps for failure `f544cf8e-1879-c59b-3f0b-1a364b92b974`. That's MSFT:45210947. (1% of our 1.19 crashes) From the dump I looked at, Looks like, * we're on Windows 10 * We're refrigerating a window * We are pumping the remaining XAML messages as we refrigerate (`_pumpRemainingXamlMessages`) * In there, we're finally getting the `TerminalPage::_CompleteInitialization` * that calls up to the `_root->Initialized` lambda set up in `TerminalWindow::Initialize` * There it tries to get the launch mode from the settings, and explodes. Presumably _settings is null, but can't see in this dump. so the window is closing before it's initialized. When we `_warmWindow = std::move(_host->Refrigerate())`, we call `AppHost::Refrigerate`, which will null out the TerminalWindow. So when we're getting to `TerminalWindow::Initialize`, we're calling that on a nullptr. That's the trick. We need to revoke the internal Initialized callback. Which makes sense. It's a lambda that binds _this_ 🤦 Service-Card-Id: 90878965 Service-Version: 1.18 --- After more looking, it really doesn't _seem_ like the stacks that are tracked in `f544cf8e-1879-c59b-3f0b-1a364b92b974` look like the same stack that I was debugging, but this _is_ a realy issue regardless. (cherry picked from commit 7073ec0)
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.
Found this while looking through dumps for failure
f544cf8e-1879-c59b-3f0b-1a364b92b974
. That's MSFT:45210947. (1% of our 1.19 crashes)From the dump I looked at,
Looks like,
_pumpRemainingXamlMessages
)TerminalPage::_CompleteInitialization
_root->Initialized
lambda set up inTerminalWindow::Initialize
so the window is closing before it's initialized.
When we
_warmWindow = std::move(_host->Refrigerate())
, we callAppHost::Refrigerate
, which will null out the TerminalWindow. So when we're getting toTerminalWindow::Initialize
, we're calling that on a nullptr. That's the trick.We need to revoke the internal Initialized callback. Which makes sense. It's a lambda that binds this 🤦
After more looking, it really doesn't seem like the stacks that are tracked in
f544cf8e-1879-c59b-3f0b-1a364b92b974
look like the same stack that I was debugging, but this is a realy issue regardless.