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

Browse buttons crash when running as admin #8957

Closed
carlos-zamora opened this issue Jan 29, 2021 · 8 comments · Fixed by #9760
Closed

Browse buttons crash when running as admin #8957

carlos-zamora opened this issue Jan 29, 2021 · 8 comments · Fixed by #9760
Assignees
@DHowett
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.8

Comments

@carlos-zamora
Copy link
Member

The optional graphical settings editor worked properly when I ran Windows Terminal as a regular user. However, if I ran Windows Terminal as Administrator, the app would crash if I clicked the "Browse..." button for either "Command line" or "Starting directory".

Originally posted by @huang63 in #6800 (comment)
@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 Jan 29, 2021
@carlos-zamora carlos-zamora added 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. Severity-Crash Crashes are real bad news. labels Jan 29, 2021
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jan 29, 2021
@carlos-zamora carlos-zamora added zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Jan 29, 2021
@carlos-zamora
Copy link
Member Author

Did a bit of digging on this one. Here's my findings:

Relevant PR: #8391

The culprit is this call right here:

terminal/src/cascadia/TerminalSettingsEditor/Profiles.cpp

Line 348 in e7d3262

StorageFolder folder = co_await picker.PickSingleFolderAsync();

I'm not exactly sure what is going on though. Looks like PickSingleFolderAsync() failed, and GetResults() returns a bad HRESULT.

Assigning @DHowett for now to provide some ideas/insight on next steps.

@carlos-zamora carlos-zamora added this to the Terminal v1.7 milestone Jan 29, 2021
@DHowett
Copy link
Member

DHowett commented Jan 29, 2021

You’re gonna need to give me that HRESULT!

I’m guessing this is the same class of problem as “i can’t drag tabs in admin mode”. Same team, same APIs...

@WSLUser
Copy link
Contributor

WSLUser commented Jan 29, 2021

I'm now betting #8950 is extremely related. It didn't matter if it was elevated or not so I think invalid paths are still an issue that likely correlate to this bug. Perhaps they have the same fix (one can hope).

@carlos-zamora
Copy link
Member Author

You’re gonna need to give me that HRESULT!

I’m guessing this is the same class of problem as “i can’t drag tabs in admin mode”. Same team, same APIs...

-2147467259 --> 0x80004005 : 'Unspecified error'. --> E_FAIL is returned from the GetResults() call.

@carlos-zamora
Copy link
Member Author

@DHowett friendly bump on this thread to get it on your radar

This was referenced Mar 2, 2021
Closed
Closed
@zadjii-msft
Copy link
Member

In #9341, @rileysea was also seeing this unelevated 😬

DHowett added a commit that referenced this issue Apr 9, 2021
@DHowett
Replace Windows.Storage.Pickers with Common File Dialogs
92a9d77 
Using Pickers from an elevated application yields an
ERROR_ACCESS_DENIED. Of course it does: it was designed for the modern
app platform.

Using the common dialog infrastructure has some downsides¹, but it
doesn't crash and is just as flexible.

Fixes #8957

¹ You've got to use raw COM, and it runs in-proc instead of out-of-proc.
@DHowett DHowett mentioned this issue Apr 9, 2021
Merged
@ghost ghost added the In-PR This issue has a related PR label Apr 9, 2021
@ghost ghost closed this as completed in #9760 Apr 12, 2021
@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 Apr 12, 2021
ghost pushed a commit that referenced this issue Apr 12, 2021
@DHowett
Replace Windows.Storage.Pickers with Common File Dialogs (#9760)
959c423 
Using Pickers from an elevated application yields an
ERROR_ACCESS_DENIED. Of course it does: it was designed for the modern
app platform.

Using the common dialog infrastructure has some downsides¹, but it
doesn't crash and is just as flexible.

I've added some fun templated functions that help us with the
complexity.

Fixes #8957

¹You've got to use raw COM, and it runs in-proc instead of out-of-proc.

## Validation Steps Performed
I tested every picker.
DHowett added a commit that referenced this issue Apr 12, 2021
@DHowett
Replace Windows.Storage.Pickers with Common File Dialogs (#9760)
a114d1b 
Using Pickers from an elevated application yields an
ERROR_ACCESS_DENIED. Of course it does: it was designed for the modern
app platform.

Using the common dialog infrastructure has some downsides¹, but it
doesn't crash and is just as flexible.

I've added some fun templated functions that help us with the
complexity.

Fixes #8957

¹You've got to use raw COM, and it runs in-proc instead of out-of-proc.

I tested every picker.

(cherry picked from commit 959c423)
@ghost
Copy link

ghost commented Apr 14, 2021

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

Handy links:

@ghost
Copy link

ghost commented Apr 14, 2021

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

Handy links:

@miniksa miniksa removed the zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. label Sep 27, 2021
@AndromedaMelody AndromedaMelody mentioned this issue Dec 16, 2022
Closed
@zadjii-msft zadjii-msft mentioned this issue Apr 15, 2024
Open
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@DHowett DHowett

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.8
Development

Successfully merging a pull request may close this issue.

Replace Windows.Storage.Pickers with Common File Dialogs microsoft/terminal
5 participants
@DHowett @carlos-zamora @miniksa @zadjii-msft @WSLUser