[PATCH v5 0/5] MR6375: mshtml: Don't process tasks recursively.
I kept the first patch (`Remember if timer was blocked.`) to avoid SetTimer on timers that weren't killed, mostly because it would apply on every task (which is likely to execute way faster than the ms granularity), not sure if it would skew the timer in such case. The other patches are split, with tests now showing why they're needed. Some places where wine-gecko callbacks into us and in which we call into external code need to be treated the same as a recursive task since it can end up in a message loop. I haven't supplied all of them from the previous MR, mostly because I didn't find a way to test those (e.g. show context menu, and the gecko async navigation). -- v5: mshtml: Don't process tasks recursively from Gecko events. mshtml: Don't process tasks recursively from script runners. mshtml: Don't process tasks recursively. mshtml: Remember if timer was blocked. mshtml: Don't cast to int to bound the timer. https://gitlab.winehq.org/wine/wine/-/merge_requests/6375
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/task.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/mshtml/task.c b/dlls/mshtml/task.c index 49f76d101a9..c1b524f9892 100644 --- a/dlls/mshtml/task.c +++ b/dlls/mshtml/task.c @@ -132,7 +132,7 @@ void remove_target_tasks(LONG target) DWORD tc = GetTickCount(); timer = LIST_ENTRY(list_head(&thread_data->timer_list), task_timer_t, entry); - SetTimer(thread_data->thread_hwnd, TIMER_ID, max( (int)(timer->time - tc), 0 ), NULL); + SetTimer(thread_data->thread_hwnd, TIMER_ID, timer->time > tc ? timer->time - tc : 0, NULL); } LIST_FOR_EACH_SAFE(liter, ltmp, &thread_data->task_list) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6375
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/mshtml_private.h | 1 + dlls/mshtml/task.c | 33 +++++++++++++++++++++++++-------- 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/dlls/mshtml/mshtml_private.h b/dlls/mshtml/mshtml_private.h index d97a206b215..582cc109d31 100644 --- a/dlls/mshtml/mshtml_private.h +++ b/dlls/mshtml/mshtml_private.h @@ -1493,6 +1493,7 @@ typedef struct { struct list *pending_xhr_events_tail; struct wine_rb_tree session_storage_map; void *blocking_xhr; + BOOL timer_blocked; } thread_data_t; thread_data_t *get_thread_data(BOOL); diff --git a/dlls/mshtml/task.c b/dlls/mshtml/task.c index c1b524f9892..245f8771ab2 100644 --- a/dlls/mshtml/task.c +++ b/dlls/mshtml/task.c @@ -112,6 +112,20 @@ static void release_task_timer(HWND thread_hwnd, task_timer_t *timer) free(timer); } +static void unblock_timers(thread_data_t *thread_data) +{ + if(!thread_data->timer_blocked) + return; + thread_data->timer_blocked = FALSE; + + if(!list_empty(&thread_data->timer_list)) { + task_timer_t *timer = LIST_ENTRY(list_head(&thread_data->timer_list), task_timer_t, entry); + DWORD tc = GetTickCount(); + + SetTimer(thread_data->thread_hwnd, TIMER_ID, timer->time > tc ? timer->time - tc : 0, NULL); + } +} + void remove_target_tasks(LONG target) { thread_data_t *thread_data = get_thread_data(FALSE); @@ -212,8 +226,12 @@ HRESULT set_task_timer(HTMLInnerWindow *window, LONG msec, enum timer_type timer IDispatch_AddRef(disp); timer->disp = disp; - if(queue_timer(thread_data, timer)) - SetTimer(thread_data->thread_hwnd, TIMER_ID, msec, NULL); + if(queue_timer(thread_data, timer)) { + if(thread_data->blocking_xhr) + thread_data->timer_blocked = TRUE; + else + SetTimer(thread_data->thread_hwnd, TIMER_ID, msec, NULL); + } *id = timer->id; return S_OK; @@ -311,6 +329,8 @@ static LRESULT process_timer(void) assert(thread_data != NULL); if(list_empty(&thread_data->timer_list) || thread_data->blocking_xhr) { + if(!list_empty(&thread_data->timer_list)) + thread_data->timer_blocked = TRUE; KillTimer(thread_data->thread_hwnd, TIMER_ID); return 0; } @@ -347,6 +367,8 @@ static LRESULT process_timer(void) IDispatch_Release(disp); }while(!list_empty(&thread_data->timer_list) && !thread_data->blocking_xhr); + if(!list_empty(&thread_data->timer_list)) + thread_data->timer_blocked = TRUE; KillTimer(thread_data->thread_hwnd, TIMER_ID); return 0; } @@ -484,10 +506,5 @@ void unblock_tasks_and_timers(thread_data_t *thread_data) if(!list_empty(&thread_data->event_task_list)) PostMessageW(thread_data->thread_hwnd, WM_PROCESSTASK, 0, 0); - if(!thread_data->blocking_xhr && !list_empty(&thread_data->timer_list)) { - task_timer_t *timer = LIST_ENTRY(list_head(&thread_data->timer_list), task_timer_t, entry); - DWORD tc = GetTickCount(); - - SetTimer(thread_data->thread_hwnd, TIMER_ID, timer->time > tc ? timer->time - tc : 0, NULL); - } + unblock_timers(thread_data); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6375
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/mshtml_private.h | 1 + dlls/mshtml/task.c | 25 ++++++++++++++++++------- dlls/mshtml/xmlhttprequest.c | 14 +++++++++++++- 3 files changed, 32 insertions(+), 8 deletions(-) diff --git a/dlls/mshtml/mshtml_private.h b/dlls/mshtml/mshtml_private.h index 582cc109d31..178abc9a559 100644 --- a/dlls/mshtml/mshtml_private.h +++ b/dlls/mshtml/mshtml_private.h @@ -1493,6 +1493,7 @@ typedef struct { struct list *pending_xhr_events_tail; struct wine_rb_tree session_storage_map; void *blocking_xhr; + unsigned tasks_locked; BOOL timer_blocked; } thread_data_t; diff --git a/dlls/mshtml/task.c b/dlls/mshtml/task.c index 245f8771ab2..955567eaabb 100644 --- a/dlls/mshtml/task.c +++ b/dlls/mshtml/task.c @@ -142,7 +142,7 @@ void remove_target_tasks(LONG target) release_task_timer(thread_data->thread_hwnd, timer); } - if(!list_empty(&thread_data->timer_list)) { + if(!list_empty(&thread_data->timer_list) && !thread_data->tasks_locked && !thread_data->blocking_xhr) { DWORD tc = GetTickCount(); timer = LIST_ENTRY(list_head(&thread_data->timer_list), task_timer_t, entry); @@ -227,7 +227,7 @@ HRESULT set_task_timer(HTMLInnerWindow *window, LONG msec, enum timer_type timer timer->disp = disp; if(queue_timer(thread_data, timer)) { - if(thread_data->blocking_xhr) + if(thread_data->tasks_locked || thread_data->blocking_xhr) thread_data->timer_blocked = TRUE; else SetTimer(thread_data->thread_hwnd, TIMER_ID, msec, NULL); @@ -328,27 +328,28 @@ static LRESULT process_timer(void) thread_data = get_thread_data(FALSE); assert(thread_data != NULL); - if(list_empty(&thread_data->timer_list) || thread_data->blocking_xhr) { + if(list_empty(&thread_data->timer_list) || thread_data->tasks_locked || thread_data->blocking_xhr) { if(!list_empty(&thread_data->timer_list)) thread_data->timer_blocked = TRUE; KillTimer(thread_data->thread_hwnd, TIMER_ID); return 0; } + thread_data->tasks_locked++; last_timer = LIST_ENTRY(list_tail(&thread_data->timer_list), task_timer_t, entry); do { tc = GetTickCount(); if(timer == last_timer) { timer = LIST_ENTRY(list_head(&thread_data->timer_list), task_timer_t, entry); SetTimer(thread_data->thread_hwnd, TIMER_ID, timer->time>tc ? timer->time-tc : 0, NULL); - return 0; + goto done; } timer = LIST_ENTRY(list_head(&thread_data->timer_list), task_timer_t, entry); if(timer->time > tc) { SetTimer(thread_data->thread_hwnd, TIMER_ID, timer->time-tc, NULL); - return 0; + goto done; } disp = timer->disp; @@ -370,6 +371,10 @@ static LRESULT process_timer(void) if(!list_empty(&thread_data->timer_list)) thread_data->timer_blocked = TRUE; KillTimer(thread_data->thread_hwnd, TIMER_ID); + +done: + if(!--thread_data->tasks_locked && (!list_empty(&thread_data->task_list) || !list_empty(&thread_data->event_task_list))) + PostMessageW(thread_data->thread_hwnd, WM_PROCESSTASK, 0, 0); return 0; } @@ -383,13 +388,16 @@ static LRESULT WINAPI hidden_proc(HWND hwnd, UINT msg, WPARAM wParam, LPARAM lPa if(!thread_data) return 0; - while(1) { + while(!thread_data->tasks_locked) { struct list *head = list_head(&thread_data->task_list); if(head) { task_t *task = LIST_ENTRY(head, task_t, entry); list_remove(&task->entry); + thread_data->tasks_locked++; task->proc(task); + if(!--thread_data->tasks_locked) + unblock_timers(thread_data); task->destr(task); free(task); continue; @@ -401,7 +409,10 @@ static LRESULT WINAPI hidden_proc(HWND hwnd, UINT msg, WPARAM wParam, LPARAM lPa if((!task->thread_blocked || !thread_data->blocking_xhr) && !task->window->blocking_depth) { unlink_event_task(task, thread_data); + thread_data->tasks_locked++; task->proc(task); + if(!--thread_data->tasks_locked) + unblock_timers(thread_data); release_event_task(task); break; } @@ -503,7 +514,7 @@ ULONGLONG get_time_stamp(void) void unblock_tasks_and_timers(thread_data_t *thread_data) { - if(!list_empty(&thread_data->event_task_list)) + if(!list_empty(&thread_data->task_list) || !list_empty(&thread_data->event_task_list)) PostMessageW(thread_data->thread_hwnd, WM_PROCESSTASK, 0, 0); unblock_timers(thread_data); diff --git a/dlls/mshtml/xmlhttprequest.c b/dlls/mshtml/xmlhttprequest.c index fffc9fab51b..c2730d4f7cd 100644 --- a/dlls/mshtml/xmlhttprequest.c +++ b/dlls/mshtml/xmlhttprequest.c @@ -269,10 +269,12 @@ static nsresult sync_xhr_send(HTMLXMLHttpRequest *xhr, nsIVariant *nsbody) thread_data_t *thread_data = get_thread_data(TRUE); HTMLXMLHttpRequest *prev_blocking_xhr; HTMLInnerWindow *window = xhr->window; + unsigned prev_tasks_locked; nsresult nsres; if(!thread_data) return NS_ERROR_OUT_OF_MEMORY; + prev_tasks_locked = thread_data->tasks_locked; prev_blocking_xhr = thread_data->blocking_xhr; /* Note: Starting with Gecko 30.0 (Firefox 30.0 / Thunderbird 30.0 / SeaMonkey 2.27), @@ -297,15 +299,25 @@ static nsresult sync_xhr_send(HTMLXMLHttpRequest *xhr, nsIVariant *nsbody) * to figure out a way to handle it... * * For details (and bunch of problems to consider) see: https://bugzil.la/697151 + * + * FIXME: Since Gecko uses a message loop to implement sync XHR, and it requires that + * all the async tasks are executed (or else it hangs indefinitely waiting for them), + * we have to enable processing of all tasks, even if we're coming from a nested loop + * that wouldn't otherwise process tasks. This isn't correct but it's niche enough. */ + if(thread_data->tasks_locked) { + thread_data->tasks_locked = 0; + unblock_tasks_and_timers(thread_data); + } window->base.outer_window->readystate_locked++; window->blocking_depth++; thread_data->blocking_xhr = xhr; nsres = nsIXMLHttpRequest_Send(xhr->nsxhr, nsbody); thread_data->blocking_xhr = prev_blocking_xhr; + thread_data->tasks_locked = prev_tasks_locked; window->base.outer_window->readystate_locked--; - if(!--window->blocking_depth) + if(!--window->blocking_depth && !thread_data->tasks_locked) unblock_tasks_and_timers(thread_data); /* Process any pending events now since they were part of the blocked send() above */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6375
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> It's a wine-gecko callback that can lead to a message loop (calling into external code). Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/mshtml_private.h | 14 +++++ dlls/mshtml/mutation.c | 8 ++- dlls/mshtml/tests/events.c | 109 +++++++++++++++++++++++++++++++++-- 3 files changed, 125 insertions(+), 6 deletions(-) diff --git a/dlls/mshtml/mshtml_private.h b/dlls/mshtml/mshtml_private.h index 178abc9a559..c2c1d933b84 100644 --- a/dlls/mshtml/mshtml_private.h +++ b/dlls/mshtml/mshtml_private.h @@ -1671,6 +1671,20 @@ static inline void traverse_variant(VARIANT *v, const char *name, nsCycleCollect note_cc_edge((nsISupports*)V_UNKNOWN(v), name, cb); } +static inline thread_data_t *block_task_processing(void) +{ + thread_data_t *thread_data = get_thread_data(FALSE); + + if(thread_data) thread_data->tasks_locked++; + return thread_data; +} + +static inline void unblock_task_processing(thread_data_t *thread_data) +{ + if(!--thread_data->tasks_locked) + unblock_tasks_and_timers(thread_data); +} + #ifdef __i386__ extern void *call_thiscall_func; #endif diff --git a/dlls/mshtml/mutation.c b/dlls/mshtml/mutation.c index d8c0cb50b10..d0041b164a4 100644 --- a/dlls/mshtml/mutation.c +++ b/dlls/mshtml/mutation.c @@ -685,8 +685,14 @@ static nsrefcnt NSAPI nsRunnable_Release(nsIRunnable *iface) static nsresult NSAPI nsRunnable_Run(nsIRunnable *iface) { nsRunnable *This = impl_from_nsIRunnable(iface); + thread_data_t *thread_data; + nsresult nsres; - return This->proc(This->doc, This->arg1, This->arg2); + if(!(thread_data = block_task_processing())) + return NS_ERROR_OUT_OF_MEMORY; + nsres = This->proc(This->doc, This->arg1, This->arg2); + unblock_task_processing(thread_data); + return nsres; } static const nsIRunnableVtbl nsRunnableVtbl = { diff --git a/dlls/mshtml/tests/events.c b/dlls/mshtml/tests/events.c index 2cf63f06dd3..f3d590b4a40 100644 --- a/dlls/mshtml/tests/events.c +++ b/dlls/mshtml/tests/events.c @@ -133,6 +133,7 @@ static IOleDocumentView *view; static BOOL is_ie9plus; static int document_mode; static unsigned in_fire_event; +static DWORD main_thread_id; typedef struct { LONG x; @@ -4863,6 +4864,46 @@ static HRESULT WINAPI sync_xhr(IDispatchEx *iface, DISPID id, LCID lcid, WORD wF EVENT_HANDLER_FUNC_OBJ(sync_xhr); +static IHTMLDocument2 *notif_doc; +static unsigned in_nav_notif_test, nav_notif_test_depth; +static BOOL doc_complete; + +static void nav_notif_test(void) +{ + IHTMLPrivateWindow *priv_window; + IHTMLWindow2 *window; + BSTR bstr, bstr2; + HRESULT hres; + VARIANT v; + + in_nav_notif_test++; + nav_notif_test_depth++; + + hres = IHTMLDocument2_get_parentWindow(notif_doc, &window); + ok(hres == S_OK, "get_parentWindow failed: %08lx\n", hres); + ok(window != NULL, "window == NULL\n"); + + V_VT(&v) = VT_EMPTY; + bstr = SysAllocString(L"about:blank"); + bstr2 = SysAllocString(L""); + hres = IHTMLWindow2_QueryInterface(window, &IID_IHTMLPrivateWindow, (void**)&priv_window); + ok(hres == S_OK, "Could not get IHTMLPrivateWindow) interface: %08lx\n", hres); + hres = IHTMLPrivateWindow_SuperNavigate(priv_window, bstr, bstr2, NULL, NULL, &v, &v, 0); + ok(hres == S_OK, "SuperNavigate failed: %08lx\n", hres); + IHTMLPrivateWindow_Release(priv_window); + IHTMLWindow2_Release(window); + SysFreeString(bstr2); + SysFreeString(bstr); + + ok(nav_notif_test_depth == 1, "nav_notif_test_depth = %u\n", nav_notif_test_depth); + pump_msgs(NULL); + pump_msgs(NULL); + pump_msgs(NULL); + ok(nav_notif_test_depth == 1, "nav_notif_test_depth = %u\n", nav_notif_test_depth); + ok(!doc_complete, "doc_complete = TRUE\n"); + nav_notif_test_depth--; +} + static HRESULT QueryInterface(REFIID,void**); static HRESULT browserservice_qi(REFIID,void**); static HRESULT wb_qi(REFIID,void**); @@ -6195,9 +6236,6 @@ static HRESULT QueryInterface(REFIID riid, void **ppv) return *ppv ? S_OK : E_NOINTERFACE; } -static IHTMLDocument2 *notif_doc; -static BOOL doc_complete; - static HRESULT WINAPI PropertyNotifySink_QueryInterface(IPropertyNotifySink *iface, REFIID riid, void**ppv) { @@ -6222,9 +6260,16 @@ static ULONG WINAPI PropertyNotifySink_Release(IPropertyNotifySink *iface) static HRESULT WINAPI PropertyNotifySink_OnChanged(IPropertyNotifySink *iface, DISPID dispID) { - if(dispID == DISPID_READYSTATE){ - BSTR state; + ok(GetCurrentThreadId() == main_thread_id, "OnChanged called on different thread\n"); + + if(dispID == DISPID_READYSTATE) { HRESULT hres; + BSTR state; + + if(in_nav_notif_test == 11) + nav_notif_test(); + + ok(nav_notif_test_depth < 2, "nav_notif_test_depth = %u\n", nav_notif_test_depth); hres = IHTMLDocument2_get_readyState(notif_doc, &state); ok(hres == S_OK, "get_readyState failed: %08lx\n", hres); @@ -6235,6 +6280,13 @@ static HRESULT WINAPI PropertyNotifySink_OnChanged(IPropertyNotifySink *iface, D SysFreeString(state); } + if(dispID == 1005) { + ok(!nav_notif_test_depth, "nav_notif_test_depth = %u\n", nav_notif_test_depth); + + if(in_nav_notif_test == 1) + nav_notif_test(); + } + return S_OK; } @@ -7445,6 +7497,51 @@ static void test_sync_xhr_events(const char *doc_str) IHTMLDocument2_Release(doc[1]); } +static void test_navigation_during_notif(void) +{ + IPersistMoniker *persist; + IHTMLDocument2 *doc; + IMoniker *mon; + HRESULT hres; + unsigned i; + BSTR url; + MSG msg; + + for(i = 0; i < 2; i++) { + if(!(doc = create_document())) + return; + + notif_doc = doc; + doc_complete = FALSE; + set_client_site(doc, TRUE); + do_advise((IUnknown*)doc, &IID_IPropertyNotifySink, (IUnknown*)&PropertyNotifySink); + + url = SysAllocString(L"about:setting"); + hres = CreateURLMoniker(NULL, url, &mon); + SysFreeString(url); + ok(hres == S_OK, "CreateUrlMoniker failed: %08lx\n", hres); + + hres = IHTMLDocument2_QueryInterface(doc, &IID_IPersistMoniker, (void**)&persist); + ok(hres == S_OK, "Could not get IPersistMoniker iface: %08lx\n", hres); + + hres = IPersistMoniker_Load(persist, FALSE, mon, NULL, 0); + ok(hres == S_OK, "Load failed: %08lx\n", hres); + IPersistMoniker_Release(persist); + IMoniker_Release(mon); + + in_nav_notif_test = i*10 + 1; + while(in_nav_notif_test != i*10 + 2 && !doc_complete && GetMessageA(&msg, NULL, 0, 0)) { + TranslateMessage(&msg); + DispatchMessageA(&msg); + } + ok(!nav_notif_test_depth, "nav_notif_test_depth = %u\n", nav_notif_test_depth); + in_nav_notif_test = 0; + + set_client_site(doc, FALSE); + IHTMLDocument2_Release(doc); + } +} + static BOOL check_ie(void) { IHTMLDocument2 *doc; @@ -7475,6 +7572,7 @@ static BOOL check_ie(void) START_TEST(events) { CoInitialize(NULL); + main_thread_id = GetCurrentThreadId(); if(check_ie()) { container_hwnd = create_container_window(); @@ -7516,6 +7614,7 @@ START_TEST(events) test_storage_events(empty_doc_ie9_str); test_sync_xhr_events(empty_doc_ie9_str); } + test_navigation_during_notif(); /* Test this last since it doesn't close the view properly. */ test_document_close(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6375
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/nsevents.c | 5 ++ dlls/mshtml/tests/events.c | 152 ++++++++++++++++++++++++++++++++++++- 2 files changed, 156 insertions(+), 1 deletion(-) diff --git a/dlls/mshtml/nsevents.c b/dlls/mshtml/nsevents.c index 0604fb997c7..eb172e8c0b9 100644 --- a/dlls/mshtml/nsevents.c +++ b/dlls/mshtml/nsevents.c @@ -145,6 +145,7 @@ static nsresult NSAPI nsDOMEventListener_HandleEvent(nsIDOMEventListener *iface, { nsEventListener *This = impl_from_nsIDOMEventListener(iface); HTMLDocumentNode *doc = This->This->doc; + thread_data_t *thread_data; HTMLInnerWindow *window; nsresult nsres; @@ -153,12 +154,16 @@ static nsresult NSAPI nsDOMEventListener_HandleEvent(nsIDOMEventListener *iface, return NS_ERROR_FAILURE; } + if(!(thread_data = block_task_processing())) + return NS_ERROR_OUT_OF_MEMORY; + /* Hold a ref to the window, as some apps load another document during handlers */ window = doc->window; if(window) IHTMLWindow2_AddRef(&window->base.IHTMLWindow2_iface); nsres = This->handler(doc, event); + unblock_task_processing(thread_data); if(window) IHTMLWindow2_Release(&window->base.IHTMLWindow2_iface); diff --git a/dlls/mshtml/tests/events.c b/dlls/mshtml/tests/events.c index f3d590b4a40..a7d66274e5c 100644 --- a/dlls/mshtml/tests/events.c +++ b/dlls/mshtml/tests/events.c @@ -31,6 +31,7 @@ #include "mshtmdid.h" #include "mshtmhst.h" #include "docobj.h" +#include "docobjectservice.h" #include "hlink.h" #include "wininet.h" #include "shdeprecated.h" @@ -5262,6 +5263,59 @@ static const IOleDocumentSiteVtbl DocumentSiteVtbl = { static IOleDocumentSite DocumentSite = { &DocumentSiteVtbl }; +static HRESULT WINAPI OleCommandTarget_QueryInterface(IOleCommandTarget *iface, REFIID riid, void **ppv) +{ + return QueryInterface(riid, ppv); +} + +static ULONG WINAPI OleCommandTarget_AddRef(IOleCommandTarget *iface) +{ + return 2; +} + +static ULONG WINAPI OleCommandTarget_Release(IOleCommandTarget *iface) +{ + return 1; +} + +static HRESULT WINAPI OleCommandTarget_QueryStatus(IOleCommandTarget *iface, const GUID *pguidCmdGroup, + ULONG cCmds, OLECMD prgCmds[], OLECMDTEXT *pCmdText) +{ + ok(!pguidCmdGroup, "pguidCmdGroup != MULL\n"); + ok(cCmds == 1, "cCmds=%ld, expected 1\n", cCmds); + ok(!pCmdText, "pCmdText != NULL\n"); + + switch(prgCmds[0].cmdID) { + case OLECMDID_SETPROGRESSTEXT: + prgCmds[0].cmdf = OLECMDF_ENABLED; + return S_OK; + case OLECMDID_OPEN: + case OLECMDID_NEW: + prgCmds[0].cmdf = 0; + return S_OK; + default: + ok(0, "unexpected command %ld\n", prgCmds[0].cmdID); + }; + + return E_FAIL; +} + +static HRESULT WINAPI OleCommandTarget_Exec(IOleCommandTarget *iface, const GUID *pguidCmdGroup, DWORD nCmdID, + DWORD nCmdexecopt, VARIANT *pvaIn, VARIANT *pvaOut) +{ + return E_FAIL; +} + +static IOleCommandTargetVtbl OleCommandTargetVtbl = { + OleCommandTarget_QueryInterface, + OleCommandTarget_AddRef, + OleCommandTarget_Release, + OleCommandTarget_QueryStatus, + OleCommandTarget_Exec +}; + +static IOleCommandTarget OleCommandTarget = { &OleCommandTargetVtbl }; + static HRESULT WINAPI TravelLog_QueryInterface(ITravelLog *iface, REFIID riid, void **ppv) { if(IsEqualGUID(&IID_IUnknown, riid) || IsEqualGUID(&IID_ITravelLog, riid)) @@ -5674,12 +5728,104 @@ static const IShellBrowserVtbl ShellBrowserVtbl = { static IShellBrowser ShellBrowser = { &ShellBrowserVtbl }; +static HRESULT WINAPI DocObjectService_QueryInterface(IDocObjectService *iface, REFIID riid, void **ppv) +{ + return browserservice_qi(riid, ppv); +} + +static ULONG WINAPI DocObjectService_AddRef(IDocObjectService *iface) +{ + return 2; +} + +static ULONG WINAPI DocObjectService_Release(IDocObjectService *iface) +{ + return 1; +} + +static HRESULT WINAPI DocObjectService_FireBeforeNavigate2(IDocObjectService *iface, IDispatch *pDispatch, LPCWSTR lpszUrl, DWORD dwFlags, + LPCWSTR lpszFrameName, BYTE *pPostData, DWORD cbPostData, LPCWSTR lpszHeaders, BOOL fPlayNavSound, BOOL *pfCancel) +{ + return S_OK; +} + +static HRESULT WINAPI DocObjectService_FireNavigateComplete2(IDocObjectService *iface, IHTMLWindow2 *pHTMLWindow2, DWORD dwFlags) +{ + return S_OK; +} + +static HRESULT WINAPI DocObjectService_FireDownloadBegin(IDocObjectService *iface) +{ + return S_OK; +} + +static HRESULT WINAPI DocObjectService_FireDownloadComplete(IDocObjectService *iface) +{ + return S_OK; +} + +static HRESULT WINAPI DocObjectService_FireDocumentComplete(IDocObjectService *iface, IHTMLWindow2 *pHTMLWindow, DWORD dwFlags) +{ + ok(!nav_notif_test_depth, "nav_notif_test_depth = %u\n", nav_notif_test_depth); + if(in_nav_notif_test == 21) + nav_notif_test(); + return S_OK; +} + +static HRESULT WINAPI DocObjectService_UpdateDesktopComponent(IDocObjectService *iface, IHTMLWindow2 *pHTMLWindow) +{ + ok(0, "unexpected call\n"); + return E_NOTIMPL; +} + +static HRESULT WINAPI DocObjectService_GetPendingUrl(IDocObjectService *iface, BSTR *pbstrPendingUrl) +{ + return E_NOTIMPL; +} + +static HRESULT WINAPI DocObjectService_ActiveElementChanged(IDocObjectService *iface, IHTMLElement *pHTMLElement) +{ + return E_NOTIMPL; +} + +static HRESULT WINAPI DocObjectService_GetUrlSearchComponent(IDocObjectService *iface, BSTR *pbstrSearch) +{ + ok(0, "unexpected call\n"); + return E_NOTIMPL; +} + +static HRESULT WINAPI DocObjectService_IsErrorUrl(IDocObjectService *iface, LPCWSTR lpszUrl, BOOL *pfIsError) +{ + *pfIsError = FALSE; + return S_OK; +} + +static IDocObjectServiceVtbl DocObjectServiceVtbl = { + DocObjectService_QueryInterface, + DocObjectService_AddRef, + DocObjectService_Release, + DocObjectService_FireBeforeNavigate2, + DocObjectService_FireNavigateComplete2, + DocObjectService_FireDownloadBegin, + DocObjectService_FireDownloadComplete, + DocObjectService_FireDocumentComplete, + DocObjectService_UpdateDesktopComponent, + DocObjectService_GetPendingUrl, + DocObjectService_ActiveElementChanged, + DocObjectService_GetUrlSearchComponent, + DocObjectService_IsErrorUrl +}; + +static IDocObjectService DocObjectService = { &DocObjectServiceVtbl }; + static HRESULT browserservice_qi(REFIID riid, void **ppv) { if(IsEqualGUID(&IID_IShellBrowser, riid)) *ppv = &ShellBrowser; else if(IsEqualGUID(&IID_IBrowserService, riid)) *ppv = &BrowserService; + else if(IsEqualGUID(&IID_IDocObjectService, riid)) + *ppv = &DocObjectService; else { *ppv = NULL; return E_NOINTERFACE; @@ -6230,6 +6376,8 @@ static HRESULT QueryInterface(REFIID riid, void **ppv) *ppv = &DocumentSite; else if(IsEqualGUID(&IID_IOleWindow, riid) || IsEqualGUID(&IID_IOleInPlaceSite, riid)) *ppv = &InPlaceSite; + else if(IsEqualGUID(&IID_IOleCommandTarget , riid)) + *ppv = &OleCommandTarget; else if(IsEqualGUID(&IID_IServiceProvider, riid)) *ppv = &ServiceProvider; @@ -6268,6 +6416,8 @@ static HRESULT WINAPI PropertyNotifySink_OnChanged(IPropertyNotifySink *iface, D if(in_nav_notif_test == 11) nav_notif_test(); + else if(in_nav_notif_test == 21) + return S_OK; ok(nav_notif_test_depth < 2, "nav_notif_test_depth = %u\n", nav_notif_test_depth); @@ -7507,7 +7657,7 @@ static void test_navigation_during_notif(void) BSTR url; MSG msg; - for(i = 0; i < 2; i++) { + for(i = 0; i < 3; i++) { if(!(doc = create_document())) return; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6375
On Thu Sep 12 13:09:48 2024 +0000, Jacek Caban wrote:
I think it would make sense to abstract thread data handling details from places like this or event handler. This could look like: ``` block_task_processing(); This->proc(...); unblock_task_processing(); ``` Maybe we could even use the same helpers in `hidden_proc` itself. I went with a way to return thread_data, mostly so I can deal with failure. If you think it's not important I can change it to your suggestion.
Also I kept the `hidden_proc` same as before since it's already "internal" to it and we only unblock timers, because we're already in WM_PROCESSTASK, no need to post a message (which would happen on almost every task, most by far won't be nested). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6375#note_82025
On Thu Sep 12 16:25:05 2024 +0000, Gabriel Ivăncescu wrote:
Yeah, what I mean is that we're supposed to block tasks (the point of the MR) if we're in a nested message loop. This works fine without sync XHRs. For sync XHRs, Gecko uses a message loop and expects the tasks to be processed (probably the ones you said in `nsio.c`). Now what if a nested message loop (that should block all tasks!) calls sync XHR send() at some point? It has to process the tasks, else Gecko hangs. So, I took the "safe" approach and process all tasks in such case—i.e. behavior as before this MR (I set `tasks_locked` to 0 during sync XHR send, then back to its previous value; nested message loops within sync XHR send will still not get processed and that's fine). Granted, I doubt it will happen in practice, so it's probably good enough, but if it does we can probably revisit it. We should probably keep it simple for now. BTW note that it's more than just the sync XHR io tasks themselves. As I said, even tasks related to prior async XHRs in the xhr.js tests had to be processed for some reason (which have nothing to do with the sync XHR), else it hanged. That's why I gave up trying to "only process tasks related to sync XHR" (IO or otherwise) which was my idea originally that failed, and went with something simpler for such a niche case. For sync XHRs it seems pretty complicated due to gecko internals. For example, my idea was to process tasks at a given "depth level" only, or those pushed on such depth level (which gets changed on a sync XHR, and otherwise is zero). This means only sync XHR tasks get processed in the sync XHR. In theory it sounds fine and correct, but the sync XHR tests fail.
That's because those tests first create two async XHRs and then a sync XHR. It appears that gecko will simply hang if we don't process the async XHR tasks in the sync XHR message loop… So maybe we should just execute all tasks at given depth level or before it—but not after. This won't be completely correct for sync XHRs but I think this is a fringe case that doesn't happen in practice? The async operation would be correct wrt. task nesting, and also sync XHRs keep their current behavior when no nested message loops happen, which is important as well. (currently battling test failures due to an issue I don't understand but yeah) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6375#note_82027
On Thu Sep 12 16:51:28 2024 +0000, Gabriel Ivăncescu wrote:
BTW note that it's more than just the sync XHR io tasks themselves. As I said, even tasks related to prior async XHRs in the xhr.js tests had to be processed for some reason (which have nothing to do with the sync XHR), else it hanged. That's why I gave up trying to "only process tasks related to sync XHR" (IO or otherwise) which was my idea originally that failed, and went with something simpler for such a niche case. Yeah, what I mean is that we're supposed to block tasks (the point of the MR) if we're in a nested message loop. This works fine without sync XHRs. For sync XHRs, Gecko uses a message loop and expects the tasks to be processed (probably the ones you said in `nsio.c`).
Now what if a nested message loop (that should block all tasks!) calls sync XHR send() at some point? It has to process the tasks, else Gecko hangs. So, I took the "safe" approach and process all tasks in such case—i.e. behavior as before this MR (I set `tasks_locked` to 0 during sync XHR send, then back to its previous value; nested message loops within sync XHR send will still not get processed and that's fine). Granted, I doubt it will happen in practice, so it's probably good enough, but if it does we can probably revisit it. We should probably keep it simple for now. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6375#note_82028
On Thu Sep 12 16:51:28 2024 +0000, Gabriel Ivăncescu wrote:
For sync XHRs it seems pretty complicated due to gecko internals. For example, my idea was to process tasks at a given "depth level" only, or those pushed on such depth level (which gets changed on a sync XHR, and otherwise is zero). This means only sync XHR tasks get processed in the sync XHR. In theory it sounds fine and correct, but the sync XHR tests fail. That's because those tests first create two async XHRs and then a sync XHR. It appears that gecko will simply hang if we don't process the async XHR tasks in the sync XHR message loop… So maybe we should just execute all tasks at given depth level or before it—but not after. This won't be completely correct for sync XHRs but I think this is a fringe case that doesn't happen in practice? The async operation would be correct wrt. task nesting, and also sync XHRs keep their current behavior when no nested message loops happen, which is important as well. (currently battling test failures due to an issue I don't understand but yeah) BTW note that it's more than just the sync XHR io tasks themselves. As I said, even tasks related to prior async XHRs in the xhr.js tests had to be processed for some reason (which have nothing to do with the sync XHR), else it hanged. That's why I gave up trying to "only process tasks related to sync XHR" (IO or otherwise) which was my idea originally that failed, and went with something simpler for such a niche case.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6375#note_82029
I went with a way to return thread_data, mostly so I can deal with failure. If you think it's not important I can change it to your suggestion.
I don't think we need to worry about error handling too much here. If we get here, we probably already needed to process some task. Not really related to this MR, but I'd even consider something like creating the thread data early in `create_document_object` and fail there if needed. With that in place, the data would be missing only if we somehow query for it from a wrong thread, which is not supported by MSHTML anyway and surely not the case for Gecko notifications. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6375#note_82117
Jacek Caban (@jacek) commented about dlls/mshtml/tests/events.c:
+ V_VT(&v) = VT_EMPTY; + bstr = SysAllocString(L"about:blank"); + bstr2 = SysAllocString(L""); + hres = IHTMLWindow2_QueryInterface(window, &IID_IHTMLPrivateWindow, (void**)&priv_window); + ok(hres == S_OK, "Could not get IHTMLPrivateWindow) interface: %08lx\n", hres); + hres = IHTMLPrivateWindow_SuperNavigate(priv_window, bstr, bstr2, NULL, NULL, &v, &v, 0); + ok(hres == S_OK, "SuperNavigate failed: %08lx\n", hres); + IHTMLPrivateWindow_Release(priv_window); + IHTMLWindow2_Release(window); + SysFreeString(bstr2); + SysFreeString(bstr); + + ok(nav_notif_test_depth == 1, "nav_notif_test_depth = %u\n", nav_notif_test_depth); + pump_msgs(NULL); + pump_msgs(NULL); + pump_msgs(NULL); This looks weird, a single `pump_msgs` should be enough. I guess you meant some sort of larger timeout here, but a timeout of three `PeekMessage` calls overhead doesn't make much sense.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6375#note_82122
participants (2)
-
Gabriel Ivăncescu -
Jacek Caban (＠jacek)