Fix crash issue on some special Windows machines.

[lonnywong]

The program crashed on Windows when selecting files for the second time, with the same behavior as https://stackoverflow.com/questions/35366998/2nd-call-to-getopenfilename-crashes-without-error-on-win-8-1-64-bit-machine

The original issue: https://github.com/trzsz/trzsz-ssh/issues/76

[ncruces]

To be honest I don't feel very comfortable with this implementation.

My implementation is definitely buggy, and I'd rather merge something that makes sense with the documentation, not something that ignores the recommendation to call CoUninitialize.

Do you have access to the machine in question?

[lonnywong]

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

CoInitializeEx must be called at least once, and is usually called only once, for each thread that uses the COM library.
Multiple calls to CoInitializeEx by the same thread are allowed as long as they pass the same concurrency flag,
but subsequent valid calls return S_FALSE.

I want to ignore the S_FALSE only. Don’t know how to do that.

I don’t have access to the machine, but I can contact him to help with some testing.

[lonnywong]

The documentation says and is usually called only once, for each thread …. I think it’s a bug on some Windows if we call CoInitializeEx and CoUninitialize multiple times.

[ncruces]

There are several bugs.

The last call to CoUninitialize must come from the same thread as the first call to CoInitializeEx. This requires runtime.LockOSThread before CoInitializeEx, and runtime.UnlockOSThread after CoUninitialize. That's my bug, I'd fix that, and apply it to the other methods, and test the affected machine.

Another bug of mine is around testing for S_FALSE. I'd like to revisit/improve that.

Problems with the current PR:

To not call CoUninitialize… is going directly against the documentation.

You say calling CoInitializeEx repeatedly is ill advised. But that's exactly what the PR does. Each time a dialog is opened, it pumps up the reference count and never allows it to go down. If you show 10k dialogs, the reference count will be 10k.

What I'd like to do, if I had access to a test machine is:

1. fix my bugs pickFolders, test (if fixed, done)
2. apply the same fix to other open file dialogs (they seem to need COM for the explorer integration)

Only if those don't work would I consider not calling CoUninitialize, and I'm not sure what's the right way to do it without pumping the reference count to infinity.

[lonnywong]

Actually, it's not crash in pickFolders, it works fine.

It crash in win.GetOpenFileName. It also crash if I copy your implementation to selectFileMultiple before win.GetOpenFileName.

The application may set another concurrency flag before calling zenity, I think it's better to continue running rather than return an error after CoInitializeEx.

[lonnywong]

