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

[Jacek Caban]

On 12/4/21 12:20 AM, Gabriel Ivăncescu wrote:
> 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.


I agree that the hook should be called somehow, but I don't agree that 
copy&paste is the right way to do it. Structuring code to workaround 
named arguments shortcomings also does not sound convincing.


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


I don't think we need that internal interface pointer.  Did you consider 
something similar to storing a builtin function as (IID,DISPID) like I 
suggested earlier? This would not need any object representing functions 
on MSHTML side.


Jacek