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

[Gabriel Ivăncescu]

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?

>>   struct dispex_data_t {
>>       dispex_static_data_t *desc;
>> @@ -837,6 +837,11 @@ static HRESULT function_value(DispatchEx *dispex, 
>> LCID lcid, WORD flags, DISPPAR
>>       case DISPATCH_METHOD:
>>           if(!This->obj)
>>               return E_UNEXPECTED;
>> +        if(This->info->hook) {
>> +            hres = This->info->hook(This->obj, This->info, lcid, 
>> flags, params, res, ei, caller);
>> +            if(hres != S_FALSE)
>> +                break;
>> +        }
>>           hres = typeinfo_invoke(This->obj, This->info, flags, params, 
>> res, ei);
>>           break;
> 
> 
> I think you could simply change typeinfo_invoke to function_invoke() here.
> 
> 

We have some named argument tests that will fail if that is used, 
because it will attempt to use invoke_builtin_function which doesn't 
have them implemented (yes, I know it's a workaround but...).

Of course if I use only typeinfo_invoke in function_invoke then this 
issue disappears, but then what's the point of invoke_builtin_function?

>>       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".

Thanks,
Gabriel