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

[Gabriel Ivăncescu]

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.

Your attached patch has removeAttribute return true in both native and 
wine. It also doesn't even use this code path, taking it out results in 
the same test failures in wine (with the extra one being the one fixed 
by the patch of course).

So what this patch is about: when an event handler fails setting to 
VT_EMPTY, and then it returns false (tested previously), it still has to 
set it to NULL. There is one other type of attribute which will use this 
code path: read-only props. However, for read-only props, the 
set-to-NULL will fail and do nothing anyway, so it doesn't require 
special casing.

Instead, what seems to be the problem here is that removeAttribute, when 
it returns true (i.e. not this patch's code path) should reset the 
underlying props to their default values. This seems unrelated and a 
completely separate change to me.

It would be hard to fix as well, since it requires doing it in every 
builtin prop, possibly via some hook (I don't know if we can always 
detect removeAttribute from input parameter, or whether it's "different" 
in behavior than setting it to VT_EMPTY or not manually).

Doesn't seem worth to do right now, I was focusing on event handlers 
since that's what were used in some js libraries.

I think the patch is good enough for what it does: set event handlers to 
NULL when removeAttribute returns false. Other behavior is, imo, 
deserving separate changes, and not about this code path at all.


Thanks,
Gabriel