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

[Gabriel Ivăncescu]

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.

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.

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.


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