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

[Gabriel Ivăncescu]

On 08/12/2021 17:22, Jacek Caban wrote:
> On 12/8/21 3:11 PM, Gabriel Ivăncescu wrote:
>> On 07/12/2021 22:34, Jacek Caban wrote:
>>> On 12/7/21 9:29 PM, Gabriel Ivăncescu wrote:
>>>> On 07/12/2021 21:18, Jacek Caban wrote:
>>>>> On 12/7/21 7:52 PM, Gabriel Ivăncescu wrote:
>>>>>>> That just doesn't make sense at all. Once we support prototypes 
>>>>>>> document.body.setAttribute === 
>>>>>>> document.createElement("div").setAttribute === 
>>>>>>> document.head.__proto__.setAttribute === Element.setAttribute. 
>>>>>>> It's one and the same function object, not separated quirk 
>>>>>>> holders. Hooks are an implementation details of such abstract 
>>>>>>> functions in context of a specific object type and there is 
>>>>>>> nothing special about that. They are not a part of function 
>>>>>>> object itself.
>>>>>>>
>>>>>>
>>>>>> I did mention the prototype means it is the same function object, 
>>>>>> I was using a hypothetical example with quirks. My point was that 
>>>>>> *if* they end up being different functions (for any reason), using 
>>>>>> func_info_t is more correct because it represents two different 
>>>>>> functions, with possibly two different hooks.
>>>>>>
>>>>>> Don't get me wrong, I did not find yet a case where such a quirk 
>>>>>> exists, that's why I mentioned it's hypothetical. Although I did 
>>>>>> find some duplicated props in some places. Anyway I think it's 
>>>>>> more technically correct, and can be easier handled in the future 
>>>>>> *if* we find such quirks.
>>>>>
>>>>>
>>>>> No, it's not correct. body element hooks from your example are free 
>>>>> to cast 'this' object to HTMLBodyElement. If it did that, you'd end 
>>>>> up casting div element object to HTMLBodyElement and likely crash.
>>>>>
>>>>
>>>> Oh I see what you mean now, but that's not an issue because the 
>>>> hooks behavior needs to change. I have patches for that of course, 
>>>> though I don't know if they'll make it before code freeze.
>>>>
>>>> In short, the hooks will not take any DispatchEx* argument at all 
>>>> (in fact, you can't really know *what* object they are called on), 
>>>> but a generic IDispatch *this_obj arg.
>>>>
>>>> This has nothing to do with the things mentioned above: it is much 
>>>> more general. Think of the following with an actual, real hook:
>>>>
>>>> elem.setAttribute.call(arbitrary_dispatch_object, "A", "B");
>>>>
>>>> As you can see we cannot guarantee that the hook itself is called on 
>>>> the respective element anymore, so casting it is already wrong. The 
>>>> correct thing would be to QueryInterface, or detect it some other 
>>>> ways (but that's not needed with the hooks we have so far).
>>>>
>>>> But the point is: hooks need to operate on arbitrary this_obj 
>>>> IDispatch, and it's a general issue, not related to the proxy 
>>>> interface. 
>>>
>>>
>>>
>>> By the time the code reaches hooks, we should already have the type 
>>> validated. I don't see why you need to change that.
>>>
>> Currently the type is validated during the builtin invocation, with 
>> QueryInterface. I think the hooks should be kept generic without type 
>> validation unless necessary (done within the hook itself) because:
>>
>> 1) We currently have no hooks requiring a *specific* type until it is 
>> forwarded (which already handles failure). Adding such validation 
>> would duplicate validation for no real gain.
> 
> 
> Hooks are part of function implementation and just like 
> HTMLElement_setAttribute is free to cast iface to HTMLElement, its hook 
> are free to do that too.
> 

But HTMLElement_setAttribute is called after its interface is validated 
in typeinfo_invoke or invoke_builtin_function (QueryInterface). It knows 
that the object is proper because its vtbl implements setAttribute for 
that given object. If we have a different object exposing same 
interface, it has a different setAttribute in the vtbl for a different 
layout.

Hooks are called *before* that, for obvious reasons.

Of course, hooks will still be free to validate it themselves and then 
cast it, if needed, and it's not hard to do. But in case of hooks we 
can't just cast it to HTMLElement if the interface matches. There's no 
guarantee that the interface has the specific layout there. We have to 
check the vtbl.

For example, imagine there's an external object implementing that 
interface, but with a completely different layout (not HTMLElement's 
layout). It passes the interface test, you can call setAttribute on it 
just fine because it invokes its own setAttribute, and so on. But you 
can't cast it to HTMLElement* outside of the vtbl method.

And I don't see how we can do that in a generic way in a simple manner.

> 
>> 2) Type checking within the hook is pretty simple. If we use a 
>> specific interface, QueryInterface() should do it. If we rely on a 
>> specific internal object layout, we can check the vtbl, because in the 
>> hook we know what vtbl we want. But checking this before calling the 
>> hook in a generic way seems non-trivial in comparison.
> 
> 
> Well, it's trivial. Just don't change them and it should be fine, they 
> already have proper validation.
> 

