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

Make Pane a proper WinRT type #3999

Open
zadjii-msft opened this issue Dec 17, 2019 · 5 comments
Open

Make Pane a proper WinRT type #3999

zadjii-msft opened this issue Dec 17, 2019 · 5 comments
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Task It's a feature request, but it doesn't really need a major design. Priority-2 A description (P2) Product-Terminal The new Windows Terminal.

Comments

@zadjii-msft
Copy link
Member

Pretty self explanatory. Right now they're plain c++ types, but in reality they should be exposed as projected WinRT types, so they can be used across binary boundaries.

@zadjii-msft zadjii-msft added Product-Terminal The new Windows Terminal. Issue-Task It's a feature request, but it doesn't really need a major design. labels Dec 17, 2019
@zadjii-msft zadjii-msft added this to the Terminal Backlog milestone Dec 17, 2019
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Dec 17, 2019
@zadjii-msft zadjii-msft added the Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. label Dec 17, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Dec 17, 2019
@DHowett-MSFT DHowett-MSFT removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Dec 19, 2019
@zadjii-msft zadjii-msft added the Priority-2 A description (P2) label Jan 4, 2022
@zadjii-msft
Copy link
Member Author

zadjii-msft commented Sep 6, 2023

We may be able to fix MSFT:451680901 and MSFT:407375662 with this. Those are both crashes in Pane because pane is setting up event handlers, using this as the callback target, but the event fires after the pane is released (which doesn't actually unregister the callback).

Footnotes

  1. TerminalPage::_updateThemeColors

  2. Pane::_ControlGotFocusHandler

@lhecker
Copy link
Member

lhecker commented Sep 6, 2023

Huh, but aren't we already using enable_shared_from_this<Pane>? Of the 17 places in Pane.cpp that use this only 2 use it unsafely without event revoker: _borderFirst/Second.Tapped(). They call _FocusFirstChild and that would match the second bug right? TerminalPage sets up the GotFocus event handlers which calls _UpdateActivePane, which then invokes the ActivePaneChanged event handlers, which TerminalPage is subscribed to, which then calls _updateThemeColors. That would explain the first bug.

In short, I think we can hotfix the two bugs right now if we use event revokers for _borderFirst/Second.Tapped() in Pane.cpp. All the other event handlers already do it too. Does that make sense? Should I do it?

That aside: FWIW strictly using projected types may make implementing certain algorithms more difficult / less effective, like WalkTree which is currently being used in a number of places outside of Pane.cpp.

@zadjii-msft
Copy link
Member Author

I think these guys might be part of it too:

// Register an event with the control to have it inform us when it gains focus.
_gotFocusRevoker = _control.GotFocus(winrt::auto_revoke, { this, &Pane::_ControlGotFocusHandler });
_lostFocusRevoker = _control.LostFocus(winrt::auto_revoke, { this, &Pane::_ControlLostFocusHandler });

Because they're registered to this, they don't get revoked when the pane is Closed.

(I can't recall if I got those in a drive-by for #997 or not)

@lhecker
Copy link
Member

lhecker commented Sep 6, 2023

Oh, is there a difference between a pane closing and a pane being deallocated? I hoped it would only be a problem when a pane is deallocated and the this pointer is actually invalid.

@zadjii-msft
Copy link
Member Author

Wait now I honestly don't know. It seems like releasing the last ref to the shared_ptr would dtor the Pane, and that would automatically clean up the _gotFocusRevoker & _lostFocusRevoker, so that they couldn't be called back at this point. Maybe there's a time slice where the smart pointer is invalid, but before the pane is actually deallocated. Maybe, if at exactly that slice, the shared_from_this call explodes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Task It's a feature request, but it doesn't really need a major design. Priority-2 A description (P2) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

No branches or pull requests

4 participants