[PATCH v12 6/7] mshtml: Set the builtin prop to NULL even when removeAttribute returns false in IE8 mode.

[Jacek Caban]

On 12/2/21 4:37 PM, Gabriel Ivăncescu wrote:
> On 02/12/2021 15:51, Jacek Caban wrote:
>> On 12/2/21 2:20 PM, Gabriel Ivăncescu wrote:
>>> On 01/12/2021 21:24, Jacek Caban wrote:
>>>> Hi Gabriel,
>>>>
>>>> On 12/1/21 2:41 PM, Gabriel Ivăncescu wrote:
>>>>> + if(dispex_compat_mode(This) >= COMPAT_MODE_IE8) {
>>>>> +                    V_VT(&var) = VT_NULL;
>>>>> +                    builtin_propput(This, func, &dp, NULL);
>>>>> +                }
>>>>
>>>>
>>>> That doesn't seem universally right, see the attached test for an 
>>>> example.
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> Jacek
>>>>
>>>
>>> Are you sure this is related to this patch? I did some digging but 
>>> it seems to be a completely different issue to me. This patch is 
>>> about removeAttribute returning false and setting to NULL, which 
>>> happens with event handlers. 
>>
>>
>> Yes, the existing code is already wrong, we probably shouldn't 
>> blindly try to set value to VT_EMPTY in the first place. Trying to 
>> set the property to another value (of type that doesn't make sense 
>> unless you assume that those are event handlers) based on a previous 
>> error that we know nothing about (it could be OOM?) is making it even 
>> worse. Is there another reason for this patch than a todo_wine in tests?
>>
>>
>> Thanks,
>>
>> Jacek
>>
>
> I think the VT_EMPTY is correct for pre IE8 modes, since I didn't get 
> any failures there, just not for IE8.
>
> Even if we assume that we do implement it correctly for IE8 to reset 
> it to default values at some point, it would *still* not work without 
> this patch for this particular case, since that would handle the case 
> where it returns true, not false. Isn't this patch still needed, then? 
> I can't see how else it would work.


I would need a deeper look to propose a solution. For example, if we had 
a way to recognize an event handler property, it could help here.


> As for the reason, well, prototype.js sets "onclick" to an array and 
> later removeAttribute on it. Not sure to what extent it's useful 
> though. But it was a pretty simple fix that I honestly see no 
> downsides to—even in the future should we implement the rest properly.


Well, it's really not that hard to read prototype.js. It just does a 
check if attributes are converted to string and if not, applies some 
hacks for early IE version soon. removeAttribute is just a clean up, it 
will take the right code path anyway. And whatever code path it takes, 
it should be fine for us.


Jacek