[PATCH v2] urlmon: Always report available data before stop_binding.

[Jacek Caban]

On 1/5/22 16:49, Jacek Caban wrote:
> On 1/5/22 08:22, Rémi Bernon wrote:
>> On 1/4/22 23:23, Jacek Caban wrote:
>>> Hi Rémi,
>>>
>>> On 1/4/22 21:31, Rémi Bernon wrote:
>>>> The HTML launcher of several Rebellion games (all probably based on 
>>>> the
>>>> same code) use a custom protocol handler which calls ReportResult in
>>>> LockRequest, causing an invalid memory access when OnDataAvailable
>>>> callback is called, as the binding has already been stopped and
>>>> terminated.
>>>>
>>>> Wine-Bug:https://bugs.winehq.org/show_bug.cgi?id=46213
>>>> Wine-Bug:https://bugs.winehq.org/show_bug.cgi?id=52286
>>>> Signed-off-by: Rémi Bernon<rbernon at codeweavers.com>
>>>> ---
>>>>
>>>> v2: Use an even more ad-hoc flag to detect LockRequest calls during
>>>>      ReportData.
>>>>
>>>>   dlls/mshtml/tests/htmldoc.c | 18 +++++++++++++++++-
>>>>   dlls/urlmon/binding.c       | 23 +++++++++++++++++++++++
>>>>   2 files changed, 40 insertions(+), 1 deletion(-)
>>>
>>>
>>> Those tests in htmldoc.c are a few layers above the problem, so they 
>>> are not very informative. I did a quick experiment (see the 
>>> attachment) with more precise test and an ad-hoc fix. It still needs 
>>> better understanding, but does it help in your case?
>>>
>>>
>>> Thanks,
>>>
>>> Jacek
>>
>> Hi Jacek,
>>
>> I must admit that I have very little idea how all this urlmon thing 
>> works or does. Your patch only partially fix the launcher issue (it 
>> doesn't crash anymore but it still doesn't show any window).
>>
>> As far as I understand, the custom protocol handler is:
>>
>> 1) Calling ReportResult within LockRequest.
>> 2) Expecting OnDataAvailable to be called after.
>> 3) Expecting OnStopBinding to be called after.
>>
>> I believe your patch doesn't do 3)? I'm not sure it expects this to 
>> happen while ReportData is called, but it's how it usually happens, 
>> and it definitely expect OnStopBinding to be called after 
>> ReportResult has been, to continue.
>
>
> Sure, but I suspect that it should be handled one layer bellow what 
> your patch does. bindprot.c is between Binding object and protocol 
> itself and it already has some tools to do deal with such cases. For 
> example, the attached patch fixes Zombie Army Trilogy and is close to 
> what I'd expect. I'd like to do some more tests to understand how 
> exactly it is supposed to work.
>
>
>>
>> It also causes ReportResult to return E_FAIL when called within 
>> LockRequest, which is not what native does (at least it's not with 
>> the mshtml version of the test). 
>
>
> That matched what my test showed, but it seems that it fails on some 
> Windows, so there was probably a problem with the test itself.


A better test confirmed my theory. I sent a series that fixes the 
problem for me.


Thanks,

Jacek