--- a/file_windows.go
+++ b/file_windows.go
@@ -3,6 +3,7 @@ package zenity
 import (
        "fmt"
        "path/filepath"
+       "runtime"
        "syscall"
        "unicode/utf16"
        "unsafe"
@@ -174,8 +205,10 @@ func pickFolders(opts options, multi bool) (string, []string, error) {
        owner, _ := opts.attach.(win.HWND)
        defer setup(owner)()

+       runtime.LockOSThread()
+       defer runtime.UnlockOSThread()
        err := win.CoInitializeEx(0, win.COINIT_APARTMENTTHREADED|win.COINIT_DISABLE_OLE1DDE)
-       if err != win.RPC_E_CHANGED_MODE {
+       if err != win.RPC_E_CHANGED_MODE && err != win.S_FALSE {
                if err != nil {
                        return "", nil, err
                }
--- a/internal/win/ole32.go
+++ b/internal/win/ole32.go
@@ -24,6 +24,7 @@ const (

        E_CANCELED         = windows.ERROR_CANCELLED | windows.FACILITY_WIN32<<16 | 0x80000000
        RPC_E_CHANGED_MODE = syscall.Errno(windows.RPC_E_CHANGED_MODE)
+       S_FALSE            = syscall.Errno(windows.S_FALSE)
 )

If we change it like this, then I call CoInitializeEx before calling zenity, it works:

runtime.LockOSThread()
defer runtime.UnlockOSThread()
_ = windows.CoInitializeEx(0, windows.COINIT_APARTMENTTHREADED|windows.COINIT_DISABLE_OLE1DDE)
files, err := zenity.SelectFileMultiple(options...)

[lonnywong]

--- a/file_windows.go
+++ b/file_windows.go
@@ -3,6 +3,7 @@ package zenity
 import (
        "fmt"
        "path/filepath"
+       "runtime"
        "syscall"
        "unicode/utf16"
        "unsafe"
@@ -59,6 +70,16 @@ func selectFileMultiple(opts options) ([]string, error) {
                return res, err
        }

+       runtime.LockOSThread()
+       defer runtime.UnlockOSThread()
+       err := win.CoInitializeEx(0, win.COINIT_APARTMENTTHREADED|win.COINIT_DISABLE_OLE1DDE)
+       if err != win.RPC_E_CHANGED_MODE && err != win.S_FALSE {
+               if err != nil {
+                       return nil, err
+               }
+               defer win.CoUninitialize()
+       }
+
        var args win.OPENFILENAME
        args.StructSize = uint32(unsafe.Sizeof(args))
        args.Owner, _ = opts.attach.(win.HWND)

If we change it like this, and I don't call CoInitializeEx before call zenity, it crashes in win.GetOpenFileName.

[lonnywong]

@ncruces If you want to keep defer win.CoUninitialize(), I can work around it as long as we add err != win.S_FALSE in pickFolders. But, the caller have to call CoInitializeEx before calling zenity, otherwise it may crash on some special machines.

[ncruces]

Can you check if 3f5b6021175dddf5c392571ee27ffeb3a81be269 fixes the issue?

[lonnywong]

Can you check if 3f5b602 fixes the issue?

Thanks. I'll contact him to help test it out. He's probably on vacation and it's going to take some time.

If CoInitializeEx returns S_FALSE, calling CoUninitialize in the previous test was not resolved.

[lonnywong]

Can you check if 3f5b602 fixes the issue?

Unfortunately it’s not working. It always crashes when calling selectFileMultiple for the third time.

[ncruces]

Only selectFileMultiple? That's so weird. So what if you call CoInitializeEx outside zenity? Do you have to call it all the time, or just once?

Again my issue is with calling it all the time in all OS threads. And never undoing it. This increases a reference count, changes global state, and goes against the spirit of the project: no threading or initialization requirements.

[lonnywong]

Not only selectFileMultiple, just test it only, and the opts.directory is false.

We only need to initialize it once for each thread. If we initialize it every time, it will ensure that every thread is initialized. According to the documentation, the second initialization of the same thread will return S_FALSE, instead of incrementing the reference count. Isn’t it?

[ncruces]

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

[ncruces]

The problem with “lets do it for every thread” is that goroutines don't map to threads, unless you runtime.LockOSThread, and if it's something I can't undo, I'd rather leave the decision up to the app.

Especially for something that looks like an (old?) Windows bug, or related to some explorer extension the user may have installed.

[lonnywong]

@ncruces How about we do it like this https://github.com/ncruces/zenity/pull/89/commits/84d5fe5fb3c2a724ce313d340487a01300e1cc8d ?

[ncruces]

That's better, yes. Can you confirm it fixes your issue?

At that point, I'd do this in the general setup, that's already called for every Windows dialog. Though I've reviewed it, and I undo everything that can/should be undone.

Let me read more about this.

Otherwise, this is something that might be done app side, right?

The “only” issue with this is that we pick the wrong apartment and we prevent the app from doing it right.

[lonnywong]

He will help test it tomorrow. According to the results of the previous test, it should be solved.

There is no need to do anything on the app side to solve that issue.

If the app call CoInitializeEx itself, zenity will call CoInitializeEx once again, but only once for each thread.

[ncruces]

Does this also happen with IFileOpenDialog? Or only with GetOpenFileName and friends? Maybe it's time to upgrade the project to always use IFileOpenDialog for everything, not just folders.

[lonnywong]

https://github.com/ncruces/zenity/commit/84d5fe5fb3c2a724ce313d340487a01300e1cc8d works.

pickFolders works fine even if CoUninitialize is called every time. The issue only happens with GetOpenFileName and friends.

[ncruces]

c56588920f359f4e7a14c7153dd796a7839e3c6b uses IFileOpenDialog everywhere. Still need to wrap IFileSaveDialog.

This is a more modern API, hopefully it's less buggy.

[ncruces]

pickFolders works fine even if CoUninitialize is called every time. The issue only happens with GetOpenFileName and friends.

Assuming the above, PR #91 should fix this and use the more modern COM API everywhere it's available. If you could please confirm, I'll merge and release. Thanks for your patience and help!

[lonnywong]

@ncruces Thanks for your help. He may be on vacation until around 19th. I'll let you know when the fix is ​​confirmed.