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

[Jacek Caban]

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.


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


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


Jacek