[PATCH v2 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). -- v2: mshtml: Don't send the notification from SuperNavigate if we're in nested mshtml: Don't process tasks recursively from handle_load. mshtml: Don't process tasks recursively from run_end_load. mshtml: Don't process tasks recursively. mshtml: Remember if timer was blocked. 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> --- Needed for next patch. --- dlls/mshtml/mshtml_private.h | 1 + dlls/mshtml/task.c | 22 ++++++++++++++++------ 2 files changed, 17 insertions(+), 6 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 49f76d101a9..9387a217109 100644 --- a/dlls/mshtml/task.c +++ b/dlls/mshtml/task.c @@ -112,6 +112,17 @@ 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 && !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(); + + thread_data->timer_blocked = FALSE; + 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); @@ -311,6 +322,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 +360,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 +499,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 | 3 ++- dlls/mshtml/task.c | 18 ++++++++++++------ dlls/mshtml/xmlhttprequest.c | 2 +- 3 files changed, 15 insertions(+), 8 deletions(-) diff --git a/dlls/mshtml/mshtml_private.h b/dlls/mshtml/mshtml_private.h index 582cc109d31..e49b2f70245 100644 --- a/dlls/mshtml/mshtml_private.h +++ b/dlls/mshtml/mshtml_private.h @@ -1493,12 +1493,13 @@ 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; thread_data_t *get_thread_data(BOOL); HWND get_thread_hwnd(void); -void unblock_tasks_and_timers(thread_data_t*); +void unblock_tasks_and_timers(thread_data_t*,BOOL); int session_storage_map_cmp(const void*,const struct wine_rb_entry*); void destroy_session_storage(thread_data_t*); diff --git a/dlls/mshtml/task.c b/dlls/mshtml/task.c index 9387a217109..0e3b45f9152 100644 --- a/dlls/mshtml/task.c +++ b/dlls/mshtml/task.c @@ -139,7 +139,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); @@ -223,7 +223,7 @@ HRESULT set_task_timer(HTMLInnerWindow *window, LONG msec, enum timer_type timer IDispatch_AddRef(disp); timer->disp = disp; - if(queue_timer(thread_data, timer)) + if(queue_timer(thread_data, timer) && !thread_data->tasks_locked && !thread_data->blocking_xhr) SetTimer(thread_data->thread_hwnd, TIMER_ID, msec, NULL); *id = timer->id; @@ -321,7 +321,7 @@ 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); @@ -358,7 +358,7 @@ static LRESULT process_timer(void) call_timer_disp(disp, timer_type); IDispatch_Release(disp); - }while(!list_empty(&thread_data->timer_list) && !thread_data->blocking_xhr); + }while(!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; @@ -379,10 +379,16 @@ static LRESULT WINAPI hidden_proc(HWND hwnd, UINT msg, WPARAM wParam, LPARAM lPa while(1) { struct list *head = list_head(&thread_data->task_list); + if(thread_data->tasks_locked) + break; + 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; @@ -494,9 +500,9 @@ ULONGLONG get_time_stamp(void) return (((ULONGLONG)time.dwHighDateTime << 32) + time.dwLowDateTime) / 10000 - time_epoch; } -void unblock_tasks_and_timers(thread_data_t *thread_data) +void unblock_tasks_and_timers(thread_data_t *thread_data, BOOL include_task_list) { - if(!list_empty(&thread_data->event_task_list)) + if(!list_empty(&thread_data->event_task_list) || (include_task_list && !list_empty(&thread_data->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..f7eff984d87 100644 --- a/dlls/mshtml/xmlhttprequest.c +++ b/dlls/mshtml/xmlhttprequest.c @@ -306,7 +306,7 @@ static nsresult sync_xhr_send(HTMLXMLHttpRequest *xhr, nsIVariant *nsbody) window->base.outer_window->readystate_locked--; if(!--window->blocking_depth) - unblock_tasks_and_timers(thread_data); + unblock_tasks_and_timers(thread_data, FALSE); /* Process any pending events now since they were part of the blocked send() above */ synthesize_pending_events(xhr); -- 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/mutation.c | 7 +++ dlls/mshtml/tests/events.c | 109 +++++++++++++++++++++++++++++++++++-- 2 files changed, 111 insertions(+), 5 deletions(-) diff --git a/dlls/mshtml/mutation.c b/dlls/mshtml/mutation.c index d8c0cb50b10..0aa44d2b3a6 100644 --- a/dlls/mshtml/mutation.c +++ b/dlls/mshtml/mutation.c @@ -293,11 +293,16 @@ static nsresult run_end_load(HTMLDocumentNode *This, nsISupports *arg1, nsISuppo { HTMLDocumentObj *doc_obj = This->doc_obj; HTMLInnerWindow *window = This->window; + thread_data_t *thread_data; TRACE("(%p)\n", This); if(!doc_obj) return NS_OK; + + if(!(thread_data = get_thread_data(FALSE))) + return NS_ERROR_OUT_OF_MEMORY; + thread_data->tasks_locked++; IHTMLWindow2_AddRef(&window->base.IHTMLWindow2_iface); if(This == doc_obj->doc_node) { @@ -317,6 +322,8 @@ static nsresult run_end_load(HTMLDocumentNode *This, nsISupports *arg1, nsISuppo set_ready_state(window->base.outer_window, READYSTATE_INTERACTIVE); } IHTMLWindow2_Release(&window->base.IHTMLWindow2_iface); + if(!--thread_data->tasks_locked) + unblock_tasks_and_timers(thread_data, TRUE); return NS_OK; } 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 | 7 ++ dlls/mshtml/tests/events.c | 152 ++++++++++++++++++++++++++++++++++++- 2 files changed, 158 insertions(+), 1 deletion(-) diff --git a/dlls/mshtml/nsevents.c b/dlls/mshtml/nsevents.c index 0604fb997c7..ba6ce056347 100644 --- a/dlls/mshtml/nsevents.c +++ b/dlls/mshtml/nsevents.c @@ -308,12 +308,15 @@ static void handle_docobj_load(HTMLDocumentObj *doc) static nsresult handle_load(HTMLDocumentNode *doc, nsIDOMEvent *event) { + thread_data_t *thread_data = get_thread_data(FALSE); HTMLDocumentObj *doc_obj = NULL; DOMEvent *load_event; HRESULT hres; TRACE("(%p)\n", doc); + if(!thread_data) + return NS_ERROR_OUT_OF_MEMORY; if(!doc->window || !doc->window->base.outer_window) return NS_ERROR_FAILURE; if(doc->doc_obj && doc->doc_obj->doc_node == doc) { @@ -321,6 +324,7 @@ static nsresult handle_load(HTMLDocumentNode *doc, nsIDOMEvent *event) IUnknown_AddRef(doc_obj->outer_unk); } connect_scripts(doc->window); + thread_data->tasks_locked++; if(doc_obj) handle_docobj_load(doc_obj); @@ -344,6 +348,9 @@ static nsresult handle_load(HTMLDocumentNode *doc, nsIDOMEvent *event) IUnknown_Release(doc_obj->outer_unk); } + if(!--thread_data->tasks_locked) + unblock_tasks_and_timers(thread_data, TRUE); + doc->window->load_event_start_time = get_time_stamp(); if(doc->dom_document) { 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
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/navigate.c | 3 ++- dlls/mshtml/tests/events.c | 14 ++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/dlls/mshtml/navigate.c b/dlls/mshtml/navigate.c index 90ca22ea8b1..7e4303a9eb8 100644 --- a/dlls/mshtml/navigate.c +++ b/dlls/mshtml/navigate.c @@ -2243,6 +2243,7 @@ static HRESULT navigate_fragment(HTMLOuterWindow *window, IUri *uri) HRESULT super_navigate(HTMLOuterWindow *window, IUri *uri, DWORD flags, const WCHAR *headers, BYTE *post_data, DWORD post_data_size) { + thread_data_t *thread_data; HTMLDocumentObj *doc_obj; nsChannelBSC *bsc; IUri *uri_nofrag; @@ -2257,7 +2258,7 @@ HRESULT super_navigate(HTMLOuterWindow *window, IUri *uri, DWORD flags, const WC doc_obj = window->browser->doc; IUnknown_AddRef(doc_obj->outer_unk); - if(doc_obj->client && !(flags & BINDING_REFRESH)) { + if(doc_obj->client && !(flags & BINDING_REFRESH) && (thread_data = get_thread_data(FALSE)) && !thread_data->tasks_locked) { IOleCommandTarget *cmdtrg; hres = IOleClientSite_QueryInterface(doc_obj->client, &IID_IOleCommandTarget, (void**)&cmdtrg); diff --git a/dlls/mshtml/tests/events.c b/dlls/mshtml/tests/events.c index a7d66274e5c..823d22e5703 100644 --- a/dlls/mshtml/tests/events.c +++ b/dlls/mshtml/tests/events.c @@ -35,6 +35,7 @@ #include "hlink.h" #include "wininet.h" #include "shdeprecated.h" +#include "shlguid.h" #include "dispex.h" extern const IID IID_IActiveScriptSite; @@ -5303,6 +5304,19 @@ static HRESULT WINAPI OleCommandTarget_QueryStatus(IOleCommandTarget *iface, con static HRESULT WINAPI OleCommandTarget_Exec(IOleCommandTarget *iface, const GUID *pguidCmdGroup, DWORD nCmdID, DWORD nCmdexecopt, VARIANT *pvaIn, VARIANT *pvaOut) { + if(!pguidCmdGroup) + return E_FAIL; + + if(IsEqualGUID(&CGID_ShellDocView, pguidCmdGroup)) { + ok(nCmdexecopt == 0, "nCmdexecopts=%08lx\n", nCmdexecopt); + + switch(nCmdID) { + case 67: + ok(!nav_notif_test_depth, "nav_notif_test_depth = %u\n", nav_notif_test_depth); + break; + } + } + return E_FAIL; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6375
Why do you block only specific bits of `hidden_proc`? Shouldn't we do the same when processing event tasks and timers? What's so special about other functions ran via script runner, shouldn't we do the blocking more generic there by putting it in `nsRunnable_Run`? FWIW, it's fine to skip some parts and it's fine if not every check has its test, but when you add, I'd like to understand the logic behind it. If the logic is that tasks should not be processed recursively (which seems right to me), then I guess we'd want to refactor `hidden_proc` to always block them. Similarly, if `run_end_load` is somehow special, then why? Also `SuperNavigate` looks suspicious to me, are you sure it's the right condition? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6375#note_80866
On Tue Sep 3 16:33:40 2024 +0000, Jacek Caban wrote:
Why do you block only specific bits of `hidden_proc`? Shouldn't we do the same when processing event tasks and timers? What's so special about other functions ran via script runner, shouldn't we do the blocking more generic there by putting it in `nsRunnable_Run`? FWIW, it's fine to skip some parts and it's fine if not every check has its test, but when you add, I'd like to understand the logic behind it. If the logic is that tasks should not be processed recursively (which seems right to me), then I guess we'd want to refactor `hidden_proc` to always block them. Similarly, if `run_end_load` is somehow special, then why? Also `SuperNavigate` looks suspicious to me, are you sure it's the right condition? Oh, for the other functions via script runner, I forgot. I wasn't looking at script runner specifically, I was instead tracking parts where Gecko callbacks ends up calling some external code/notification, and I placed them there. I can move it if you prefer, though it won't really do anything since the others don't have any notifications.
BTW, it's the same with `handle_load`. Do you also want me to move this to HandleEvent there, instead? On second thought, you're right that I should also block during events or timers. But the problem is, that breaks sync XHR tests, since Gecko will enter a (nested) message loop from an event, and then it needs tasks dispatched to do the actual connection. Although honestly that's already a problem if we do that from a non-event notification… so it's already an issue I just unveiled and hadn't considered. It doesn't seem too trivial to fix. We could track `tasks_locked` at the point of a sync XHR Send(), but then it would also process other tasks that were previously blocked, and ideally we'd only process those related to the sync XHR itself. Any ideas how I should proceed? I don't want to waste too much time on trying things that won't be accepted. And lastly for `SuperNavigate`, I just found it out during testing various things, else the new test would fail. I can remove it. Do you have other way to interpret that test that's added in the commit? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6375#note_80907
BTW, it's the same with `handle_load`. Do you also want me to move this to HandleEvent there, instead?
That sounds like an improvement, yes.
Any ideas how I should proceed? I don't want to waste too much time on trying things that won't be accepted.
I don't know, it indeed looks like a complication. We could maybe have a concept of binding tasks, make sure they don't interfere with anything that could end up doing any notifications and process them in such cases. I'm open to better ideas...
And lastly for `SuperNavigate`, I just found it out during testing various things, else the new test would fail. I can remove it. Do you have other way to interpret that test that's added in the commit?
No, I don't have a better interpretation without deeper look. It's probably best to just drop if if it doesn't fix anything. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6375#note_81074
On Wed Sep 4 15:16:26 2024 +0000, Jacek Caban wrote:
BTW, it's the same with `handle_load`. Do you also want me to move this to HandleEvent there, instead? That sounds like an improvement, yes. Any ideas how I should proceed? I don't want to waste too much time on trying things that won't be accepted. I don't know, it indeed looks like a complication. We could maybe have a concept of binding tasks, make sure they don't interfere with anything that could end up doing any notifications and process them in such cases. I'm open to better ideas... And lastly for `SuperNavigate`, I just found it out during testing various things, else the new test would fail. I can remove it. Do you have other way to interpret that test that's added in the commit? No, I don't have a better interpretation without deeper look. It's probably best to just drop if if it doesn't fix anything. This is a repost of an email I sent on Monday that clearly didn't come through (and it isn't even using the wine-gitlab bridge, since I replied to the gitlab notification itself; I wonder when this will get fixed because it's extremely annoying and it's *only* gitlab that has issues).
--- 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_81898
participants (2)
-
Gabriel Ivăncescu -
Jacek Caban (＠jacek)