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

[Gabriel Ivăncescu]

On 07/12/2021 14:28, Jacek Caban wrote:
> On 12/6/21 4:40 PM, Gabriel Ivăncescu wrote:
>> On 06/12/2021 16:48, Jacek Caban wrote:
>>> On 12/5/21 5:54 PM, Gabriel Ivăncescu wrote:
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 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. 
>>>
>>>
>>> I still don't see why you need that. IID is enough to make sure that 
>>> we're trying to call the function on the right object type. Once we 
>>> know that the object type is right, DISPID already uniquely 
>>> identifies func_info_t, which MSHTML may get as needed when the 
>>> function is called.
>>>
>>>
>>
>> Well it's simpler to pass an opaque pointer that includes all the 
>> information rather than store two separate pieces and then have to 
>> look it up. The lookup would have to be done on mshtml side anyway 
>> (since it has func_info_t and hooks), so there's no advantage.
> 
> 
> As long as you need a function object in mshtml, even if you don't call 
> it an actual object, I don't see how it's simpler.
> 

I don't really use an object at all. Here's a rough sketch of how it is:

On mshtml side:

static HRESULT proxy_func_invoke(IDispatch *this_obj, void *func_info, 
DISPPARAMS *dp, VARIANT *res, EXCEPINFO *ei, IServiceProvider *caller)
{
     func_info_t *func = func_info;
     return invoke_builtin_function(this_obj, func, DISPATCH_METHOD, dp, 
res, ei, caller);
}

then in the proxy interface, we have a method that returns...

1) Address of proxy_func_invoke
2) The func_info_t

...back to jscript. Jscript uses it when creating a ProxyFunction and 
stores it inside, then during ProxyFunction_call:

ProxyFunction_call(...):
{
     ...
     function->func.invoke(this_disp, function->func.info, ...);
     ...
}

Where 'invoke' is the function pointer from mshtml (that points to 
proxy_func_invoke) and 'info' is the func_into_t from mshtml. jscript 
doesn't have to know anything about them.

The catch is that, of course, I need the *same* underlying mechanism as 
invoking a function object without proxies, so that's why I'm sending it 
now. It doesn't mean I actually use the function object, though, but 
since the function object needs the same mechanism more or less (and 
invoke_builtin_function fixed to call hooks and do everything on 
arbitrary "this" dispatches), it's an opportunity to fix it as well.

Of course, I also have to do this for accessors (which can be retrieved 
with getOwnPropertyDescriptor), but it's the same idea, nothing special.

> 
> Also note that (at least currently, prototypes support may change it, 
> but likely not) func_info_t, separated object types will use separated 
> function infos. For example div element's function info for setAttribute 
> is not the same as body element's. It happens to work if you use the 
> other one, but that's now how it's meant to be used.
> 

Isn't that technically correct? Imagine, for a hypothetical example, 
that body element's setAttribute has a quirk on native, where div 
element's does not; i.e. it does not have the stringifying hook, where 
all other elements do. In this case, doing something like:

document.body.setAttribute.call(divElem, "attr", obj);

Needs to *not* stringify the obj value, because we are using body's 
setAttribute (which has the quirk) rather than divElem's, so in fact 
using func_info_t is more correct than IID/DIPSID pair, which would look 
it up on divElem and use the wrong setAttribute with the wrong hook.

This is of course, not a problem in practice, also because setAttribute 
is part of the prototype, so it's the same thing that's called.

That said there's some special cases (for example, style's prototype 
having a separate setAttribute, not using the element prototype chain), 
so I believe should we encounter such quirks in the future, it's still 
more correct anyway.