[PATCH v9 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. -- v9: comdlg32: Enable visual styles when showing IFileDialog. comdlg32: Return E_UNEXPECTED if IFileDialog is already shown. 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 1feb1deda00..b483193d76b 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 | 97 +++++++++++++++++++++++++++++++++ 5 files changed, 132 insertions(+), 1 deletion(-) create mode 100644 dlls/comdlg32/comdlg32.manifest diff --git a/dlls/comdlg32/cdlg.h b/dlls/comdlg32/cdlg.h index 8039d6d6bd0..ae3c3d1ef37 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 b9e19b78a34..19f4333acf5 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; @@ -59,13 +60,26 @@ BOOL WINAPI DllMain(HINSTANCE hInstance, DWORD Reason, LPVOID Reserved) switch(Reason) { case DLL_PROCESS_ATTACH: + { + ACTCTXW actctx = {0}; + COMDLG32_hInstance = hInstance; DisableThreadLibraryCalls(hInstance); - 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); + if (COMDLG32_hActCtx != INVALID_HANDLE_VALUE) 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 b483193d76b..ee9e58e8179 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,35 @@ 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()); + This->user_actctx = INVALID_HANDLE_VALUE; + } + + 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 (This->user_actctx != INVALID_HANDLE_VALUE) + { + ReleaseActCtx(This->user_actctx); + This->user_actctx = INVALID_HANDLE_VALUE; + } + if(res == -1) { ERR("Failed to show dialog (LastError: %ld)\n", GetLastError()); @@ -4650,6 +4745,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
Added missing period in message of first commit, and rebase while I'm at it. Also tried looking into the test crash. Out of 16 test bot runs, [only 1 failed](https://testbot.winehq.org/JobDetails.pl?Key=134116&f301=exe64.report#k301). Thankfully, with a slightly useful log: ```log comdlg32:itemdlg start - itemdlg.c:346: test_instantiation itemdlg.c:531: test_instantiation end itemdlg.c:561: test_basics itemdlg.c:1050: test_basics end itemdlg.c:1153: Testing FileOpenDialog (advise) itemdlg.c:1161: Testing FileSaveDialog (advise) itemdlg.c:1169: test_advise end itemdlg.c:1213: test_events itemdlg.c:1334: test_events end itemdlg.c:1531: test_filename 0894:itemdlg: unhandled exception c0000005 at 000007FEFD1E90A9 comdlg32:itemdlg:0894 done (-1073741819) in 4s ``` Unfortunately, still doesn't tell why, things of note here are the crash happened in `test_filename` (at least this specific crash) and exception handler failed to print last seen `winetest_set_location`. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2068#note_36777
On Mon Jun 26 10:29:19 2023 +0000, Vladislav Timonin wrote:
Added missing period in message of first commit, and rebase while I'm at it. Also tried looking into the test crash. Out of 16 test bot runs, [only 1 failed](https://testbot.winehq.org/JobDetails.pl?Key=134116&f301=exe64.report#k301). Thankfully, with a slightly useful log: ```log comdlg32:itemdlg start - itemdlg.c:346: test_instantiation itemdlg.c:531: test_instantiation end itemdlg.c:561: test_basics itemdlg.c:1050: test_basics end itemdlg.c:1153: Testing FileOpenDialog (advise) itemdlg.c:1161: Testing FileSaveDialog (advise) itemdlg.c:1169: test_advise end itemdlg.c:1213: test_events itemdlg.c:1334: test_events end itemdlg.c:1531: test_filename 0894:itemdlg: unhandled exception c0000005 at 000007FEFD1E90A9 comdlg32:itemdlg:0894 done (-1073741819) in 4s ``` Unfortunately, still doesn't tell why, things of note here are the crash happened in `test_filename` (at least this specific crash) and exception handler failed to print last seen `winetest_set_location`. It is probably unrelated, but shouldn't the WM_TIMER handler return 0 if the timer message was handled by the window proc, instead of TRUE like in tests/itemdlg.c, in both the existing cases and the new case? (Also one case with {} and the other two without {} looks a bit odd in my opinion)
I find MS docs difficult to interpret sometimes, but [this](https://learn.microsoft.com/en-us/windows/win32/winmsg/wm-timer) seems like it is the corresponding documentation, and I've found some example code snippets that return 0:
An application should return zero if it processes this message.
I've used the "WINETEST_REPORT_SUCCESS" checkbox on the testbot to get an idea where crashes occur. I believe it doesn't work when there are lots of successes to report though, I believe the log was cut or something. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2068#note_36781
participants (3)
-
Florian Will (＠w-flo) -
Vladislav Timonin
-
Vladislav Timonin (＠vt)