[PATCH 0/1] MR6859: mshtml: Copy POST data in BindStatusCallback_GetBindInfo().
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/mshtml/navigate.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/dlls/mshtml/navigate.c b/dlls/mshtml/navigate.c index 90ca22ea8b1..89950d15482 100644 --- a/dlls/mshtml/navigate.c +++ b/dlls/mshtml/navigate.c @@ -377,9 +377,9 @@ static HRESULT WINAPI BindStatusCallback_GetBindInfo(IBindStatusCallback *iface, pbindinfo->dwBindVerb = BINDVERB_POST; pbindinfo->stgmedData.tymed = TYMED_HGLOBAL; - pbindinfo->stgmedData.hGlobal = This->request_data.post_data; - pbindinfo->stgmedData.pUnkForRelease = (IUnknown*)&This->IBindStatusCallback_iface; - IBindStatusCallback_AddRef(&This->IBindStatusCallback_iface); + if (!(pbindinfo->stgmedData.hGlobal = GlobalAlloc(0, This->request_data.post_data_len))) + return E_OUTOFMEMORY; + memcpy(pbindinfo->stgmedData.hGlobal, This->request_data.post_data, This->request_data.post_data_len); } return S_OK; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6859
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=149792 Your paranoid android. === debian11b (64 bit WoW report) === kernel32: comm.c:1586: Test failed: Unexpected time 1002, expected around 500 user32: win.c:4070: Test failed: Expected active window 0000000005490160, got 0000000000000000. win.c:4071: Test failed: Expected focus window 0000000005490160, got 0000000000000000.
This should avoid crashes in Final Fantasy XIV launcher when pressing login button after entering credentials. The crash happens in xul.dll because ccref_decr() is called from an unrelated thread (thread pool thread used in wininet) while Gecko code expects to find thread local sCollectorData variable in NS_CycleCollectorSuspect3(). The POST request is canceled from Gecko / mshtml resulting in urlmon.HttpProtocol_Terminate() which results in wininet.InternetCloseHandle for request. That eventually results in INTERNEL_HANDLE_CLOSING callback from wininet which is sent from a threadpool thread. The handling in urlmon releases BINDINFO data stored in Protocol (with urlmon.ReleaseBindInfo). For POST requests that currently contains pUnkForRelease which is released inside ole32.ReleaseStgMedium. That is the last reference to mshtml's BindStatusCallback and destroying it results in xul ccref_decr call from a random thread. I checked that: - wininet behaves the same as Windows in critical aspect here: INTERNEL_HANDLE_CLOSING is sent from another thread on Windows as well when the handle is closed during active request processing. There is the difference, Windows aborts the requests, delivers appropriate status code and calls INTERNEL_HANDLE_CLOSING much sooner, but still on a different thread and after InternetCloseHandle exited; - urlmon behaves the same in principle: I instrumented tests with setting pUnkForRelease in test's BindInfo, and if terminating a just started request that Release is called on another thread and after return from _Terminate (even if much sooner than with Wine where wininet requests are currently not aborted). So that looks like mshtml + Gecko issue to me and avoiding referencing BindStatusCallback from BINDATA avoids it (as the last reference is never freed from random wininet threadpool threads). The copied data are freed by ole32.ReleaseStgMedium() now when pUnkForRelease is NULL. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6859#note_88118
Is it about `BindStatusCallback` releasing `nsChannel`? It seems that Gecko's channel implementations don't use CC and have thread-safe ref counting, so we should probably do the same. That would essentially mean reverting 668d8afd741b. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6859#note_88167
Yes, it is nsChannel release which crashes in ccref_decr. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6859#note_88169
Unfortunately reverting that causes leaks. Even the "activex" test leaks at least one nsChannel with it reverted. You will need [my hack attached here](/uploads/4beb8b01bab1de73557b6eac420f150c/leak-tests-hack.diff) to flush all links to be able to test this properly. (this hack is not safe for general use, but it is OK for testing leaks in mshtml tests) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6859#note_88171
There are other ways to fix those leaks, do you know the problematic cycle? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6859#note_88172
Nevermind, I didn't revert it correctly, because I only released what was in `unlink` instead of `delete_cycle_collectable`. It seems there's no leaks now from what I can see (I couldn't test script.c though as I didn't rebase the CC integration yet), so I guess it should be reverted indeed. Sorry about that. EDIT: no "extra" leaks compared to the CC-ref'd version, there's still some weird nsChannel/nsURI leaks but that applies even with the current patch so it's not related. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6859#note_88173
@insn So I presume you will care about proper revert (for which I don't have much context) and I am closing this? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6859#note_88174
Yeah I will revert it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6859#note_88175
This merge request was closed by Paul Gofman. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6859
participants (5)
-
Gabriel Ivăncescu -
Jacek Caban (＠jacek)
-
Marvin -
Paul Gofman
-
Paul Gofman (＠gofman)