[PATCH 4/9] mshtml: Use builtin hooks even when calling function dispatch objects.
Gabriel Ivăncescu
gabrielopcode at gmail.com
Sun Dec 5 10:54:00 CST 2021
On 04/12/2021 17:45, Jacek Caban wrote:
> 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.
>
Oh I agree about that. My plan was to consolidate all of this into a
single function (e.g. invoke_builtin_function), which does everything:
1) it calls the hook first, if any.
2) then, if it has no vtbl_off, it forwards to typeinfo_invoke.
3) otherwise, it does the things invoke_builtin_function does now.
The problem is that we have an existing test (line 1976 in script.c)
that uses named args on a function dispatch object, so it will fail
(that's because it has a vtbl_off, but now it calls typeinfo_invoke
always). That said, it's really just DISPID_THIS, so I guess it
shouldn't be too hard to handle it.
Worst case, I'll just add a FIXME if we have named args and fallback to
typeinfo_invoke.
>
>>
>>>>>> 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.
>
Yes, I had it that way, but I had to change it to handle hooks. I don't
actually use any object though, so it's not a problem. I just give the
func_info_t as an opaque pointer to jscript, and a function pointer so
that it knows what to call (in ProxyFunction_call) and pass it there.
The function pointer points to a function in mshtml that simply does the
necessary things (typically, just invokes invoke_builtin_function, or
for accessors, builtin_propget/put and the hooks). No actual object is
used, but I still need the hooks patch of course.
More information about the wine-devel
mailing list