[PATCH 4/9] mshtml: Use builtin hooks even when calling function dispatch objects.
Gabriel Ivăncescu
gabrielopcode at gmail.com
Fri Dec 3 17:20:39 CST 2021
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).
More information about the wine-devel
mailing list