[PATCH 1/2] jscript: Handle detached scope objects 'this' in Object.toString.

[Jacek Caban]

Hi Gabriel,

On 5/6/22 15:19, Gabriel Ivăncescu wrote:
> On 06/05/2022 02:56, Jacek Caban wrote:
>> On 5/6/22 01:50, Jacek Caban wrote:
>>>> Honestly, I don't particularly care about toString, it's just the 
>>>> only builtin I could think off the top of my head that could be 
>>>> tested for this. Because right now we handle it in exec_source, 
>>>> which is only for interpreted functions... but it's a good test 
>>>> case of such behavior.
>>>>
>>>> Do you think a fix like in 2/2 is more appropriate (also for quirks 
>>>> modes, but with null instead of undefined)? And perhaps fix 
>>>> whatever is causing toString to fail, which seems unrelated I guess.
>>>>
>>>> That might mean moving it out of exec_source though. 
>>>
>>>
>>> Same story there, why do you think it's somehow related to detached 
>>> scopes? The attached test fails with your patch. 
>>
>>
>> I attached a wrong test, here is the right one.
>>
>>
>> Jacek
>>
>
> Because without handling detached scopes we actually *trigger the 
> assertion* so it's quite bad. But that's just for toString, it's not 
> really important. I've added a test with hasOwnProperty now, which 
> *must* fail on detached scopes, but it doesn't currently (since 
> they're js objects). They need special handling either way.
>
> The attached test looks to me like a different issue: it passes null 
> instead of undefined, which is an ES5 specific thing, and probably 
> deserves a separate fix. It's not much of a problem either way.


I'm not convinced that it's unrelated, see below.


> Note that we are *already* handling detached scopes in exec_source, so 
> it's not like I'm adding custom code to deal with that separately; we 
> already do. I'm just moving it out of exec_source because:
>
> 1) Nothing in the (specified) spec says it's only for source functions.
> 2) This will become a problem with proxy functions later (which is 
> where it's needed); I'd rather not duplicate that logic in them for no 
> reason.
> 3) We are already handling it in exec_source "as a special case".
>
> I'm trying to find fix for the attach problem and send it separately 
> but honestly I can't see how such a fix would fix detached scopes at all.


The main problem of those patches is that they seem like random special 
cases fixing specific tests without enough understanding of the problem. 
Tests that I attached were meant to help you find the root cause of 
problems, but v2 is not really looking convincing yet. For example, do 
we really want to use builtin info class type for this? I'm attaching 
another test.


BTW, any time detached scopes need special treatment is a worrying sign. 
There is no such thing as detached scope in the spec. It's purely our 
implementation detail (it's in fact just an optimization). If they have 
observable differences in behaviour, that's alarming on its own.


Jacek
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch.diff
Type: text/x-patch
Size: 682 bytes
Desc: not available
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20220512/91ffb354/attachment.bin>