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

Use IFileDialog where available. #91

Merged
merged 7 commits into from
Feb 23, 2024
Merged

Use IFileDialog where available. #91

merged 7 commits into from
Feb 23, 2024

Conversation

ncruces
Copy link
Owner

@ncruces ncruces commented Feb 7, 2024

Alternative fix for #89.

@ncruces ncruces changed the title Use IFileDialog where available Use IFileDialog where available. Feb 7, 2024
@ncruces
Copy link
Owner Author

ncruces commented Feb 21, 2024

Thinking of merging this regardless, wdyt @lonnywong?

@lonnywong
Copy link

Thinking of merging this regardless, wdyt @lonnywong?

Sorry for the delay. It doesn't work on that machine. It may fall back to GetOpenFileName, I'd like to try debugging to confirm, but he's not free recently.

@ncruces
Copy link
Owner Author

ncruces commented Feb 22, 2024

That's disappointing. If you can help debug, it'd be great. I'll test more just to see if this big change isn't a regression. And I'll probably merge it too.

file_windows.go Outdated
func coInitialize() (context.CancelFunc, error) {
runtime.LockOSThread()
err := win.CoInitializeEx(0, win.COINIT_APARTMENTTHREADED|win.COINIT_DISABLE_OLE1DDE)
if err == nil || err == win.S_FALSE {

Choose a reason for hiding this comment

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

If the app calls CoInitializeEx with win.COINIT_APARTMENTTHREADED|win.COINIT_DISABLE_OLE1DDE before calling zenity, the err will be win.S_FALSE. zenity should not call win.CoUninitialize().
So, err == win.S_FALSE should be moved to below: err == win.RPC_E_CHANGED_MODE || err == win.S_FALSE.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think we've covered this before:

To close the COM library gracefully on a thread, each successful call to CoInitialize or CoInitializeEx, including any call that returns S_FALSE, must be balanced by a corresponding call to CoUninitialize.

https://learn.microsoft.com/en-us/windows/win32/api/combaseapi/nf-combaseapi-coinitializeex

Copy link
Owner Author

Choose a reason for hiding this comment

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

But if you don't want COM to be closed, you can CoInitializeEx yourself, and never CoUninitialize.
This is a reference count, if you don't CoUninitialize it never goes back to zero.

@lonnywong
Copy link

@ncruces It crashes without falling back to GetOpenFileName.
Should we use the solution 84d5fe5 ? It works with the patch.

@ncruces
Copy link
Owner Author

ncruces commented Feb 23, 2024

If we're going to leave COM initialized, windows can do it for us.

When it returns S_OK (nil), we leave it initialized.
When it returns S_FALSE, it was already initialized, so we can CoUninitialize (drop the ref count back to what it was).
When it returns RPC_E_CHANGED_MODE, we asked it to change mode, but we don't care about the mode.

Otherwise, it's an error.

@ncruces ncruces merged commit 8707cba into master Feb 23, 2024
5 checks passed
@ncruces ncruces deleted the winfile branch February 23, 2024 13:14
@lonnywong
Copy link

@ncruces Thanks.

#89 has been fixed by 99f1a25:

zenity/file_windows.go

Lines 453 to 472 in 99f1a25

func coInitialize() (context.CancelFunc, error) {
runtime.LockOSThread()
// .NET uses MTA for all background threads, so do the same.
// If someone needs STA because they're doing UI,
// they should initialize COM themselves before.
err := win.CoInitializeEx(0, win.COINIT_MULTITHREADED|win.COINIT_DISABLE_OLE1DDE)
if err == win.S_FALSE {
// COM was already initialized, we simply increased the ref count.
// Make this a no-op by decreasing our ref count.
win.CoUninitialize()
return runtime.UnlockOSThread, nil
}
// Don't uninitialize COM; this is against the docs, but it's what .NET does.
// Eventually all threads will have COM initialized.
if err == nil || err == win.RPC_E_CHANGED_MODE {
return runtime.UnlockOSThread, nil
}
runtime.UnlockOSThread()
return nil, err
}

As long as the reference count does not decrease to 0, the bug will not be triggered.

@ncruces
Copy link
Owner Author

ncruces commented Feb 26, 2024

That was the expectation, great to see it confirmed! It just felt better to leave it to windows to manage the thread local stuff.

@ncruces ncruces added bug Something isn't working enhancement New feature or request labels Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants