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

[Jacek Caban]

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().


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


Thanks,

Jacek