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

[Jacek Caban]

On 12/8/21 7:05 PM, Gabriel Ivăncescu wrote:
> 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?


The problem exists only because you expose functions as func_info_t 
(even if you make it an opaque; it's important how you treat them, not 
what's in the header). If you didn't do that, the problem wouldn't exist 
in the first place. That's one of the reasons why I'd like to revisit 
that decision.


Jacek