[PATCH 0/1] MR6859: mshtml: Copy POST data in BindStatusCallback_GetBindInfo().

[PATCH 1/1] mshtml: Copy POST data in BindStatusCallback_GetBindInfo().

[Paul Gofman]

From: Paul Gofman

---
 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;

[PATCH 1/1] mshtml: Copy POST data in BindStatusCallback_GetBindInfo().

[Marvin]

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.

[Paul Gofman (＠gofman)]

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.

[Jacek Caban (＠jacek)]

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.

[Paul Gofman (＠gofman)]

Yes, it is nsChannel release which crashes in ccref_decr.

[Gabriel Ivăncescu (＠insn)]

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)

[Jacek Caban (＠jacek)]

There are other ways to fix those leaks, do you know the problematic cycle?

[Gabriel Ivăncescu (＠insn)]

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.

[Paul Gofman (＠gofman)]

@insn So I presume you will care about proper revert (for which I don't have much context) and I am closing this?

[Gabriel Ivăncescu (＠insn)]

Yeah I will revert it.

[Paul Gofman (＠gofman)]

Thanks!

[PATCH 0/1] MR6859: mshtml: Copy POST data in BindStatusCallback_GetBindInfo(). - closed

[Paul Gofman (＠gofman)]

This merge request was closed by Paul Gofman.