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

Deleting a (last?) profile in the SUI sends the Terminal to a farm upstate #13017

Closed
zadjii-msft opened this issue May 2, 2022 · 7 comments · Fixed by #13044
Closed

Deleting a (last?) profile in the SUI sends the Terminal to a farm upstate #13017

zadjii-msft opened this issue May 2, 2022 · 7 comments · Fixed by #13044
Assignees
@zadjii-msft
Labels
Area-SettingsUI Anything specific to the SUI Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-Crash Crashes are real bad news.
Milestone
Terminal v1.14

Comments

@zadjii-msft
Copy link
Member

  1. Make a new blank profile
  2. Save the settings
  3. Head over to that "delete profile" button and hit it
  4. ZOOP, no Terminal

Stack was useless - something above a DispatcherLoop with a ReportUnhandledError about a "The application attempted to access data outside the valid range" but I apparently don't have !xamlstowed installed anymore.
@zadjii-msft zadjii-msft added Issue-Bug It either shouldn't be doing this or needs an investigation. Severity-Crash Crashes are real bad news. Product-Terminal The new Windows Terminal. Priority-1 A description (P1) Area-SettingsUI Anything specific to the SUI labels May 2, 2022
@zadjii-msft zadjii-msft added this to the Terminal v1.14 milestone May 2, 2022
@ghost ghost added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label May 2, 2022
@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 May 2, 2022
@PankajBhojwani
Copy link
Contributor

I cannot repro this on current main nor on preview (1.14.891.0). Do you consistently hit this or only sometimes?

@zadjii-msft
Copy link
Member Author

Okay, got it to hit the second time I tried this. First time, no repro. Second time, I made the Terminal wide enough that the nav view was in its expanded state, and then the terminal died on the delete.

@zadjii-msft zadjii-msft self-assigned this May 5, 2022
@zadjii-msft
Copy link
Member Author

Gosh I can't find it now, but we had some crash in the past about changing the elements of a nav view. Something about the vector of items.... @carlos-zamora do you remember what I'm thinking of?

@zadjii-msft
Copy link
Member Author

THIS ONE
#8773

Oh my god it's the same: #8747

@zadjii-msft
Copy link
Member Author 

        // Add an event handler for when the user wants to delete a profile.
        profile.DeleteProfile({ this, &MainPage::_DeleteProfile });

I'd bet we're adding the delete handler TWICE. Stepping through MainPage::_DeleteProfile, the first time it succeeds, but then there's a SECOND call to that handler. Why is there a second call? That's not right.

It might mean that the repro is

  1. Make a new blank profile
  2. Save the settings
  3. Click on the profile ABOVE the new profile
  4. Click back to the new profile
  5. Head over to that "delete profile" button and hit it
  6. ZOOP, no Terminal

zadjii-msft added a commit that referenced this issue May 5, 2022
@zadjii-msft
fixes #13017
eea14e3
@zadjii-msft zadjii-msft mentioned this issue May 5, 2022
Merged
1 task
@ghost ghost added the In-PR This issue has a related PR label May 5, 2022
@ghost ghost closed this as completed in #13044 May 6, 2022
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels May 6, 2022
ghost pushed a commit that referenced this issue May 6, 2022
@zadjii-msft
Only add the ProfileVM.DeleteProfile handler once (#13044)
d072314 
Turns out if you add that Delete handler there, then every time you navigate to the profile, we'll add another Delete handler to the list of handlers. That's bad - that'll cause us to try and delete the profile multiple times.

The repro I had before was 100%, now it's fixed.

* [x] Closes #13017
carlos-zamora pushed a commit that referenced this issue May 6, 2022
@zadjii-msft @carlos-zamora
Only add the ProfileVM.DeleteProfile handler once (#13044)
17b67a7 
Turns out if you add that Delete handler there, then every time you navigate to the profile, we'll add another Delete handler to the list of handlers. That's bad - that'll cause us to try and delete the profile multiple times.

The repro I had before was 100%, now it's fixed.

* [x] Closes #13017

(cherry picked from commit d072314)
Service-Card-Id: 81599094
Service-Version: 1.14
carlos-zamora pushed a commit that referenced this issue May 12, 2022
@zadjii-msft @carlos-zamora
Only add the ProfileVM.DeleteProfile handler once (#13044)
5c8d596 
Turns out if you add that Delete handler there, then every time you navigate to the profile, we'll add another Delete handler to the list of handlers. That's bad - that'll cause us to try and delete the profile multiple times.

The repro I had before was 100%, now it's fixed.

* [x] Closes #13017

(cherry picked from commit d072314)
Service-Card-Id: 81744856
Service-Version: 1.13
This was referenced May 21, 2022
Closed
Closed
@ghost
Copy link

ghost commented May 24, 2022

🎉This issue was addressed in #13044, which has now been successfully released as Windows Terminal v1.13.1143.:tada:

Handy links:

@ghost
Copy link

ghost commented May 24, 2022

🎉This issue was addressed in #13044, which has now been successfully released as Windows Terminal Preview v1.14.143.:tada:

Handy links:

@zadjii-msft zadjii-msft mentioned this issue Aug 18, 2022
Closed
This issue was closed.
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-SettingsUI Anything specific to the SUI Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-Crash Crashes are real bad news.
Projects
None yet
Milestone
Terminal v1.14
Development

Successfully merging a pull request may close this issue.

Only add the ProfileVM.DeleteProfile handler once microsoft/terminal
2 participants
@zadjii-msft @PankajBhojwani