[PATCH v5 0/2] MR2068: comdlg32: Enable visual styles when showing IFileDialog.
Required for !1857 to not break when comctl32 version 6 isn't requested by application (in particular, found this in [qapitrace](https://github.com/apitrace/apitrace)). Manifest resource id and assembly identity name match with Windows. For isolation purposes, activation context is disabled while emitting events. -- v5: comdlg32: Enable visual styles when showing IFileDialog. https://gitlab.winehq.org/wine/wine/-/merge_requests/2068
From: Vladislav Timonin <timoninvlad(a)yandex.ru> --- dlls/comdlg32/itemdlg.c | 3 +++ dlls/comdlg32/tests/itemdlg.c | 48 ++++++++++++++++++++++++++++++++++- 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/dlls/comdlg32/itemdlg.c b/dlls/comdlg32/itemdlg.c index f3b9a217e79..5ecf46d7e5b 100644 --- a/dlls/comdlg32/itemdlg.c +++ b/dlls/comdlg32/itemdlg.c @@ -2246,6 +2246,9 @@ static HRESULT create_dialog(FileDialogImpl *This, HWND parent) { INT_PTR res; + if (This->dlg_hwnd) + return E_UNEXPECTED; + SetLastError(0); res = DialogBoxParamW(COMDLG32_hInstance, MAKEINTRESOURCEW(NEWFILEOPENV3ORD), diff --git a/dlls/comdlg32/tests/itemdlg.c b/dlls/comdlg32/tests/itemdlg.c index 8ce0ea806b9..ec681cf3c33 100644 --- a/dlls/comdlg32/tests/itemdlg.c +++ b/dlls/comdlg32/tests/itemdlg.c @@ -27,12 +27,14 @@ #define IDT_CHANGEFILETYPE 500 #define IDT_CLOSEDIALOG 501 +#define IDT_SHOWDIALOG 502 typedef enum { IFDEVENT_TEST_NONE = 0, IFDEVENT_TEST1 = 0x1, IFDEVENT_TEST2 = 0x2, - IFDEVENT_TEST3 = 0x3 + IFDEVENT_TEST3 = 0x3, + IFDEVENT_TEST4 = 0x4, } FileDialogEventsTest; static HRESULT (WINAPI *pSHCreateShellItem)(LPCITEMIDLIST,IShellFolder*,LPCITEMIDLIST,IShellItem**); @@ -193,6 +195,15 @@ static LRESULT CALLBACK test_customize_dlgproc(HWND hwnd, UINT message, WPARAM w br = PostMessageW(hwnd, WM_COMMAND, IDCANCEL, 0); ok(br, "Failed\n"); return TRUE; + case IDT_SHOWDIALOG: + { + HRESULT hr; + KillTimer(hwnd, IDT_SHOWDIALOG); + hr = IFileDialog_Show(pfd, NULL); + ok(hr == E_UNEXPECTED, "got 0x%08lx.\n", hr); + SetTimer(hwnd, IDT_CLOSEDIALOG, 100, 0); + return TRUE; + } } } @@ -249,6 +260,9 @@ static HRESULT WINAPI IFileDialogEvents_fnOnFolderChange(IFileDialogEvents *ifac case IFDEVENT_TEST3: SetTimer(dlg_hwnd, IDT_CHANGEFILETYPE, 100, 0); break; + case IFDEVENT_TEST4: + SetTimer(dlg_hwnd, IDT_SHOWDIALOG, 100, 0); + break; default: ok(FALSE, "Should not happen (%d)\n", This->events_test); } @@ -2496,6 +2510,37 @@ static void test_customize_remove_from_empty_combobox(void) IFileDialog_Release(pfod); } +static void test_double_show(void) +{ + IFileDialogEventsImpl *pfdeimpl; + IFileDialogEvents *pfde; + IFileDialog *pfd; + DWORD cookie; + HRESULT hr; + + hr = CoCreateInstance(&CLSID_FileOpenDialog, NULL, CLSCTX_INPROC_SERVER, + &IID_IFileDialog, (void**)&pfd); + ok(hr == S_OK, "got 0x%08lx.\n", hr); + + pfde = IFileDialogEvents_Constructor(); + pfdeimpl = impl_from_IFileDialogEvents(pfde); + pfdeimpl->events_test = IFDEVENT_TEST4; + + hr = IFileDialog_Advise(pfd, pfde, &cookie); + ok(hr == S_OK, "got 0x%08lx.\n", hr); + + ensure_zero_events(pfdeimpl); + + hr = IFileDialog_Show(pfd, NULL); + ok(hr == HRESULT_FROM_WIN32(ERROR_CANCELLED), "got 0x%08lx.\n", hr); + + hr = IFileDialog_Unadvise(pfd, cookie); + ok(hr == S_OK, "got 0x%08lx.\n", hr); + + IFileDialogEvents_Release(pfde); + IFileDialog_Release(pfd); +} + START_TEST(itemdlg) { OleInitialize(NULL); @@ -2521,6 +2566,7 @@ START_TEST(itemdlg) test_persistent_state(); test_overwrite(); test_customize_remove_from_empty_combobox(); + test_double_show(); } else skip("Skipping all Item Dialog tests.\n"); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2068
From: Vladislav Timonin <timoninvlad(a)yandex.ru> --- dlls/comdlg32/cdlg.h | 1 + dlls/comdlg32/cdlg32.c | 16 +++++- dlls/comdlg32/comdlg32.manifest | 16 ++++++ dlls/comdlg32/comdlg32.rc | 3 ++ dlls/comdlg32/itemdlg.c | 88 +++++++++++++++++++++++++++++++++ 5 files changed, 123 insertions(+), 1 deletion(-) create mode 100644 dlls/comdlg32/comdlg32.manifest diff --git a/dlls/comdlg32/cdlg.h b/dlls/comdlg32/cdlg.h index 1ddd157b880..e47d685fac3 100644 --- a/dlls/comdlg32/cdlg.h +++ b/dlls/comdlg32/cdlg.h @@ -27,6 +27,7 @@ #define COMDLG32_Atom MAKEINTATOM(0xa000) /* MS uses this one to identify props */ extern HINSTANCE COMDLG32_hInstance DECLSPEC_HIDDEN; +extern HANDLE COMDLG32_hActCtx DECLSPEC_HIDDEN; void COMDLG32_SetCommDlgExtendedError(DWORD err) DECLSPEC_HIDDEN; LPVOID COMDLG32_AllocMem(int size) __WINE_ALLOC_SIZE(1) DECLSPEC_HIDDEN; diff --git a/dlls/comdlg32/cdlg32.c b/dlls/comdlg32/cdlg32.c index f9619cc6a49..9941d44c4e9 100644 --- a/dlls/comdlg32/cdlg32.c +++ b/dlls/comdlg32/cdlg32.c @@ -40,6 +40,7 @@ WINE_DEFAULT_DEBUG_CHANNEL(commdlg); DECLSPEC_HIDDEN HINSTANCE COMDLG32_hInstance = 0; +HANDLE COMDLG32_hActCtx = INVALID_HANDLE_VALUE; static DWORD COMDLG32_TlsIndex = TLS_OUT_OF_INDEXES; @@ -72,6 +73,9 @@ BOOL WINAPI DllMain(HINSTANCE hInstance, DWORD Reason, LPVOID Reserved) switch(Reason) { case DLL_PROCESS_ATTACH: + { + ACTCTXW actctx = {0}; + COMDLG32_hInstance = hInstance; DisableThreadLibraryCalls(hInstance); @@ -79,11 +83,21 @@ BOOL WINAPI DllMain(HINSTANCE hInstance, DWORD Reason, LPVOID Reserved) /* SHELL */ GPA(COMDLG32_SHSimpleIDListFromPathAW, SHELL32_hInstance, (LPCSTR)162); - break; + actctx.cbSize = sizeof(actctx); + actctx.hModule = COMDLG32_hInstance; + actctx.lpResourceName = MAKEINTRESOURCEW(123); + actctx.dwFlags = ACTCTX_FLAG_HMODULE_VALID | ACTCTX_FLAG_RESOURCE_NAME_VALID; + COMDLG32_hActCtx = CreateActCtxW(&actctx); + if (COMDLG32_hActCtx == INVALID_HANDLE_VALUE) + ERR("failed to create activation context, last error %lu\n", GetLastError()); + + break; + } case DLL_PROCESS_DETACH: if (Reserved) break; if (COMDLG32_TlsIndex != TLS_OUT_OF_INDEXES) TlsFree(COMDLG32_TlsIndex); + ReleaseActCtx(&COMDLG32_hActCtx); break; } return TRUE; diff --git a/dlls/comdlg32/comdlg32.manifest b/dlls/comdlg32/comdlg32.manifest new file mode 100644 index 00000000000..f9e7b7cc115 --- /dev/null +++ b/dlls/comdlg32/comdlg32.manifest @@ -0,0 +1,16 @@ +<?xml version="1.0" encoding="UTF-8" standalone="yes"?> +<assembly xmlns="urn:schemas-microsoft-com:asm.v1" manifestVersion="1.0"> +<assemblyIdentity type="win32" name="Microsoft.Windows.Shell.comdlg32" version="0.0.0.0"/> +<dependency> + <dependentAssembly> + <assemblyIdentity + type="win32" + name="Microsoft.Windows.Common-Controls" + version="6.0.0.0" + processorArchitecture="*" + publicKeyToken="6595b64144ccf1df" + language="*" + /> + </dependentAssembly> +</dependency> +</assembly> diff --git a/dlls/comdlg32/comdlg32.rc b/dlls/comdlg32/comdlg32.rc index a5d09faa563..474bafd8760 100644 --- a/dlls/comdlg32/comdlg32.rc +++ b/dlls/comdlg32/comdlg32.rc @@ -602,3 +602,6 @@ NETWORK ICON network.ico /* @makedep: fontpics.bmp */ 38 BITMAP fontpics.bmp + +/* @makedep: comdlg32.manifest */ +123 RT_MANIFEST comdlg32.manifest diff --git a/dlls/comdlg32/itemdlg.c b/dlls/comdlg32/itemdlg.c index 5ecf46d7e5b..08a844ce593 100644 --- a/dlls/comdlg32/itemdlg.c +++ b/dlls/comdlg32/itemdlg.c @@ -145,17 +145,45 @@ typedef struct FileDialogImpl { DWORD opendropdown_selection; GUID client_guid; + + HANDLE user_actctx; } FileDialogImpl; +typedef struct { + BOOL is_active; + ULONG_PTR cookie; +} actctx_state; + +static void activate_user_activation_context(FileDialogImpl *This, actctx_state *state) +{ + if (This->user_actctx == INVALID_HANDLE_VALUE) + return; + + if (!(state->is_active = ActivateActCtx(This->user_actctx, &state->cookie))) + ERR("Failed to activate user activation context\n"); +} + +static void deactivate_user_activation_context(actctx_state *state) +{ + if (!state->is_active) + return; + + if (!DeactivateActCtx(0, state->cookie)) + ERR("Failed to deactivate user activation context\n"); +} + /************************************************************************** * Event wrappers. */ static HRESULT events_OnFileOk(FileDialogImpl *This) { + actctx_state user_actctx_state = {0}; events_client *cursor; HRESULT hr = S_OK; TRACE("%p\n", This); + activate_user_activation_context(This, &user_actctx_state); + LIST_FOR_EACH_ENTRY(cursor, &This->events_clients, events_client, entry) { TRACE("Notifying %p\n", cursor); @@ -164,6 +192,8 @@ static HRESULT events_OnFileOk(FileDialogImpl *This) break; } + deactivate_user_activation_context(&user_actctx_state); + if(hr == E_NOTIMPL) hr = S_OK; @@ -172,10 +202,13 @@ static HRESULT events_OnFileOk(FileDialogImpl *This) static HRESULT events_OnFolderChanging(FileDialogImpl *This, IShellItem *folder) { + actctx_state user_actctx_state = {0}; events_client *cursor; HRESULT hr = S_OK; TRACE("%p (%p)\n", This, folder); + activate_user_activation_context(This, &user_actctx_state); + LIST_FOR_EACH_ENTRY(cursor, &This->events_clients, events_client, entry) { TRACE("Notifying %p\n", cursor); @@ -184,6 +217,8 @@ static HRESULT events_OnFolderChanging(FileDialogImpl *This, IShellItem *folder) break; } + deactivate_user_activation_context(&user_actctx_state); + if(hr == E_NOTIMPL) hr = S_OK; @@ -192,47 +227,65 @@ static HRESULT events_OnFolderChanging(FileDialogImpl *This, IShellItem *folder) static void events_OnFolderChange(FileDialogImpl *This) { + actctx_state user_actctx_state = {0}; events_client *cursor; TRACE("%p\n", This); + activate_user_activation_context(This, &user_actctx_state); + LIST_FOR_EACH_ENTRY(cursor, &This->events_clients, events_client, entry) { TRACE("Notifying %p\n", cursor); IFileDialogEvents_OnFolderChange(cursor->pfde, (IFileDialog*)&This->IFileDialog2_iface); } + + deactivate_user_activation_context(&user_actctx_state); } static void events_OnSelectionChange(FileDialogImpl *This) { + actctx_state user_actctx_state = {0}; events_client *cursor; TRACE("%p\n", This); + activate_user_activation_context(This, &user_actctx_state); + LIST_FOR_EACH_ENTRY(cursor, &This->events_clients, events_client, entry) { TRACE("Notifying %p\n", cursor); IFileDialogEvents_OnSelectionChange(cursor->pfde, (IFileDialog*)&This->IFileDialog2_iface); } + + deactivate_user_activation_context(&user_actctx_state); } static void events_OnTypeChange(FileDialogImpl *This) { + actctx_state user_actctx_state = {0}; events_client *cursor; TRACE("%p\n", This); + activate_user_activation_context(This, &user_actctx_state); + LIST_FOR_EACH_ENTRY(cursor, &This->events_clients, events_client, entry) { TRACE("Notifying %p\n", cursor); IFileDialogEvents_OnTypeChange(cursor->pfde, (IFileDialog*)&This->IFileDialog2_iface); } + + deactivate_user_activation_context(&user_actctx_state); } static HRESULT events_OnOverwrite(FileDialogImpl *This, IShellItem *shellitem) { + actctx_state user_actctx_state = {0}; events_client *cursor; HRESULT hr = S_OK; FDE_OVERWRITE_RESPONSE response = FDEOR_DEFAULT; TRACE("%p %p\n", This, shellitem); + activate_user_activation_context(This, &user_actctx_state); + LIST_FOR_EACH_ENTRY(cursor, &This->events_clients, events_client, entry) { TRACE("Notifying %p\n", cursor); @@ -242,6 +295,8 @@ static HRESULT events_OnOverwrite(FileDialogImpl *This, IShellItem *shellitem) break; } + deactivate_user_activation_context(&user_actctx_state); + if(hr == E_NOTIMPL) hr = S_OK; @@ -274,9 +329,12 @@ static inline HRESULT get_cctrl_event(IFileDialogEvents *pfde, IFileDialogContro static HRESULT cctrl_event_OnButtonClicked(FileDialogImpl *This, DWORD ctl_id) { + actctx_state user_actctx_state = {0}; events_client *cursor; TRACE("%p\n", This); + activate_user_activation_context(This, &user_actctx_state); + LIST_FOR_EACH_ENTRY(cursor, &This->events_clients, events_client, entry) { IFileDialogControlEvents *pfdce; @@ -288,14 +346,19 @@ static HRESULT cctrl_event_OnButtonClicked(FileDialogImpl *This, DWORD ctl_id) } } + deactivate_user_activation_context(&user_actctx_state); + return S_OK; } static HRESULT cctrl_event_OnItemSelected(FileDialogImpl *This, DWORD ctl_id, DWORD item_id) { + actctx_state user_actctx_state = {0}; events_client *cursor; TRACE("%p %li %li\n", This, ctl_id, item_id); + activate_user_activation_context(This, &user_actctx_state); + LIST_FOR_EACH_ENTRY(cursor, &This->events_clients, events_client, entry) { IFileDialogControlEvents *pfdce; @@ -307,14 +370,19 @@ static HRESULT cctrl_event_OnItemSelected(FileDialogImpl *This, DWORD ctl_id, DW } } + deactivate_user_activation_context(&user_actctx_state); + return S_OK; } static HRESULT cctrl_event_OnCheckButtonToggled(FileDialogImpl *This, DWORD ctl_id, BOOL checked) { + actctx_state user_actctx_state = {0}; events_client *cursor; TRACE("%p\n", This); + activate_user_activation_context(This, &user_actctx_state); + LIST_FOR_EACH_ENTRY(cursor, &This->events_clients, events_client, entry) { IFileDialogControlEvents *pfdce; @@ -326,15 +394,20 @@ static HRESULT cctrl_event_OnCheckButtonToggled(FileDialogImpl *This, DWORD ctl_ } } + deactivate_user_activation_context(&user_actctx_state); + return S_OK; } static HRESULT cctrl_event_OnControlActivating(FileDialogImpl *This, DWORD ctl_id) { + actctx_state user_actctx_state = {0}; events_client *cursor; TRACE("%p\n", This); + activate_user_activation_context(This, &user_actctx_state); + LIST_FOR_EACH_ENTRY(cursor, &This->events_clients, events_client, entry) { IFileDialogControlEvents *pfdce; @@ -346,6 +419,8 @@ static HRESULT cctrl_event_OnControlActivating(FileDialogImpl *This, } } + deactivate_user_activation_context(&user_actctx_state); + return S_OK; } @@ -2245,15 +2320,26 @@ static INT_PTR CALLBACK itemdlg_dlgproc(HWND hwnd, UINT umessage, WPARAM wparam, static HRESULT create_dialog(FileDialogImpl *This, HWND parent) { INT_PTR res; + actctx_state comdlg32_actctx_state = {0}; if (This->dlg_hwnd) return E_UNEXPECTED; + if (!GetCurrentActCtx(&This->user_actctx)) + ERR("Failed to get current activation context, last error %lx\n", GetLastError()); + + if (!(comdlg32_actctx_state.is_active = ActivateActCtx(COMDLG32_hActCtx, &comdlg32_actctx_state.cookie))) + ERR("Failed to activate activation context, last error %lx\n", GetLastError()); + SetLastError(0); res = DialogBoxParamW(COMDLG32_hInstance, MAKEINTRESOURCEW(NEWFILEOPENV3ORD), parent, itemdlg_dlgproc, (LPARAM)This); This->dlg_hwnd = NULL; + + if (comdlg32_actctx_state.is_active && !DeactivateActCtx(0, comdlg32_actctx_state.cookie)) + ERR("Failed to deactivate activation context, last error %lx\n", GetLastError()); + if(res == -1) { ERR("Failed to show dialog (LastError: %ld)\n", GetLastError()); @@ -4650,6 +4736,8 @@ static HRESULT FileDialog_constructor(IUnknown *pUnkOuter, REFIID riid, void **p return E_FAIL; } + fdimpl->user_actctx = INVALID_HANDLE_VALUE; + hr = IFileDialog2_QueryInterface(&fdimpl->IFileDialog2_iface, riid, ppv); IFileDialog2_Release(&fdimpl->IFileDialog2_iface); return hr; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2068
On Wed Jun 7 18:59:26 2023 +0000, Vladislav Timonin wrote:
changed this line in [version 5 of the diff](/wine/wine/-/merge_requests/2068/diffs?diff_id=50645&start_sha=01a6f633c739eb4eaf6deba74fade11dec4fffee#941712bdad220aba7d6798d478d2fe9a0ed5c6d0_2341_2340) The original thinking was probably to make it more readable like this, in retrospective, && is as good I guess. Changed it.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2068#note_34987
On Tue Jun 6 23:57:31 2023 +0000, **** wrote:
Marvin replied on the mailing list: ``` Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=133521 Your paranoid android. === w7pro64 (64 bit report) === comdlg32: 0720:itemdlg: unhandled exception c0000005 at 000007FEFD1E90A9 ``` Can't reproduce locally, and didn't seem to fail again on next run (https://testbot.winehq.org/JobDetails.pl?Key=133552), some existing test randomly failing somewhere?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2068#note_34988
This merge request was approved by Esme Povirk. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2068
Jinoh Kang (@iamahuman) commented about dlls/comdlg32/itemdlg.c:
static HRESULT create_dialog(FileDialogImpl *This, HWND parent) { INT_PTR res; + actctx_state comdlg32_actctx_state = {0}; + + if (This->dlg_hwnd) + return E_UNEXPECTED; + + if (!GetCurrentActCtx(&This->user_actctx)) + ERR("Failed to get current activation context, last error %lx\n", GetLastError()); There is a memory leak. `GetCurrentActCtx` outputs a reference-counted handle that shall be freed later with [`ReleaseActCtx`].
Also, note that `GetCurrentActCtx` is not supposed to output `INVALID_HANDLE_VALUE` on failure. [`ReleaseActCtx`]: https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-relea... ```suggestion:-1+0 if (!GetCurrentActCtx(&This->user_actctx)) { ERR("Failed to get current activation context, last error %lx\n", GetLastError()); This->user_actctx = INVALID_HANDLE_VALUE; } ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2068#note_36113
Jinoh Kang (@iamahuman) commented about dlls/comdlg32/itemdlg.c:
+ + if (!GetCurrentActCtx(&This->user_actctx)) + ERR("Failed to get current activation context, last error %lx\n", GetLastError()); + + if (!(comdlg32_actctx_state.is_active = ActivateActCtx(COMDLG32_hActCtx, &comdlg32_actctx_state.cookie))) + ERR("Failed to activate activation context, last error %lx\n", GetLastError());
SetLastError(0); res = DialogBoxParamW(COMDLG32_hInstance, MAKEINTRESOURCEW(NEWFILEOPENV3ORD), parent, itemdlg_dlgproc, (LPARAM)This); This->dlg_hwnd = NULL; + + if (comdlg32_actctx_state.is_active && !DeactivateActCtx(0, comdlg32_actctx_state.cookie)) + ERR("Failed to deactivate activation context, last error %lx\n", GetLastError()); + You can add ReleaseActCtx() call here.
```suggestion:-0+0 if (This->user_actctx != INVALID_HANDLE_VALUE) ReleaseActCtx(This->user_actctx); ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2068#note_36114
After fixing a small memory leak issue, I think this is good to go. Thank you @vt for your dedication! -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2068#note_36115
participants (4)
-
Esme Povirk (＠madewokherd) -
Jinoh Kang (＠iamahuman)
-
Vladislav Timonin
-
Vladislav Timonin (＠vt)