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