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

[Gabriel Ivăncescu]

On 08/12/2021 18:28, Jacek Caban wrote:
> On 12/8/21 5:02 PM, Gabriel Ivăncescu wrote:
>> 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.
> 
> 
> They are retrieved from the current object info, so we know that they 
> match info.
> 
> 
>>
>> 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.
> 
> 
> They only reason hooks need to do that is because of your design choices 
> and the above is one of the reasons why I think that some of those 
> choices are wrong.
> 
> 
>>
>> 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.
> 
> 
> Sure and you shouldn't call HTMLElement's hook on such external object 
> in the first place. They are HTMLElement's implementation detail and 
> applying them to external implementation is wrong. If the external 
> object has its own hooks, it's up to this object to call them.
> 

I see. But to have a generic validation, this would require to somehow 
store the object's vtbl address in the info so it can be validated with 
it and then skip the hook otherwise. The IID is obviously not sufficient 
since that only checks for the exposed interface.

Alternatively, we could just *always* call the hook like in my design, 
and validate it and skip it in the hook if the vtbl doesn't match (i.e. 
the hook itself does the validation). This would also work for the weird 
prototypes later where the hooks return undefined without validation, 
otherwise I'd have to add a special casing...

Which one do you prefer?