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.
From: Vladislav Timonin timoninvlad@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");
From: Vladislav Timonin timoninvlad@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;
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.
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?
This merge request was approved by Esme Povirk.
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; } ```
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); ```
After fixing a small memory leak issue, I think this is good to go. Thank you @vt for your dedication!
-
Esme Povirk (＠madewokherd) -
Jinoh Kang (＠iamahuman)
-
Vladislav Timonin
-
Vladislav Timonin (＠vt)