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

[Gabriel Ivăncescu]

On 12/05/2022 13:45, Jacek Caban wrote:
> 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

Right. The spec I referenced (but also the code in exec_source for 3rd 
edition) does state that it's replaced with null/undefined under certain 
conditions. One of them being not a reference. Since detached scopes 
aren't supposed to exist, doesn't that apply to them?

I don't know if builtin_info is the best way to handle it either, I was 
just moving the logic out of exec_source (which uses that), because this 
needs to happen for all function calls, not just interpreted functions, 
that's all.

Oh, and the reason I added the JS_GLOBAL workaround is to avoid 
potential regressions, for the moment. It will be completely removed 
when the JS global becomes window (which is how it should work, but need 
it to be a proper JS object first with prototype etc, so it will be a 
while).

I'm not looking to fix builtin functions specially here. The main 
motivation is for proxy functions later which depend on the same 
"logic", it just doesn't make sense to keep it in exec_source and 
specific to interpreted functions. But for now we only have builtin 
functions to test with, other than exec_source (since bind functions 
replace 'this' anyway, so they can't be tested).

Well I will look into it but tbh, fixing builtins (at least in jscript, 
not sure about mshtml) seems to require some sort of extra internal flag 
passed in call/apply when invoking, to ALL builtins. I don't think it's 
worth it. Maybe I can figure out a different way, but either way, that 
logic definitely needs to be moved out of exec_source, whether it's 
fixed differently or not...

Thanks,
Gabriel