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

INVALID_POINTER_READ_c0000005_WindowsTerminal.exe!AppHost::_WindowInitializedHandler$_ResumeCoro$1 #16061

Closed
zadjii-msft opened this issue Sep 29, 2023 · 0 comments · Fixed by #16065
Closed

INVALID_POINTER_READ_c0000005_WindowsTerminal.exe!AppHost::_WindowInitializedHandler$_ResumeCoro$1 #16061

zadjii-msft opened this issue Sep 29, 2023 · 0 comments · Fixed by #16065
Assignees
@zadjii-msft
Labels
Area-Windowing Window frame, quake mode, tearout In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal.

Comments

@zadjii-msft
Copy link
Member

MSFT:46763065.

Failure ID 8ab19ec3-2532-a209-0ca4-5e6b9aa9133b

8.70% of our crashes in 1.19 so far.

Bug is here, on L1396.

terminal/src/cascadia/WindowsTerminal/AppHost.cpp

Lines 1390 to 1402 in 4382a17

// For inexplicable reasons, again, hop to the BG thread, then back to the
// UI thread. This is shockingly load bearing - without this, then
// sometimes, we'll _still_ show the HWND before the XAML island actually
// paints.
co_await wil::resume_foreground(_windowLogic.GetRoot().Dispatcher(), winrt::Windows::UI::Core::CoreDispatcherPriority::Low);
ShowWindow(_window->GetHandle(), nCmdShow);
// If we didn't start the window hidden (in one way or another), then try to
// pull ourselves to the foreground. Don't necessarily do a whole "summon",
// we don't really want to STEAL foreground if someone rightfully took it
const bool noForeground = nCmdShow == SW_SHOWMINIMIZED ||

Undoubtably, this is us not using a "weak ref". The app host got dtored during a Refrigerate, and so this is invalid on the other side of the co_await. Yikes.
@zadjii-msft zadjii-msft added Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Area-Windowing Window frame, quake mode, tearout labels Sep 29, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Sep 29, 2023
@zadjii-msft zadjii-msft self-assigned this Sep 29, 2023
@zadjii-msft zadjii-msft removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Sep 29, 2023
zadjii-msft added a commit that referenced this issue Sep 29, 2023
@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
@zadjii-msft zadjii-msft mentioned this issue Sep 29, 2023
Merged
@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR This issue has a related PR label Sep 29, 2023
DHowett pushed a commit that referenced this issue Oct 3, 2023
@zadjii-msft
Use weak_ptrs for AppHost for coroutines (#16065)
8521aae 
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 the Needs-Tag-Fix Doesn't match tag requirements label Oct 3, 2023
DHowett pushed a commit that referenced this issue 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 issue 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 issue Oct 27, 2023
Closed
@zadjii-msft zadjii-msft mentioned this issue Nov 6, 2023
Merged
DHowett pushed a commit that referenced this issue 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 issue 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 issue 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 issue 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
Assignees

@zadjii-msft zadjii-msft

Labels
Area-Windowing Window frame, quake mode, tearout In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Use weak_ptrs for AppHost for coroutines microsoft/terminal
1 participant
@zadjii-msft