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.

Sign up for GitHub

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

Jump to bottom

Use weak_ptrs for AppHost for coroutines #16065

Merged
merged 1 commit into from
Oct 3, 2023
Merged

Use weak_ptrs for AppHost for coroutines #16065

merged 1 commit into from
Oct 3, 2023

Conversation

zadjii-msft
Copy link
Member

See MSFT:46763065. Looks like we're in the middle of being Refrigerated, we're pumping messages, and as we pump messages, we get to a co_await in AppHost::_WindowInitializedHandler. When we resume, we just try to use this like everything's fine but OH NO, IT'S NOT.

To fix this, I'm

  • Adding enable_shared_from_this to AppHost
  • Holding the AppHost in a shared_ptr in WindowThread
    • though, this is a singular owning shared_ptr. This is probably ripe for other footguns, but there's little we can do about this.
  • whenever we co_await in AppHost, make sure we grab a weak ref first, and check it on the other side.

This is another "squint and yep that's a bug" fix, that I haven't been able to verify locally. This is
allegedly about 10% of our 1.19 crashes after 3 days.

Closes #16061

@zadjii-msft
Use weak_ptrs for AppHost for coroutines
8b6c7c2 
See MSFT:46763065. Looks like we're in the middle of being `Refrigerate`d, we're
pumping messages, and as we pump messages, we get to a `co_await` in
`AppHost::_WindowInitializedHandler`. When we resume, we just try to use `this`
like everything's fine but OH NO, IT'S NOT.

To fix this, I'm
* Adding `enable_shared_from_this` to `AppHost`
* Holding the `AppHost` in a shared_ptr in WindowThread
  - though, this is a singular owning `shared_ptr`. This is probably ripe for
    other footguns, but there's little we can do about this.
* whenever we `co_await` in `AppHost`, make sure we grab a weak ref first, and
  check it on the other side.