They don't though. They have validation right now because hooks do not 
work with arbitrary this_obj IDispatches, as it is now.

The only thing implemented currently is the function object dispatches 
for pre IE9 modes, which only have the "value" implemented, as in they 
act on the same object they were retrieved from.

apply or call are not implemented, and those *have* to work on arbitrary 
dispatch objects. It's the same with proxies of course, same thing 
applies. Both need changed hooks.

> 
>> 3) There's some special cases wrt the special constructors/prototypes 
>> (in compat modes) where all builtin accessors on them return undefined 
>> / do nothing (but only accessors). I currently use a hook to do that, 
>> which makes it trivial. They are already called on an "incompatible" 
>> object, though, because calling the prototype props *on the prototype* 
>> should fail in normal circumstances (error code depends on doc mode). 
>> So if I do type validation *before* the hook, the hook will never run 
>> in this case.
> 
> 
> It seems to me like you're doing something else wrong, but it's hard to 
> comment without seeing the code.
> 
> 
>>
>> Maybe I'm missing something but what would be a way to validate the 
>> type in generic way then? If we are to check for the exact internal 
>> object layout, rather than just one exposing the interface, we'll have 
>> to check the vtbl. This is assuming hooks can use the internal object 
>> layout (and not just the interface, which the forwarded builtin does).
> 
> 
> If we have (IID,DISPID), then you could just do that in jscript:
> 
> if (FAILED(IDispatch_QueryInterface(this_obj, iid, &iface)) //object has 
> wrong type
> 
> IJSDispatch_call(this_obj, dispid, ....); // now we know that DISPID is 
> valid in context of this object, call it
> 
> 
> then in MSHTML, you can get func_info_t from dispid and be sure that 
> it's the right func_info_t for this object, but that's an internal 
> detail of mshtml/dispid.c and nothing to worry in the rest of the 
> interface.
> 

Well that doesn't really help at all, it's no different than now. With 
the way I have it, the interface is checked before it is used in mshtml 
side, not in jscript (already done in invoke_builtin_function for 
example, nothing extra).

The problem, of course, are hooks. When you call this function on an 
*arbitrary* dispatch, you have no guarantee that the object has the 
layout you want. You are *not* free to cast it to, for example, 
HTMLElement, even if it implements IID_IHTMLElement. The interface being 
present does not tell you anything about its layout, it's just an interface.

The actual methods implemented are allowed to, because they are called 
through the interface, so they know the object is the proper one. Hooks, 
however, are *not* part of the vtbl. They will be called on *any* object 
implementing this interface, whether it's HTMLElement or some other 
structure.

So they are not free to cast it to HTMLElement at all without validating 
the vtbl. And that's what I mean: the hooks should do this validation 
*if* they need to, within the hook, and accept and know that they will 
be called on arbitrary IDispatch objects. Frankly, I don't see a need 
for hooks to ever check the vtbl, but it's still an available option.

Otherwise, how exactly will we implement apply/call (proxy or not, 
doesn't matter) with proper hooks?

e.g. this_obj = IDispatch external object implementing IHTMLElement but 
not being ours, so not HTMLElement struct. How to call the setAttribute 
hook now?

I know it's not likely, but I'd like to implement this correctly, 
especially since it's not hard at all for hooks to accept arbitrary 
IDispatch objects.