[PATCH 4/9] mshtml: Use builtin hooks even when calling function dispatch objects.

[Gabriel Ivăncescu]

On 03/12/2021 21:19, Jacek Caban wrote:
> On 12/3/21 7:21 PM, Gabriel Ivăncescu wrote:
>> Hi Jacek,
>>
>> Thanks for the review.
>>
>> On 03/12/2021 16:35, Jacek Caban wrote:
>>> Hi Gabriel,
>>>
>>> On 12/3/21 2:57 PM, Gabriel Ivăncescu wrote:
>>>> This fixes a discrepancy between builtins called via a function 
>>>> dispatch
>>>> object and others.
>>>>
>>>> Signed-off-by: Gabriel Ivăncescu <gabrielopcode at gmail.com>
>>>> ---
>>>>   dlls/mshtml/dispex.c              | 37 
>>>> ++++++++++++++++++++-----------
>>>>   dlls/mshtml/htmlelem.c            |  7 +++---
>>>>   dlls/mshtml/htmlevent.c           | 10 ++++-----
>>>>   dlls/mshtml/mshtml_private.h      |  6 +++--
>>>>   dlls/mshtml/tests/documentmode.js | 14 +++++++++++-
>>>>   dlls/mshtml/xmlhttprequest.c      |  5 ++---
>>>>   6 files changed, 50 insertions(+), 29 deletions(-)
>>>> diff --git a/dlls/mshtml/dispex.c b/dlls/mshtml/dispex.c
>>>> index 9f56a56..4ba00f8 100644
>>>> --- a/dlls/mshtml/dispex.c
>>>> +++ b/dlls/mshtml/dispex.c
>>>> @@ -49,7 +49,7 @@ typedef struct {
>>>>       VARIANT default_value;
>>>>   } func_arg_info_t;
>>>> -typedef struct {
>>>> +struct func_info_t {
>>>>       DISPID id;
>>>>       BSTR name;
>>>>       tid_t tid;
>>>> @@ -63,7 +63,7 @@ typedef struct {
>>>>       VARTYPE prop_vt;
>>>>       VARTYPE *arg_types;
>>>>       func_arg_info_t *arg_info;
>>>> -} func_info_t;
>>>> +};
>>>
>>>
>>> This could be kept local to dispex.c.
>>>
>>>
>>
>> How are hooks supposed to call it then? It's opaque outside of 
>> dispex.c and just forwarded in hooks where necessary. Should I change 
>> the argument in hooks to void* then?
> 
> 
> I see, the problematic case is when we call builtin, but the original 
> property was overwritten. Still, I don't think it's the right to call 
> the hook directly in function_value().
> 
> 

Why not? In fact, the hook *should* be called everytime before 
invoke_builtin_function (or typeinfo_invoke) except when it's called 
from the hook itself. It makes sense, since the hook is supposed to 
override *all* calls to the builtin in any other case.

Maybe I can use a helper function instead? Which calls the hook at the 
top and then invoke_builtin_function or typeinfo_invoke.

Tbh I feel like implementing named args for invoke_builtin_function just 
to get rid of this workaround, but I've no idea how difficult it would be.

>>>>       case DISPATCH_PROPERTYGET: {
>>>> @@ -1163,13 +1168,17 @@ static HRESULT builtin_propput(DispatchEx 
>>>> *This, func_info_t *func, DISPPARAMS *
>>>>       return hres;
>>>>   }
>>>> -static HRESULT invoke_builtin_function(DispatchEx *This, 
>>>> func_info_t *func, DISPPARAMS *dp, VARIANT *res, IServiceProvider 
>>>> *caller)
>>>> +HRESULT invoke_builtin_function(DispatchEx *This, func_info_t 
>>>> *func, WORD flags, DISPPARAMS *dp, VARIANT *res,
>>>> +        EXCEPINFO *ei, IServiceProvider *caller)
>>>>   {
>>>>       VARIANT arg_buf[MAX_ARGS], *arg_ptrs[MAX_ARGS], *arg, retv, 
>>>> ret_ref, vhres;
>>>>       unsigned i, nconv = 0;
>>>>       IUnknown *iface;
>>>>       HRESULT hres;
>>>> +    if(!func->call_vtbl_off)
>>>> +        return typeinfo_invoke(This, func, flags, dp, res, ei);
>>>> +
>>>>       if(dp->cNamedArgs) {
>>>>           FIXME("Named arguments not supported\n");
>>>>           return E_NOTIMPL;
>>>> @@ -1265,8 +1274,8 @@ static HRESULT 
>>>> invoke_builtin_function(DispatchEx *This, func_info_t *func, DISP
>>>>       return V_ERROR(&vhres);
>>>>   }
>>>> -static HRESULT function_invoke(DispatchEx *This, func_info_t *func, 
>>>> WORD flags, DISPPARAMS *dp, VARIANT *res,
>>>> -        EXCEPINFO *ei, IServiceProvider *caller)
>>>> +static HRESULT function_invoke(DispatchEx *This, func_info_t *func, 
>>>> LCID lcid, WORD flags, DISPPARAMS *dp,
>>>> +        VARIANT *res, EXCEPINFO *ei, IServiceProvider *caller)
>>>>   {
>>>>       HRESULT hres;
>>>> @@ -1296,10 +1305,12 @@ static HRESULT function_invoke(DispatchEx 
>>>> *This, func_info_t *func, WORD flags,
>>>>               }
>>>>           }
>>>> -        if(func->call_vtbl_off)
>>>> -            hres = invoke_builtin_function(This, func, dp, res, 
>>>> caller);
>>>> -        else
>>>> -            hres = typeinfo_invoke(This, func, flags, dp, res, ei);
>>>> +        if(func->hook) {
>>>> +            hres = func->hook(This, func, lcid, flags, dp, res, ei, 
>>>> caller);
>>>> +            if(hres != S_FALSE)
>>>> +                break;
>>>> +        }
>>>> +        hres = invoke_builtin_function(This, func, flags, dp, res, 
>>>> ei, caller);
>>>
>>>
>>> This seems to be a good place to call the hook, but could you just 
>>> keep typeinfo_invoke call here and don't expose invoke_builtin_function?
>>>
>>>
>>
>> But I need to expose it for hooks so they can forward to it. It's 
>> either that or I expose typeinfo_invoke. Keep in mind that, for hooks, 
>> the Dispatch object itself may not exist at all, we can't use it to 
>> look up the function, that's incorrect.
>>
>> That's not the case right now, no, but it will be when I'll either 
>> implement function.apply/call (for all modes) or the proxy jscript 
>> implementation, in further patches.
>>
>> Here's a future example:
>>
>> f = elem.setAttribute;
>> f.call(some_other_obj, "a", blah);
>>
>> The hook needs to run on some_other_obj and stringify blah 
>> appropriately before calling the builtin function on some_other_obj. 
>> In fact, we won't have a DispatchEx at all, just a generic IDispatch 
>> with "some_other_obj". 
> 
> 
> I hoped that the result of your 'proxy' patches will be that those 
> functions will be true JavaScript objects. It means that MSHTML's 
> function objects will not be relevant in ES5 mode. Why do you still need 
> them?
>  >

Yes, that's true, but I need a way to encapsulate a builtin function 
(including its hook) into a jscript function. Currently, I create a 
ProxyFunction that holds the func_info opaque pointer and an internal 
interface pointer that it uses to call into mshtml.

mshtml then uses this entry point to call the hook first, passing it the 
func_info and the 'this' dispatch object, and if no hook reverts back to 
invoke_builtin_function. Of course the hook also has to use 
invoke_builtin_function at some point, after massaging the args.

Either way, the point is that the hook *has* to be called on an 
arbitrary object, not the original dispatch it's from, so that's why I'm 
passing func_info_t to it, which is needed to be forwarded into 
invoke_builtin_function (or another wrapper).