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

[Jacek Caban]

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.


>
> 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 fact that they don't need explicit validation is not an accident, 
that's how the code is structured.


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


Again: no, they don't. They need that change only because of your other 
choices.


>>> 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?


Again, don't call those hooks on arbitrary objects at all. This hook is 
irrelevant in this case.


Jacek