This is another "squint and yep that's a bug" fix, that I haven't been able to
verify locally. This is
[allegedly](https://media.tenor.com/VQi3bktwLdIAAAAC/allegedly-supposedly.gif)
about 10% of our 1.19 crashes after 3 days.

Closes #16061
@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-Windowing Window frame, quake mode, tearout Product-Terminal The new Windows Terminal. labels Sep 29, 2023
lhecker
lhecker approved these changes Sep 29, 2023
View reviewed changes
src/cascadia/WindowsTerminal/AppHost.cpp
@@ -931,8 +931,17 @@ winrt::fire_and_forget AppHost::_peasantNotifyActivateWindow()
const auto peasant = _peasant;
const auto hwnd = _window->GetHandle();

auto weakThis{ weak_from_this() };

co_await winrt::resume_background();
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to run on the BG thread in the first place?

Copy link
Member Author

Choose a reason for hiding this comment

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

I had it on a background thread so that a hung Monarch wouldn't hang a peasant process when the peasant gets activated.

I suppose these days, it's all the same process, so we could pull this one.

@DHowett DHowett merged commit 8521aae into main Oct 3, 2023
17 checks passed
@DHowett DHowett deleted the dev/migrie/b/16061-another-psychic-fix branch October 3, 2023 20:31
DHowett pushed a commit that referenced this pull request Oct 3, 2023
@zadjii-msft @DHowett
Use weak_ptrs for AppHost for coroutines (#16065)
335abe3 
See MSFT:46763065. Looks like we're in the middle of being
`Refrigerate`d, we're pumping messages, and as we pump messages, we get
to a `co_await` in `AppHost::_WindowInitializedHandler`. When we resume,
we just try to use `this` like everything's fine but OH NO, IT'S NOT.

To fix this, I'm
* Adding `enable_shared_from_this` to `AppHost`
* Holding the `AppHost` in a shared_ptr in WindowThread
- though, this is a singular owning `shared_ptr`. This is probably ripe
for other footguns, but there's little we can do about this.
* whenever we `co_await` in `AppHost`, make sure we grab a weak ref
first, and check it on the other side.

This is another "squint and yep that's a bug" fix, that I haven't been
able to verify locally. This is

[allegedly](https://media.tenor.com/VQi3bktwLdIAAAAC/allegedly-supposedly.gif)
about 10% of our 1.19 crashes after 3 days.

Closes #16061

(cherry picked from commit 8521aae)
Service-Card-Id: 90731961
Service-Version: 1.18
DHowett pushed a commit that referenced this pull request Oct 3, 2023
@zadjii-msft @DHowett
Use weak_ptrs for AppHost for coroutines (#16065)
70ad670 
See MSFT:46763065. Looks like we're in the middle of being
`Refrigerate`d, we're pumping messages, and as we pump messages, we get
to a `co_await` in `AppHost::_WindowInitializedHandler`. When we resume,
we just try to use `this` like everything's fine but OH NO, IT'S NOT.

To fix this, I'm
* Adding `enable_shared_from_this` to `AppHost`
* Holding the `AppHost` in a shared_ptr in WindowThread
- though, this is a singular owning `shared_ptr`. This is probably ripe
for other footguns, but there's little we can do about this.
* whenever we `co_await` in `AppHost`, make sure we grab a weak ref
first, and check it on the other side.

This is another "squint and yep that's a bug" fix, that I haven't been
able to verify locally. This is

[allegedly](https://media.tenor.com/VQi3bktwLdIAAAAC/allegedly-supposedly.gif)
about 10% of our 1.19 crashes after 3 days.

Closes #16061

(cherry picked from commit 8521aae)
Service-Card-Id: 90731962
Service-Version: 1.19
@zadjii-msft zadjii-msft mentioned this pull request Oct 27, 2023
Closed
@zadjii-msft zadjii-msft mentioned this pull request Nov 6, 2023
Merged
DHowett pushed a commit that referenced this pull request Nov 6, 2023
@zadjii-msft
Another theoretical fix for a crash (#16267)
59dcbbe 
For history: 

> This is MSFT:46763065 internally. Dumps show this repros on 1.19 too. 
> 
> This was previously #16061 which had a theoretical fix in #16065.
Looks like you're on Terminal Stable v1.18.2822.0, and
https://github.com/microsoft/terminal/releases/tag/v1.18.2822.0 is
supposed to have had that fix in it. Dang.

> well this is embarrassing ... I never actually checked if we _still
had a `_window`_. We're alive, yay! But we're still in the middle of
refrigerating. So, there's no HWND anymore

Attempt to fix this by actually ensuring there's a `_window` in
`AppHost::_WindowInitializedHandler`

Closes #16235
DHowett pushed a commit that referenced this pull request Nov 7, 2023
@zadjii-msft @DHowett
Another theoretical fix for a crash (#16267)
ba2b1ce 
For history:

> This is MSFT:46763065 internally. Dumps show this repros on 1.19 too.
>
> This was previously #16061 which had a theoretical fix in #16065.
Looks like you're on Terminal Stable v1.18.2822.0, and
https://github.com/microsoft/terminal/releases/tag/v1.18.2822.0 is
supposed to have had that fix in it. Dang.

> well this is embarrassing ... I never actually checked if we _still
had a `_window`_. We're alive, yay! But we're still in the middle of
refrigerating. So, there's no HWND anymore

Attempt to fix this by actually ensuring there's a `_window` in
`AppHost::_WindowInitializedHandler`

Closes #16235

(cherry picked from commit 59dcbbe)
Service-Card-Id: 91041358
Service-Version: 1.18
DHowett pushed a commit that referenced this pull request Nov 7, 2023
@zadjii-msft @DHowett
Another theoretical fix for a crash (#16267)
83a2bc1 
For history:

> This is MSFT:46763065 internally. Dumps show this repros on 1.19 too.
>
> This was previously #16061 which had a theoretical fix in #16065.
Looks like you're on Terminal Stable v1.18.2822.0, and
https://github.com/microsoft/terminal/releases/tag/v1.18.2822.0 is
supposed to have had that fix in it. Dang.

> well this is embarrassing ... I never actually checked if we _still
had a `_window`_. We're alive, yay! But we're still in the middle of
refrigerating. So, there's no HWND anymore

Attempt to fix this by actually ensuring there's a `_window` in
`AppHost::_WindowInitializedHandler`

Closes #16235

(cherry picked from commit 59dcbbe)
Service-Card-Id: 91041359
Service-Version: 1.19
DHowett pushed a commit that referenced this pull request Nov 7, 2023
@zadjii-msft @DHowett
Another theoretical fix for a crash (#16267)
8a02fd9 
For history:

> This is MSFT:46763065 internally. Dumps show this repros on 1.19 too.
>
> This was previously #16061 which had a theoretical fix in #16065.
Looks like you're on Terminal Stable v1.18.2822.0, and
https://github.com/microsoft/terminal/releases/tag/v1.18.2822.0 is
supposed to have had that fix in it. Dang.

> well this is embarrassing ... I never actually checked if we _still
had a `_window`_. We're alive, yay! But we're still in the middle of
refrigerating. So, there's no HWND anymore

Attempt to fix this by actually ensuring there's a `_window` in
`AppHost::_WindowInitializedHandler`

Closes #16235

(cherry picked from commit 59dcbbe)
Service-Card-Id: 91041358
Service-Version: 1.18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlos-zamora carlos-zamora carlos-zamora approved these changes

@lhecker lhecker lhecker approved these changes

@DHowett DHowett DHowett approved these changes

Assignees
No one assigned
Labels
Area-Windowing Window frame, quake mode, tearout Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal.
Projects
1.18 Servicing Pipeline
Status: Shipped
1.19 Servicing Pipeline
Status: Shipped
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

INVALID_POINTER_READ_c0000005_WindowsTerminal.exe!AppHost::_WindowInitializedHandler$_ResumeCoro$1
4 participants
@zadjii-msft @DHowett @lhecker @carlos-zamora